[Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
Thomas Zimmermann
tzimmermann at suse.de
Fri Feb 28 09:56:12 UTC 2020
Hi
Am 28.02.20 um 09:43 schrieb Daniel Vetter:
> On Fri, Feb 28, 2020 at 8:30 AM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>>
>> Hi Daniel
>>
>> Am 27.02.20 um 19:14 schrieb Daniel Vetter:
>>> drm_mode_config_cleanup is idempotent, so no harm in calling this
>>> twice. This allows us to gradually switch drivers over by removing
>>> explicit drm_mode_config_cleanup calls.
>>>
>>> With this step it's not also possible that (at least for simple
>>> drivers) automatic resource cleanup can be done correctly without a
>>> drm_driver->release hook. Therefore allow this now in
>>> devm_drm_dev_init().
>>>
>>> Also with drmm_ explicit drm_driver->release hooks are kinda not the
>>> best option, so deprecate that hook to discourage future users.
>>>
>>> v2: Fixup the example in the kerneldoc too.
>>>
>>> v3:
>>> - For paranoia, double check that minor->dev == dev in the release
>>> hook, because I botched the pointer math in the drmm library.
>>> - Call drm_mode_config_cleanup when drmm_add_action fails, we'd be
>>> missing some mutex_destroy and ida_cleanup otherwise (Laurent)
>>>
>>> v4: Add a drmm_add_action_or_reset (like devm_ has) to encapsulate this
>>> pattern (Noralf).
>>>
>>> v5: Fix oversight in the new add_action_or_reset macro (Noralf)
>>>
>>> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> Cc: "Noralf Trønnes" <noralf at tronnes.org>
>>> Cc: Sam Ravnborg <sam at ravnborg.org>
>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>> Acked-by: Noralf Trønnes <noralf at tronnes.org>
>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>>> ---
>>> drivers/gpu/drm/drm_drv.c | 23 +++++++----------------
>>> drivers/gpu/drm/drm_managed.c | 14 ++++++++++++++
>>> drivers/gpu/drm/drm_mode_config.c | 13 ++++++++++++-
>>> include/drm/drm_managed.h | 7 +++++++
>>> include/drm/drm_mode_config.h | 2 +-
>>> 5 files changed, 41 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 3cf40864d4a6..bb326b9bcde0 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -98,6 +98,8 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data)
>>> struct drm_minor *minor = data;
>>> unsigned long flags;
>>>
>>> + WARN_ON(dev != minor->dev);
>>> +
>>> put_device(minor->kdev);
>>>
>>> spin_lock_irqsave(&drm_minor_lock, flags);
>>> @@ -267,8 +269,7 @@ void drm_minor_release(struct drm_minor *minor)
>>> *
>>> * The following example shows a typical structure of a DRM display driver.
>>> * The example focus on the probe() function and the other functions that is
>>> - * almost always present and serves as a demonstration of devm_drm_dev_init()
>>> - * usage with its accompanying drm_driver->release callback.
>>> + * almost always present and serves as a demonstration of devm_drm_dev_init().
>>> *
>>> * .. code-block:: c
>>> *
>>> @@ -278,16 +279,8 @@ void drm_minor_release(struct drm_minor *minor)
>>> * struct clk *pclk;
>>> * };
>>> *
>>> - * static void driver_drm_release(struct drm_device *drm)
>>> - * {
>>> - * struct driver_device *priv = container_of(...);
>>> - *
>>> - * drm_mode_config_cleanup(drm);
>>> - * }
>>> - *
>>> * static struct drm_driver driver_drm_driver = {
>>> * [...]
>>> - * .release = driver_drm_release,
>>> * };
>>> *
>>> * static int driver_probe(struct platform_device *pdev)
>>> @@ -312,7 +305,9 @@ void drm_minor_release(struct drm_minor *minor)
>>> * }
>>> * drmm_add_final_kfree(drm, priv);
>>> *
>>> - * drm_mode_config_init(drm);
>>> + * ret = drm_mode_config_init(drm);
>>> + * if (ret)
>>> + * return ret;
>>> *
>>> * priv->userspace_facing = drmm_kzalloc(..., GFP_KERNEL);
>>> * if (!priv->userspace_facing)
>>> @@ -710,8 +705,7 @@ static void devm_drm_dev_init_release(void *data)
>>> * @driver: DRM driver
>>> *
>>> * Managed drm_dev_init(). The DRM device initialized with this function is
>>> - * automatically put on driver detach using drm_dev_put(). You must supply a
>>> - * &drm_driver.release callback to control the finalization explicitly.
>>> + * automatically put on driver detach using drm_dev_put().
>>> *
>>> * RETURNS:
>>> * 0 on success, or error code on failure.
>>> @@ -722,9 +716,6 @@ int devm_drm_dev_init(struct device *parent,
>>> {
>>> int ret;
>>>
>>> - if (WARN_ON(!driver->release))
>>> - return -EINVAL;
>>> -
>>> ret = drm_dev_init(dev, driver, parent);
>>> if (ret)
>>> return ret;
>>> diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
>>> index 626656369f0b..6376be01bbc8 100644
>>> --- a/drivers/gpu/drm/drm_managed.c
>>> +++ b/drivers/gpu/drm/drm_managed.c
>>> @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev,
>>> }
>>> EXPORT_SYMBOL(__drmm_add_action);
>>>
>>> +int __drmm_add_action_or_reset(struct drm_device *dev,
>>> + drmres_release_t action,
>>> + void *data, const char *name)
>>> +{
>>> + int ret;
>>> +
>>> + ret = __drmm_add_action(dev, action, data, name);
>>> + if (ret)
>>> + action(dev, data);
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL(__drmm_add_action_or_reset);
>>> +
>>> void drmm_remove_action(struct drm_device *dev,
>>> drmres_release_t action,
>>> void *data)
>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
>>> index 08e6eff6a179..6f7005bc597f 100644
>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>> @@ -25,6 +25,7 @@
>>> #include <drm/drm_drv.h>
>>> #include <drm/drm_encoder.h>
>>> #include <drm/drm_file.h>
>>> +#include <drm/drm_managed.h>
>>> #include <drm/drm_mode_config.h>
>>> #include <drm/drm_print.h>
>>> #include <linux/dma-resv.h>
>>> @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>>> return 0;
>>> }
>>>
>>> +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr)
>>> +{
>>> + drm_mode_config_cleanup(dev);
>>> +}
>>> +
>>> /**
>>> * drm_mode_config_init - initialize DRM mode_configuration structure
>>> * @dev: DRM device
>>> @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
>>> * problem, since this should happen single threaded at init time. It is the
>>> * driver's problem to ensure this guarantee.
>>> *
>>> + * Cleanup is automatically handled through registering drm_mode_config_cleanup
>>> + * with drmm_add_action().
>>> */
>>> -void drm_mode_config_init(struct drm_device *dev)
>>> +int drm_mode_config_init(struct drm_device *dev)
>>> {
>>> mutex_init(&dev->mode_config.mutex);
>>> drm_modeset_lock_init(&dev->mode_config.connection_mutex);
>>> @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev)
>>> drm_modeset_acquire_fini(&modeset_ctx);
>>> dma_resv_fini(&resv);
>>> }
>>> +
>>> + return drmm_add_action_or_reset(dev, drm_mode_config_init_release,
>>> + NULL);
>>> }
>>> EXPORT_SYMBOL(drm_mode_config_init);
>>
>> It might be a bit late in the review cycle, but could this function be
>> renamed to drmm_mode_config_init() to reflect the garbage-collected
>> implementation?
>
> I think that makes sense, at least as a wrapper with a __must_check
> annotation (and keeping the old one). I tried to avoid the rename
> because it'd be a flag day patch.
>
> If you think this would be a great improvement then I can do that -
> I'm positive enough to bother with the rebase to touch up all the
> driver patches, but only if there's someone else who's enthusiastic
> about it :-)
When reading the patches, I thought that it's now non-obvious why some
drivers cleanup drm_mode_config_init() manually and some don't. So you
may not need a big flag-day patch. A simple wrapper with the drmm_
prefix should do. At some point the old interface will hopefully be gone.
Best regards
Thomas
> -Daniel
>
>> Best regards
>> Thomas
>>
>>>
>>> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
>>> index 2b1ba2ad5582..1e6291407586 100644
>>> --- a/include/drm/drm_managed.h
>>> +++ b/include/drm/drm_managed.h
>>> @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev,
>>> drmres_release_t action,
>>> void *data, const char *name);
>>>
>>> +#define drmm_add_action_or_reset(dev, action, data) \
>>> + __drmm_add_action_or_reset(dev, action, data, #action)
>>> +
>>> +int __must_check __drmm_add_action_or_reset(struct drm_device *dev,
>>> + drmres_release_t action,
>>> + void *data, const char *name);
>>> +
>>> void drmm_remove_action(struct drm_device *dev,
>>> drmres_release_t action,
>>> void *data);
>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>>> index 3bcbe30339f0..160a3e4b51c3 100644
>>> --- a/include/drm/drm_mode_config.h
>>> +++ b/include/drm/drm_mode_config.h
>>> @@ -929,7 +929,7 @@ struct drm_mode_config {
>>> const struct drm_mode_config_helper_funcs *helper_private;
>>> };
>>>
>>> -void drm_mode_config_init(struct drm_device *dev);
>>> +int drm_mode_config_init(struct drm_device *dev);
>>> void drm_mode_config_reset(struct drm_device *dev);
>>> void drm_mode_config_cleanup(struct drm_device *dev);
>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20200228/963acd13/attachment-0001.sig>
More information about the Intel-gfx
mailing list