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); + void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset) { diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0230762..fec1776 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -513,7 +513,8 @@ struct drm_driver {
void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset); - +int devm_drm_irq_install(struct device *parent, struct drm_device *dev, + int irq); /** * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance * @parent: Parent device object
Hi
Thanks, the code looks good already. There just are a few nits below.
Am 03.11.20 um 03:10 schrieb Tian Tao:
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
The implementation should rather go to drm_irq.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);
void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset) { diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0230762..fec1776 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h
And the declaration should go to drm_irq.h
We generally don't merge unused code, so you should convert at least one KMS driver, say hibmc, to use the new interface.
Best regards Thomas
@@ -513,7 +513,8 @@ struct drm_driver {
void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset);
+int devm_drm_irq_install(struct device *parent, struct drm_device *dev,
int irq);/**
- devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance
- @parent: Parent device object
在 2020/11/3 15:56, Thomas Zimmermann 写道:
Hi
Thanks, the code looks good already. There just are a few nits below.
Thanks for the help with the review code. Add the new api devm_drm_irq_install and himbc use the new interface as one patch or two?
Am 03.11.20 um 03:10 schrieb Tian Tao:
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
The implementation should rather go to drm_irq.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);
- void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset) {
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0230762..fec1776 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h
And the declaration should go to drm_irq.h
We generally don't merge unused code, so you should convert at least one KMS driver, say hibmc, to use the new interface.
Best regards Thomas
@@ -513,7 +513,8 @@ struct drm_driver {
void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset);
+int devm_drm_irq_install(struct device *parent, struct drm_device *dev,
int irq);
- devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance
- @parent: Parent device object
Hi
Am 03.11.20 um 09:57 schrieb tiantao (H):
在 2020/11/3 15:56, Thomas Zimmermann 写道:
Hi
Thanks, the code looks good already. There just are a few nits below.
Thanks for the help with the review code. Add the new api devm_drm_irq_install and himbc use the new interface as one patch or two?
Better make two patches from it.
Best regards Thomas
Am 03.11.20 um 03:10 schrieb Tian Tao:
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
The implementation should rather go to drm_irq.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);
void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset) { diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0230762..fec1776 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h
And the declaration should go to drm_irq.h
We generally don't merge unused code, so you should convert at least one KMS driver, say hibmc, to use the new interface.
Best regards Thomas
@@ -513,7 +513,8 @@ struct drm_driver { void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset);
+int devm_drm_irq_install(struct device *parent, struct drm_device *dev, + int irq); /** * devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance * @parent: Parent device object
Hi Tian.
Good to see more infrastructure support so one does not have to think about cleanup.
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)
devm_drm_irq_uninstall?? It matches other names if you drop the _dev part.
+{
- drm_irq_uninstall(data);
+}
+int devm_drm_irq_install(struct device *parent,
struct drm_device *dev, int irq)
As this is an exported function please add appropriate kernel-doc documentation. It should for example explain that there is no need to uninstall as this is done automagically.
+{
- 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);
The above are in addition to Thomas' feedback.
Sam
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?
Maxime
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?
Best regards Thomas
Maxime
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.
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.
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.
Maxime
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 :-)
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
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. :)
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
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
Hi!
On Tue, Nov 03, 2020 at 11:55:08AM +0100, Daniel Vetter wrote:
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.
drm_dev_put is either called through devm or in remove / unbind, and the drm_device takes a reference on its parent device, so how can the drm_device outlive its parent device?
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 :-)
Well, yeah it's usually a pretty bad situation, but if we can fix it for free it doesn't hurt?
Maxime
On Tue, Nov 03, 2020 at 12:25:22PM +0100, Maxime Ripard wrote:
Hi!
On Tue, Nov 03, 2020 at 11:55:08AM +0100, Daniel Vetter wrote:
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.
drm_dev_put is either called through devm or in remove / unbind, and the drm_device takes a reference on its parent device, so how can the drm_device outlive its parent device?
Oh there's more than just that going on. struct device has 2 lifetime things: - devres resources: These are release on a) on driver unbind b) driver bind failure. Which means if you hotunplug, then devres is gone - the kmalloced piece of memory containing the struct device, refcounted with kref. Totally independent.
So hw resources like irq should be managed with devres. Memory allocations (to prevent use-after-free) should be refcounted by a kref somewhere. In the case of struct device that's done by the driver core. In the case of struct drm_device and all the stuff hanging off that, it's done by drmm_ (ideally at least, since in practice all drivers except i915 get it wrong without drmm_).
Managing memory allocations with devres is almost always a bug.
So when you unbind/hotunplug a device, the following happens: - driver unbind gets called and finishes - devres cleans up hw resources - as one of the last devres action the drm_dev_put gets called - (if no userspace is around or anything else that holds a drm_device reference) drmm_ cleans up drm_device resources - as the last cleanup drmm_ calls put_device() - the actual struct device gets released
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 :-)
Well, yeah it's usually a pretty bad situation, but if we can fix it for free it doesn't hurt?
See my comment somewhere, if the devm_drm_irq_install also holds a drm_dev_get reference, then no matter which wrong way you set stuff up, the right thing should happen. -Daniel
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,
parent argument should not be needed, we have drm_device->dev. If that doesn't work, then don't use the drm irq helpers, they're optional (and there's already devm versions of the core irq functions).
Just a detail aside from all the other things alreay mentioned. -Daniel
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);
void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset) { diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 0230762..fec1776 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -513,7 +513,8 @@ struct drm_driver {
void *__devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver, size_t size, size_t offset);
+int devm_drm_irq_install(struct device *parent, struct drm_device *dev,
int irq);/**
- devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance
- @parent: Parent device object
-- 2.7.4
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maxime Ripard -
Sam Ravnborg -
Thomas Zimmermann -
Tian Tao -
tiantao (H)