[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(_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
- * _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,
{
int
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(_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
> - * _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(_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
- * _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;
+}
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(_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).
> > *
> > *
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(_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
> - * _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
> +++
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(_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
>>> - * _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,
>>> +
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(_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
> > - * _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
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(_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
> - * _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(_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
- * _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
+++
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(>mode_config.mutex);
> drm_modeset_lock_init(>mode_config.connection_mutex);
> @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev)
> drm_modeset_acquire_fini(_ctx);
> dma_resv_fini();
> }
> +
> + 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(_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
- * _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
#include
