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

[Ezequiel Garcia <ezequiel@collabora.com>]

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