Re: [PATCH 01/11] drm: Add devm_drm_dev_init/register

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Jan 20, 2019 at 12:43:08PM +0100, Noralf Trønnes wrote:
> This adds resource managed (devres) versions of drm_dev_init() and
> drm_dev_register().
> 
> Also added is devm_drm_dev_register_with_fbdev() which sets up generic
> fbdev emulation as well.
> 
> devm_drm_dev_register() isn't exported since there are no users.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  Documentation/driver-model/devres.txt |   4 +
>  drivers/gpu/drm/drm_drv.c             | 106 ++++++++++++++++++++++++++
>  include/drm/drm_drv.h                 |   6 ++
>  3 files changed, 116 insertions(+)
> 
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index b277cafce71e..6eebc28d4c21 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -254,6 +254,10 @@ DMA
>    dmam_pool_create()
>    dmam_pool_destroy()
>  
> +DRM
> +  devm_drm_dev_init()
> +  devm_drm_dev_register_with_fbdev()
> +
>  GPIO
>    devm_gpiod_get()
>    devm_gpiod_get_index()
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 381581b01d48..12129772be45 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -36,6 +36,7 @@
>  
>  #include <drm/drm_client.h>
>  #include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
>  #include <drm/drmP.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -871,6 +872,111 @@ void drm_dev_unregister(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_dev_unregister);
>  
> +static void devm_drm_dev_init_release(void *data)
> +{
> +	drm_dev_put(data);
> +}
> +
> +/**
> + * devm_drm_dev_init - Resource managed drm_dev_init()
> + * @parent: Parent device object
> + * @dev: DRM device
> + * @driver: DRM driver
> + *
> + * Managed drm_dev_init(). The DRM device initialized with this function is
> + * automatically released on driver detach. You must supply a
> + * &drm_driver.release callback to control the finalization explicitly.
> + *
> + * Note: This function must be used together with
> + * devm_drm_dev_register_with_fbdev().
> + *
> + * RETURNS:
> + * 0 on success, or error code on failure.
> + */
> +int devm_drm_dev_init(struct device *parent,
> +		      struct drm_device *dev,
> +		      struct drm_driver *driver)
> +{
> +	int ret;
> +
> +	if (WARN_ON(!parent || !driver->release))
> +		return -EINVAL;
> +
> +	ret = drm_dev_init(dev, driver, parent);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * This is a temporary release action that is used if probing fails
> +	 * before devm_drm_dev_register() is called.
> +	 */
> +	ret = devm_add_action(parent, devm_drm_dev_init_release, dev);
> +	if (ret)
> +		devm_drm_dev_init_release(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(devm_drm_dev_init);
> +
> +static void devm_drm_dev_register_release(void *data)
> +{
> +	drm_dev_unplug(data);
> +}
> +
> +static int devm_drm_dev_register(struct drm_device *dev)
> +{
> +	int ret;
> +
> +	ret = drm_dev_register(dev, 0);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * This has now served it's purpose, remove it to not mess up ref
> +	 * counting.
> +	 */
> +	devm_remove_action(dev->dev, devm_drm_dev_init_release, dev);
> +
> +	ret = devm_add_action(dev->dev, devm_drm_dev_register_release, dev);

If this fails I think the cleanup would go wrong. I think simpler if you
never remove the devm action from dev_init, and just grab an additional
drm_dev_get() reference here for drm_dev_unplug. We also need that for
correct onion cleanup in, so that for a normal unbind/driver unload we can
still do:

1. drm_dev_unregister() through drm_dev_unplug()

2. drm_atomic_helper_shutdown() and similar things (needs the drm_device
to still be around)

3. drm_dev_put()

I think that'll give us the cleaner onion unwrapping in the
unload/hotunplug/error case cleanup cases.

Also, this allows drivers to start using devm_drm_dev_init without having
to use devm_drm_dev_register(), which they have to do if they still have
non-devm-ified things in step 2 above in there unbind callback.
-Daniel

> +	if (ret)
> +		devm_drm_dev_register_release(dev);
> +
> +	return ret;
> +}
> +
> +/**
> + * devm_drm_dev_register_with_fbdev - Resource managed drm_dev_register()
> + *                                    including generic fbdev emulation
> + * @dev: DRM device to register
> + * @fbdev_bpp: Preferred bits per pixel for fbdev (optional)
> + *
> + * Managed drm_dev_register() that also calls drm_fbdev_generic_setup().
> + * The DRM device registered with this function is automatically unregistered on
> + * driver detach using drm_dev_unplug().
> + *
> + * Note: This function must be used together with devm_drm_dev_init().
> + *
> + * For testing driver detach can be triggered manually by writing to the driver
> + * 'unbind' file.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on failure.
> + */
> +int devm_drm_dev_register_with_fbdev(struct drm_device *dev,
> +				     unsigned int fbdev_bpp)
> +{
> +	int ret;
> +
> +	ret = devm_drm_dev_register(dev);
> +	if (ret)
> +		return ret;
> +
> +	drm_fbdev_generic_setup(dev, fbdev_bpp);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(devm_drm_dev_register_with_fbdev);
> +
>  /**
>   * drm_dev_set_unique - Set the unique name of a DRM device
>   * @dev: device of which to set the unique name
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 35af23f5fa0d..c3f0477f2e7f 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -628,6 +628,12 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>  int drm_dev_register(struct drm_device *dev, unsigned long flags);
>  void drm_dev_unregister(struct drm_device *dev);
>  
> +int devm_drm_dev_init(struct device *parent,
> +		      struct drm_device *dev,
> +		      struct drm_driver *driver);
> +int devm_drm_dev_register_with_fbdev(struct drm_device *dev,
> +				     unsigned int fbdev_bpp);
> +
>  void drm_dev_get(struct drm_device *dev);
>  void drm_dev_put(struct drm_device *dev);
>  void drm_put_dev(struct drm_device *dev);
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel