[PATCH] drm/panfrost: Prefix interrupt handlers' names

[PATCH] drm/panfrost: Prefix interrupt handlers' names

[Ezequiel Garcia]

Currently, the interrupt lines requested by Panfrost
use ambiguous names, which adds some obscurity
to interrupt introspection (i.e. any tool based
on procfs' interrupts file).

In order to improve this, prefix each requested
interrupt with either the module name or the device
name, where possible.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +-
 drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++++--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index f67ed925c0ef..0355c4a78eaa 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -348,7 +348,7 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
 		return -ENODEV;
 
 	err = devm_request_irq(pfdev->dev, irq, panfrost_gpu_irq_handler,
-			       IRQF_SHARED, "gpu", pfdev);
+			       IRQF_SHARED, dev_name(pfdev->dev), pfdev);
 	if (err) {
 		dev_err(pfdev->dev, "failed to request gpu irq");
 		return err;
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 05c85f45a0de..3bd79ebb6c40 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -480,7 +480,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
 		return -ENODEV;
 
 	ret = devm_request_irq(pfdev->dev, irq, panfrost_job_irq_handler,
-			       IRQF_SHARED, "job", pfdev);
+			       IRQF_SHARED, KBUILD_MODNAME "-job", pfdev);
 	if (ret) {
 		dev_err(pfdev->dev, "failed to request job irq");
 		return ret;
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 842bdd7cf6be..806958434726 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -612,9 +612,11 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
 	if (irq <= 0)
 		return -ENODEV;
 
-	err = devm_request_threaded_irq(pfdev->dev, irq, panfrost_mmu_irq_handler,
+	err = devm_request_threaded_irq(pfdev->dev, irq,
+					panfrost_mmu_irq_handler,
 					panfrost_mmu_irq_handler_thread,
-					IRQF_SHARED, "mmu", pfdev);
+					IRQF_SHARED, KBUILD_MODNAME "-mmu",
+					pfdev);
 
 	if (err) {
 		dev_err(pfdev->dev, "failed to request mmu irq");
-- 
2.22.0

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

Re: [PATCH] drm/panfrost: Prefix interrupt handlers' names

[Neil Armstrong]

Hi,

On 13/12/2019 13:39, Ezequiel Garcia wrote:
> Currently, the interrupt lines requested by Panfrost
> use ambiguous names, which adds some obscurity
> to interrupt introspection (i.e. any tool based
> on procfs' interrupts file).
> 
> In order to improve this, prefix each requested
> interrupt with either the module name or the device
> name, where possible.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++++--
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index f67ed925c0ef..0355c4a78eaa 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -348,7 +348,7 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
>  		return -ENODEV;
>  
>  	err = devm_request_irq(pfdev->dev, irq, panfrost_gpu_irq_handler,
> -			       IRQF_SHARED, "gpu", pfdev);
> +			       IRQF_SHARED, dev_name(pfdev->dev), pfdev);
>  	if (err) {
>  		dev_err(pfdev->dev, "failed to request gpu irq");
>  		return err;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 05c85f45a0de..3bd79ebb6c40 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -480,7 +480,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>  		return -ENODEV;
>  
>  	ret = devm_request_irq(pfdev->dev, irq, panfrost_job_irq_handler,
> -			       IRQF_SHARED, "job", pfdev);
> +			       IRQF_SHARED, KBUILD_MODNAME "-job", pfdev);
>  	if (ret) {
>  		dev_err(pfdev->dev, "failed to request job irq");
>  		return ret;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index 842bdd7cf6be..806958434726 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -612,9 +612,11 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>  	if (irq <= 0)
>  		return -ENODEV;
>  
> -	err = devm_request_threaded_irq(pfdev->dev, irq, panfrost_mmu_irq_handler,
> +	err = devm_request_threaded_irq(pfdev->dev, irq,
> +					panfrost_mmu_irq_handler,
>  					panfrost_mmu_irq_handler_thread,
> -					IRQF_SHARED, "mmu", pfdev);
> +					IRQF_SHARED, KBUILD_MODNAME "-mmu",
> +					pfdev);
>  
>  	if (err) {
>  		dev_err(pfdev->dev, "failed to request mmu irq");
> 

Why don't you use dev_name(pfdev->dev) everywhere ?

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

Re: [PATCH] drm/panfrost: Prefix interrupt handlers' names

[Robin Murphy]

On 13/12/2019 1:18 pm, Neil Armstrong wrote:
> Hi,
> 
> On 13/12/2019 13:39, Ezequiel Garcia wrote:
>> Currently, the interrupt lines requested by Panfrost
>> use ambiguous names, which adds some obscurity
>> to interrupt introspection (i.e. any tool based
>> on procfs' interrupts file).
>>
>> In order to improve this, prefix each requested
>> interrupt with either the module name or the device
>> name, where possible.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +-
>>   drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
>>   drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++++--
>>   3 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> index f67ed925c0ef..0355c4a78eaa 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> @@ -348,7 +348,7 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
>>   		return -ENODEV;
>>   
>>   	err = devm_request_irq(pfdev->dev, irq, panfrost_gpu_irq_handler,
>> -			       IRQF_SHARED, "gpu", pfdev);
>> +			       IRQF_SHARED, dev_name(pfdev->dev), pfdev);
>>   	if (err) {
>>   		dev_err(pfdev->dev, "failed to request gpu irq");
>>   		return err;
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
>> index 05c85f45a0de..3bd79ebb6c40 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
>> @@ -480,7 +480,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>>   		return -ENODEV;
>>   
>>   	ret = devm_request_irq(pfdev->dev, irq, panfrost_job_irq_handler,
>> -			       IRQF_SHARED, "job", pfdev);
>> +			       IRQF_SHARED, KBUILD_MODNAME "-job", pfdev);
>>   	if (ret) {
>>   		dev_err(pfdev->dev, "failed to request job irq");
>>   		return ret;
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> index 842bdd7cf6be..806958434726 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> @@ -612,9 +612,11 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
>>   	if (irq <= 0)
>>   		return -ENODEV;
>>   
>> -	err = devm_request_threaded_irq(pfdev->dev, irq, panfrost_mmu_irq_handler,
>> +	err = devm_request_threaded_irq(pfdev->dev, irq,
>> +					panfrost_mmu_irq_handler,
>>   					panfrost_mmu_irq_handler_thread,
>> -					IRQF_SHARED, "mmu", pfdev);
>> +					IRQF_SHARED, KBUILD_MODNAME "-mmu",
>> +					pfdev);
>>   
>>   	if (err) {
>>   		dev_err(pfdev->dev, "failed to request mmu irq");
>>
> 
> Why don't you use dev_name(pfdev->dev) everywhere ?

Agreed, while the current implementation may be confusing it is at least 
self-consistent. TBH it would probably be sufficient to save the bother 
of allocating strings and just settle on "panfrost-{gpu,job,mmu}", since 
upstream users are unlikely to ever come across a system with more than 
one Mali in it ;)

And FWIW note that "GPU" really is the specific hardware name of that 
IRQ output; it's not just a generic fill-in for "the one that isn't the 
Job or MMU IRQ".

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

Re: [PATCH] drm/panfrost: Prefix interrupt handlers' names

[Alyssa Rosenzweig]

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

> TBH it would probably be sufficient to save the bother of allocating
> strings and just settle on "panfrost-{gpu,job,mmu}", since upstream
> users are unlikely to ever come across a system with more than one
> Mali in it ;)

Agreed.

----Wait, you said *upstream*? Are there .... oh no

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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] drm/panfrost: Prefix interrupt handlers' names

[Robin Murphy]

On 13/12/2019 2:32 pm, Alyssa Rosenzweig wrote:
>> TBH it would probably be sufficient to save the bother of allocating
>> strings and just settle on "panfrost-{gpu,job,mmu}", since upstream
>> users are unlikely to ever come across a system with more than one
>> Mali in it ;)
> 
> Agreed.
> 
> ----Wait, you said *upstream*? Are there .... oh no

Heh, fear not - I'm only thinking of the "Juno board with FPGA 
prototyping stack" setup, and the people using those in anger are all 
working on some other driver anyway ;)

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

Re: [PATCH] drm/panfrost: Prefix interrupt handlers' names

[Ezequiel Garcia]

Hey everyone,

Thanks for the quick comments.

(Feedback for kernel patches on the same day, am I dreaming??)

On Fri, 2019-12-13 at 13:46 +0000, Robin Murphy wrote:
> On 13/12/2019 1:18 pm, Neil Armstrong wrote:
> > Hi,
> > 
> > On 13/12/2019 13:39, Ezequiel Garcia wrote:
> > > Currently, the interrupt lines requested by Panfrost
> > > use ambiguous names, which adds some obscurity
> > > to interrupt introspection (i.e. any tool based
> > > on procfs' interrupts file).
> > > 
> > > In order to improve this, prefix each requested
> > > interrupt with either the module name or the device
> > > name, where possible.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > >   drivers/gpu/drm/panfrost/panfrost_gpu.c | 2 +-
> > >   drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> > >   drivers/gpu/drm/panfrost/panfrost_mmu.c | 6 ++++--
> > >   3 files changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> > > index f67ed925c0ef..0355c4a78eaa 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> > > @@ -348,7 +348,7 @@ int panfrost_gpu_init(struct panfrost_device *pfdev)
> > >   		return -ENODEV;
> > >   
> > >   	err = devm_request_irq(pfdev->dev, irq, panfrost_gpu_irq_handler,
> > > -			       IRQF_SHARED, "gpu", pfdev);
> > > +			       IRQF_SHARED, dev_name(pfdev->dev), pfdev);
> > >   	if (err) {
> > >   		dev_err(pfdev->dev, "failed to request gpu irq");
> > >   		return err;
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> > > index 05c85f45a0de..3bd79ebb6c40 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> > > @@ -480,7 +480,7 @@ int panfrost_job_init(struct panfrost_device *pfdev)
> > >   		return -ENODEV;
> > >   
> > >   	ret = devm_request_irq(pfdev->dev, irq, panfrost_job_irq_handler,
> > > -			       IRQF_SHARED, "job", pfdev);
> > > +			       IRQF_SHARED, KBUILD_MODNAME "-job", pfdev);
> > >   	if (ret) {
> > >   		dev_err(pfdev->dev, "failed to request job irq");
> > >   		return ret;
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > > index 842bdd7cf6be..806958434726 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > > @@ -612,9 +612,11 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
> > >   	if (irq <= 0)
> > >   		return -ENODEV;
> > >   
> > > -	err = devm_request_threaded_irq(pfdev->dev, irq, panfrost_mmu_irq_handler,
> > > +	err = devm_request_threaded_irq(pfdev->dev, irq,
> > > +					panfrost_mmu_irq_handler,
> > >   					panfrost_mmu_irq_handler_thread,
> > > -					IRQF_SHARED, "mmu", pfdev);
> > > +					IRQF_SHARED, KBUILD_MODNAME "-mmu",
> > > +					pfdev);
> > >   
> > >   	if (err) {
> > >   		dev_err(pfdev->dev, "failed to request mmu irq");
> > > 
> > 
> > Why don't you use dev_name(pfdev->dev) everywhere ?
> 
> Agreed, while the current implementation may be confusing it is at least 
> self-consistent. TBH it would probably be sufficient to save the bother 
> of allocating strings and just settle on "panfrost-{gpu,job,mmu}", since 
> upstream users are unlikely to ever come across a system with more than 
> one Mali in it ;)
> 
> And FWIW note that "GPU" really is the specific hardware name of that 
> IRQ output; it's not just a generic fill-in for "the one that isn't the 
> Job or MMU IRQ".
> 

Yeah, that makese sense. So how about we go for "panfrost-{job,mmu}"
and leave the "gpu" one?

Or "panfrost-{gpu,job,mmu}" for consistency?

Thanks,
Eze

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

Re: [PATCH] drm/panfrost: Prefix interrupt handlers' names

[Alyssa Rosenzweig]

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

On Fri, Dec 13, 2019 at 03:31:45PM +0000, Robin Murphy wrote:
> On 13/12/2019 2:32 pm, Alyssa Rosenzweig wrote:
> > > TBH it would probably be sufficient to save the bother of allocating
> > > strings and just settle on "panfrost-{gpu,job,mmu}", since upstream
> > > users are unlikely to ever come across a system with more than one
> > > Mali in it ;)
> > 
> > Agreed.
> > 
> > ----Wait, you said *upstream*? Are there .... oh no
> 
> Heh, fear not - I'm only thinking of the "Juno board with FPGA prototyping
> stack" setup, and the people using those in anger are all working on some
> other driver anyway ;)

Gotcha :)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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] drm/panfrost: Prefix interrupt handlers' names

[Alyssa Rosenzweig]

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

> (Feedback for kernel patches on the same day, am I dreaming??)

That's panfrost!

> > Agreed, while the current implementation may be confusing it is at least 
> > self-consistent. TBH it would probably be sufficient to save the bother 
> > of allocating strings and just settle on "panfrost-{gpu,job,mmu}", since 
> > upstream users are unlikely to ever come across a system with more than 
> > one Mali in it ;)
> > 
> > And FWIW note that "GPU" really is the specific hardware name of that 
> > IRQ output; it's not just a generic fill-in for "the one that isn't the 
> > Job or MMU IRQ".
> > 
> 
> Yeah, that makese sense. So how about we go for "panfrost-{job,mmu}"
> and leave the "gpu" one?
> 
> Or "panfrost-{gpu,job,mmu}" for consistency?

I would prefer "panfrost-{gpu,job,mmu}" for consistency, I think.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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