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

[Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>]

Hi,

On 14/03/24 8:24 pm, Kieran Bingham wrote:
> Quoting Umang Jain (2024-03-14 10:06:06)
> > The variables tracking allocated pages fragments in vchiq_arm.c
> > can be easily moved to struct vchiq_drvdata instead of being global.
> > This helps us to drop the non-essential global variables in the vchiq
> > interface.
> >
> > Drop the TODO item as well, as it is now totally addressed:
> > > Get rid of all non essential global structures and create a proper per
> >    device structure
> >
> > No functional changes intended in this patch.
> >
> > Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/staging/vc04_services/interface/TODO  |  8 ---
> >   .../interface/vchiq_arm/vchiq_arm.c           | 53 +++++++++----------
> >   .../interface/vchiq_arm/vchiq_arm.h           |  6 +++
> >   3 files changed, 30 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
> > index 05eb5140d096..15f12b8f213e 100644
> > --- a/drivers/staging/vc04_services/interface/TODO
> > +++ b/drivers/staging/vc04_services/interface/TODO
> > @@ -41,14 +41,6 @@ The code follows the 80 characters limitation yet tends to go 3 or 4 levels of
> >   indentation deep making it very unpleasant to read. This is specially relevant
> >   in the character driver ioctl code and in the core thread functions.
> >   
> > -* Get rid of all non essential global structures and create a proper per
> > -device structure
> > -
> > -The first thing one generally sees in a probe function is a memory allocation
> > -for all the device specific data. This structure is then passed all over the
> > -driver. This is good practice since it makes the driver work regardless of the
> > -number of devices probed.
> > -
> >   * Clean up Sparse warnings from __user annotations. See
> >   vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter"
> >   is never disclosed to userspace.
> > 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

(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)

> > -static unsigned int g_fragments_size;
> > -static char *g_fragments_base;
> > -static char *g_free_fragments;
> > -static struct semaphore g_free_fragments_sema;
> > -
> > -static DEFINE_SEMAPHORE(g_free_fragments_mutex, 1);
> >   
> >   static int
> >   vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handle, void *data,
> > @@ -376,20 +370,20 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
> >              (drvdata->cache_line_size - 1)))) {
> >                  char *fragments;
> >   
> > -               if (down_interruptible(&g_free_fragments_sema)) {
> > +               if (down_interruptible(&drvdata->free_fragments_sema)) {
> >                          cleanup_pagelistinfo(instance, pagelistinfo);
> >                          return NULL;
> >                  }
> >   
> > -               WARN_ON(!g_free_fragments);
> > +               WARN_ON(!drvdata->free_fragments);
> >   
> > -               down(&g_free_fragments_mutex);
> > -               fragments = g_free_fragments;
> > +               down(&drvdata->free_fragments_mutex);
> > +               fragments = drvdata->free_fragments;
> >                  WARN_ON(!fragments);
> > -               g_free_fragments = *(char **)g_free_fragments;
> > -               up(&g_free_fragments_mutex);
> > +               drvdata->free_fragments = *(char **)drvdata->free_fragments;
> > +               up(&drvdata->free_fragments_mutex);
> >                  pagelist->type = PAGELIST_READ_WITH_FRAGMENTS +
> > -                       (fragments - g_fragments_base) / g_fragments_size;
> > +                       (fragments - drvdata->fragments_base) / drvdata->fragments_size;
> >          }
> >   
> >          return pagelistinfo;
> > @@ -421,10 +415,10 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
> >          pagelistinfo->scatterlist_mapped = 0;
> >   
> >          /* Deal with any partial cache lines (fragments) */
> > -       if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && g_fragments_base) {
> > -               char *fragments = g_fragments_base +
> > +       if (pagelist->type >= PAGELIST_READ_WITH_FRAGMENTS && drvdata->fragments_base) {
> > +               char *fragments = drvdata->fragments_base +
> >                          (pagelist->type - PAGELIST_READ_WITH_FRAGMENTS) *
> > -                       g_fragments_size;
> > +                       drvdata->fragments_size;
> >                  int head_bytes, tail_bytes;
> >   
> >                  head_bytes = (cache_line_size - pagelist->offset) &
> > @@ -449,11 +443,11 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
> >                                  fragments + cache_line_size,
> >                                  tail_bytes);
> >   
> > -               down(&g_free_fragments_mutex);
> > -               *(char **)fragments = g_free_fragments;
> > -               g_free_fragments = fragments;
> > -               up(&g_free_fragments_mutex);
> > -               up(&g_free_fragments_sema);
> > +               down(&drvdata->free_fragments_mutex);
> > +               *(char **)fragments = drvdata->free_fragments;
> > +               drvdata->free_fragments = fragments;
> > +               up(&drvdata->free_fragments_mutex);
> > +               up(&drvdata->free_fragments_sema);
> >          }
> >   
> >          /* Need to mark all the pages dirty. */
> > @@ -489,11 +483,11 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
> >          if (err < 0)
> >                  return err;
> >   
> > -       g_fragments_size = 2 * drvdata->cache_line_size;
> > +       drvdata->fragments_size = 2 * drvdata->cache_line_size;
> >   
> >          /* Allocate space for the channels in coherent memory */
> >          slot_mem_size = PAGE_ALIGN(TOTAL_SLOTS * VCHIQ_SLOT_SIZE);
> > -       frag_mem_size = PAGE_ALIGN(g_fragments_size * MAX_FRAGMENTS);
> > +       frag_mem_size = PAGE_ALIGN(drvdata->fragments_size * MAX_FRAGMENTS);
> >   
> >          slot_mem = dmam_alloc_coherent(dev, slot_mem_size + frag_mem_size,
> >                                         &slot_phys, GFP_KERNEL);
> > @@ -513,15 +507,16 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
> >          vchiq_slot_zero->platform_data[VCHIQ_PLATFORM_FRAGMENTS_COUNT_IDX] =
> >                  MAX_FRAGMENTS;
> >   
> > -       g_fragments_base = (char *)slot_mem + slot_mem_size;
> > +       drvdata->fragments_base = (char *)slot_mem + slot_mem_size;
> >   
> > -       g_free_fragments = g_fragments_base;
> > +       drvdata->free_fragments = drvdata->fragments_base;
> >          for (i = 0; i < (MAX_FRAGMENTS - 1); i++) {
> > -               *(char **)&g_fragments_base[i * g_fragments_size] =
> > -                       &g_fragments_base[(i + 1) * g_fragments_size];
> > +               *(char **)&drvdata->fragments_base[i * drvdata->fragments_size] =
> > +                       &drvdata->fragments_base[(i + 1) * drvdata->fragments_size];
> >          }
> > -       *(char **)&g_fragments_base[i * g_fragments_size] = NULL;
> > -       sema_init(&g_free_fragments_sema, MAX_FRAGMENTS);
> > +       *(char **)&drvdata->fragments_base[i * drvdata->fragments_size] = NULL;
> > +       sema_init(&drvdata->free_fragments_sema, MAX_FRAGMENTS);
> > +       sema_init(&drvdata->free_fragments_mutex, 1);
> >   
> >          err = vchiq_init_state(state, vchiq_slot_zero, dev);
> >          if (err)
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > index f2fd572df2b3..d5c0a8e9fa2b 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h
> > @@ -34,6 +34,12 @@ struct vchiq_drvdata {
> >          struct rpi_firmware *fw;
> >   
> >          struct vchiq_connected drv_connected;
> > +
> > +       struct semaphore free_fragments_sema;
> > +       struct semaphore free_fragments_mutex;
> > +       char *fragments_base;
> > +       char *free_fragments;
> > +       unsigned int fragments_size;
> >   };
> >   
> >   struct user_service {
> > --
> > 2.43.0