On Tue, Nov 03, 2020 at 12:25:06PM +0100, Thomas Zimmermann wrote:
Hi
Am 03.11.20 um 11:55 schrieb Daniel Vetter:
On Tue, Nov 03, 2020 at 11:38:32AM +0100, Maxime Ripard wrote:
On Tue, Nov 03, 2020 at 11:10:27AM +0100, Thomas Zimmermann wrote:
Hi
Am 03.11.20 um 10:52 schrieb Maxime Ripard:
On Tue, Nov 03, 2020 at 10:10:41AM +0800, Tian Tao wrote:
Add new api devm_drm_irq_install() to register interrupts, no need to call drm_irq_uninstall() when the drm module is removed.
v2: fixed the wrong parameter.
Signed-off-by: Tian Tao tiantao6@hisilicon.com
drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ include/drm/drm_drv.h | 3 ++- 2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index cd162d4..0fe5243 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -39,6 +39,7 @@ #include <drm/drm_color_mgmt.h> #include <drm/drm_drv.h> #include <drm/drm_file.h> +#include <drm/drm_irq.h> #include <drm/drm_managed.h> #include <drm/drm_mode_object.h> #include <drm/drm_print.h> @@ -678,6 +679,28 @@ static int devm_drm_dev_init(struct device *parent, return ret; }
+static void devm_drm_dev_irq_uninstall(void *data) +{
- drm_irq_uninstall(data);
+}
+int devm_drm_irq_install(struct device *parent,
struct drm_device *dev, int irq)+{
- int ret;
- ret = drm_irq_install(dev, irq);
- if (ret)
return ret;- ret = devm_add_action(parent, devm_drm_dev_irq_uninstall, dev);
- if (ret)
devm_drm_dev_irq_uninstall(dev);- return ret;
+} +EXPORT_SYMBOL(devm_drm_irq_install);
Shouldn't we tie the IRQ to the drm device (so with drmm_add_action) instead of tying it to the underlying device?
If the HW device goes away, there won't be any more interrupts. So it's similar to devm_ functions for I/O memory. Why would you use the drmm_ interface?
drm_irq_install/uninstall do more that just calling request_irq and free_irq though, they will also run (among other things) the irq-related hooks in the drm driver (irq_preinstall, irq_postinstall irq_uninstall) and wake anything waiting for a vblank to occur, so we need the DRM device and driver to still be around when we run drm_irq_uninstall. That's why (I assume) you have to pass the drm_device as an argument and not simply the device.
drm_device is guaranteed to outlive devm_, plus the hooks are meant to shut down hw. hw isn't around anymore when we do drmm_ cleanup, at least not in full generality.
This probably works in most case since you would allocate the drm_device with devm_drm_dev_alloc, and then run drm_irq_install, so in the undoing phase you would have first drm_irq_uninstall to run, and everything is fine.
However, if one doesn't use devm_drm_dev_alloc but would use devm_drm_irq_install, you would have first remove being called that would free the drm device, and then drm_irq_uninstall which will use a free'd pointer.
Don't do that, it's broken :-)
Umm, I just saw that hibmc doesn't use devm_drm_dev_alloc. So maybe we have to convert it first before using the managed irq function. OTOH converting it is a good idea in any case, so why not. :)
Yeah that doesn't work as-is.
I guess the second option would be if devm_drm_dev_irqinstall would take a drm_dev_get() reference of its own. Not sure that's a good idea, but it would make the thing a bit more flexible. -Daniel
Best regards Thomas
So yeah, even though the interrupt line itself is tied to the device, all the logic we have around the interrupt that is dealt with in drm_irq_install is really tied to the drm_device. And since we tie the life of drm_device to its underlying device already (either implicitly through devm_drm_dev_alloc, or explictly through manual allocation in probe and free in remove) we can't end up in a situation where the drm_device outlives its device.
Most drivers get their drm_device lifetime completely wrong. That's why I typed drmm_ stuff. So this isn't a surprise at all, but it might motiveate a bunch more people to fix this up correctly.
Also, these drm_irq functions are 100% optional helpers (should maybe rename them to make this clearer) for modern drivers. They're only built in for DRIVER_LEGACY, because hysterical raisins. -Daniel
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel