[Intel-gfx] [PATCH 26/51] 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 now 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: Drivers can always just register their current release hook with drmm_add_action, but even better they could split them up to simplify the unwinding for the driver load failure case. 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 drmm_add_action_or_reset macro (Noralf) v4: Review from Sam: - drmm_mode_config_init wrapper (also suggested by Thomas) - improve commit message, explain better why ->relase is deprecated v5: - Make drmm_ the main function, with the old one as compat wrapper (Sam) - Add FIXME comments to drm_mode_config_cleanup/init() that drivers shouldn't use these anymore. - Move drmm_add_action_or_reset helper to an earlier patch. Reviewed-by: Sam Ravnborg Cc: Laurent Pinchart Cc: "Noralf Trønnes" Cc: Sam Ravnborg Cc: Thomas Zimmermann Acked-by: Noralf Trønnes Signed-off-by: Daniel Vetter --- Documentation/gpu/drm-kms.rst | 2 +- drivers/gpu/drm/drm_drv.c | 23 +++ drivers/gpu/drm/drm_mode_config.c | 23 --- include/drm/drm_mode_config.h | 18 +- 4 files changed, 45 insertions(+), 21 deletions(-) diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 906771e03103..e1f685015807 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -3,7 +3,7 @@ Kernel Mode Setting (KMS) = Drivers must initialize the mode setting core by calling -drm_mode_config_init() on the DRM device. The function +drmm_mode_config_init() on the DRM device. The function initializes the :c:type:`struct drm_device ` mode_config field and never fails. Once done, mode configuration must be setup by initializing the following fields. diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 03f5cb829957..ea687c5ead15 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); @@ -265,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 * @@ -276,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) @@ -310,7 +303,9 @@ void drm_minor_release(struct drm_minor *minor) * } * drmm_add_final_kfree(drm, priv); * - * drm_mode_config_init(drm); + * ret = drmm_mode_config_init(drm); + * if (ret) + * return ret; * * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL); * if (!priv->userspace_facing) @@ -708,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. @@ -720,9 +714,6 @@ int devm_drm_dev_init(struct device *parent, {
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
Hi Daniel. On Mon, Mar 02, 2020 at 11:26:06PM +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 now 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: Drivers can always just register their current release > hook with drmm_add_action, but even better they could split them up to > simplify the unwinding for the driver load failure case. 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 drmm_add_action_or_reset macro (Noralf) > > v4: Review from Sam: > - drmm_mode_config_init wrapper (also suggested by Thomas) > - improve commit message, explain better why ->relase is deprecated The idea was to rename drm_mode_config_init() to drmm_mode_config_init(). And then provide a wrapper for backward compatibility. - So the kernel-doc documented function in drm_mode_config.c is the recommened choice - And the wrapper in drm_mode_config.h was only for backward compatibility In other words - the wrapper should be an undocumented: static inline int drm_mode_config_init(struct drm_device *dev) { return drmm_mode_config_init(dev); } When all users have transitioned to drmm_mode_config_init() then the wrapper could be dropped. With this fixed, or a good reason not to do so: Reviewed-by: Sam Ravnborg Sam > > Cc: Laurent Pinchart > Cc: "Noralf Trønnes" > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Acked-by: Noralf Trønnes > Signed-off-by: Daniel Vetter > --- > 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 | 19 ++- > 5 files changed, 58 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index c709a0ce018c..a82702d0c2fb 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
[Intel-gfx] [PATCH 26/51] 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 now 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: Drivers can always just register their current release hook with drmm_add_action, but even better they could split them up to simplify the unwinding for the driver load failure case. 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 drmm_add_action_or_reset macro (Noralf) v4: Review from Sam: - drmm_mode_config_init wrapper (also suggested by Thomas) - improve commit message, explain better why ->relase is deprecated Cc: Laurent Pinchart Cc: "Noralf Trønnes" Cc: Sam Ravnborg Cc: Thomas Zimmermann Acked-by: Noralf Trønnes Signed-off-by: Daniel Vetter --- 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 | 19 ++- 5 files changed, 58 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index c709a0ce018c..a82702d0c2fb 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 0883615c2088..8c5f1f03c485 100644 --- a/drivers/gpu/drm/drm_managed.c +++ b/drivers/gpu/drm/drm_managed.c @@ -142,6 +142,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; +} +EXPO
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
On Sat, Feb 29, 2020 at 12:11:28AM +0100, Daniel Vetter wrote: > On Fri, Feb 28, 2020 at 9:26 PM Sam Ravnborg 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
On Fri, Feb 28, 2020 at 9:26 PM Sam Ravnborg wrote: > > Hi Daniel. > > Some bikeshedding in the following. > with or with addressing (IMHO valid points) consider the patch: > > Reviewed-by: Sam Ravnborg > > 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 > > Cc: "Noralf Trønnes" > > Cc: Sam Ravnborg > > Cc: Thomas Zimmermann > > Acked-by: Noralf Trønnes > > Signed-off-by: Daniel Vetter > > --- > > 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
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
Hi Daniel. Some bikeshedding in the following. with or with addressing (IMHO valid points) consider the patch: Reviewed-by: Sam Ravnborg 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 > Cc: "Noralf Trønnes" > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Acked-by: Noralf Trønnes > Signed-off-by: Daniel Vetter > --- > 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_m
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
Hi Am 28.02.20 um 09:43 schrieb Daniel Vetter: > On Fri, Feb 28, 2020 at 8:30 AM Thomas Zimmermann 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 >>> Cc: "Noralf Trønnes" >>> Cc: Sam Ravnborg >>> Cc: Thomas Zimmermann >>> Acked-by: Noralf Trønnes >>> Signed-off-by: Daniel Vetter >>> --- >>> 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, >>> +drmr
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
On Fri, Feb 28, 2020 at 8:30 AM Thomas Zimmermann 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 > > Cc: "Noralf Trønnes" > > Cc: Sam Ravnborg > > Cc: Thomas Zimmermann > > Acked-by: Noralf Trønnes > > Signed-off-by: Daniel Vetter > > --- > > 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, > > +
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
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 > Cc: "Noralf Trønnes" > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Acked-by: Noralf Trønnes > Signed-off-by: Daniel Vetter > --- > 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, >
[Intel-gfx] [PATCH 26/51] 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. 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 Cc: "Noralf Trønnes" Cc: Sam Ravnborg Cc: Thomas Zimmermann Acked-by: Noralf Trønnes Signed-off-by: Daniel Vetter --- 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/g
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
Den 21.02.2020 22.02, skrev 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). > > Cc: Laurent Pinchart > Cc: "Noralf Trønnes" > Cc: Sam Ravnborg > Cc: Thomas Zimmermann > Signed-off-by: Daniel Vetter > --- > 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 > #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(&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..684f884b6cea 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(dev, action, data, #action) Copy-paste error here, you want __drmm_add_action_or_reset(). Apart from that it looks good: Acked-by: Noralf Trønnes > + > +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); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 26/51] 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. 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). Cc: Laurent Pinchart Cc: "Noralf Trønnes" Cc: Sam Ravnborg Cc: Thomas Zimmermann Signed-off-by: Daniel Vetter --- 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 #include #include +#include #in