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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jan 21, 2019 at 01:21:46PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 21.01.2019 10.55, skrev Daniel Vetter:
> > On Mon, Jan 21, 2019 at 10:10:14AM +0100, Daniel Vetter wrote:
> >> 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>
> >>
> 
> <snip>
> 
> >>> 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);
> > 
> > We need drm_dev_unplug() here, or this isn't safe.
> 
> This function is only used to cover the error path if probe fails before
> devm_drm_dev_register() is called. devm_drm_dev_register_release() is
> the one that calls unplug. There are comments about this in the functions.

I think I get a prize for being ignorant and blind :-/

> 
> > 
> >>> +}
> >>> +
> >>> +/**
> >>> + * 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
> > 
> > I think a bit more clarity here would be good:
> > 
> > "... automatically released on driver unbind by callind drm_dev_unplug()."
> > 
> >>> + * &drm_driver.release callback to control the finalization explicitly.
> > 
> > I think a loud warning for these is in order:
> > 
> > "WARNING:
> > 
> > "In generally it is unsafe to use devm functions for drm structures
> > because the lifetimes of &drm_device and the underlying &device do not
> > match. This here works because it doesn't immediately free anything, but
> > only calls drm_dev_unplug(), which internally decrements the &drm_device
> > refcount through drm_dev_put().
> > 
> > "All other drm structures must still be explicitly released in the
> > &drm_driver.release callback."
> > 
> > While thinking about this I just realized that with this design we have no
> > good place to call drm_atomic_helper_shutdown(). Which we need to, or all
> > kinds of things will leak badly (connectors, fb, ...), but there's no
> > place to call it:
> > - unbind is too early, since we haven't yet called drm_dev_unplug, and the
> >   drm_dev_unregister in there must be called _before_ we start to shut
> >   down anything.
> > - drm_driver.release is way too late.
> > 
> > Ofc for a real hotunplug there's no point in shutting down the hw (it's
> > already gone), but for a driver unload/unbind it would be nice if this
> > happens automatically and in the right order.
> > 
> > So not sure what to do here really.
> 
> How about this change: (it breaks the rule of pulling helpers into the
> core, so maybe we should put the devm_ functions into the simple KMS
> helper instead?)

Yeah smells a bit much like midlayer ... What would work is having a pile
more devm_ helper functions, so that we onion-unwrap everything correctly,
and in the right order. So:

- devm_drm_dev_init (always does a drm_dev_put())

- devm_drm_poll_enable (shuts down the poll helper with a devm action)

- devm_drm_mode_config_reset (does an atomic_helper_shutdown() as it's cleanup action)

- devm_drm_dev_register (grabs an additional drm_dev_get() reference so it
  can call drm_dev_unplug() unconditionally).

We'd need to make sure some of the cleanup actions dtrt when the device is
gone, but I think we can achieve that by liberally sprinkling
drm_dev_enter/exit over them, e.g. the the cleanup action for
drm_mode_config_reset would be:

{
	if (drm_dev_enter())
		return;

	drm_atomic_helper_shutdown();

	drm_dev_exit();
}

This would be analog to your shutdown parameter below.

Essentially I think we can only do the drm_dev_unplug autocleanup in a
given driver from devm_drm_dev_register iff all the cleanup actions have
been devm-ified, and there's nothing left in it's unbind callback. Because
if anything is left in its unbind callback that's a bug, since
drm_dev_unregister() (called through drm_dev_unplug) is the very first (or
at least one of the very first, before we start cleanup up) functions that
need to be called.
-Daniel

> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 12129772be45..7ed9550baff6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -34,7 +34,9 @@
>  #include <linux/slab.h>
>  #include <linux/srcu.h>
> 
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_client.h>
> +#include <drm/drm_crtc_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drmP.h>
> @@ -355,17 +357,7 @@ void drm_dev_exit(int idx)
>  }
>  EXPORT_SYMBOL(drm_dev_exit);
> 
> -/**
> - * drm_dev_unplug - unplug a DRM device
> - * @dev: DRM device
> - *
> - * This unplugs a hotpluggable DRM device, which makes it inaccessible to
> - * userspace operations. Entry-points can use drm_dev_enter() and
> - * drm_dev_exit() to protect device resources in a race free manner. This
> - * essentially unregisters the device like drm_dev_unregister(), but can be
> - * called while there are still open users of @dev.
> - */
> -void drm_dev_unplug(struct drm_device *dev)
> +static void __drm_dev_unplug(struct drm_device *dev, bool shutdown)
>  {
>         /*
>          * After synchronizing any critical read section is guaranteed
> to see
> @@ -378,11 +370,32 @@ void drm_dev_unplug(struct drm_device *dev)
> 
>         drm_dev_unregister(dev);
> 
> +       if (shutdown)
> +               drm_kms_helper_poll_fini(dev);
> +
>         mutex_lock(&drm_global_mutex);
> -       if (dev->open_count == 0)
> +       if (dev->open_count == 0) {
> +               if (shutdown)
> +                       drm_atomic_helper_shutdown(dev);
>                 drm_dev_put(dev);
> +       }
>         mutex_unlock(&drm_global_mutex);
>  }
> +
> +/**
> + * drm_dev_unplug - unplug a DRM device
> + * @dev: DRM device
> + *
> + * This unplugs a hotpluggable DRM device, which makes it inaccessible to
> + * userspace operations. Entry-points can use drm_dev_enter() and
> + * drm_dev_exit() to protect device resources in a race free manner. This
> + * essentially unregisters the device like drm_dev_unregister(), but can be
> + * called while there are still open users of @dev.
> + */
> +void drm_dev_unplug(struct drm_device *dev)
> +{
> +       __drm_dev_unplug(dev, false);
> +}
>  EXPORT_SYMBOL(drm_dev_unplug);
> 
>  /*
> @@ -920,7 +933,7 @@ EXPORT_SYMBOL(devm_drm_dev_init);
> 
>  static void devm_drm_dev_register_release(void *data)
>  {
> -       drm_dev_unplug(data);
> +       __drm_dev_unplug(data, true);
>  }
> 
>  static int devm_drm_dev_register(struct drm_device *dev)
> 
> 
> I realised that we should take a ref on the parent device so it can be
> accessed by the DRM_DEV_ functions after unplug.
> 
> 
> Noralf.
> 
> 
> > 
> >>> + *
> >>> + * 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
> > 

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