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>
>
> 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.
Strike all that, because I wasn't awak yet. Sorry for the confusion.
> > ---
> > 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);
We need drm_dev_unplug() here, or this isn't safe.
> > +}
> > +
> > +/**
> > + * 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.
-Daniel
> > + *
> > + * 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
