Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline

[Laurent Pinchart]

Hi Kieran,

On Saturday, 28 April 2018 21:58:53 EEST Kieran Bingham wrote:
> On 22/04/18 23:34, Laurent Pinchart wrote:
> > The DISCOM is used to compute CRCs on display frames. Integrate it in
> > the display pipeline at the output of the blending unit to process
> > output frames.
> > 
> > Computing CRCs on input frames is possible by positioning the DISCOM at
> > a different point in the pipeline. This use case isn't supported at the
> > moment and could be implemented by extending the API between the VSP1
> > and DU drivers if needed.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> 
> Only a couple of small questions - but nothing to block an RB tag.
> 
> Reviewed-by: Kieran Bingham 
> 
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_drm.c | 115 +++-
> >  drivers/media/platform/vsp1/vsp1_drm.h |  12 
> >  2 files changed, 124 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index 5fc31578f9b0..7864b43a90e1
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c

[snip]

> > @@ -748,6 +847,9 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned
> > int pipe_index,> 
> > struct vsp1_drm_pipeline *drm_pipe = >drm->pipe[pipe_index];
> > struct vsp1_pipeline *pipe = _pipe->pipe;
> > 
> > +   drm_pipe->crc.source = cfg->crc.source;
> > +   drm_pipe->crc.index = cfg->crc.index;
> 
> I think this could be shortened to
> 
>   drm_pipe->crc = cfg->crc;
> 
> Or is that a GCC extension. Either way, it's just a matter of taste, and you
> might prefer to be more explicit.

That's what I had written in the first place, only to have gcc throwing an 
error because the crc fields are anonymous structures.

> > +
> > vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> > vsp1_du_pipeline_configure(pipe);
> > mutex_unlock(>drm->lock);

[snip]

> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> > b/drivers/media/platform/vsp1/vsp1_drm.h index e5b88b28806c..1e7670955ef0
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.h
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> > @@ -13,6 +13,8 @@
> >  #include 
> >  #include 
> > 
> > +#include 
> > +
> >  #include "vsp1_pipe.h"
> >  
> >  /**
> > @@ -22,6 +24,9 @@
> >   * @height: output display height
> >   * @force_brx_release: when set, release the BRx during the next
> >   reconfiguration
> >   * @wait_queue: wait queue to wait for BRx release completion
> > + * @uif: UIF entity if available for the pipeline
> > + * @crc.source: source for CRC calculation
> > + * @crc.index: index of the CRC source plane (when crc.source is set to
> > plane)
> >   * @du_complete: frame completion callback for the DU driver (optional)
> >   * @du_private: data to be passed to the du_complete callback
> >   */
> > @@ -34,6 +39,13 @@ struct vsp1_drm_pipeline {
> > bool force_brx_release;
> > wait_queue_head_t wait_queue;
> > 
> > +   struct vsp1_entity *uif;
> > +
> > +   struct {
> > +   enum vsp1_du_crc_source source;
> > +   unsigned int index;
> > +   } crc;
> 
> Does this have to be duplicated ? Or can it be included from the API
> header...

It's a good point. I thought it might not be worth it just for two fields, but 
I see no compelling reason against it. I'll introduce a new structure.

> > +
> > /* Frame synchronisation */
> > void (*du_complete)(void *data, bool completed, u32 crc);
> > void *du_private;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline

[Kieran Bingham]

Hi Laurent,

On 22/04/18 23:34, Laurent Pinchart wrote:
> The DISCOM is used to compute CRCs on display frames. Integrate it in
> the display pipeline at the output of the blending unit to process
> output frames.
> 
> Computing CRCs on input frames is possible by positioning the DISCOM at
> a different point in the pipeline. This use case isn't supported at the
> moment and could be implemented by extending the API between the VSP1
> and DU drivers if needed.
> 
> Signed-off-by: Laurent Pinchart 

Only a couple of small questions - but nothing to block an RB tag.

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 115 
> -
>  drivers/media/platform/vsp1/vsp1_drm.h |  12 
>  2 files changed, 124 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 5fc31578f9b0..7864b43a90e1 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -22,6 +22,7 @@
>  #include "vsp1_lif.h"
>  #include "vsp1_pipe.h"
>  #include "vsp1_rwpf.h"
> +#include "vsp1_uif.h"
>  
>  #define BRX_NAME(e)  (e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"
>  
> @@ -35,8 +36,13 @@ static void vsp1_du_pipeline_frame_end(struct 
> vsp1_pipeline *pipe,
>   struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
>   bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
>  
> - if (drm_pipe->du_complete)
> - drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
> + if (drm_pipe->du_complete) {
> + struct vsp1_entity *uif = drm_pipe->uif;
> + u32 crc;
> +
> + crc = uif ? vsp1_uif_get_crc(to_uif(>subdev)) : 0;
> + drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
> + }
>  
>   if (completion & VSP1_DL_FRAME_END_INTERNAL) {
>   drm_pipe->force_brx_release = false;
> @@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct 
> vsp1_pipeline *pipe,
>   * Pipeline Configuration
>   */
>  
> +/*
> + * Insert the UIF in the pipeline between the prev and next entities. If no 
> UIF
> + * is available connect the two entities directly.
> + */
> +static int vsp1_du_insert_uif(struct vsp1_device *vsp1,
> +   struct vsp1_pipeline *pipe,
> +   struct vsp1_entity *uif,
> +   struct vsp1_entity *prev, unsigned int prev_pad,
> +   struct vsp1_entity *next, unsigned int next_pad)
> +{
> + int ret;
> +
> + if (uif) {
> + struct v4l2_subdev_format format;
> +
> + prev->sink = uif;
> + prev->sink_pad = UIF_PAD_SINK;
> +
> + memset(, 0, sizeof(format));
> + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + format.pad = prev_pad;
> +
> + ret = v4l2_subdev_call(>subdev, pad, get_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + format.pad = UIF_PAD_SINK;
> +
> + ret = v4l2_subdev_call(>subdev, pad, set_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code);
> +
> + /*
> +  * The UIF doesn't mangle the format between its sink and
> +  * source pads, so there is no need to retrieve the format on
> +  * its source pad.
> +  */
> +
> + uif->sink = next;
> + uif->sink_pad = next_pad;
> + } else {
> + prev->sink = next;
> + prev->sink_pad = next_pad;
> + }
> +
> + return 0;
> +}
> +
>  /* Setup one RPF and the connected BRx sink pad. */
>  static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
> struct vsp1_pipeline *pipe,
> struct vsp1_rwpf *rpf,
> +   struct vsp1_entity *uif,
> unsigned int brx_input)
>  {
>   struct v4l2_subdev_selection sel;
> @@ -122,6 +183,12 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device 
> *vsp1,
>   if (ret < 0)
>   return ret;
>  
> + /* Insert and configure the UIF if available. */
> + ret = vsp1_du_insert_uif(vsp1, pipe, uif, >entity, RWPF_PAD_SOURCE,
> +  pipe->brx, brx_input);
> + if (ret < 0)
> + return ret;
> +
>   /* BRx sink, propagate the format from the RPF source. */
>   format.pad = brx_input;
>  
> @@ -297,7 +364,10 @@ static unsigned int 

Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline

[Laurent Pinchart]

Hi Jacopo,

On Saturday, 28 April 2018 14:00:26 EEST jacopo mondi wrote:
> On Mon, Apr 23, 2018 at 01:34:29AM +0300, Laurent Pinchart wrote:
> > The DISCOM is used to compute CRCs on display frames. Integrate it in
> > the display pipeline at the output of the blending unit to process
> > output frames.
> > 
> > Computing CRCs on input frames is possible by positioning the DISCOM at
> > a different point in the pipeline. This use case isn't supported at the
> > moment and could be implemented by extending the API between the VSP1
> > and DU drivers if needed.
> > 
> > Signed-off-by: Laurent Pinchart
> > 
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_drm.c | 115 +++-
> >  drivers/media/platform/vsp1/vsp1_drm.h |  12 
> >  2 files changed, 124 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index 5fc31578f9b0..7864b43a90e1
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c

[snip]

> > @@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct
> > vsp1_pipeline *pipe,> 
> >   * Pipeline Configuration
> >   */
> > 
> > +/*
> > + * Insert the UIF in the pipeline between the prev and next entities. If
> > no UIF + * is available connect the two entities directly.
> > + */
> > +static int vsp1_du_insert_uif(struct vsp1_device *vsp1,
> > + struct vsp1_pipeline *pipe,
> > + struct vsp1_entity *uif,
> > + struct vsp1_entity *prev, unsigned int prev_pad,
> > + struct vsp1_entity *next, unsigned int next_pad)
> > +{
> > +   int ret;
> > +
> > +   if (uif) {
> > +   struct v4l2_subdev_format format;
> > +
> > +   prev->sink = uif;
> > +   prev->sink_pad = UIF_PAD_SINK;
> > +
> > +   memset(, 0, sizeof(format));
> > +   format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +   format.pad = prev_pad;
> > +
> > +   ret = v4l2_subdev_call(>subdev, pad, get_fmt, NULL,
> > +  );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   format.pad = UIF_PAD_SINK;
> > +
> > +   ret = v4l2_subdev_call(>subdev, pad, set_fmt, NULL,
> > +  );
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
> > +   __func__, format.format.width, format.format.height,
> > +   format.format.code);
> > +
> > +   /*
> > +* The UIF doesn't mangle the format between its sink and
> > +* source pads, so there is no need to retrieve the format on
> > +* its source pad.
> > +*/
> > +
> > +   uif->sink = next;
> > +   uif->sink_pad = next_pad;
> > +   } else {
> > +   prev->sink = next;
> > +   prev->sink_pad = next_pad;
> 
> Isn't the !uif case better handled in the caller? See below...
> Or otherwise, if you prefer handling it here, shouldn't the
> indentation be reduced with

I don't think it's better handled in the caller, I prefer keeping the entities 
linking code in a single place. I'll reduce the indentation.

> if (!uif) {
>   prev->sink = next;
>   prev->sink_pad = next_pad;
> return 0;
> }
> 
> > +   }
> > +
> > +   return 0;
> > +}

[snip]

> > @@ -367,6 +441,31 @@ static int vsp1_du_pipeline_setup_inputs(struct
> > vsp1_device *vsp1,
> > }
> > }
> > 
> > +   /* Insert and configure the UIF at the BRx output if available. */
> > +   uif = drm_pipe->crc.source == VSP1_DU_CRC_OUTPUT ? drm_pipe->uif : 
NULL;
> > +   if (uif)
> > +   use_uif = true;
> > +   ret = vsp1_du_insert_uif(vsp1, pipe, uif,
> > +pipe->brx, pipe->brx->source_pad,
> > +>output->entity, 0);
> > +   if (ret < 0)
> > +   dev_err(vsp1->dev, "%s: failed to setup UIF after %s\n",
> > +   __func__, BRX_NAME(pipe->brx));
> > +
> > +   /*
> > +* If the UIF is not in use schedule it for removal by setting its pipe
> > +* pointer to NULL, vsp1_du_pipeline_configure() will remove it from 
the
> > +* hardware pipeline and from the pipeline's list of entities. 
Otherwise
> > +* make sure it is present in the pipeline's list of entities if it
> > +* wasn't already.
> > +*/
> > +   if (!use_uif) {
> 
> ... here. If you don't use uif, don't call vspi1_du_insert_uif().
> True, you have to link entities explicitly here if there is not uif,
> but I may be missing where this was happening before this code was
> added.
> 
> > +   drm_pipe->uif->pipe = NULL;
> > +   } else if (!drm_pipe->uif->pipe) {
> > +   

Re: [PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline

[jacopo mondi]

HI Laurent,

On Mon, Apr 23, 2018 at 01:34:29AM +0300, Laurent Pinchart wrote:
> The DISCOM is used to compute CRCs on display frames. Integrate it in
> the display pipeline at the output of the blending unit to process
> output frames.
>
> Computing CRCs on input frames is possible by positioning the DISCOM at
> a different point in the pipeline. This use case isn't supported at the
> moment and could be implemented by extending the API between the VSP1
> and DU drivers if needed.
>
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 115 
> -
>  drivers/media/platform/vsp1/vsp1_drm.h |  12 
>  2 files changed, 124 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 5fc31578f9b0..7864b43a90e1 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -22,6 +22,7 @@
>  #include "vsp1_lif.h"
>  #include "vsp1_pipe.h"
>  #include "vsp1_rwpf.h"
> +#include "vsp1_uif.h"
>
>  #define BRX_NAME(e)  (e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"
>
> @@ -35,8 +36,13 @@ static void vsp1_du_pipeline_frame_end(struct 
> vsp1_pipeline *pipe,
>   struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
>   bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
>
> - if (drm_pipe->du_complete)
> - drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
> + if (drm_pipe->du_complete) {
> + struct vsp1_entity *uif = drm_pipe->uif;
> + u32 crc;
> +
> + crc = uif ? vsp1_uif_get_crc(to_uif(>subdev)) : 0;
> + drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
> + }
>
>   if (completion & VSP1_DL_FRAME_END_INTERNAL) {
>   drm_pipe->force_brx_release = false;
> @@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct 
> vsp1_pipeline *pipe,
>   * Pipeline Configuration
>   */
>
> +/*
> + * Insert the UIF in the pipeline between the prev and next entities. If no 
> UIF
> + * is available connect the two entities directly.
> + */
> +static int vsp1_du_insert_uif(struct vsp1_device *vsp1,
> +   struct vsp1_pipeline *pipe,
> +   struct vsp1_entity *uif,
> +   struct vsp1_entity *prev, unsigned int prev_pad,
> +   struct vsp1_entity *next, unsigned int next_pad)
> +{
> + int ret;
> +
> + if (uif) {
> + struct v4l2_subdev_format format;
> +
> + prev->sink = uif;
> + prev->sink_pad = UIF_PAD_SINK;
> +
> + memset(, 0, sizeof(format));
> + format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + format.pad = prev_pad;
> +
> + ret = v4l2_subdev_call(>subdev, pad, get_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + format.pad = UIF_PAD_SINK;
> +
> + ret = v4l2_subdev_call(>subdev, pad, set_fmt, NULL,
> +);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
> + __func__, format.format.width, format.format.height,
> + format.format.code);
> +
> + /*
> +  * The UIF doesn't mangle the format between its sink and
> +  * source pads, so there is no need to retrieve the format on
> +  * its source pad.
> +  */
> +
> + uif->sink = next;
> + uif->sink_pad = next_pad;
> + } else {
> + prev->sink = next;
> + prev->sink_pad = next_pad;

Isn't the !uif case better handled in the caller? See below...
Or otherwise, if you prefer handling it here, shouldn't the
indentation be reduced with

if (!uif) {
prev->sink = next;
prev->sink_pad = next_pad;

return 0;
}

> + }
> +
> + return 0;
> +}
> +
>  /* Setup one RPF and the connected BRx sink pad. */
>  static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
> struct vsp1_pipeline *pipe,
> struct vsp1_rwpf *rpf,
> +   struct vsp1_entity *uif,
> unsigned int brx_input)
>  {
>   struct v4l2_subdev_selection sel;
> @@ -122,6 +183,12 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device 
> *vsp1,
>   if (ret < 0)
>   return ret;
>
> + /* Insert and configure the UIF if available. */
> + ret = vsp1_du_insert_uif(vsp1, pipe, uif, >entity, RWPF_PAD_SOURCE,
> +  pipe->brx, brx_input);
> + if (ret < 0)
> + 

[PATCH v2 7/8] v4l: vsp1: Integrate DISCOM in display pipeline

[Laurent Pinchart]

The DISCOM is used to compute CRCs on display frames. Integrate it in
the display pipeline at the output of the blending unit to process
output frames.

Computing CRCs on input frames is possible by positioning the DISCOM at
a different point in the pipeline. This use case isn't supported at the
moment and could be implemented by extending the API between the VSP1
and DU drivers if needed.

Signed-off-by: Laurent Pinchart 
---
 drivers/media/platform/vsp1/vsp1_drm.c | 115 -
 drivers/media/platform/vsp1/vsp1_drm.h |  12 
 2 files changed, 124 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 5fc31578f9b0..7864b43a90e1 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -22,6 +22,7 @@
 #include "vsp1_lif.h"
 #include "vsp1_pipe.h"
 #include "vsp1_rwpf.h"
+#include "vsp1_uif.h"
 
 #define BRX_NAME(e)(e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"
 
@@ -35,8 +36,13 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline 
*pipe,
struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
 
-   if (drm_pipe->du_complete)
-   drm_pipe->du_complete(drm_pipe->du_private, complete, 0);
+   if (drm_pipe->du_complete) {
+   struct vsp1_entity *uif = drm_pipe->uif;
+   u32 crc;
+
+   crc = uif ? vsp1_uif_get_crc(to_uif(>subdev)) : 0;
+   drm_pipe->du_complete(drm_pipe->du_private, complete, crc);
+   }
 
if (completion & VSP1_DL_FRAME_END_INTERNAL) {
drm_pipe->force_brx_release = false;
@@ -48,10 +54,65 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline 
*pipe,
  * Pipeline Configuration
  */
 
+/*
+ * Insert the UIF in the pipeline between the prev and next entities. If no UIF
+ * is available connect the two entities directly.
+ */
+static int vsp1_du_insert_uif(struct vsp1_device *vsp1,
+ struct vsp1_pipeline *pipe,
+ struct vsp1_entity *uif,
+ struct vsp1_entity *prev, unsigned int prev_pad,
+ struct vsp1_entity *next, unsigned int next_pad)
+{
+   int ret;
+
+   if (uif) {
+   struct v4l2_subdev_format format;
+
+   prev->sink = uif;
+   prev->sink_pad = UIF_PAD_SINK;
+
+   memset(, 0, sizeof(format));
+   format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+   format.pad = prev_pad;
+
+   ret = v4l2_subdev_call(>subdev, pad, get_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   format.pad = UIF_PAD_SINK;
+
+   ret = v4l2_subdev_call(>subdev, pad, set_fmt, NULL,
+  );
+   if (ret < 0)
+   return ret;
+
+   dev_dbg(vsp1->dev, "%s: set format %ux%u (%x) on UIF sink\n",
+   __func__, format.format.width, format.format.height,
+   format.format.code);
+
+   /*
+* The UIF doesn't mangle the format between its sink and
+* source pads, so there is no need to retrieve the format on
+* its source pad.
+*/
+
+   uif->sink = next;
+   uif->sink_pad = next_pad;
+   } else {
+   prev->sink = next;
+   prev->sink_pad = next_pad;
+   }
+
+   return 0;
+}
+
 /* Setup one RPF and the connected BRx sink pad. */
 static int vsp1_du_pipeline_setup_rpf(struct vsp1_device *vsp1,
  struct vsp1_pipeline *pipe,
  struct vsp1_rwpf *rpf,
+ struct vsp1_entity *uif,
  unsigned int brx_input)
 {
struct v4l2_subdev_selection sel;
@@ -122,6 +183,12 @@ static int vsp1_du_pipeline_setup_rpf(struct vsp1_device 
*vsp1,
if (ret < 0)
return ret;
 
+   /* Insert and configure the UIF if available. */
+   ret = vsp1_du_insert_uif(vsp1, pipe, uif, >entity, RWPF_PAD_SOURCE,
+pipe->brx, brx_input);
+   if (ret < 0)
+   return ret;
+
/* BRx sink, propagate the format from the RPF source. */
format.pad = brx_input;
 
@@ -297,7 +364,10 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1, 
struct vsp1_rwpf *rpf)
 static int vsp1_du_pipeline_setup_inputs(struct vsp1_device *vsp1,
struct vsp1_pipeline *pipe)
 {
+   struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL,