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>

devm_ considered harmful unfortunately. You cannot allocate any structures
which might outlive the lifetime of your device using devm_ on the device.
Since drm_device has it's own lifetime, that structure and _everything_ we
allocate tied to it (i.e. drm_encoder/plane/connector/crtc/whatever)
cannot be allocated using devm_ infrastructure tied to the underlying
parent device.

Yes this means almost all the devm usage in drm drivers is fundamentally
broken. You also generally don't unplug gpus, so no one cares :-/

What could instead is add a struct device to drm_device somehow, use that
struct device to manage the lifetime of the drm_device (would still need
our special open counter maybe), and then use devm to allocate all the drm
bits tied to the drm_device.

This here unfortunataly doesn't work, even if it looks like a really neat
idea.
-Daniel


> ---
>  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 (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