Re: [PATCH v2 5/6] staging: vc04_services: Drop global variables tracking allocated pages

[Dan Carpenter <dan.carpenter@linaro.org>]

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