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 Laurent,

On 17/02/2019 20:06, Kieran Bingham wrote:
> Hi Laurent,
> 
> Thank you for updating the patch,
> 
> 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.
> 
>> - 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 :)
> 
> 
>>
>> 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.
> 
> 
>>  };
>>  
>>  /**
>> @@ -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/
> 
> "Store an update to an existing register for handling at display-start
> interrupt...."? (or as you see fit)
> 
> 
>> + * @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)",

And I'm sure you can handle the %lu here as highlighted by the kbuild-robot.


>> +		      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().
> 
> 
>> +		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...)
> 
> 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.
> 
> 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'.
> 
> 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
--
Kieran



[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