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@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
>
> > }
> >
> > /*
> > @@ -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
