Re: [PATCH v6 11/18] media: vsp1: drm: Implement writeback support

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

Hi Kieran,

On Wed, Mar 13, 2019 at 11:42:34AM +0000, Kieran Bingham wrote:
> On 13/03/2019 00:05, Laurent Pinchart wrote:
> > Extend the vsp1_du_atomic_flush() API with writeback support by adding
> > format, pitch and memory addresses of the writeback framebuffer.
> > Writeback completion is reported through the existing frame completion
> > callback with a new VSP1_DU_STATUS_WRITEBACK status flag.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/vsp1/vsp1_dl.c  | 14 ++++++++++++++
> >  drivers/media/platform/vsp1/vsp1_dl.h  |  3 ++-
> >  drivers/media/platform/vsp1/vsp1_drm.c | 25 ++++++++++++++++++++++++-
> >  include/media/vsp1.h                   | 15 +++++++++++++++
> >  4 files changed, 55 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> > index ed7cda4130f2..104b6f514536 100644
> > --- a/drivers/media/platform/vsp1/vsp1_dl.c
> > +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> > @@ -958,6 +958,9 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
> >   *
> >   * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
> >   * became active had been queued with the internal notification flag.
> > + *
> > + * The VSP1_DL_FRAME_END_WRITEBACK flag indicates that the previously active
> > + * display list had been queued with the writeback flag.
> 
> How does this interact with the possibility of the writeback being
> disabled by the WPF in the event of it failing to get a DL.
> 
> It's only a small corner case, but will the 'writeback' report back as
> though it succeeded? (without writing to memory, and thus giving an
> unmodified buffer back?)

Wrteback completion will never be reported in that case. This shouldn't
happen as we should never fail to get a display list. Do you think it
would be better to fake completion ?

> >   */
> >  unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> >  {
> > @@ -995,6 +998,17 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> >  	if (status & VI6_STATUS_FLD_STD(dlm->index))
> >  		goto done;
> >  
> > +	/*
> > +	 * If the active display list has the writeback flag set, the frame
> > +	 * completion marks the end of the writeback capture. Return the
> > +	 * VSP1_DL_FRAME_END_WRITEBACK flag and reset the display list's
> > +	 * writeback flag.
> > +	 */
> > +	if (dlm->active && (dlm->active->flags & VSP1_DL_FRAME_END_WRITEBACK)) {
> > +		flags |= VSP1_DL_FRAME_END_WRITEBACK;
> > +		dlm->active->flags &= ~VSP1_DL_FRAME_END_WRITEBACK;
> > +	}
> > +
> >  	/*
> >  	 * The device starts processing the queued display list right after the
> >  	 * frame end interrupt. The display list thus becomes active.
> > diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
> > index e0fdb145e6ed..4d7bcfdc9bd9 100644
> > --- a/drivers/media/platform/vsp1/vsp1_dl.h
> > +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> > @@ -18,7 +18,8 @@ struct vsp1_dl_list;
> >  struct vsp1_dl_manager;
> >  
> >  #define VSP1_DL_FRAME_END_COMPLETED		BIT(0)
> > -#define VSP1_DL_FRAME_END_INTERNAL		BIT(1)
> > +#define VSP1_DL_FRAME_END_WRITEBACK		BIT(1)
> 
> So below BIT(2) (code above) the flags match the externally exposed
> bitfield for the VSP1_DU_STATUS_
> 
> While above (code below), are 'private' bitfields.
> 
> Should this requirement be documented here somehow? especially the
> mapping of FRAME_END_{COMPLETED,WRITEBACK} to
> DU_STATUS_{COMPLETED,WRITEBACK}.

I've added a comment here, as explained in my reply to your review of
10/18, to document this.

> > +#define VSP1_DL_FRAME_END_INTERNAL		BIT(2)
> >  
> >  /**
> >   * struct vsp1_dl_ext_cmd - Extended Display command
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 0367f88135bf..16826bf184c7 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -37,7 +37,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> >  
> >  	if (drm_pipe->du_complete) {
> >  		struct vsp1_entity *uif = drm_pipe->uif;
> > -		unsigned int status = completion & VSP1_DU_STATUS_COMPLETE;
> > +		unsigned int status = completion
> > +				    & (VSP1_DU_STATUS_COMPLETE |
> > +				       VSP1_DU_STATUS_WRITEBACK);
> >  		u32 crc;
> >  
> >  		crc = uif ? vsp1_uif_get_crc(to_uif(&uif->subdev)) : 0;
> > @@ -541,6 +543,8 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
> >  
> >  	if (drm_pipe->force_brx_release)
> >  		dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
> > +	if (pipe->output->writeback)
> > +		dl_flags |= VSP1_DL_FRAME_END_WRITEBACK;
> >  
> >  	dl = vsp1_dl_list_get(pipe->output->dlm);
> >  	dlb = vsp1_dl_list_get_body0(dl);
> > @@ -870,12 +874,31 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> >  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> >  	struct vsp1_drm_pipeline *drm_pipe = &vsp1->drm->pipe[pipe_index];
> >  	struct vsp1_pipeline *pipe = &drm_pipe->pipe;
> > +	int ret;
> >  
> >  	drm_pipe->crc = cfg->crc;
> >  
> >  	mutex_lock(&vsp1->drm->lock);
> > +
> > +	if (pipe->output->has_writeback && cfg->writeback.pixelformat) {
> 
> Is pipe->output->has_writeback necessary here? Can
> cfg->writeback.pixelformat be set if pipe->output->has_writeback is false?
> 
> Hrm ... actually - Perhaps it is useful. It validates both sides of the
> system.
> 
> pipe->output->has_writeback is a capability of the VSP, where as
> cfg->writeback.pixelformat is a 'request' from the DU.

Correct, I think it's best to check both, to ensure we don't try to
queue a writeback request on a system that doesn't support writeback. On
the other hand this shouldn't happen as the DU driver shouldn't expose
writeback to userspace in that case, so if you don't think the check is
worth it I can remove the has_writeback field completely.

> > +		const struct vsp1_du_writeback_config *wb_cfg = &cfg->writeback;
> > +
> > +		ret = vsp1_du_pipeline_set_rwpf_format(vsp1, pipe->output,
> > +						       wb_cfg->pixelformat,
> > +						       wb_cfg->pitch);
> > +		if (WARN_ON(ret < 0))
> > +			goto done;
> > +
> > +		pipe->output->mem.addr[0] = wb_cfg->mem[0];
> > +		pipe->output->mem.addr[1] = wb_cfg->mem[1];
> > +		pipe->output->mem.addr[2] = wb_cfg->mem[2];
> > +		pipe->output->writeback = true;
> > +	}
> > +
> >  	vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> >  	vsp1_du_pipeline_configure(pipe);
> > +
> > +done:
> >  	mutex_unlock(&vsp1->drm->lock);
> >  }
> >  EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
> > diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> > index 877496936487..cc1b0d42ce95 100644
> > --- a/include/media/vsp1.h
> > +++ b/include/media/vsp1.h
> > @@ -18,6 +18,7 @@ struct device;
> >  int vsp1_du_init(struct device *dev);
> >  
> >  #define VSP1_DU_STATUS_COMPLETE		BIT(0)
> > +#define VSP1_DU_STATUS_WRITEBACK	BIT(1)
> >  
> >  /**
> >   * struct vsp1_du_lif_config - VSP LIF configuration
> > @@ -83,12 +84,26 @@ struct vsp1_du_crc_config {
> >  	unsigned int index;
> >  };
> >  
> > +/**
> > + * struct vsp1_du_writeback_config - VSP writeback configuration parameters
> > + * @pixelformat: plane pixel format (V4L2 4CC)
> > + * @pitch: line pitch in bytes for the first plane
> > + * @mem: DMA memory address for each plane of the frame buffer
> > + */
> > +struct vsp1_du_writeback_config {
> > +	u32 pixelformat;
> > +	unsigned int pitch;
> > +	dma_addr_t mem[3];
> > +};
> > +
> >  /**
> >   * struct vsp1_du_atomic_pipe_config - VSP atomic pipe configuration parameters
> >   * @crc: CRC computation configuration
> > + * @writeback: writeback configuration
> >   */
> >  struct vsp1_du_atomic_pipe_config {
> >  	struct vsp1_du_crc_config crc;
> > +	struct vsp1_du_writeback_config writeback;
> >  };
> >  
> >  void vsp1_du_atomic_begin(struct device *dev, unsigned int pipe_index);

-- 
Regards,

Laurent Pinchart