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.
>
>>> +}
>>> +
>>> +/**
>>> + * 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?)
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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
