Re: [Intel-gfx] [PATCH 27/52] drm: Manage drm_mode_config_init with drmm_
On Wed, Feb 19, 2020 at 6:30 PM Noralf Trønnes wrote: > > > > Den 19.02.2020 17.23, skrev Daniel Vetter: > > On Wed, Feb 19, 2020 at 5:08 PM Laurent Pinchart > > 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" > > Cc: Sam Ravnborg > > Cc: Thomas Zimmermann > > Signed-off-by: Daniel Vetter > > --- > > > > > 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 > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -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(>mode_config.mutex); > > drm_modeset_lock_init(>mode_config.connection_mutex); > > @@ -443,6 +451,8 @@ void drm_mode_config_init(struct drm_device *dev) > > drm_modeset_acquire_fini(_ctx); > > dma_resv_fini(); > > } > > + > > + 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 27/52] drm: Manage drm_mode_config_init with drmm_
Den 19.02.2020 17.23, skrev Daniel Vetter: > On Wed, Feb 19, 2020 at 5:08 PM Laurent Pinchart > 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" > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Signed-off-by: Daniel Vetter > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -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(>mode_config.mutex); > drm_modeset_lock_init(>mode_config.connection_mutex); > @@ -443,6 +451,8 @@ void drm_mode_config_init(struct drm_device *dev) > drm_modeset_acquire_fini(_ctx); > dma_resv_fini(); > } > + > + 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); > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 27/52] drm: Manage drm_mode_config_init with drmm_
On Wed, Feb 19, 2020 at 5:08 PM Laurent Pinchart 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" > > > > Cc: Sam Ravnborg > > > > Cc: Thomas Zimmermann > > > > Signed-off-by: Daniel Vetter > > > > --- > > > > 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 > > > > - * _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 > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -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 > > > >
Re: [Intel-gfx] [PATCH 27/52] drm: Manage drm_mode_config_init with drmm_
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" > > > Cc: Sam Ravnborg > > > Cc: Thomas Zimmermann > > > Signed-off-by: Daniel Vetter > > > --- > > > 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 > > > - * _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 > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -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 > > >
Re: [Intel-gfx] [PATCH 27/52] drm: Manage drm_mode_config_init with drmm_
On Wed, Feb 19, 2020 at 2:50 PM Laurent Pinchart 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" > > Cc: Sam Ravnborg > > Cc: Thomas Zimmermann > > Signed-off-by: Daniel Vetter > > --- > > 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 > > - * _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 > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -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(>mode_config.mutex); > > drm_modeset_lock_init(>mode_config.connection_mutex); > > @@ -443,6 +451,8 @@ void
Re: [Intel-gfx] [PATCH 27/52] drm: Manage drm_mode_config_init with drmm_
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" > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Signed-off-by: Daniel Vetter > --- > 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 > - * _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 > #include > #include > +#include > #include > #include > #include > @@ -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(>mode_config.mutex); > drm_modeset_lock_init(>mode_config.connection_mutex); > @@ -443,6 +451,8 @@ void drm_mode_config_init(struct drm_device *dev) > drm_modeset_acquire_fini(_ctx); > dma_resv_fini(); > } > + > + 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
[Intel-gfx] [PATCH 27/52] drm: Manage drm_mode_config_init with drmm_
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" Cc: Sam Ravnborg Cc: Thomas Zimmermann Signed-off-by: Daniel Vetter --- 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 - * _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 #include #include +#include #include #include #include @@ -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(>mode_config.mutex); drm_modeset_lock_init(>mode_config.connection_mutex); @@ -443,6 +451,8 @@ void drm_mode_config_init(struct drm_device *dev) drm_modeset_acquire_fini(_ctx); dma_resv_fini(); } + + 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