| Message ID | 20191213123942.22891-1-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Prefix interrupt handlers' names | expand |
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
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.
> 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
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.
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
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 :)
> (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.
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");
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(-)