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