Re: [PATCH 22/24] drm: Nerf the preclose callback for modern drivers

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 13, 2017 at 03:29:20PM -0400, Sean Paul wrote:
> On Wed, Mar 08, 2017 at 03:12:55PM +0100, Daniel Vetter wrote:
> > With all drivers converted there's only legacy dri1 drivers using it.
> > Not going to touch those, instead just hide it like we've done with
> > other dri1 driver hooks like firstopen.
> > 
> > In all this I didn't find any real reason why we'd needed 2 hooks, and
> > having symmetry between open and close just appeases my OCD better.
> > Yeah, someone else could do an s/postclose/close/, but that's for
> > someone who understands cocci. And maybe after this series is reviewed
> > and landed, to avoid patch-regen churn.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_file.c |  8 ++++----
> >  include/drm/drm_drv.h      | 23 ++---------------------
> >  2 files changed, 6 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index a8813a1115dc..f8483fc6d3d7 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -350,9 +350,8 @@ void drm_lastclose(struct drm_device * dev)
> >   *
> >   * This function must be used by drivers as their &file_operations.release
> >   * method. It frees any resources associated with the open file, and calls the
> > - * &drm_driver.preclose and &drm_driver.lastclose driver callbacks. If this is
> > - * the last open file for the DRM device also proceeds to call the
> > - * &drm_driver.lastclose driver callback.
> > + * &drm_driver.lastclose driver callback. If this is the last open file for the
> 
> s/lastclose/postclose/
> 
> Once that's fixed,
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Fixed locally and will include it when I'm resending the entire pile with
the stragglers filled out.
-Daniel

> 
> > + * DRM device also proceeds to call the &drm_driver.lastclose driver callback.
> >   *
> >   * RETURNS:
> >   *
> > @@ -372,7 +371,8 @@ int drm_release(struct inode *inode, struct file *filp)
> >  	list_del(&file_priv->lhead);
> >  	mutex_unlock(&dev->filelist_mutex);
> >  
> > -	if (dev->driver->preclose)
> > +	if (drm_core_check_feature(dev, DRIVER_LEGACY) &&
> > +	    dev->driver->preclose)
> >  		dev->driver->preclose(dev, file_priv);
> >  
> >  	/* ========================================================
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index 8f900fb30275..fde343e0d581 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -104,23 +104,6 @@ struct drm_driver {
> >  	int (*open) (struct drm_device *, struct drm_file *);
> >  
> >  	/**
> > -	 * @preclose:
> > -	 *
> > -	 * One of the driver callbacks when a new &struct drm_file is closed.
> > -	 * Useful for tearing down driver-private data structures allocated in
> > -	 * @open like buffer allocators, execution contexts or similar things.
> > -	 *
> > -	 * Since the display/modeset side of DRM can only be owned by exactly
> > -	 * one &struct drm_file (see &drm_file.is_master and &drm_device.master)
> > -	 * there should never be a need to tear down any modeset related
> > -	 * resources in this callback. Doing so would be a driver design bug.
> > -	 *
> > -	 * FIXME: It is not really clear why there's both @preclose and
> > -	 * @postclose. Without a really good reason, use @postclose only.
> > -	 */
> > -	void (*preclose) (struct drm_device *, struct drm_file *file_priv);
> > -
> > -	/**
> >  	 * @postclose:
> >  	 *
> >  	 * One of the driver callbacks when a new &struct drm_file is closed.
> > @@ -131,9 +114,6 @@ struct drm_driver {
> >  	 * one &struct drm_file (see &drm_file.is_master and &drm_device.master)
> >  	 * there should never be a need to tear down any modeset related
> >  	 * resources in this callback. Doing so would be a driver design bug.
> > -	 *
> > -	 * FIXME: It is not really clear why there's both @preclose and
> > -	 * @postclose. Without a really good reason, use @postclose only.
> >  	 */
> >  	void (*postclose) (struct drm_device *, struct drm_file *);
> >  
> > @@ -150,7 +130,7 @@ struct drm_driver {
> >  	 * state changes, e.g. in conjunction with the :ref:`vga_switcheroo`
> >  	 * infrastructure.
> >  	 *
> > -	 * This is called after @preclose and @postclose have been called.
> > +	 * This is called after @postclose hook has been called.
> >  	 *
> >  	 * NOTE:
> >  	 *
> > @@ -516,6 +496,7 @@ struct drm_driver {
> >  	/* List of devices hanging off this driver with stealth attach. */
> >  	struct list_head legacy_dev_list;
> >  	int (*firstopen) (struct drm_device *);
> > +	void (*preclose) (struct drm_device *, struct drm_file *file_priv);
> >  	int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv);
> >  	int (*dma_quiescent) (struct drm_device *);
> >  	int (*context_dtor) (struct drm_device *dev, int context);
> > -- 
> > 2.11.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx