Re: [PATCH 07/15] v4l: vsp1: Move DRM atomic commit pipeline setup to separate function

[Laurent Pinchart]

Hi Kieran,

On Wednesday, 28 March 2018 17:43:13 EEST Kieran Bingham wrote:
> On 26/02/18 21:45, Laurent Pinchart wrote:
> > The DRM pipeline setup code used at atomic commit time is similar to the
> > setup code used when enabling the pipeline. Move it to a separate
> > function in order to share it.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Assuming no hidden secret code addition in this code move that I haven't
> seen..
> 
> Only a minor nit below asking if the function should be pluralised (_inputs,
> rather than _input)

I'll fix that in v2, thanks.

> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_drm.c | 347
> >  + 1 file changed, 180 insertions(+), 167
> >  deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index 9a043a915c0b..7bf697ba7969
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -46,6 +46,185 @@ static void vsp1_du_pipeline_frame_end(struct
> > vsp1_pipeline *pipe,> 
> >   * Pipeline Configuration
> >   */
> > 
> > +/* Setup one RPF and the connected BRU sink pad. */
> > +static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
> > + struct vsp1_pipeline *pipe,
> > + struct vsp1_rwpf *rpf,
> > + unsigned int bru_input)
> > +{
> > +   struct v4l2_subdev_selection sel;
> > +   struct v4l2_subdev_format format;
> > +   const struct v4l2_rect *crop;
> > +   int ret;
> > +
> > +   /*
> > +* Configure the format on the RPF sink pad and propagate it up to the
> > +* BRU sink pad.
> > +*/
> > +   crop = >drm->inputs[rpf->entity.index].crop;
> > +
> > +   memset(, 0, sizeof(format));
> > +   format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +   format.pad = RWPF_PAD_SINK;
> > +   format.format.width = crop->width + crop->left;
> > +   format.format.height = crop->height + crop->top;
> > +   format.format.code = rpf->fmtinfo->mbus;
> > +   format.format.field = V4L2_FIELD_NONE;
> > +
> > +   ret = v4l2_subdev_call(>entity.subdev, pad, set_fmt, NULL,
> > +  );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   dev_dbg(vsp1->dev,
> > +   "%s: set format %ux%u (%x) on RPF%u sink\n",
> > +   __func__, format.format.width, format.format.height,
> > +   format.format.code, rpf->entity.index);
> > +
> > +   memset(, 0, sizeof(sel));
> > +   sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +   sel.pad = RWPF_PAD_SINK;
> > +   sel.target = V4L2_SEL_TGT_CROP;
> > +   sel.r = *crop;
> > +
> > +   ret = v4l2_subdev_call(>entity.subdev, pad, set_selection, NULL,
> > +  );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   dev_dbg(vsp1->dev,
> > +   "%s: set selection (%u,%u)/%ux%u on RPF%u sink\n",
> > +   __func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
> > +   rpf->entity.index);
> > +
> > +   /*
> > +* RPF source, hardcode the format to ARGB to turn on format
> > +* conversion if needed.
> > +*/
> > +   format.pad = RWPF_PAD_SOURCE;
> > +
> > +   ret = v4l2_subdev_call(>entity.subdev, pad, get_fmt, NULL,
> > +  );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   dev_dbg(vsp1->dev,
> > +   "%s: got format %ux%u (%x) on RPF%u source\n",
> > +   __func__, format.format.width, format.format.height,
> > +   format.format.code, rpf->entity.index);
> > +
> > +   format.format.code = MEDIA_BUS_FMT_ARGB_1X32;
> > +
> > +   ret = v4l2_subdev_call(>entity.subdev, pad, set_fmt, NULL,
> > +  );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* BRU sink, propagate the format from the RPF source. */
> > +   format.pad = bru_input;
> > +
> > +   ret = v4l2_subdev_call(>bru->subdev, pad, set_fmt, NULL,
> > +  );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on %s pad %u\n",
> > +   __func__, format.format.width, format.format.height,
> > +   format.format.code, BRU_NAME(pipe->bru), format.pad);
> > +
> > +   sel.pad = bru_input;
> > +   sel.target = V4L2_SEL_TGT_COMPOSE;
> > +   sel.r = vsp1->drm->inputs[rpf->entity.index].compose;
> > +
> > +   ret = v4l2_subdev_call(>bru->subdev, pad, set_selection, NULL,
> > +  );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   dev_dbg(vsp1->dev, "%s: set selection (%u,%u)/%ux%u on %s pad %u\n",
> > +   __func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
> > +   BRU_NAME(pipe->bru), sel.pad);
> > +
> > +   return 0;
> > +}
> > +
> > +static unsigned int rpf_zpos(struct vsp1_device 

Re: [PATCH 07/15] v4l: vsp1: Move DRM atomic commit pipeline setup to separate function

[Kieran Bingham]

Hi Laurent,

On 26/02/18 21:45, Laurent Pinchart wrote:
> The DRM pipeline setup code used at atomic commit time is similar to the
> setup code used when enabling the pipeline. Move it to a separate
> function in order to share it.
> 
> Signed-off-by: Laurent Pinchart 

Assuming no hidden secret code addition in this code move that I haven't seen..

Only a minor nit below asking if the function should be pluralised (_inputs,
rather than _input)

Reviewed-by: Kieran Bingham 


> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 347 
> +
>  1 file changed, 180 insertions(+), 167 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 9a043a915c0b..7bf697ba7969 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -46,6 +46,185 @@ static void vsp1_du_pipeline_frame_end(struct 
> vsp1_pipeline *pipe,
>   * Pipeline Configuration
>   */
>  
> +/* Setup one RPF and the connected BRU sink pad. */
> +static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
> +   struct vsp1_pipeline *pipe,
> +   struct vsp1_rwpf *rpf,
> +   unsigned int bru_input)
> +{
> + struct v4l2_subdev_selection sel;
> + struct v4l2_subdev_format format;
> + const struct v4l2_rect *crop;
> + int ret;
> +
> + /*
> +  * Configure the format on the RPF sink pad and propagate it up to the
> +  * BRU sink pad.
> +  */
> + crop = >drm->inputs[rpf->entity.index].crop;
> +
> + memset(, 0, sizeof(format));
> + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + format.pad = RWPF_PAD_SINK;
> + format.format.width = crop->width + crop->left;
> + format.format.height = crop->height + crop->top;
> + format.format.code = rpf->fmtinfo->mbus;
> + format.format.field = V4L2_FIELD_NONE;
> +
> + ret = v4l2_subdev_call(>entity.subdev, pad, set_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev,
> + "%s: set format %ux%u (%x) on RPF%u sink\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code, rpf->entity.index);
> +
> + memset(, 0, sizeof(sel));
> + sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + sel.pad = RWPF_PAD_SINK;
> + sel.target = V4L2_SEL_TGT_CROP;
> + sel.r = *crop;
> +
> + ret = v4l2_subdev_call(>entity.subdev, pad, set_selection, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev,
> + "%s: set selection (%u,%u)/%ux%u on RPF%u sink\n",
> + __func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
> + rpf->entity.index);
> +
> + /*
> +  * RPF source, hardcode the format to ARGB to turn on format
> +  * conversion if needed.
> +  */
> + format.pad = RWPF_PAD_SOURCE;
> +
> + ret = v4l2_subdev_call(>entity.subdev, pad, get_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev,
> + "%s: got format %ux%u (%x) on RPF%u source\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code, rpf->entity.index);
> +
> + format.format.code = MEDIA_BUS_FMT_ARGB_1X32;
> +
> + ret = v4l2_subdev_call(>entity.subdev, pad, set_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + /* BRU sink, propagate the format from the RPF source. */
> + format.pad = bru_input;
> +
> + ret = v4l2_subdev_call(>bru->subdev, pad, set_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on %s pad %u\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code, BRU_NAME(pipe->bru), format.pad);
> +
> + sel.pad = bru_input;
> + sel.target = V4L2_SEL_TGT_COMPOSE;
> + sel.r = vsp1->drm->inputs[rpf->entity.index].compose;
> +
> + ret = v4l2_subdev_call(>bru->subdev, pad, set_selection, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: set selection (%u,%u)/%ux%u on %s pad %u\n",
> + __func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
> + BRU_NAME(pipe->bru), sel.pad);
> +
> + return 0;
> +}
> +
> +static unsigned int rpf_zpos(struct vsp1_device *vsp1, struct vsp1_rwpf *rpf)
> +{
> + return vsp1->drm->inputs[rpf->entity.index].zpos;
> +}
> +
> +/* Setup the input side of the pipeline (RPFs and BRU sink pads). */
> +static int 

[PATCH 07/15] v4l: vsp1: Move DRM atomic commit pipeline setup to separate function

[Laurent Pinchart]

The DRM pipeline setup code used at atomic commit time is similar to the
setup code used when enabling the pipeline. Move it to a separate
function in order to share it.

Signed-off-by: Laurent Pinchart 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 347 +
 1 file changed, 180 insertions(+), 167 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 9a043a915c0b..7bf697ba7969 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -46,6 +46,185 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline 
*pipe,
  * Pipeline Configuration
  */
 
+/* Setup one RPF and the connected BRU sink pad. */
+static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
+ struct vsp1_pipeline *pipe,
+ struct vsp1_rwpf *rpf,
+ unsigned int bru_input)
+{
+   struct v4l2_subdev_selection sel;
+   struct v4l2_subdev_format format;
+   const struct v4l2_rect *crop;
+   int ret;
+
+   /*
+* Configure the format on the RPF sink pad and propagate it up to the
+* BRU sink pad.
+*/
+   crop = >drm->inputs[rpf->entity.index].crop;
+
+   memset(, 0, sizeof(format));
+   format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+   format.pad = RWPF_PAD_SINK;
+   format.format.width = crop->width + crop->left;
+   format.format.height = crop->height + crop->top;
+   format.format.code = rpf->fmtinfo->mbus;
+   format.format.field = V4L2_FIELD_NONE;
+
+   ret = v4l2_subdev_call(>entity.subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev,
+   "%s: set format %ux%u (%x) on RPF%u sink\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, rpf->entity.index);
+
+   memset(, 0, sizeof(sel));
+   sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+   sel.pad = RWPF_PAD_SINK;
+   sel.target = V4L2_SEL_TGT_CROP;
+   sel.r = *crop;
+
+   ret = v4l2_subdev_call(>entity.subdev, pad, set_selection, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev,
+   "%s: set selection (%u,%u)/%ux%u on RPF%u sink\n",
+   __func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
+   rpf->entity.index);
+
+   /*
+* RPF source, hardcode the format to ARGB to turn on format
+* conversion if needed.
+*/
+   format.pad = RWPF_PAD_SOURCE;
+
+   ret = v4l2_subdev_call(>entity.subdev, pad, get_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev,
+   "%s: got format %ux%u (%x) on RPF%u source\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, rpf->entity.index);
+
+   format.format.code = MEDIA_BUS_FMT_ARGB_1X32;
+
+   ret = v4l2_subdev_call(>entity.subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   /* BRU sink, propagate the format from the RPF source. */
+   format.pad = bru_input;
+
+   ret = v4l2_subdev_call(>bru->subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on %s pad %u\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code, BRU_NAME(pipe->bru), format.pad);
+
+   sel.pad = bru_input;
+   sel.target = V4L2_SEL_TGT_COMPOSE;
+   sel.r = vsp1->drm->inputs[rpf->entity.index].compose;
+
+   ret = v4l2_subdev_call(>bru->subdev, pad, set_selection, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev, "%s: set selection (%u,%u)/%ux%u on %s pad %u\n",
+   __func__, sel.r.left, sel.r.top, sel.r.width, sel.r.height,
+   BRU_NAME(pipe->bru), sel.pad);
+
+   return 0;
+}
+
+static unsigned int rpf_zpos(struct vsp1_device *vsp1, struct vsp1_rwpf *rpf)
+{
+   return vsp1->drm->inputs[rpf->entity.index].zpos;
+}
+
+/* Setup the input side of the pipeline (RPFs and BRU sink pads). */
+static int vsp1_du_pipeline_setup_input(struct vsp1_device *vsp1,
+   struct vsp1_pipeline *pipe)
+{
+   struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
+   struct vsp1_bru *bru = to_bru(>bru->subdev);
+   unsigned int i;
+   int ret;
+
+   /* Count the number of enabled inputs and sort them by Z-order. */
+   pipe->num_inputs = 0;
+
+   for (i = 0; i <