Hi

Am 28.02.20 um 09:43 schrieb Daniel Vetter:
> On Fri, Feb 28, 2020 at 8:30 AM Thomas Zimmermann <tzimmermann@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@ideasonboard.com>
>>> Cc: "Noralf Trønnes" <noralf@tronnes.org>
>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Acked-by: Noralf Trønnes <noralf@tronnes.org>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@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