[PATCH v2] drm: Add the new api to install irq

[PATCH v2] drm: Add the new api to install irq

[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
@@ -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
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2655 bytes --]

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
> 

-- 
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

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 4259 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[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?

> 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
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 3369 bytes --]

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
>>>
>>
> 

-- 
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

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 4259 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Sam Ravnborg]

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

Re: [PATCH v2] drm: Add the new api to install irq

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1598 bytes --]

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
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2086 bytes --]

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
>>

-- 
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

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 4259 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Daniel Vetter]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3409 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[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 :-)

> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Thomas Zimmermann]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 4732 bytes --]

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
> 

-- 
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

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 4259 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3907 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Daniel Vetter]

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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm: Add the new api to install irq

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel