| Message ID | 20200227181522.2711142-27-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm managed resources, v3 | expand |
Hi Daniel Am 27.02.20 um 19:14 schrieb Daniel Vetter: > drm_mode_config_cleanup is idempotent, so no harm in calling this > twice. This allows us to gradually switch drivers over by removing > explicit drm_mode_config_cleanup calls. > > With this step it's not also possible that (at least for simple > drivers) automatic resource cleanup can be done correctly without a > drm_driver->release hook. Therefore allow this now in > devm_drm_dev_init(). > > Also with drmm_ explicit drm_driver->release hooks are kinda not the > best option, so deprecate that hook to discourage future users. > > v2: Fixup the example in the kerneldoc too. > > v3: > - For paranoia, double check that minor->dev == dev in the release > hook, because I botched the pointer math in the drmm library. > - Call drm_mode_config_cleanup when drmm_add_action fails, we'd be > missing some mutex_destroy and ida_cleanup otherwise (Laurent) > > v4: Add a drmm_add_action_or_reset (like devm_ has) to encapsulate this > pattern (Noralf). > > v5: Fix oversight in the new add_action_or_reset macro (Noralf) > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: "Noralf Trønnes" <noralf@tronnes.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Acked-by: Noralf Trønnes <noralf@tronnes.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_drv.c | 23 +++++++---------------- > drivers/gpu/drm/drm_managed.c | 14 ++++++++++++++ > drivers/gpu/drm/drm_mode_config.c | 13 ++++++++++++- > include/drm/drm_managed.h | 7 +++++++ > include/drm/drm_mode_config.h | 2 +- > 5 files changed, 41 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 3cf40864d4a6..bb326b9bcde0 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -98,6 +98,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) > struct drm_minor *minor = data; > unsigned long flags; > > + WARN_ON(dev != minor->dev); > + > put_device(minor->kdev); > > spin_lock_irqsave(&drm_minor_lock, flags); > @@ -267,8 +269,7 @@ void drm_minor_release(struct drm_minor *minor) > * > * The following example shows a typical structure of a DRM display driver. > * The example focus on the probe() function and the other functions that is > - * almost always present and serves as a demonstration of devm_drm_dev_init() > - * usage with its accompanying drm_driver->release callback. > + * almost always present and serves as a demonstration of devm_drm_dev_init(). > * > * .. code-block:: c > * > @@ -278,16 +279,8 @@ void drm_minor_release(struct drm_minor *minor) > * struct clk *pclk; > * }; > * > - * static void driver_drm_release(struct drm_device *drm) > - * { > - * struct driver_device *priv = container_of(...); > - * > - * drm_mode_config_cleanup(drm); > - * } > - * > * static struct drm_driver driver_drm_driver = { > * [...] > - * .release = driver_drm_release, > * }; > * > * static int driver_probe(struct platform_device *pdev) > @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor) > * } > * drmm_add_final_kfree(drm, priv); > * > - * drm_mode_config_init(drm); > + * ret = drm_mode_config_init(drm); > + * if (ret) > + * return ret; > * > * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL); > * if (!priv->userspace_facing) > @@ -710,8 +705,7 @@ static void devm_drm_dev_init_release(void *data) > * @driver: DRM driver > * > * Managed drm_dev_init(). The DRM device initialized with this function is > - * automatically put on driver detach using drm_dev_put(). You must supply a > - * &drm_driver.release callback to control the finalization explicitly. > + * automatically put on driver detach using drm_dev_put(). > * > * RETURNS: > * 0 on success, or error code on failure. > @@ -722,9 +716,6 @@ int devm_drm_dev_init(struct device *parent, > { > int ret; > > - if (WARN_ON(!driver->release)) > - return -EINVAL; > - > ret = drm_dev_init(dev, driver, parent); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > index 626656369f0b..6376be01bbc8 100644 > --- a/drivers/gpu/drm/drm_managed.c > +++ b/drivers/gpu/drm/drm_managed.c > @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev, > } > EXPORT_SYMBOL(__drmm_add_action); > > +int __drmm_add_action_or_reset(struct drm_device *dev, > + drmres_release_t action, > + void *data, const char *name) > +{ > + int ret; > + > + ret = __drmm_add_action(dev, action, data, name); > + if (ret) > + action(dev, data); > + > + return ret; > +} > +EXPORT_SYMBOL(__drmm_add_action_or_reset); > + > void drmm_remove_action(struct drm_device *dev, > drmres_release_t action, > void *data) > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index 08e6eff6a179..6f7005bc597f 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -25,6 +25,7 @@ > #include <drm/drm_drv.h> > #include <drm/drm_encoder.h> > #include <drm/drm_file.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mode_config.h> > #include <drm/drm_print.h> > #include <linux/dma-resv.h> > @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return 0; > } > > +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr) > +{ > + drm_mode_config_cleanup(dev); > +} > + > /** > * drm_mode_config_init - initialize DRM mode_configuration structure > * @dev: DRM device > @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > * problem, since this should happen single threaded at init time. It is the > * driver's problem to ensure this guarantee. > * > + * Cleanup is automatically handled through registering drm_mode_config_cleanup > + * with drmm_add_action(). > */ > -void drm_mode_config_init(struct drm_device *dev) > +int drm_mode_config_init(struct drm_device *dev) > { > mutex_init(&dev->mode_config.mutex); > drm_modeset_lock_init(&dev->mode_config.connection_mutex); > @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev) > drm_modeset_acquire_fini(&modeset_ctx); > dma_resv_fini(&resv); > } > + > + return drmm_add_action_or_reset(dev, drm_mode_config_init_release, > + NULL); > } > EXPORT_SYMBOL(drm_mode_config_init); It might be a bit late in the review cycle, but could this function be renamed to drmm_mode_config_init() to reflect the garbage-collected implementation? Best regards Thomas > > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h > index 2b1ba2ad5582..1e6291407586 100644 > --- a/include/drm/drm_managed.h > +++ b/include/drm/drm_managed.h > @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev, > drmres_release_t action, > void *data, const char *name); > > +#define drmm_add_action_or_reset(dev, action, data) \ > + __drmm_add_action_or_reset(dev, action, data, #action) > + > +int __must_check __drmm_add_action_or_reset(struct drm_device *dev, > + drmres_release_t action, > + void *data, const char *name); > + > void drmm_remove_action(struct drm_device *dev, > drmres_release_t action, > void *data); > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 3bcbe30339f0..160a3e4b51c3 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -929,7 +929,7 @@ struct drm_mode_config { > const struct drm_mode_config_helper_funcs *helper_private; > }; > > -void drm_mode_config_init(struct drm_device *dev); > +int drm_mode_config_init(struct drm_device *dev); > void drm_mode_config_reset(struct drm_device *dev); > void drm_mode_config_cleanup(struct drm_device *dev); > >
On Fri, Feb 28, 2020 at 8:30 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi Daniel > > Am 27.02.20 um 19:14 schrieb Daniel Vetter: > > drm_mode_config_cleanup is idempotent, so no harm in calling this > > twice. This allows us to gradually switch drivers over by removing > > explicit drm_mode_config_cleanup calls. > > > > With this step it's not also possible that (at least for simple > > drivers) automatic resource cleanup can be done correctly without a > > drm_driver->release hook. Therefore allow this now in > > devm_drm_dev_init(). > > > > Also with drmm_ explicit drm_driver->release hooks are kinda not the > > best option, so deprecate that hook to discourage future users. > > > > v2: Fixup the example in the kerneldoc too. > > > > v3: > > - For paranoia, double check that minor->dev == dev in the release > > hook, because I botched the pointer math in the drmm library. > > - Call drm_mode_config_cleanup when drmm_add_action fails, we'd be > > missing some mutex_destroy and ida_cleanup otherwise (Laurent) > > > > v4: Add a drmm_add_action_or_reset (like devm_ has) to encapsulate this > > pattern (Noralf). > > > > v5: Fix oversight in the new add_action_or_reset macro (Noralf) > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Acked-by: Noralf Trønnes <noralf@tronnes.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_drv.c | 23 +++++++---------------- > > drivers/gpu/drm/drm_managed.c | 14 ++++++++++++++ > > drivers/gpu/drm/drm_mode_config.c | 13 ++++++++++++- > > include/drm/drm_managed.h | 7 +++++++ > > include/drm/drm_mode_config.h | 2 +- > > 5 files changed, 41 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > index 3cf40864d4a6..bb326b9bcde0 100644 > > --- a/drivers/gpu/drm/drm_drv.c > > +++ b/drivers/gpu/drm/drm_drv.c > > @@ -98,6 +98,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) > > struct drm_minor *minor = data; > > unsigned long flags; > > > > + WARN_ON(dev != minor->dev); > > + > > put_device(minor->kdev); > > > > spin_lock_irqsave(&drm_minor_lock, flags); > > @@ -267,8 +269,7 @@ void drm_minor_release(struct drm_minor *minor) > > * > > * The following example shows a typical structure of a DRM display driver. > > * The example focus on the probe() function and the other functions that is > > - * almost always present and serves as a demonstration of devm_drm_dev_init() > > - * usage with its accompanying drm_driver->release callback. > > + * almost always present and serves as a demonstration of devm_drm_dev_init(). > > * > > * .. code-block:: c > > * > > @@ -278,16 +279,8 @@ void drm_minor_release(struct drm_minor *minor) > > * struct clk *pclk; > > * }; > > * > > - * static void driver_drm_release(struct drm_device *drm) > > - * { > > - * struct driver_device *priv = container_of(...); > > - * > > - * drm_mode_config_cleanup(drm); > > - * } > > - * > > * static struct drm_driver driver_drm_driver = { > > * [...] > > - * .release = driver_drm_release, > > * }; > > * > > * static int driver_probe(struct platform_device *pdev) > > @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor) > > * } > > * drmm_add_final_kfree(drm, priv); > > * > > - * drm_mode_config_init(drm); > > + * ret = drm_mode_config_init(drm); > > + * if (ret) > > + * return ret; > > * > > * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL); > > * if (!priv->userspace_facing) > > @@ -710,8 +705,7 @@ static void devm_drm_dev_init_release(void *data) > > * @driver: DRM driver > > * > > * Managed drm_dev_init(). The DRM device initialized with this function is > > - * automatically put on driver detach using drm_dev_put(). You must supply a > > - * &drm_driver.release callback to control the finalization explicitly. > > + * automatically put on driver detach using drm_dev_put(). > > * > > * RETURNS: > > * 0 on success, or error code on failure. > > @@ -722,9 +716,6 @@ int devm_drm_dev_init(struct device *parent, > > { > > int ret; > > > > - if (WARN_ON(!driver->release)) > > - return -EINVAL; > > - > > ret = drm_dev_init(dev, driver, parent); > > if (ret) > > return ret; > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > > index 626656369f0b..6376be01bbc8 100644 > > --- a/drivers/gpu/drm/drm_managed.c > > +++ b/drivers/gpu/drm/drm_managed.c > > @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev, > > } > > EXPORT_SYMBOL(__drmm_add_action); > > > > +int __drmm_add_action_or_reset(struct drm_device *dev, > > + drmres_release_t action, > > + void *data, const char *name) > > +{ > > + int ret; > > + > > + ret = __drmm_add_action(dev, action, data, name); > > + if (ret) > > + action(dev, data); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(__drmm_add_action_or_reset); > > + > > void drmm_remove_action(struct drm_device *dev, > > drmres_release_t action, > > void *data) > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > index 08e6eff6a179..6f7005bc597f 100644 > > --- a/drivers/gpu/drm/drm_mode_config.c > > +++ b/drivers/gpu/drm/drm_mode_config.c > > @@ -25,6 +25,7 @@ > > #include <drm/drm_drv.h> > > #include <drm/drm_encoder.h> > > #include <drm/drm_file.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mode_config.h> > > #include <drm/drm_print.h> > > #include <linux/dma-resv.h> > > @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > > return 0; > > } > > > > +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr) > > +{ > > + drm_mode_config_cleanup(dev); > > +} > > + > > /** > > * drm_mode_config_init - initialize DRM mode_configuration structure > > * @dev: DRM device > > @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > > * problem, since this should happen single threaded at init time. It is the > > * driver's problem to ensure this guarantee. > > * > > + * Cleanup is automatically handled through registering drm_mode_config_cleanup > > + * with drmm_add_action(). > > */ > > -void drm_mode_config_init(struct drm_device *dev) > > +int drm_mode_config_init(struct drm_device *dev) > > { > > mutex_init(&dev->mode_config.mutex); > > drm_modeset_lock_init(&dev->mode_config.connection_mutex); > > @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev) > > drm_modeset_acquire_fini(&modeset_ctx); > > dma_resv_fini(&resv); > > } > > + > > + return drmm_add_action_or_reset(dev, drm_mode_config_init_release, > > + NULL); > > } > > EXPORT_SYMBOL(drm_mode_config_init); > > It might be a bit late in the review cycle, but could this function be > renamed to drmm_mode_config_init() to reflect the garbage-collected > implementation? I think that makes sense, at least as a wrapper with a __must_check annotation (and keeping the old one). I tried to avoid the rename because it'd be a flag day patch. If you think this would be a great improvement then I can do that - I'm positive enough to bother with the rebase to touch up all the driver patches, but only if there's someone else who's enthusiastic about it :-) -Daniel > Best regards > Thomas > > > > > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h > > index 2b1ba2ad5582..1e6291407586 100644 > > --- a/include/drm/drm_managed.h > > +++ b/include/drm/drm_managed.h > > @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev, > > drmres_release_t action, > > void *data, const char *name); > > > > +#define drmm_add_action_or_reset(dev, action, data) \ > > + __drmm_add_action_or_reset(dev, action, data, #action) > > + > > +int __must_check __drmm_add_action_or_reset(struct drm_device *dev, > > + drmres_release_t action, > > + void *data, const char *name); > > + > > void drmm_remove_action(struct drm_device *dev, > > drmres_release_t action, > > void *data); > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > index 3bcbe30339f0..160a3e4b51c3 100644 > > --- a/include/drm/drm_mode_config.h > > +++ b/include/drm/drm_mode_config.h > > @@ -929,7 +929,7 @@ struct drm_mode_config { > > const struct drm_mode_config_helper_funcs *helper_private; > > }; > > > > -void drm_mode_config_init(struct drm_device *dev); > > +int drm_mode_config_init(struct drm_device *dev); > > void drm_mode_config_reset(struct drm_device *dev); > > void drm_mode_config_cleanup(struct drm_device *dev); > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
Hi Am 28.02.20 um 09:43 schrieb Daniel Vetter: > On Fri, Feb 28, 2020 at 8:30 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi Daniel >> >> Am 27.02.20 um 19:14 schrieb Daniel Vetter: >>> drm_mode_config_cleanup is idempotent, so no harm in calling this >>> twice. This allows us to gradually switch drivers over by removing >>> explicit drm_mode_config_cleanup calls. >>> >>> With this step it's not also possible that (at least for simple >>> drivers) automatic resource cleanup can be done correctly without a >>> drm_driver->release hook. Therefore allow this now in >>> devm_drm_dev_init(). >>> >>> Also with drmm_ explicit drm_driver->release hooks are kinda not the >>> best option, so deprecate that hook to discourage future users. >>> >>> v2: Fixup the example in the kerneldoc too. >>> >>> v3: >>> - For paranoia, double check that minor->dev == dev in the release >>> hook, because I botched the pointer math in the drmm library. >>> - Call drm_mode_config_cleanup when drmm_add_action fails, we'd be >>> missing some mutex_destroy and ida_cleanup otherwise (Laurent) >>> >>> v4: Add a drmm_add_action_or_reset (like devm_ has) to encapsulate this >>> pattern (Noralf). >>> >>> v5: Fix oversight in the new add_action_or_reset macro (Noralf) >>> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> Cc: "Noralf Trønnes" <noralf@tronnes.org> >>> Cc: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>> Acked-by: Noralf Trønnes <noralf@tronnes.org> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> --- >>> drivers/gpu/drm/drm_drv.c | 23 +++++++---------------- >>> drivers/gpu/drm/drm_managed.c | 14 ++++++++++++++ >>> drivers/gpu/drm/drm_mode_config.c | 13 ++++++++++++- >>> include/drm/drm_managed.h | 7 +++++++ >>> include/drm/drm_mode_config.h | 2 +- >>> 5 files changed, 41 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c >>> index 3cf40864d4a6..bb326b9bcde0 100644 >>> --- a/drivers/gpu/drm/drm_drv.c >>> +++ b/drivers/gpu/drm/drm_drv.c >>> @@ -98,6 +98,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) >>> struct drm_minor *minor = data; >>> unsigned long flags; >>> >>> + WARN_ON(dev != minor->dev); >>> + >>> put_device(minor->kdev); >>> >>> spin_lock_irqsave(&drm_minor_lock, flags); >>> @@ -267,8 +269,7 @@ void drm_minor_release(struct drm_minor *minor) >>> * >>> * The following example shows a typical structure of a DRM display driver. >>> * The example focus on the probe() function and the other functions that is >>> - * almost always present and serves as a demonstration of devm_drm_dev_init() >>> - * usage with its accompanying drm_driver->release callback. >>> + * almost always present and serves as a demonstration of devm_drm_dev_init(). >>> * >>> * .. code-block:: c >>> * >>> @@ -278,16 +279,8 @@ void drm_minor_release(struct drm_minor *minor) >>> * struct clk *pclk; >>> * }; >>> * >>> - * static void driver_drm_release(struct drm_device *drm) >>> - * { >>> - * struct driver_device *priv = container_of(...); >>> - * >>> - * drm_mode_config_cleanup(drm); >>> - * } >>> - * >>> * static struct drm_driver driver_drm_driver = { >>> * [...] >>> - * .release = driver_drm_release, >>> * }; >>> * >>> * static int driver_probe(struct platform_device *pdev) >>> @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor) >>> * } >>> * drmm_add_final_kfree(drm, priv); >>> * >>> - * drm_mode_config_init(drm); >>> + * ret = drm_mode_config_init(drm); >>> + * if (ret) >>> + * return ret; >>> * >>> * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL); >>> * if (!priv->userspace_facing) >>> @@ -710,8 +705,7 @@ static void devm_drm_dev_init_release(void *data) >>> * @driver: DRM driver >>> * >>> * Managed drm_dev_init(). The DRM device initialized with this function is >>> - * automatically put on driver detach using drm_dev_put(). You must supply a >>> - * &drm_driver.release callback to control the finalization explicitly. >>> + * automatically put on driver detach using drm_dev_put(). >>> * >>> * RETURNS: >>> * 0 on success, or error code on failure. >>> @@ -722,9 +716,6 @@ int devm_drm_dev_init(struct device *parent, >>> { >>> int ret; >>> >>> - if (WARN_ON(!driver->release)) >>> - return -EINVAL; >>> - >>> ret = drm_dev_init(dev, driver, parent); >>> if (ret) >>> return ret; >>> diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c >>> index 626656369f0b..6376be01bbc8 100644 >>> --- a/drivers/gpu/drm/drm_managed.c >>> +++ b/drivers/gpu/drm/drm_managed.c >>> @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev, >>> } >>> EXPORT_SYMBOL(__drmm_add_action); >>> >>> +int __drmm_add_action_or_reset(struct drm_device *dev, >>> + drmres_release_t action, >>> + void *data, const char *name) >>> +{ >>> + int ret; >>> + >>> + ret = __drmm_add_action(dev, action, data, name); >>> + if (ret) >>> + action(dev, data); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL(__drmm_add_action_or_reset); >>> + >>> void drmm_remove_action(struct drm_device *dev, >>> drmres_release_t action, >>> void *data) >>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >>> index 08e6eff6a179..6f7005bc597f 100644 >>> --- a/drivers/gpu/drm/drm_mode_config.c >>> +++ b/drivers/gpu/drm/drm_mode_config.c >>> @@ -25,6 +25,7 @@ >>> #include <drm/drm_drv.h> >>> #include <drm/drm_encoder.h> >>> #include <drm/drm_file.h> >>> +#include <drm/drm_managed.h> >>> #include <drm/drm_mode_config.h> >>> #include <drm/drm_print.h> >>> #include <linux/dma-resv.h> >>> @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) >>> return 0; >>> } >>> >>> +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr) >>> +{ >>> + drm_mode_config_cleanup(dev); >>> +} >>> + >>> /** >>> * drm_mode_config_init - initialize DRM mode_configuration structure >>> * @dev: DRM device >>> @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) >>> * problem, since this should happen single threaded at init time. It is the >>> * driver's problem to ensure this guarantee. >>> * >>> + * Cleanup is automatically handled through registering drm_mode_config_cleanup >>> + * with drmm_add_action(). >>> */ >>> -void drm_mode_config_init(struct drm_device *dev) >>> +int drm_mode_config_init(struct drm_device *dev) >>> { >>> mutex_init(&dev->mode_config.mutex); >>> drm_modeset_lock_init(&dev->mode_config.connection_mutex); >>> @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev) >>> drm_modeset_acquire_fini(&modeset_ctx); >>> dma_resv_fini(&resv); >>> } >>> + >>> + return drmm_add_action_or_reset(dev, drm_mode_config_init_release, >>> + NULL); >>> } >>> EXPORT_SYMBOL(drm_mode_config_init); >> >> It might be a bit late in the review cycle, but could this function be >> renamed to drmm_mode_config_init() to reflect the garbage-collected >> implementation? > > I think that makes sense, at least as a wrapper with a __must_check > annotation (and keeping the old one). I tried to avoid the rename > because it'd be a flag day patch. > > If you think this would be a great improvement then I can do that - > I'm positive enough to bother with the rebase to touch up all the > driver patches, but only if there's someone else who's enthusiastic > about it :-) When reading the patches, I thought that it's now non-obvious why some drivers cleanup drm_mode_config_init() manually and some don't. So you may not need a big flag-day patch. A simple wrapper with the drmm_ prefix should do. At some point the old interface will hopefully be gone. Best regards Thomas > -Daniel > >> Best regards >> Thomas >> >>> >>> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h >>> index 2b1ba2ad5582..1e6291407586 100644 >>> --- a/include/drm/drm_managed.h >>> +++ b/include/drm/drm_managed.h >>> @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev, >>> drmres_release_t action, >>> void *data, const char *name); >>> >>> +#define drmm_add_action_or_reset(dev, action, data) \ >>> + __drmm_add_action_or_reset(dev, action, data, #action) >>> + >>> +int __must_check __drmm_add_action_or_reset(struct drm_device *dev, >>> + drmres_release_t action, >>> + void *data, const char *name); >>> + >>> void drmm_remove_action(struct drm_device *dev, >>> drmres_release_t action, >>> void *data); >>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >>> index 3bcbe30339f0..160a3e4b51c3 100644 >>> --- a/include/drm/drm_mode_config.h >>> +++ b/include/drm/drm_mode_config.h >>> @@ -929,7 +929,7 @@ struct drm_mode_config { >>> const struct drm_mode_config_helper_funcs *helper_private; >>> }; >>> >>> -void drm_mode_config_init(struct drm_device *dev); >>> +int drm_mode_config_init(struct drm_device *dev); >>> void drm_mode_config_reset(struct drm_device *dev); >>> void drm_mode_config_cleanup(struct drm_device *dev); >>> >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> > >
Hi Daniel. Some bikeshedding in the following. with or with addressing (IMHO valid points) consider the patch: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Sam On Thu, Feb 27, 2020 at 07:14:57PM +0100, Daniel Vetter wrote: > drm_mode_config_cleanup is idempotent, so no harm in calling this > twice. This allows us to gradually switch drivers over by removing > explicit drm_mode_config_cleanup calls. > > With this step it's not also possible that (at least for simple > drivers) automatic resource cleanup can be done correctly without a > drm_driver->release hook. Therefore allow this now in > devm_drm_dev_init(). I am not really sure what you try to explain here? Should the "not" be deleted? > > Also with drmm_ explicit drm_driver->release hooks are kinda not the > best option, so deprecate that hook to discourage future users. The ->release hooks has others uses until everything is moved over to drmm_, or so I think. So deprecation seems a lttle too soon. > > v2: Fixup the example in the kerneldoc too. > > v3: > - For paranoia, double check that minor->dev == dev in the release > hook, because I botched the pointer math in the drmm library. > - Call drm_mode_config_cleanup when drmm_add_action fails, we'd be > missing some mutex_destroy and ida_cleanup otherwise (Laurent) > > v4: Add a drmm_add_action_or_reset (like devm_ has) to encapsulate this > pattern (Noralf). > > v5: Fix oversight in the new add_action_or_reset macro (Noralf) ^ drmm_add_action_or_reset > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: "Noralf Trønnes" <noralf@tronnes.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Acked-by: Noralf Trønnes <noralf@tronnes.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_drv.c | 23 +++++++---------------- > drivers/gpu/drm/drm_managed.c | 14 ++++++++++++++ > drivers/gpu/drm/drm_mode_config.c | 13 ++++++++++++- > include/drm/drm_managed.h | 7 +++++++ > include/drm/drm_mode_config.h | 2 +- > 5 files changed, 41 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 3cf40864d4a6..bb326b9bcde0 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -98,6 +98,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) > struct drm_minor *minor = data; > unsigned long flags; > > + WARN_ON(dev != minor->dev); > + > put_device(minor->kdev); > > spin_lock_irqsave(&drm_minor_lock, flags); > @@ -267,8 +269,7 @@ void drm_minor_release(struct drm_minor *minor) > * > * The following example shows a typical structure of a DRM display driver. > * The example focus on the probe() function and the other functions that is > - * almost always present and serves as a demonstration of devm_drm_dev_init() > - * usage with its accompanying drm_driver->release callback. > + * almost always present and serves as a demonstration of devm_drm_dev_init(). > * > * .. code-block:: c > * > @@ -278,16 +279,8 @@ void drm_minor_release(struct drm_minor *minor) > * struct clk *pclk; > * }; > * > - * static void driver_drm_release(struct drm_device *drm) > - * { > - * struct driver_device *priv = container_of(...); > - * > - * drm_mode_config_cleanup(drm); > - * } > - * > * static struct drm_driver driver_drm_driver = { > * [...] > - * .release = driver_drm_release, > * }; > * > * static int driver_probe(struct platform_device *pdev) > @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor) > * } > * drmm_add_final_kfree(drm, priv); > * > - * drm_mode_config_init(drm); > + * ret = drm_mode_config_init(drm); > + * if (ret) > + * return ret; We do not print anything in drm_mode_config_init() - so should we do it here? Otherwise we only get the more generic error from the driver core. > * > * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL); > * if (!priv->userspace_facing) > @@ -710,8 +705,7 @@ static void devm_drm_dev_init_release(void *data) > * @driver: DRM driver > * > * Managed drm_dev_init(). The DRM device initialized with this function is > - * automatically put on driver detach using drm_dev_put(). You must supply a > - * &drm_driver.release callback to control the finalization explicitly. > + * automatically put on driver detach using drm_dev_put(). > * > * RETURNS: > * 0 on success, or error code on failure. > @@ -722,9 +716,6 @@ int devm_drm_dev_init(struct device *parent, > { > int ret; > > - if (WARN_ON(!driver->release)) > - return -EINVAL; > - > ret = drm_dev_init(dev, driver, parent); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > index 626656369f0b..6376be01bbc8 100644 > --- a/drivers/gpu/drm/drm_managed.c > +++ b/drivers/gpu/drm/drm_managed.c > @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev, > } > EXPORT_SYMBOL(__drmm_add_action); > > +int __drmm_add_action_or_reset(struct drm_device *dev, > + drmres_release_t action, > + void *data, const char *name) > +{ > + int ret; > + > + ret = __drmm_add_action(dev, action, data, name); > + if (ret) > + action(dev, data); > + > + return ret; > +} > +EXPORT_SYMBOL(__drmm_add_action_or_reset); Bikeshedding - but why oh why prefixing the function with two underscores? - It makes it less readable - It says "internal", at least to me - but it is exported - It makes the casual reader wonder why, removing focus from other more relevant things - It makes me writing several lines of rant drmm_add_action_or_reset_named(...) would do the trick. Same rant above goes for __drmm_add_action()... > + > void drmm_remove_action(struct drm_device *dev, > drmres_release_t action, > void *data) > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index 08e6eff6a179..6f7005bc597f 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -25,6 +25,7 @@ > #include <drm/drm_drv.h> > #include <drm/drm_encoder.h> > #include <drm/drm_file.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mode_config.h> > #include <drm/drm_print.h> > #include <linux/dma-resv.h> > @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return 0; > } > > +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr) > +{ > + drm_mode_config_cleanup(dev); > +} > + > /** > * drm_mode_config_init - initialize DRM mode_configuration structure > * @dev: DRM device > @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > * problem, since this should happen single threaded at init time. It is the > * driver's problem to ensure this guarantee. > * > + * Cleanup is automatically handled through registering drm_mode_config_cleanup > + * with drmm_add_action(). > */ > -void drm_mode_config_init(struct drm_device *dev) > +int drm_mode_config_init(struct drm_device *dev) > { > mutex_init(&dev->mode_config.mutex); > drm_modeset_lock_init(&dev->mode_config.connection_mutex); > @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev) > drm_modeset_acquire_fini(&modeset_ctx); > dma_resv_fini(&resv); > } > + > + return drmm_add_action_or_reset(dev, drm_mode_config_init_release, > + NULL); > } > EXPORT_SYMBOL(drm_mode_config_init); As this is now a drmm_ managed function it should be named such. Maybe add a small drm_mode_config_init() wrapper in the header file for those that has not migrated yet. It is confusing if we are not consistent in naming and everywhere else the drm managed functions are named drmm_ > > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h > index 2b1ba2ad5582..1e6291407586 100644 > --- a/include/drm/drm_managed.h > +++ b/include/drm/drm_managed.h > @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev, > drmres_release_t action, > void *data, const char *name); > > +#define drmm_add_action_or_reset(dev, action, data) \ > + __drmm_add_action_or_reset(dev, action, data, #action) > + > +int __must_check __drmm_add_action_or_reset(struct drm_device *dev, > + drmres_release_t action, > + void *data, const char *name); > + > void drmm_remove_action(struct drm_device *dev, > drmres_release_t action, > void *data); > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 3bcbe30339f0..160a3e4b51c3 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -929,7 +929,7 @@ struct drm_mode_config { > const struct drm_mode_config_helper_funcs *helper_private; > }; > > -void drm_mode_config_init(struct drm_device *dev); > +int drm_mode_config_init(struct drm_device *dev); > void drm_mode_config_reset(struct drm_device *dev); > void drm_mode_config_cleanup(struct drm_device *dev); > > -- > 2.24.1
On Fri, Feb 28, 2020 at 9:26 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Daniel. > > Some bikeshedding in the following. > with or with addressing (IMHO valid points) consider the patch: > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > Sam > > On Thu, Feb 27, 2020 at 07:14:57PM +0100, Daniel Vetter wrote: > > drm_mode_config_cleanup is idempotent, so no harm in calling this > > twice. This allows us to gradually switch drivers over by removing > > explicit drm_mode_config_cleanup calls. > > > > > With this step it's not also possible that (at least for simple > > drivers) automatic resource cleanup can be done correctly without a > > drm_driver->release hook. Therefore allow this now in > > devm_drm_dev_init(). > I am not really sure what you try to explain here? > Should the "not" be deleted? s/not/now/ somehow that's a typo I do all the time, dunno why. > > Also with drmm_ explicit drm_driver->release hooks are kinda not the > > best option, so deprecate that hook to discourage future users. > The ->release hooks has others uses until everything is moved over to > drmm_, or so I think. So deprecation seems a lttle too soon. You can just add a drmm action which calls your release function. The upshot is that you can be more fine-grained (useful for unwinding when driver load fails halfway through). That's why I think new drivers after this patch shouldn't use ->release anymore, it's strictly less useful than drmm actions. The less unwind code I have to review carefully to make sure the right stuff gets released (and not more!) the better. > > v2: Fixup the example in the kerneldoc too. > > > > v3: > > - For paranoia, double check that minor->dev == dev in the release > > hook, because I botched the pointer math in the drmm library. > > - Call drm_mode_config_cleanup when drmm_add_action fails, we'd be > > missing some mutex_destroy and ida_cleanup otherwise (Laurent) > > > > v4: Add a drmm_add_action_or_reset (like devm_ has) to encapsulate this > > pattern (Noralf). > > > > v5: Fix oversight in the new add_action_or_reset macro (Noralf) > ^ drmm_add_action_or_reset > > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Acked-by: Noralf Trønnes <noralf@tronnes.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_drv.c | 23 +++++++---------------- > > drivers/gpu/drm/drm_managed.c | 14 ++++++++++++++ > > drivers/gpu/drm/drm_mode_config.c | 13 ++++++++++++- > > include/drm/drm_managed.h | 7 +++++++ > > include/drm/drm_mode_config.h | 2 +- > > 5 files changed, 41 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > index 3cf40864d4a6..bb326b9bcde0 100644 > > --- a/drivers/gpu/drm/drm_drv.c > > +++ b/drivers/gpu/drm/drm_drv.c > > @@ -98,6 +98,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) > > struct drm_minor *minor = data; > > unsigned long flags; > > > > + WARN_ON(dev != minor->dev); > > + > > put_device(minor->kdev); > > > > spin_lock_irqsave(&drm_minor_lock, flags); > > @@ -267,8 +269,7 @@ void drm_minor_release(struct drm_minor *minor) > > * > > * The following example shows a typical structure of a DRM display driver. > > * The example focus on the probe() function and the other functions that is > > - * almost always present and serves as a demonstration of devm_drm_dev_init() > > - * usage with its accompanying drm_driver->release callback. > > + * almost always present and serves as a demonstration of devm_drm_dev_init(). > > * > > * .. code-block:: c > > * > > @@ -278,16 +279,8 @@ void drm_minor_release(struct drm_minor *minor) > > * struct clk *pclk; > > * }; > > * > > - * static void driver_drm_release(struct drm_device *drm) > > - * { > > - * struct driver_device *priv = container_of(...); > > - * > > - * drm_mode_config_cleanup(drm); > > - * } > > - * > > * static struct drm_driver driver_drm_driver = { > > * [...] > > - * .release = driver_drm_release, > > * }; > > * > > * static int driver_probe(struct platform_device *pdev) > > @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor) > > * } > > * drmm_add_final_kfree(drm, priv); > > * > > - * drm_mode_config_init(drm); > > + * ret = drm_mode_config_init(drm); > > + * if (ret) > > + * return ret; > We do not print anything in drm_mode_config_init() - so should > we do it here? > Otherwise we only get the more generic error from the driver core. I can add a printk to drm_mode_config if people feel like. But it's guaranteed dead code in reality, because of linux' small memory allocation guarantee. Small mallocs like this one here of just 2 cachelines never fail (at least not with GFP_KERNEL). > > * > > * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL); > > * if (!priv->userspace_facing) > > @@ -710,8 +705,7 @@ static void devm_drm_dev_init_release(void *data) > > * @driver: DRM driver > > * > > * Managed drm_dev_init(). The DRM device initialized with this function is > > - * automatically put on driver detach using drm_dev_put(). You must supply a > > - * &drm_driver.release callback to control the finalization explicitly. > > + * automatically put on driver detach using drm_dev_put(). > > * > > * RETURNS: > > * 0 on success, or error code on failure. > > @@ -722,9 +716,6 @@ int devm_drm_dev_init(struct device *parent, > > { > > int ret; > > > > - if (WARN_ON(!driver->release)) > > - return -EINVAL; > > - > > ret = drm_dev_init(dev, driver, parent); > > if (ret) > > return ret; > > diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c > > index 626656369f0b..6376be01bbc8 100644 > > --- a/drivers/gpu/drm/drm_managed.c > > +++ b/drivers/gpu/drm/drm_managed.c > > @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev, > > } > > EXPORT_SYMBOL(__drmm_add_action); > > > > +int __drmm_add_action_or_reset(struct drm_device *dev, > > + drmres_release_t action, > > + void *data, const char *name) > > +{ > > + int ret; > > + > > + ret = __drmm_add_action(dev, action, data, name); > > + if (ret) > > + action(dev, data); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(__drmm_add_action_or_reset); > > Bikeshedding - but why oh why prefixing the function with two > underscores? > - It makes it less readable > - It says "internal", at least to me - but it is exported > - It makes the casual reader wonder why, removing focus from other more > relevant things > - It makes me writing several lines of rant > > drmm_add_action_or_reset_named(...) would do the trick. It _is_ an internal function. I don't think drivers should ever call it. Hence __ and hence no kerneldoc in the last patch. Now if someone finds a legit use for this I guess we cold rename it, and give it some nice kerneldoc. But imo no point before that. A thing that might be useful is drm_add_action_f() with a format string at the end, for cases where you set a non-NULL data pointer, and want to include that somehow in the name. This might be useful for cleanup actions for stuff like connectors and so on. But we're not there yet at all, and you can also do that kind of meaningful debug output from cleanup function directly. > Same rant above goes for __drmm_add_action()... > > > + > > void drmm_remove_action(struct drm_device *dev, > > drmres_release_t action, > > void *data) > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > index 08e6eff6a179..6f7005bc597f 100644 > > --- a/drivers/gpu/drm/drm_mode_config.c > > +++ b/drivers/gpu/drm/drm_mode_config.c > > @@ -25,6 +25,7 @@ > > #include <drm/drm_drv.h> > > #include <drm/drm_encoder.h> > > #include <drm/drm_file.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mode_config.h> > > #include <drm/drm_print.h> > > #include <linux/dma-resv.h> > > @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > > return 0; > > } > > > > +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr) > > +{ > > + drm_mode_config_cleanup(dev); > > +} > > + > > /** > > * drm_mode_config_init - initialize DRM mode_configuration structure > > * @dev: DRM device > > @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > > * problem, since this should happen single threaded at init time. It is the > > * driver's problem to ensure this guarantee. > > * > > + * Cleanup is automatically handled through registering drm_mode_config_cleanup > > + * with drmm_add_action(). > > */ > > -void drm_mode_config_init(struct drm_device *dev) > > +int drm_mode_config_init(struct drm_device *dev) > > { > > mutex_init(&dev->mode_config.mutex); > > drm_modeset_lock_init(&dev->mode_config.connection_mutex); > > @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev) > > drm_modeset_acquire_fini(&modeset_ctx); > > dma_resv_fini(&resv); > > } > > + > > + return drmm_add_action_or_reset(dev, drm_mode_config_init_release, > > + NULL); > > } > > EXPORT_SYMBOL(drm_mode_config_init); > As this is now a drmm_ managed function it should be named such. > Maybe add a small drm_mode_config_init() wrapper in the header file for > those that has not migrated yet. > It is confusing if we are not consistent in naming and everywhere else > the drm managed functions are named drmm_ Yeah ok I guess I'm convinced on this :-) Cheers, Daniel > > > > > > diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h > > index 2b1ba2ad5582..1e6291407586 100644 > > --- a/include/drm/drm_managed.h > > +++ b/include/drm/drm_managed.h > > @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev, > > drmres_release_t action, > > void *data, const char *name); > > > > +#define drmm_add_action_or_reset(dev, action, data) \ > > + __drmm_add_action_or_reset(dev, action, data, #action) > > + > > +int __must_check __drmm_add_action_or_reset(struct drm_device *dev, > > + drmres_release_t action, > > + void *data, const char *name); > > + > > void drmm_remove_action(struct drm_device *dev, > > drmres_release_t action, > > void *data); > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > > index 3bcbe30339f0..160a3e4b51c3 100644 > > --- a/include/drm/drm_mode_config.h > > +++ b/include/drm/drm_mode_config.h > > @@ -929,7 +929,7 @@ struct drm_mode_config { > > const struct drm_mode_config_helper_funcs *helper_private; > > }; > > > > -void drm_mode_config_init(struct drm_device *dev); > > +int drm_mode_config_init(struct drm_device *dev); > > void drm_mode_config_reset(struct drm_device *dev); > > void drm_mode_config_cleanup(struct drm_device *dev); > > > > -- > > 2.24.1
Hi Daniel. > > > Also with drmm_ explicit drm_driver->release hooks are kinda not the > > > best option, so deprecate that hook to discourage future users. > > The ->release hooks has others uses until everything is moved over to > > drmm_, or so I think. So deprecation seems a lttle too soon. > > You can just add a drmm action which calls your release function. The > upshot is that you can be more fine-grained (useful for unwinding when > driver load fails halfway through). That's why I think new drivers > after this patch shouldn't use ->release anymore, it's strictly less > useful than drmm actions. The less unwind code I have to review > carefully to make sure the right stuff gets released (and not more!) > the better. From that perspective I agree - gently pushing people to use drmm_ is only good. Sam
On Sat, Feb 29, 2020 at 12:11:28AM +0100, Daniel Vetter wrote: > On Fri, Feb 28, 2020 at 9:26 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > > @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor) > > > * } > > > * drmm_add_final_kfree(drm, priv); > > > * > > > - * drm_mode_config_init(drm); > > > + * ret = drm_mode_config_init(drm); > > > + * if (ret) > > > + * return ret; > > We do not print anything in drm_mode_config_init() - so should > > we do it here? > > Otherwise we only get the more generic error from the driver core. > > I can add a printk to drm_mode_config if people feel like. But it's > guaranteed dead code in reality, because of linux' small memory > allocation guarantee. Small mallocs like this one here of just 2 > cachelines never fail (at least not with GFP_KERNEL). To make this not quite pointless I decided to add debug output to drmm_add_action and drm_kmalloc. I think there it's actually useful for debugging. Will squash that into other patches. -Daniel
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 3cf40864d4a6..bb326b9bcde0 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -98,6 +98,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) struct drm_minor *minor = data; unsigned long flags; + WARN_ON(dev != minor->dev); + put_device(minor->kdev); spin_lock_irqsave(&drm_minor_lock, flags); @@ -267,8 +269,7 @@ void drm_minor_release(struct drm_minor *minor) * * The following example shows a typical structure of a DRM display driver. * The example focus on the probe() function and the other functions that is - * almost always present and serves as a demonstration of devm_drm_dev_init() - * usage with its accompanying drm_driver->release callback. + * almost always present and serves as a demonstration of devm_drm_dev_init(). * * .. code-block:: c * @@ -278,16 +279,8 @@ void drm_minor_release(struct drm_minor *minor) * struct clk *pclk; * }; * - * static void driver_drm_release(struct drm_device *drm) - * { - * struct driver_device *priv = container_of(...); - * - * drm_mode_config_cleanup(drm); - * } - * * static struct drm_driver driver_drm_driver = { * [...] - * .release = driver_drm_release, * }; * * static int driver_probe(struct platform_device *pdev) @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor) * } * drmm_add_final_kfree(drm, priv); * - * drm_mode_config_init(drm); + * ret = drm_mode_config_init(drm); + * if (ret) + * return ret; * * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL); * if (!priv->userspace_facing) @@ -710,8 +705,7 @@ static void devm_drm_dev_init_release(void *data) * @driver: DRM driver * * Managed drm_dev_init(). The DRM device initialized with this function is - * automatically put on driver detach using drm_dev_put(). You must supply a - * &drm_driver.release callback to control the finalization explicitly. + * automatically put on driver detach using drm_dev_put(). * * RETURNS: * 0 on success, or error code on failure. @@ -722,9 +716,6 @@ int devm_drm_dev_init(struct device *parent, { int ret; - if (WARN_ON(!driver->release)) - return -EINVAL; - ret = drm_dev_init(dev, driver, parent); if (ret) return ret; diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c index 626656369f0b..6376be01bbc8 100644 --- a/drivers/gpu/drm/drm_managed.c +++ b/drivers/gpu/drm/drm_managed.c @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev, } EXPORT_SYMBOL(__drmm_add_action); +int __drmm_add_action_or_reset(struct drm_device *dev, + drmres_release_t action, + void *data, const char *name) +{ + int ret; + + ret = __drmm_add_action(dev, action, data, name); + if (ret) + action(dev, data); + + return ret; +} +EXPORT_SYMBOL(__drmm_add_action_or_reset); + void drmm_remove_action(struct drm_device *dev, drmres_release_t action, void *data) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 08e6eff6a179..6f7005bc597f 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -25,6 +25,7 @@ #include <drm/drm_drv.h> #include <drm/drm_encoder.h> #include <drm/drm_file.h> +#include <drm/drm_managed.h> #include <drm/drm_mode_config.h> #include <drm/drm_print.h> #include <linux/dma-resv.h> @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return 0; } +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr) +{ + drm_mode_config_cleanup(dev); +} + /** * drm_mode_config_init - initialize DRM mode_configuration structure * @dev: DRM device @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) * problem, since this should happen single threaded at init time. It is the * driver's problem to ensure this guarantee. * + * Cleanup is automatically handled through registering drm_mode_config_cleanup + * with drmm_add_action(). */ -void drm_mode_config_init(struct drm_device *dev) +int drm_mode_config_init(struct drm_device *dev) { mutex_init(&dev->mode_config.mutex); drm_modeset_lock_init(&dev->mode_config.connection_mutex); @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev) drm_modeset_acquire_fini(&modeset_ctx); dma_resv_fini(&resv); } + + return drmm_add_action_or_reset(dev, drm_mode_config_init_release, + NULL); } EXPORT_SYMBOL(drm_mode_config_init); diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h index 2b1ba2ad5582..1e6291407586 100644 --- a/include/drm/drm_managed.h +++ b/include/drm/drm_managed.h @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev, drmres_release_t action, void *data, const char *name); +#define drmm_add_action_or_reset(dev, action, data) \ + __drmm_add_action_or_reset(dev, action, data, #action) + +int __must_check __drmm_add_action_or_reset(struct drm_device *dev, + drmres_release_t action, + void *data, const char *name); + void drmm_remove_action(struct drm_device *dev, drmres_release_t action, void *data); diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 3bcbe30339f0..160a3e4b51c3 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -929,7 +929,7 @@ struct drm_mode_config { const struct drm_mode_config_helper_funcs *helper_private; }; -void drm_mode_config_init(struct drm_device *dev); +int drm_mode_config_init(struct drm_device *dev); void drm_mode_config_reset(struct drm_device *dev); void drm_mode_config_cleanup(struct drm_device *dev);