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@xxxxxxxxxxx>
> >>
>
> <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@xxxxxxxxxxxxxxxxxxxxx
> >>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
