Re: [PATCH v4 7/7] media: vsp1: Provide a writeback video device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Kieran,

On Sun, Feb 17, 2019 at 08:06:32PM +0000, Kieran Bingham wrote:
> On 17/02/2019 02:48, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> > 
> > When the VSP1 is used in an active display pipeline, the output of the
> > WPF can supply the LIF entity directly and simultaneously write to
> > memory.
> > 
> > Support this functionality in the VSP1 driver through a V4L2 video
> > device node connected to the WPF.
> > 
> > The writeback video node support RGB formats only due to the WPF
> > outputting RGB to the LIF. The pixel format can otherwise be configured
> > freely.
> > 
> > The resolution of the captured frames are set by the display mode. A
> > different resolution can be selected on the video node, in which case
> > cropping or composing will be performed.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> 
> For your changes,
> 
> With the documentation updated for vsp1_dl_body_write_patch(),
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> 
> > ---
> > Changes since v3:
> > 
> > - Infer has_writeback from the number of LIFs and the generation
> > - Remove vsp1_video::is_writeback
> > - Describe writeback video nodes as 'writeback'
> > - Add mechanism to patch active display lists
> > - Handle writeback format restrictions
> > - Don't wait for next page flip before completing writeback buffer
> 
> This is a nice addition.

Thank you. It was the hard part :)

> > - Periods at the end of sentences.
> 
> You also fix writeback for the bitrot that the previous sets had now
> that we have partition handling.
> 
> Interestingly my local implementation for this bitrot was just to
> allocate a table for a single partition. I like your implementation
> better :)

Thank you.

> > Changes since v2:
> >  - Rebased to v4.12-rc1
> > 
> > Changes since RFC
> >  - Fix checkpatch.pl warnings
> >  - Adapt to use a single source pad for the Writeback and LIF
> >  - Use WPF properties to determine when to create links instead of VSP
> >  - Remove incorrect vsp1_video_verify_format() changes
> >  - Spelling and grammar fixes
> > ---
> >  drivers/media/platform/vsp1/vsp1_dl.c    |  83 +++++++++++++
> >  drivers/media/platform/vsp1/vsp1_dl.h    |   4 +
> >  drivers/media/platform/vsp1/vsp1_drm.c   |  14 ++-
> >  drivers/media/platform/vsp1/vsp1_drv.c   |  17 ++-
> >  drivers/media/platform/vsp1/vsp1_pipe.c  |   5 +
> >  drivers/media/platform/vsp1/vsp1_pipe.h  |   6 +
> >  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> >  drivers/media/platform/vsp1/vsp1_video.c | 150 +++++++++++++++++++++--
> >  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> >  drivers/media/platform/vsp1/vsp1_wpf.c   |  49 ++++++--
> >  10 files changed, 318 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> > index 886b3a69d329..591544382946 100644
> > --- a/drivers/media/platform/vsp1/vsp1_dl.c
> > +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> > @@ -115,6 +115,12 @@ struct vsp1_dl_body {
> >  
> >  	unsigned int num_entries;
> >  	unsigned int max_entries;
> > +
> > +	unsigned int num_patches;
> > +	struct {
> > +		struct vsp1_dl_entry *entry;
> > +		u32 data;
> > +	} patches[2];
> 
> What's the significance of [2] ?
> Perhaps it will be clear what two entry's support patching later...
> 
> Ok - yes - it's to patch both the Writeback enable and the display start
> enable.

Yes, we only need two registers. This is a bit of a hack, I'd like to
come up with a better API, but making it generic will be unneedingly
costly.

> >  };
> >  
> >  /**
> > @@ -361,6 +367,7 @@ void vsp1_dl_body_put(struct vsp1_dl_body *dlb)
> >  		return;
> >  
> >  	dlb->num_entries = 0;
> > +	dlb->num_patches = 0;
> >  
> >  	spin_lock_irqsave(&dlb->pool->lock, flags);
> >  	list_add_tail(&dlb->free, &dlb->pool->free);
> > @@ -388,6 +395,37 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data)
> >  	dlb->num_entries++;
> >  }
> >  
> > +/**
> > + * vsp1_dl_body_write - Write a register to a display list body
> 
> s/_write/_write_patch/

Oops. I forgot to update the documentation :-/

> "Store an update to an existing register for handling at display-start
> interrupt...."? (or as you see fit)

The function writes a register and stores the update. I'll fix the
documentation.

/**
 * vsp1_dl_body_write_patch - Write a register to a display list body with an
 *      update for the next vblank
 * @dlb: The body
 * @reg: The register address
 * @data: The register value
 * @patch: The register value for the next vblank
 *
 * Display lists in continuous mode are re-used by the hardware for successive
 * frames until a new display list is committed. Changing the VSP configuration
 * normally requires creating and committing a new display list. This function
 * offers an alternative race-free way by writing a @value to the @register in
 * the display list body, along with an updated @patch value to be written to
 * the VSP one vblank after the display list is committed.
 *
 * The maximum number of patch entries that can be written in a body is 2.
 */

> > + * @dlb: The body
> > + * @reg: The register address
> > + * @data: The register value
> > + *
> 
> + patch: The updated value to modify...
> 
> > + * Write the given register and value to the display list body. The maximum
> > + * number of entries that can be written in a body is specified when the body is
> > + * allocated by vsp1_dl_body_alloc().
> 
> I guess this is a copy/paste hangover.
> 
> How about:
> 
> "Display lists in continuous mode are re-used by the hardware for
> successive frames without needed to recommit a new display list. A patch
> allows us to apply small changes to the display list before it is reused
> to allow minor configuration changes without involving a full rewrite of
> the list or facing a race at commit."
> 
> (Or however you see fit...)
> 
> > + */
> > +void vsp1_dl_body_write_patch(struct vsp1_dl_body *dlb, u32 reg, u32 data,
> > +			      u32 patch)
> > +{
> > +	if (WARN_ONCE(dlb->num_entries >= dlb->max_entries,
> > +		      "DLB size exceeded (max %u)", dlb->max_entries))
> > +		return;
> > +
> > +	if (WARN_ONCE(dlb->num_patches >= ARRAY_SIZE(dlb->patches),
> > +		      "DLB patches size exceeded (max %lu)",
> > +		      ARRAY_SIZE(dlb->patches)))
> > +		return;
> > +
> > +	dlb->patches[dlb->num_patches].entry = &dlb->entries[dlb->num_entries];
> > +	dlb->patches[dlb->num_patches].data = patch;
> > +	dlb->num_patches++;
> > +
> > +	dlb->entries[dlb->num_entries].addr = reg;
> > +	dlb->entries[dlb->num_entries].data = data;
> > +	dlb->num_entries++;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Display List Extended Command Management
> >   */
> > @@ -652,6 +690,7 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
> >  	 * has at least one body, thus we reinitialise the entries list.
> >  	 */
> >  	dl->body0->num_entries = 0;
> > +	dl->body0->num_patches = 0;
> >  
> >  	list_add_tail(&dl->list, &dl->dlm->free);
> >  }
> > @@ -930,6 +969,36 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
> >   * Display List Manager
> >   */
> >  
> > +/**
> > + * vsp1_dlm_irq_display_start - Display list handler for the display start
> > + *	interrupt
> > + * @dlm: the display list manager
> > + *
> > + * Apply all patches registered for the active display list. This is used to
> > + * disable writeback for the next frame.
> > + */
> > +void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm)
> > +{
> > +	struct vsp1_dl_body *dlb;
> > +	struct vsp1_dl_list *dl;
> > +	unsigned int i;
> > +
> > +	spin_lock(&dlm->lock);
> > +
> > +	dl = dlm->active;
> > +	if (!dl)
> > +		goto done;
> > +
> > +	list_for_each_entry(dlb, &dl->bodies, list) {
> > +		for (i = 0; i < dlb->num_patches; ++i)
> > +			dlb->patches[i].entry->data = dlb->patches[i].data;
> > +		dlb->num_patches = 0;
> > +	}
> > +
> > +done:
> > +	spin_unlock(&dlm->lock);
> > +}
> > +
> >  /**
> >   * vsp1_dlm_irq_frame_end - Display list handler for the frame end interrupt
> >   * @dlm: the display list manager
> > @@ -947,6 +1016,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.
> >   */
> >  unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> >  {
> > @@ -984,6 +1056,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..cbaa0bf0cbc2 100644
> > --- a/drivers/media/platform/vsp1/vsp1_dl.h
> > +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> > @@ -19,6 +19,7 @@ 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(2)
> >  
> >  /**
> >   * struct vsp1_dl_ext_cmd - Extended Display command
> > @@ -54,6 +55,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
> >  					unsigned int prealloc);
> >  void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
> >  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
> > +void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm);
> >  unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
> >  struct vsp1_dl_body *vsp1_dlm_dl_body_get(struct vsp1_dl_manager *dlm);
> >  
> > @@ -71,6 +73,8 @@ struct vsp1_dl_body *vsp1_dl_body_get(struct vsp1_dl_body_pool *pool);
> >  void vsp1_dl_body_put(struct vsp1_dl_body *dlb);
> >  
> >  void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 reg, u32 data);
> > +void vsp1_dl_body_write_patch(struct vsp1_dl_body *dlb, u32 reg, u32 data,
> > +			      u32 patch);
> >  int vsp1_dl_list_add_body(struct vsp1_dl_list *dl, struct vsp1_dl_body *dlb);
> >  int vsp1_dl_list_add_chain(struct vsp1_dl_list *head, struct vsp1_dl_list *dl);
> >  
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> > index 9d20ef5cd5f8..e9d0ce432a2c 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -23,6 +23,7 @@
> >  #include "vsp1_pipe.h"
> >  #include "vsp1_rwpf.h"
> >  #include "vsp1_uif.h"
> > +#include "vsp1_video.h"
> >  
> >  #define BRX_NAME(e)	(e)->type == VSP1_ENTITY_BRU ? "BRU" : "BRS"
> >  
> > @@ -34,7 +35,7 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> >  				       unsigned int completion)
> >  {
> >  	struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
> > -	bool complete = completion == VSP1_DL_FRAME_END_COMPLETED;
> > +	bool complete = completion & VSP1_DL_FRAME_END_COMPLETED;
> >  
> >  	if (drm_pipe->du_complete) {
> >  		struct vsp1_entity *uif = drm_pipe->uif;
> > @@ -48,6 +49,9 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
> >  		drm_pipe->force_brx_release = false;
> >  		wake_up(&drm_pipe->wait_queue);
> >  	}
> > +
> > +	if (completion & VSP1_DL_FRAME_END_WRITEBACK)
> > +		vsp1_video_wb_frame_end(pipe->output->video);
> >  }
> >  
> >  /* -----------------------------------------------------------------------------
> > @@ -541,6 +545,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);
> > @@ -859,8 +865,14 @@ void vsp1_du_atomic_flush(struct device *dev, unsigned int pipe_index,
> >  	drm_pipe->crc = cfg->crc;
> >  
> >  	mutex_lock(&vsp1->drm->lock);
> > +
> > +	/* If we have a writeback node attached, update the video buffers. */
> > +	if (pipe->output->video)
> > +		vsp1_video_wb_prepare(pipe->output->video);
> > +
> >  	vsp1_du_pipeline_setup_inputs(vsp1, pipe);
> >  	vsp1_du_pipeline_configure(pipe);
> > +
> >  	mutex_unlock(&vsp1->drm->lock);
> >  }
> >  EXPORT_SYMBOL_GPL(vsp1_du_atomic_flush);
> > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
> > index c650e45bb0ad..235febd18ffa 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drv.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> > @@ -63,6 +63,21 @@ static irqreturn_t vsp1_irq_handler(int irq, void *data)
> >  			vsp1_pipeline_frame_end(wpf->entity.pipe);
> >  			ret = IRQ_HANDLED;
> >  		}
> > +
> > +		/*
> > +		 * Process the display start interrupt after the frame end
> > +		 * interrupt to make sure the display list queue is correctly
> > +		 * updated when processing the display start.
> > +		 */
> > +		if (wpf->has_writeback) {
> > +			status = vsp1_read(vsp1, VI6_DISP_IRQ_STA(i));
> > +			vsp1_write(vsp1, VI6_DISP_IRQ_STA(i), ~status & mask);
> > +
> > +			if (status & VI6_DISP_IRQ_STA_DST) {
> > +				vsp1_pipeline_display_start(wpf->entity.pipe);
> > +				ret = IRQ_HANDLED;
> > +			}
> > +		}
> >  	}
> >  
> >  	return ret;
> > @@ -435,7 +450,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
> >  		vsp1->wpf[i] = wpf;
> >  		list_add_tail(&wpf->entity.list_dev, &vsp1->entities);
> >  
> > -		if (vsp1->info->uapi) {
> > +		if (vsp1->info->uapi || wpf->has_writeback) {
> >  			struct vsp1_video *video = vsp1_video_create(vsp1, wpf);
> >  
> >  			if (IS_ERR(video)) {
> > diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c
> > index 54ff539ffea0..016800c45bc1 100644
> > --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> > +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> > @@ -309,6 +309,11 @@ bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe)
> >  	return pipe->buffers_ready == mask;
> >  }
> >  
> > +void vsp1_pipeline_display_start(struct vsp1_pipeline *pipe)
> > +{
> > +	vsp1_dlm_irq_display_start(pipe->output->dlm);
> > +}
> > +
> >  void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
> >  {
> >  	unsigned int flags;
> > diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
> > index ae646c9ef337..82d51b891f1e 100644
> > --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> > +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> > @@ -47,6 +47,11 @@ struct vsp1_format_info {
> >  	bool alpha;
> >  };
> >  
> > +static inline bool vsp1_format_is_rgb(const struct vsp1_format_info *info)
> > +{
> > +	return info->mbus == MEDIA_BUS_FMT_ARGB8888_1X32;
> > +}
> > +
> >  enum vsp1_pipeline_state {
> >  	VSP1_PIPELINE_STOPPED,
> >  	VSP1_PIPELINE_RUNNING,
> > @@ -158,6 +163,7 @@ bool vsp1_pipeline_stopped(struct vsp1_pipeline *pipe);
> >  int vsp1_pipeline_stop(struct vsp1_pipeline *pipe);
> >  bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe);
> >  
> > +void vsp1_pipeline_display_start(struct vsp1_pipeline *pipe);
> >  void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe);
> >  
> >  void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe,
> > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h b/drivers/media/platform/vsp1/vsp1_rwpf.h
> > index 70742ecf766f..910990b27617 100644
> > --- a/drivers/media/platform/vsp1/vsp1_rwpf.h
> > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
> > @@ -35,6 +35,7 @@ struct vsp1_rwpf {
> >  	struct v4l2_ctrl_handler ctrls;
> >  
> >  	struct vsp1_video *video;
> > +	bool has_writeback;
> >  
> >  	unsigned int max_width;
> >  	unsigned int max_height;
> > @@ -61,6 +62,7 @@ struct vsp1_rwpf {
> >  	} flip;
> >  
> >  	struct vsp1_rwpf_memory mem;
> > +	bool writeback;
> >  
> >  	struct vsp1_dl_manager *dlm;
> >  };
> > diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> > index 8feaa8d89fe8..0ac3430292d0 100644
> > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > @@ -34,7 +34,6 @@
> >  #include "vsp1_uds.h"
> >  #include "vsp1_video.h"
> >  
> > -#define VSP1_VIDEO_DEF_FORMAT		V4L2_PIX_FMT_YUYV
> >  #define VSP1_VIDEO_DEF_WIDTH		1024
> >  #define VSP1_VIDEO_DEF_HEIGHT		768
> >  
> > @@ -45,6 +44,14 @@
> >   * Helper functions
> >   */
> >  
> > +static inline unsigned int vsp1_video_default_format(struct vsp1_video *video)
> > +{
> > +	if (video->rwpf->has_writeback)
> > +		return V4L2_PIX_FMT_RGB24;
> > +	else
> > +		return V4L2_PIX_FMT_YUYV;
> > +}
> > +
> >  static struct v4l2_subdev *
> >  vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
> >  {
> > @@ -113,11 +120,13 @@ static int __vsp1_video_try_format(struct vsp1_video *video,
> >  
> >  	/*
> >  	 * Retrieve format information and select the default format if the
> > -	 * requested format isn't supported.
> > +	 * requested format isn't supported. Writeback video nodes only support
> > +	 * RGB formats.
> >  	 */
> >  	info = vsp1_get_format_info(video->vsp1, pix->pixelformat);
> > -	if (info == NULL)
> > -		info = vsp1_get_format_info(video->vsp1, VSP1_VIDEO_DEF_FORMAT);
> > +	if (!info || (video->rwpf->has_writeback && !vsp1_format_is_rgb(info)))
> > +		info = vsp1_get_format_info(video->vsp1,
> > +					    vsp1_video_default_format(video));
> >  
> >  	pix->pixelformat = info->fourcc;
> >  	pix->colorspace = V4L2_COLORSPACE_SRGB;
> > @@ -946,6 +955,122 @@ static const struct vb2_ops vsp1_video_queue_qops = {
> >  	.stop_streaming = vsp1_video_stop_streaming,
> >  };
> >  
> > +/* -----------------------------------------------------------------------------
> > + * Writeback Support
> > + */
> > +
> > +static void vsp1_video_wb_buffer_queue(struct vb2_buffer *vb)
> > +{
> > +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > +	struct vsp1_video *video = vb2_get_drv_priv(vb->vb2_queue);
> > +	struct vsp1_vb2_buffer *buf = to_vsp1_vb2_buffer(vbuf);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&video->irqlock, flags);
> > +	list_add_tail(&buf->queue, &video->irqqueue);
> > +	spin_unlock_irqrestore(&video->irqlock, flags);
> > +}
> > +
> > +static int vsp1_video_wb_start_streaming(struct vb2_queue *vq,
> > +					 unsigned int count)
> > +{
> > +	struct vsp1_video *video = vb2_get_drv_priv(vq);
> > +
> > +	video->wb_running = true;
> > +	return 0;
> > +}
> > +
> > +static bool vsp1_video_wb_stopped(struct vsp1_video *video)
> > +{
> > +	unsigned long flags;
> > +	bool stopped;
> > +
> > +	spin_lock_irqsave(&video->irqlock, flags);
> > +	stopped = list_empty(&video->wb_queue);
> > +	spin_unlock_irqrestore(&video->irqlock, flags);
> > +
> > +	return stopped;
> > +}
> > +
> > +static void vsp1_video_wb_stop_streaming(struct vb2_queue *vq)
> > +{
> > +	struct vsp1_video *video = vb2_get_drv_priv(vq);
> > +	struct vsp1_rwpf *rwpf = video->rwpf;
> > +	struct vsp1_pipeline *pipe = rwpf->entity.pipe;
> > +	unsigned long flags;
> > +	int ret;
> > +
> > +	/* Disable writeback and wait for the pending frames to complete. */
> > +	spin_lock_irqsave(&video->irqlock, flags);
> > +	video->wb_running = false;
> > +	spin_unlock_irqrestore(&video->irqlock, flags);
> > +
> > +	ret = wait_event_timeout(pipe->wq, vsp1_video_wb_stopped(video),
> > +				 msecs_to_jiffies(500));
> > +	if (ret == 0) {
> > +		dev_err(video->vsp1->dev, "writeback stop timeout\n");
> > +		list_splice_init(&video->wb_queue, &video->irqqueue);
> > +	}
> > +
> > +	vsp1_video_release_buffers(video);
> > +}
> > +
> > +static const struct vb2_ops vsp1_video_wb_queue_qops = {
> > +	.queue_setup = vsp1_video_queue_setup,
> > +	.buf_prepare = vsp1_video_buffer_prepare,
> > +	.buf_queue = vsp1_video_wb_buffer_queue,
> > +	.wait_prepare = vb2_ops_wait_prepare,
> > +	.wait_finish = vb2_ops_wait_finish,
> > +	.start_streaming = vsp1_video_wb_start_streaming,
> > +	.stop_streaming = vsp1_video_wb_stop_streaming,
> > +};
> > +
> > +void vsp1_video_wb_prepare(struct vsp1_video *video)
> > +{
> > +	struct vsp1_vb2_buffer *buf;
> > +	unsigned long flags;
> > +
> > +	/*
> > +	 * If writeback is disabled, return immediately. Otherwise remove the
> > +	 * first buffer from the irqqueue, add it to the writeback queue and
> > +	 * configure the WPF for writeback.
> > +	 */
> > +
> > +	spin_lock_irqsave(&video->irqlock, flags);
> > +
> > +	if (!video->wb_running) {
> > +		spin_unlock_irqrestore(&video->irqlock, flags);
> > +		return;
> > +	}
> > +
> > +	buf = list_first_entry_or_null(&video->irqqueue, struct vsp1_vb2_buffer,
> > +				       queue);
> > +	if (buf)
> > +		list_move_tail(&buf->queue, &video->wb_queue);
> > +
> > +	spin_unlock_irqrestore(&video->irqlock, flags);
> > +
> > +	if (buf) {
> > +		video->rwpf->mem = buf->mem;
> > +		video->rwpf->writeback = true;
> > +	}
> > +}
> > +
> > +void vsp1_video_wb_frame_end(struct vsp1_video *video)
> > +{
> > +	struct vsp1_vb2_buffer *done;
> > +	unsigned long flags;
> > +
> > +	/* Complete the first buffer from the writeback queue. */
> > +	spin_lock_irqsave(&video->irqlock, flags);
> > +	done = list_first_entry(&video->wb_queue, struct vsp1_vb2_buffer,
> > +				queue);
> > +	list_del(&done->queue);
> > +	spin_unlock_irqrestore(&video->irqlock, flags);
> > +
> > +	vsp1_video_complete_buffer(video, done);
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * V4L2 ioctls
> >   */
> > @@ -1045,6 +1170,13 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
> >  	if (video->queue.owner && video->queue.owner != file->private_data)
> >  		return -EBUSY;
> >  
> > +	/*
> > +	 * Writeback video devices don't need to interface with the pipeline or
> > +	 * verify formats, just turn streaming on.
> > +	 */
> > +	if (video->rwpf->has_writeback)
> > +		return vb2_streamon(&video->queue, type);
> > +
> >  	/*
> >  	 * Get a pipeline for the video node and start streaming on it. No link
> >  	 * touching an entity in the pipeline can be activated or deactivated
> > @@ -1273,7 +1405,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
> >  		video->pad.flags = MEDIA_PAD_FL_SOURCE;
> >  		video->video.vfl_dir = VFL_DIR_TX;
> >  	} else {
> > -		direction = "output";
> > +		direction = rwpf->has_writeback ? "writeback" : "output";
> >  		video->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> >  		video->pad.flags = MEDIA_PAD_FL_SINK;
> >  		video->video.vfl_dir = VFL_DIR_RX;
> > @@ -1282,6 +1414,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
> >  	mutex_init(&video->lock);
> >  	spin_lock_init(&video->irqlock);
> >  	INIT_LIST_HEAD(&video->irqqueue);
> > +	INIT_LIST_HEAD(&video->wb_queue);
> >  
> >  	/* Initialize the media entity... */
> >  	ret = media_entity_pads_init(&video->video.entity, 1, &video->pad);
> > @@ -1289,7 +1422,7 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
> >  		return ERR_PTR(ret);
> >  
> >  	/* ... and the format ... */
> > -	rwpf->format.pixelformat = VSP1_VIDEO_DEF_FORMAT;
> > +	rwpf->format.pixelformat = vsp1_video_default_format(video);
> >  	rwpf->format.width = VSP1_VIDEO_DEF_WIDTH;
> >  	rwpf->format.height = VSP1_VIDEO_DEF_HEIGHT;
> >  	__vsp1_video_try_format(video, &rwpf->format, &rwpf->fmtinfo);
> > @@ -1310,7 +1443,10 @@ struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
> >  	video->queue.lock = &video->lock;
> >  	video->queue.drv_priv = video;
> >  	video->queue.buf_struct_size = sizeof(struct vsp1_vb2_buffer);
> > -	video->queue.ops = &vsp1_video_queue_qops;
> > +	if (rwpf->has_writeback)
> > +		video->queue.ops = &vsp1_video_wb_queue_qops;
> > +	else
> > +		video->queue.ops = &vsp1_video_queue_qops;
> >  	video->queue.mem_ops = &vb2_dma_contig_memops;
> >  	video->queue.timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> >  	video->queue.dev = video->vsp1->bus_master;
> > diff --git a/drivers/media/platform/vsp1/vsp1_video.h b/drivers/media/platform/vsp1/vsp1_video.h
> > index f3cf5e2fdf5a..13b357b07ee2 100644
> > --- a/drivers/media/platform/vsp1/vsp1_video.h
> > +++ b/drivers/media/platform/vsp1/vsp1_video.h
> > @@ -44,6 +44,9 @@ struct vsp1_video {
> >  	struct vb2_queue queue;
> >  	spinlock_t irqlock;
> >  	struct list_head irqqueue;
> > +
> > +	bool wb_running;
> > +	struct list_head wb_queue;
> >  };
> >  
> >  static inline struct vsp1_video *to_vsp1_video(struct video_device *vdev)
> > @@ -54,6 +57,9 @@ static inline struct vsp1_video *to_vsp1_video(struct video_device *vdev)
> >  void vsp1_video_suspend(struct vsp1_device *vsp1);
> >  void vsp1_video_resume(struct vsp1_device *vsp1);
> >  
> > +void vsp1_video_wb_prepare(struct vsp1_video *video);
> > +void vsp1_video_wb_frame_end(struct vsp1_video *video);
> > +
> >  struct vsp1_video *vsp1_video_create(struct vsp1_device *vsp1,
> >  				     struct vsp1_rwpf *rwpf);
> >  void vsp1_video_cleanup(struct vsp1_video *video);
> > diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> > index 18c49e3a7875..81650a625185 100644
> > --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> > +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> > @@ -240,6 +240,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >  	struct vsp1_device *vsp1 = wpf->entity.vsp1;
> >  	const struct v4l2_mbus_framefmt *source_format;
> >  	const struct v4l2_mbus_framefmt *sink_format;
> > +	unsigned int index = wpf->entity.index;
> >  	unsigned int i;
> >  	u32 outfmt = 0;
> >  	u32 srcrpf = 0;
> > @@ -250,8 +251,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >  	source_format = vsp1_entity_get_pad_format(&wpf->entity,
> >  						   wpf->entity.config,
> >  						   RWPF_PAD_SOURCE);
> > +
> >  	/* Format */
> > -	if (!pipe->lif) {
> > +	if (!pipe->lif || wpf->writeback) {
> >  		const struct v4l2_pix_format_mplane *format = &wpf->format;
> >  		const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
> >  
> > @@ -276,8 +278,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >  
> >  		vsp1_wpf_write(wpf, dlb, VI6_WPF_DSWAP, fmtinfo->swap);
> >  
> > -		if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) &&
> > -		    wpf->entity.index == 0)
> > +		if (vsp1_feature(vsp1, VSP1_HAS_WPF_HFLIP) && index == 0)
> >  			vsp1_wpf_write(wpf, dlb, VI6_WPF_ROT_CTRL,
> >  				       VI6_WPF_ROT_CTRL_LN16 |
> >  				       (256 << VI6_WPF_ROT_CTRL_LMEM_WD_SHIFT));
> > @@ -288,11 +289,9 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >  
> >  	wpf->outfmt = outfmt;
> >  
> > -	vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
> > +	vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(index),
> >  			   VI6_DPR_WPF_FPORCH_FP_WPFN);
> >  
> > -	vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(wpf->entity.index), 0);
> > -
> >  	/*
> >  	 * Sources. If the pipeline has a single input and BRx is not used,
> >  	 * configure it as the master layer. Otherwise configure all
> > @@ -318,9 +317,24 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
> >  	vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
> >  
> >  	/* Enable interrupts. */
> > -	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(wpf->entity.index), 0);
> > -	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(wpf->entity.index),
> > +	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(index), 0);
> > +	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(index),
> >  			   VI6_WFP_IRQ_ENB_DFEE);
> > +
> > +	/*
> > +	 * Configure writeback for display pipelines. The wpf writeback flag is
> > +	 * never set for memory-to-memory pipelines.
> > +	 */
> > +	vsp1_dl_body_write(dlb, VI6_DISP_IRQ_STA(index), 0);
> > +	if (wpf->writeback) {
> 
> I feel like a comment here would be useful to make it clear that by
> using _patch the VI6_DISP_IRQ_ENB_DSTE, and VI6_WPF_WRBCK_CTRL_WBMD will
> be reset to 0 at the frame-end?
> 
> But maybe that's too verbose ... and won't be an issue once the function
> documentation is updated for vsp1_dl_body_write_patch().

I'll add a comment.

> > +		vsp1_dl_body_write_patch(dlb, VI6_DISP_IRQ_ENB(index),
> > +					 VI6_DISP_IRQ_ENB_DSTE, 0);
> > +		vsp1_dl_body_write_patch(dlb, VI6_WPF_WRBCK_CTRL(index),
> > +					 VI6_WPF_WRBCK_CTRL_WBMD, 0);
> > +	} else {
> > +		vsp1_dl_body_write(dlb, VI6_DISP_IRQ_ENB(index), 0);
> > +		vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL(index), 0);
> > +	}
> >  }
> >  
> >  static void wpf_configure_frame(struct vsp1_entity *entity,
> > @@ -390,7 +404,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> >  		       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
> >  		       (height << VI6_WPF_SZCLIP_SIZE_SHIFT));
> >  
> > -	if (pipe->lif)
> > +	/*
> > +	 * For display pipelines without writeback enabled there's no memory
> > +	 * address to configure, return now.
> > +	 */
> > +	if (pipe->lif && !wpf->writeback)
> >  		return;
> >  
> >  	/*
> > @@ -479,6 +497,12 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> >  	vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_Y, mem.addr[0]);
> >  	vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C0, mem.addr[1]);
> >  	vsp1_wpf_write(wpf, dlb, VI6_WPF_DSTM_ADDR_C1, mem.addr[2]);
> > +
> > +	/*
> > +	 * Writeback operates in single-shot mode and lasts for a single frame,
> > +	 * reset the writeback flag to false for the next frame.
> > +	 */
> > +	wpf->writeback = false;
> 
> This differs from my implementation right? I think my version just ran
> whenever there was a buffer available. (Except that when there was no
> atomic_flush - there was a large amount of latency...)

Yes, it differs quite a bit.

> I guess this comes down to the fact that we will not queue up frames
> except unless there is a real frame-change caused by a fresh
> atomic_flush ... and so the buffer rate does not reflect the display
> refresh rate.

You can queue buffers at any time but they will only be processed on an
atomic commit.

> Now that I've written the above, I think that's made the reasoning
> clearer for me so I've answered my own questions :)
> 
> >  }
> >  
> >  static unsigned int wpf_max_width(struct vsp1_entity *entity,
> > @@ -529,6 +553,13 @@ struct vsp1_rwpf *vsp1_wpf_create(struct vsp1_device *vsp1, unsigned int index)
> >  		wpf->max_height = WPF_GEN3_MAX_HEIGHT;
> >  	}
> >  
> > +	/*
> > +	 * On Gen3 WPFs with a LIF output can also write to memory for display
> 
> Hrm ... I might have said 'with an LIF'. ... but it depends whether you say:
>    'with a liph'
> 
> or you spell it out:
> 
>    'with an ell-eye-eff
> 
> I personally spell out acronyms in my head, so that makes is 'an'.

I do it the other way :-)

> It doesn't matter which really here though :-)
> 
> > +	 * writeback.
> > +	 */
> > +	if (vsp1->info->gen > 2 && index < vsp1->info->lif_count)
> > +		wpf->has_writeback = true;
> > +
> >  	wpf->entity.ops = &wpf_entity_ops;
> >  	wpf->entity.type = VSP1_ENTITY_WPF;
> >  	wpf->entity.index = index;

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux