On Fri, Mar 15, 2024 at 11:17:15AM +0530, Umang Jain wrote:
> Hi,
>
> On 14/03/24 8:24 pm, Kieran Bingham wrote:
> > Quoting Umang Jain (2024-03-14 10:06:06)
> > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > index 282f83b335d4..666ab73ce0d1 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > @@ -140,12 +140,6 @@ struct vchiq_pagelist_info {
> > > };
> > > static void __iomem *g_regs;
> > What happens with this one ? Is it essential to be global? Can it be
> > part of the device? Who's mapping it? or unmapping?
> >
> > If we're keeping it global, I'd add a comment...
>
> The keyword in the series in 'non-essential' and I feel this one is
> essential one.
>
> It's on the irq request path so encapsulating it in the platform driver data
> and then again trying to get it from there, will make this affect
> performance.
>
> See remote_event_signal() and vchiq_doorbell_irq() in vchiq_arm.c
>
The fast path argument was convincing up to here
> (For testing, a added a dev_info() in the vchiq_doorbell_irq() and it
> brought down the consistent 30fps IMX219 streaming to (15-26)fps
> inconsistent range)
>
but doing a dev_info() seems way way more expensive than dereferencing
in some pointers in driver data...
Anyway, adding a comment seems like a good idea.
regards,
dan carpenter
