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

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

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
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Only a couple of small questions - but nothing to block an RB tag.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> > 
> >  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 = &vsp1->drm->pipe[pipe_index];
> >  	struct vsp1_pipeline *pipe = &drm_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(&vsp1->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 <linux/videodev2.h>
> >  #include <linux/wait.h>
> > 
> > +#include <media/vsp1.h>
> > +
> >  #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