Re: [Intel-gfx] [PATCH 22/24] drm: Nerf the preclose callback for modern drivers
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> > --- > > 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 _operations.release > > * method. It frees any resources associated with the open file, and calls > > the > > - * _driver.preclose and _driver.lastclose driver callbacks. If > > this is > > - * the last open file for the DRM device also proceeds to call the > > - * _driver.lastclose driver callback. > > + * _driver.lastclose driver callback. If this is the last open file > > for the > > s/lastclose/postclose/ > > Once that's fixed, > > Reviewed-by: Sean Paul 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 _driver.lastclose driver > > callback. > > * > > * RETURNS: > > * > > @@ -372,7 +371,8 @@ int drm_release(struct inode *inode, struct file *filp) > > list_del(_priv->lhead); > > mutex_unlock(>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 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 drm_file (see _file.is_master and _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 drm_file is closed. > > @@ -131,9 +114,6 @@ struct drm_driver { > > * one drm_file (see _file.is_master and _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 > > > >
Re: [Intel-gfx] [PATCH 22/24] drm: Nerf the preclose callback for modern drivers
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> --- > 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 _operations.release > * method. It frees any resources associated with the open file, and calls > the > - * _driver.preclose and _driver.lastclose driver callbacks. If this > is > - * the last open file for the DRM device also proceeds to call the > - * _driver.lastclose driver callback. > + * _driver.lastclose driver callback. If this is the last open file for > the s/lastclose/postclose/ Once that's fixed, Reviewed-by: Sean Paul > + * DRM device also proceeds to call the _driver.lastclose driver > callback. > * > * RETURNS: > * > @@ -372,7 +371,8 @@ int drm_release(struct inode *inode, struct file *filp) > list_del(_priv->lhead); > mutex_unlock(>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 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 drm_file (see _file.is_master and _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 drm_file is closed. > @@ -131,9 +114,6 @@ struct drm_driver { >* one drm_file (see _file.is_master and _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-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 22/24] drm: Nerf the preclose callback for modern drivers
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 VetterFrom irc: Acked-by: Dave Airlie > --- > 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 _operations.release > * method. It frees any resources associated with the open file, and calls > the > - * _driver.preclose and _driver.lastclose driver callbacks. If this > is > - * the last open file for the DRM device also proceeds to call the > - * _driver.lastclose driver callback. > + * _driver.lastclose driver callback. If this is the last open file for > the > + * DRM device also proceeds to call the _driver.lastclose driver > callback. > * > * RETURNS: > * > @@ -372,7 +371,8 @@ int drm_release(struct inode *inode, struct file *filp) > list_del(_priv->lhead); > mutex_unlock(>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 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 drm_file (see _file.is_master and _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 drm_file is closed. > @@ -131,9 +114,6 @@ struct drm_driver { >* one drm_file (see _file.is_master and _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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 22/24] drm: Nerf the preclose callback for modern drivers
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--- 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 _operations.release * method. It frees any resources associated with the open file, and calls the - * _driver.preclose and _driver.lastclose driver callbacks. If this is - * the last open file for the DRM device also proceeds to call the - * _driver.lastclose driver callback. + * _driver.lastclose driver callback. If this is the last open file for the + * DRM device also proceeds to call the _driver.lastclose driver callback. * * RETURNS: * @@ -372,7 +371,8 @@ int drm_release(struct inode *inode, struct file *filp) list_del(_priv->lhead); mutex_unlock(>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 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 drm_file (see _file.is_master and _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 drm_file is closed. @@ -131,9 +114,6 @@ struct drm_driver { * one drm_file (see _file.is_master and _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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel