| Message ID | 20200219102122.1607365-28-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm_device managed resources | expand |
Hi Daniel, Thank you for the patch. On Wed, Feb 19, 2020 at 11:20:57AM +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(). > > 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. > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_drv.c | 21 +++++---------------- > drivers/gpu/drm/drm_mode_config.c | 12 +++++++++++- > include/drm/drm_mode_config.h | 2 +- > 3 files changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 3cf40864d4a6..428c569aaaf1 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -267,8 +267,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 +277,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 +303,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 +703,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 +714,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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index 08e6eff6a179..957db1edba0c 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,8 @@ void drm_mode_config_init(struct drm_device *dev) > drm_modeset_acquire_fini(&modeset_ctx); > dma_resv_fini(&resv); > } > + > + return drmm_add_action(dev, drm_mode_config_init_release, NULL); If this fails, shouldn't drm_mode_config_cleanup() be called here ? > } > EXPORT_SYMBOL(drm_mode_config_init); > > 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 Wed, Feb 19, 2020 at 2:50 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > Thank you for the patch. > > On Wed, Feb 19, 2020 at 11:20:57AM +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(). > > > > 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. > > > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_drv.c | 21 +++++---------------- > > drivers/gpu/drm/drm_mode_config.c | 12 +++++++++++- > > include/drm/drm_mode_config.h | 2 +- > > 3 files changed, 17 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > index 3cf40864d4a6..428c569aaaf1 100644 > > --- a/drivers/gpu/drm/drm_drv.c > > +++ b/drivers/gpu/drm/drm_drv.c > > @@ -267,8 +267,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 +277,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 +303,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 +703,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 +714,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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > index 08e6eff6a179..957db1edba0c 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,8 @@ void drm_mode_config_init(struct drm_device *dev) > > drm_modeset_acquire_fini(&modeset_ctx); > > dma_resv_fini(&resv); > > } > > + > > + return drmm_add_action(dev, drm_mode_config_init_release, NULL); > > If this fails, shouldn't drm_mode_config_cleanup() be called here ? Maybe for ocd reasons, but not for actually cleaning up anything. It's just a bunch of empty lists that drm_mode_config_cleanup will walk and do nothing about. Not sure I should add that ... -Daniel > > } > > EXPORT_SYMBOL(drm_mode_config_init); > > > > 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); > > > > -- > Regards, > > Laurent Pinchart
Hi Daniel, On Wed, Feb 19, 2020 at 04:47:55PM +0100, Daniel Vetter wrote: > On Wed, Feb 19, 2020 at 2:50 PM Laurent Pinchart wrote: > > On Wed, Feb 19, 2020 at 11:20:57AM +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(). > > > > > > 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. > > > > > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/drm_drv.c | 21 +++++---------------- > > > drivers/gpu/drm/drm_mode_config.c | 12 +++++++++++- > > > include/drm/drm_mode_config.h | 2 +- > > > 3 files changed, 17 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > index 3cf40864d4a6..428c569aaaf1 100644 > > > --- a/drivers/gpu/drm/drm_drv.c > > > +++ b/drivers/gpu/drm/drm_drv.c > > > @@ -267,8 +267,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 +277,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 +303,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 +703,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 +714,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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > > index 08e6eff6a179..957db1edba0c 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,8 @@ void drm_mode_config_init(struct drm_device *dev) > > > drm_modeset_acquire_fini(&modeset_ctx); > > > dma_resv_fini(&resv); > > > } > > > + > > > + return drmm_add_action(dev, drm_mode_config_init_release, NULL); > > > > If this fails, shouldn't drm_mode_config_cleanup() be called here ? > > Maybe for ocd reasons, but not for actually cleaning up anything. It's > just a bunch of empty lists that drm_mode_config_cleanup will walk and > do nothing about. Not sure I should add that ... How about the ida init, and the mutex_init() that isn't a no-op when lock debugging is enabled ? > > > } > > > EXPORT_SYMBOL(drm_mode_config_init); > > > > > > 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 Wed, Feb 19, 2020 at 5:08 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > On Wed, Feb 19, 2020 at 04:47:55PM +0100, Daniel Vetter wrote: > > On Wed, Feb 19, 2020 at 2:50 PM Laurent Pinchart wrote: > > > On Wed, Feb 19, 2020 at 11:20:57AM +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(). > > > > > > > > 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. > > > > > > > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > > --- > > > > drivers/gpu/drm/drm_drv.c | 21 +++++---------------- > > > > drivers/gpu/drm/drm_mode_config.c | 12 +++++++++++- > > > > include/drm/drm_mode_config.h | 2 +- > > > > 3 files changed, 17 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > > index 3cf40864d4a6..428c569aaaf1 100644 > > > > --- a/drivers/gpu/drm/drm_drv.c > > > > +++ b/drivers/gpu/drm/drm_drv.c > > > > @@ -267,8 +267,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 +277,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 +303,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 +703,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 +714,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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > > > index 08e6eff6a179..957db1edba0c 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,8 @@ void drm_mode_config_init(struct drm_device *dev) > > > > drm_modeset_acquire_fini(&modeset_ctx); > > > > dma_resv_fini(&resv); > > > > } > > > > + > > > > + return drmm_add_action(dev, drm_mode_config_init_release, NULL); > > > > > > If this fails, shouldn't drm_mode_config_cleanup() be called here ? > > > > Maybe for ocd reasons, but not for actually cleaning up anything. It's > > just a bunch of empty lists that drm_mode_config_cleanup will walk and > > do nothing about. Not sure I should add that ... > > How about the ida init, and the mutex_init() that isn't a no-op when > lock debugging is enabled ? Hm right, I'll fix this. Fun thing is that I've found a pile of missing mutex_destroy and ida_cleanup() while reviewing all the code I've read. Not sure I've fixed them all up ... -Daniel > > > > > } > > > > EXPORT_SYMBOL(drm_mode_config_init); > > > > > > > > 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); > > > > > > -- > Regards, > > Laurent Pinchart
Den 19.02.2020 17.23, skrev Daniel Vetter: > On Wed, Feb 19, 2020 at 5:08 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> >> Hi Daniel, >> >> On Wed, Feb 19, 2020 at 04:47:55PM +0100, Daniel Vetter wrote: >>> On Wed, Feb 19, 2020 at 2:50 PM Laurent Pinchart wrote: >>>> On Wed, Feb 19, 2020 at 11:20:57AM +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(). >>>>> >>>>> 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. >>>>> >>>>> Cc: "Noralf Trønnes" <noralf@tronnes.org> >>>>> Cc: Sam Ravnborg <sam@ravnborg.org> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>>>> --- <snip> >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >>>>> index 08e6eff6a179..957db1edba0c 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,8 @@ void drm_mode_config_init(struct drm_device *dev) >>>>> drm_modeset_acquire_fini(&modeset_ctx); >>>>> dma_resv_fini(&resv); >>>>> } >>>>> + >>>>> + return drmm_add_action(dev, drm_mode_config_init_release, NULL); >>>> >>>> If this fails, shouldn't drm_mode_config_cleanup() be called here ? >>> >>> Maybe for ocd reasons, but not for actually cleaning up anything. It's >>> just a bunch of empty lists that drm_mode_config_cleanup will walk and >>> do nothing about. Not sure I should add that ... >> >> How about the ida init, and the mutex_init() that isn't a no-op when >> lock debugging is enabled ? > > Hm right, I'll fix this. > You could make a drmm_ version of devm_add_action_or_reset() for this. Noralf. > Fun thing is that I've found a pile of missing mutex_destroy and > ida_cleanup() while reviewing all the code I've read. Not sure I've > fixed them all up ... > -Daniel > >> >>>>> } >>>>> EXPORT_SYMBOL(drm_mode_config_init); >>>>>
On Wed, Feb 19, 2020 at 6:30 PM Noralf Trønnes <noralf@tronnes.org> wrote: > > > > Den 19.02.2020 17.23, skrev Daniel Vetter: > > On Wed, Feb 19, 2020 at 5:08 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > >> > >> Hi Daniel, > >> > >> On Wed, Feb 19, 2020 at 04:47:55PM +0100, Daniel Vetter wrote: > >>> On Wed, Feb 19, 2020 at 2:50 PM Laurent Pinchart wrote: > >>>> On Wed, Feb 19, 2020 at 11:20:57AM +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(). > >>>>> > >>>>> 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. > >>>>> > >>>>> Cc: "Noralf Trønnes" <noralf@tronnes.org> > >>>>> Cc: Sam Ravnborg <sam@ravnborg.org> > >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >>>>> --- > > <snip> > > >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > >>>>> index 08e6eff6a179..957db1edba0c 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,8 @@ void drm_mode_config_init(struct drm_device *dev) > >>>>> drm_modeset_acquire_fini(&modeset_ctx); > >>>>> dma_resv_fini(&resv); > >>>>> } > >>>>> + > >>>>> + return drmm_add_action(dev, drm_mode_config_init_release, NULL); > >>>> > >>>> If this fails, shouldn't drm_mode_config_cleanup() be called here ? > >>> > >>> Maybe for ocd reasons, but not for actually cleaning up anything. It's > >>> just a bunch of empty lists that drm_mode_config_cleanup will walk and > >>> do nothing about. Not sure I should add that ... > >> > >> How about the ida init, and the mutex_init() that isn't a no-op when > >> lock debugging is enabled ? > > > > Hm right, I'll fix this. > > > > You could make a drmm_ version of devm_add_action_or_reset() for this. ... How did I not see that in devres.c. In my defense, I've never written a driver using devm because of all the lifetime bugs that would cause in drm, but yes this is exactly what we want here. Thanks a lot for the pointer, I'll go do some typing! Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 3cf40864d4a6..428c569aaaf1 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -267,8 +267,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 +277,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 +303,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 +703,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 +714,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_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 08e6eff6a179..957db1edba0c 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,8 @@ void drm_mode_config_init(struct drm_device *dev) drm_modeset_acquire_fini(&modeset_ctx); dma_resv_fini(&resv); } + + return drmm_add_action(dev, drm_mode_config_init_release, NULL); } EXPORT_SYMBOL(drm_mode_config_init); 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);
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. Cc: "Noralf Trønnes" <noralf@tronnes.org> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_drv.c | 21 +++++---------------- drivers/gpu/drm/drm_mode_config.c | 12 +++++++++++- include/drm/drm_mode_config.h | 2 +- 3 files changed, 17 insertions(+), 18 deletions(-)