Re: [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_

From: Sam Ravnborg <sam@ravnborg.org>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	m.felsch@pengutronix.de,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
Date: Fri, 28 Feb 2020 21:26:32 +0100	[thread overview]
Message-ID: <20200228202632.GB22966@ravnborg.org> (raw)
In-Reply-To: <20200227181522.2711142-27-daniel.vetter@ffwll.ch>

Hi Daniel.

Some bikeshedding in the following.
with or with addressing (IMHO valid points) consider the patch:

Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

	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 <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;
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_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);

Bikeshedding - but why oh why prefixing the function with two
underscores?
- It makes it less readable
- It says "internal", at least to me - but it is exported
- It makes the casual reader wonder why, removing focus from other more
  relevant things
- It makes me writing several lines of rant

drmm_add_action_or_reset_named(...) would do the trick.

Same rant above goes for __drmm_add_action()...

> +
>  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);
As this is now a drmm_ managed function it should be named such.
Maybe add a small drm_mode_config_init() wrapper in the header file for
those that has not migrated yet.
It is confusing if we are not consistent in naming and everywhere else
the drm managed functions are named drmm_

>  
> 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);
>  
> -- 
> 2.24.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel