Re: [PATCH v2 12/19] media: renesas: vsp1: Keep the DRM pipeline entities sorted

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Jun 19, 2024 at 01:20:49PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-06-19 01:17:15)
> > Some of the code that handles pipeline configuration assumes that
> > entities in a pipeline's entities list are sorted from sink to source.
> > To prepare for using that code with the DRM pipeline, insert the BRx
> > just before the WPF, and the RPFs at the head of the list.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/renesas/vsp1/vsp1_drm.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_drm.c b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > index 1aa59a74672f..e44359b661b6 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_drm.c
> > @@ -317,7 +317,10 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
> >                         list_add_tail(&released_brx->list_pipe,
> >                                       &pipe->entities);
> >  
> > -               /* Add the BRx to the pipeline. */
> > +               /*
> > +                * Add the BRx to the pipeline, inserting it just before the
> > +                * WPF.
> 
> So - the pipe->output is from what I recall/can see the output wpf.
>  (struct vsp1_rwpf *output)
> 
> > +                */
> >                 dev_dbg(vsp1->dev, "%s: pipe %u: acquired %s\n",
> >                         __func__, pipe->lif->index, BRX_NAME(brx));
> >  
> > @@ -326,7 +329,8 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device *vsp1,
> >                 pipe->brx->sink = &pipe->output->entity;
> >                 pipe->brx->sink_pad = 0;
> >  
> > -               list_add_tail(&pipe->brx->list_pipe, &pipe->entities);
> > +               list_add_tail(&pipe->brx->list_pipe,
> > +                             &pipe->output->entity.list_pipe);
> 
> But this reads to me as if we're adding the brx after ('the tail') of
> the output WPF....
> 
> Now ... of course if we open up list_add_tail()
> 
>  * Insert a new entry before the specified head.
> 
> And that checks out - because of course the list_add adds it as the
> 'next' item in the list... and we're using list_add_tail as a convenient
> way to provide list_add_before() ...
> 
> So I believe this is correct, but the nuance of it reads back to front to me.
> 
> Because of that it possibly deserves a better comment to be explicit on
> what it's doing, or makes me wonder if list.h should have something that
> explicitly impliments
> 
> #define list_add_before list_add_tail

https://lore.kernel.org/all/alpine.LSU.2.11.1406061242370.16010@eggly.anvils/

I'll let you argue :-)

> but otherwise - it does check out.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >         }
> >  
> >         /*
> > @@ -420,7 +424,7 @@ static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
> >  
> >                 if (!rpf->entity.pipe) {
> >                         rpf->entity.pipe = pipe;
> > -                       list_add_tail(&rpf->entity.list_pipe, &pipe->entities);
> > +                       list_add(&rpf->entity.list_pipe, &pipe->entities);
> >                 }
> >  
> >                 brx->inputs[i].rpf = rpf;

-- 
Regards,

Laurent Pinchart