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

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Dan Carpenter (2024-03-16 08:41:50)
> 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...

Indeed, I think even in IRQ context here we should be fine to
dereference the pointers here, and can get rid of the last global.

> Anyway, adding a comment seems like a good idea.

If there's a reason this doesn't work, or if it does break the tests
then a comment should describe that for sure.

--
Kieran


> 
> regards,
> dan carpenter
>