Re: [PATCH v2 1/2] drm/file: Rehabilitate the firstopen hook for non-legacy drivers

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

Hi,

Le mercredi 20 mars 2019 à 09:56 -0700, Eric Anholt a écrit :
> Paul Kocialkowski <paul.kocialkowski@bootlin.com> writes:
> 
> > The firstopen DRM driver hook was initially used to perform hardware
> > initialization, which is now considered legacy. Only a single user of
> > firstopen remains at this point (savage).
> > 
> > In some specific cases, non-legacy drivers may also need to implement
> > these hooks. For instance on VC4, we need to allocate a 16 MiB buffer
> > for the GPU. Because it's not required for fbcon, it's a waste to
> > allocate it before userspace starts using the DRM device.
> > 
> > Using firstopen and lastclose for this allocation seems like the best
> > fit, so re-habilitate the hook to allow it to be called for non-legacy
> > drivers.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  drivers/gpu/drm/drm_file.c | 3 +--
> >  include/drm/drm_drv.h      | 2 +-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index b1838a41ad43..c011b5cbfb6b 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -266,8 +266,7 @@ static int drm_setup(struct drm_device * dev)
> >  {
> >  	int ret;
> >  
> > -	if (dev->driver->firstopen &&
> > -	    drm_core_check_feature(dev, DRIVER_LEGACY)) {
> > +	if (dev->driver->firstopen) {
> >  		ret = dev->driver->firstopen(dev);
> >  		if (ret != 0)
> >  			return ret;
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index ca46a45a9cce..aa14607e54d4 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -236,7 +236,7 @@ struct drm_driver {
> >  	 * to set/unset the VT into raw mode.
> >  	 *
> >  	 * Legacy drivers initialize the hardware in the @firstopen callback,
> > -	 * which isn't even called for modern drivers.
> > +	 * modern drivers can use it for other purposes only.
> >  	 */
> >  	void (*lastclose) (struct drm_device *);
> 
> Our usage in vc4 is not very different from what we called "hardware
> initialization" in other devices.  I would rather just delete this
> sentence entirely.

Sounds good to me!

> The only alternative I can think of to using a firstopen/lastclose-style
> allocation for this would be to allocate the bin bo on the first
> (non-dumb?) V3D BO allocation and refcount those to free the binner.

I don't see other options either, and using firstclose/lastopen feels
overall more readable in the driver code.

I'm not sure there is such a big overhead associated with allocating
the binner BO (it seems that the current implementation tries to alloc
until the specific memory constraints for the buffer are met, so
perhaps that can take time). But if there is, I suppose it's best to
have that when starting up rather than delaying the first render
operation.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com