[PATCH v4 0/7] VSP1: Display writeback support

[PATCH v4 0/7] VSP1: Display writeback support

[Laurent Pinchart]

Hello,

This patch series implements display writeback support for the R-Car
Gen3 platforms in the VSP1 driver.

DRM/KMS provides a writeback API through a special type of writeback
connectors. This series takes a different approach by exposing writeback
as a V4L2 device. While there is nothing fundamentally wrong with
writeback connectors, display for R-Car Gen3 platforms relies on the
VSP1 driver behind the scene, which already implements V4L2 support.
Enabling writeback through V4L2 is thus significantly easier in this
case.

The writeback pixel format is restricted to RGB, due to the VSP1
outputting RGB to the display and lacking a separate colour space
conversion unit for writeback. The resolution can be freely picked by
will result in cropping or composing, not scaling.

Writeback requests are queued to the hardware on page flip (atomic
flush), and complete at the next vblank. This means that a queued
writeback buffer will not be processed until the next page flip, but
once it starts being written to by the VSP, it will complete at the next
vblank regardless of whether another page flip occurs at that time.

The code is based on a merge of the media master branch, the drm-next
branch and the R-Car DT next branch. For convenience patches can be
found at

	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback

Kieran Bingham (2):
  Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
  media: vsp1: Provide a writeback video device

Laurent Pinchart (5):
  media: vsp1: wpf: Fix partition configuration for display pipelines
  media: vsp1: Replace leftover occurrence of fragment with body
  media: vsp1: Fix addresses of display-related registers for VSP-DL
  media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
  media: vsp1: Replace the display list internal flag with a flags field

 drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
 drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
 drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
 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_regs.h  |   6 +-
 drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
 drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
 drivers/media/platform/vsp1/vsp1_video.h |   6 +
 drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
 11 files changed, 378 insertions(+), 75 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v4 1/7] Revert "[media] v4l: vsp1: Supply frames to the DU continuously"

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

This reverts commit 3299ba5c0b21 ("[media] v4l: vsp1: Supply frames to
the DU continuously")

The DU output mode does not rely on frames being supplied on the WPF as
its pipeline is supplied from DRM. For the upcoming WPF writeback
functionality, we will choose to enable writeback mode if there is an
output buffer, or disable it (leaving the existing display pipeline
unharmed) otherwise.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_video.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 7ceaf3222145..328d686189be 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -307,11 +307,6 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
  * This function completes the current buffer by filling its sequence number,
  * time stamp and payload size, and hands it back to the videobuf core.
  *
- * When operating in DU output mode (deep pipeline to the DU through the LIF),
- * the VSP1 needs to constantly supply frames to the display. In that case, if
- * no other buffer is queued, reuse the one that has just been processed instead
- * of handing it back to the videobuf core.
- *
  * Return the next queued buffer or NULL if the queue is empty.
  */
 static struct vsp1_vb2_buffer *
@@ -333,12 +328,6 @@ vsp1_video_complete_buffer(struct vsp1_video *video)
 	done = list_first_entry(&video->irqqueue,
 				struct vsp1_vb2_buffer, queue);
 
-	/* In DU output mode reuse the buffer if the list is singular. */
-	if (pipe->lif && list_is_singular(&video->irqqueue)) {
-		spin_unlock_irqrestore(&video->irqlock, flags);
-		return done;
-	}
-
 	list_del(&done->queue);
 
 	if (!list_empty(&video->irqqueue))
-- 
Regards,

Laurent Pinchart

[PATCH v4 2/7] media: vsp1: wpf: Fix partition configuration for display pipelines

[Laurent Pinchart]

The WPF accesses partition configuration from pipe->partition in the
partition configuration that is not used for display pipelines.
Writeback support will require full configuration of the WPF while not
providing a valid pipe->partition. Rework the configuration code to fall
back to the full image width in that case, as is already done for the
part of the configuration currently relevant for display pipelines.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_wpf.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index 32bb207b2007..a07c5944b598 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -362,6 +362,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
 	const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
 	unsigned int width;
 	unsigned int height;
+	unsigned int left;
 	unsigned int offset;
 	unsigned int flip;
 	unsigned int i;
@@ -371,13 +372,16 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
 						 RWPF_PAD_SINK);
 	width = sink_format->width;
 	height = sink_format->height;
+	left = 0;
 
 	/*
 	 * Cropping. The partition algorithm can split the image into
 	 * multiple slices.
 	 */
-	if (pipe->partitions > 1)
+	if (pipe->partitions > 1) {
 		width = pipe->partition->wpf.width;
+		left = pipe->partition->wpf.left;
+	}
 
 	vsp1_wpf_write(wpf, dlb, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
 		       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
@@ -408,13 +412,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
 	flip = wpf->flip.active;
 
 	if (flip & BIT(WPF_CTRL_HFLIP) && !wpf->flip.rotate)
-		offset = format->width - pipe->partition->wpf.left
-			- pipe->partition->wpf.width;
+		offset = format->width - left - width;
 	else if (flip & BIT(WPF_CTRL_VFLIP) && wpf->flip.rotate)
-		offset = format->height - pipe->partition->wpf.left
-			- pipe->partition->wpf.width;
+		offset = format->height - left - width;
 	else
-		offset = pipe->partition->wpf.left;
+		offset = left;
 
 	for (i = 0; i < format->num_planes; ++i) {
 		unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
@@ -436,7 +438,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
 		 * image height.
 		 */
 		if (wpf->flip.rotate)
-			height = pipe->partition->wpf.width;
+			height = width;
 		else
 			height = format->height;
 
-- 
Regards,

Laurent Pinchart

[PATCH v4 3/7] media: vsp1: Replace leftover occurrence of fragment with body

[Laurent Pinchart]

Display list fragments have been renamed to bodies. Replace one last
occurrence of the word fragment in the documentation.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_dl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index 26289adaf658..64af449791b0 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -699,8 +699,8 @@ struct vsp1_dl_body *vsp1_dl_list_get_body0(struct vsp1_dl_list *dl)
  * which bodies are added.
  *
  * Adding a body to a display list passes ownership of the body to the list. The
- * caller retains its reference to the fragment when adding it to the display
- * list, but is not allowed to add new entries to the body.
+ * caller retains its reference to the body when adding it to the display list,
+ * but is not allowed to add new entries to the body.
  *
  * The reference must be explicitly released by a call to vsp1_dl_body_put()
  * when the body isn't needed anymore.
-- 
Regards,

Laurent Pinchart

[PATCH v4 4/7] media: vsp1: Fix addresses of display-related registers for VSP-DL

[Laurent Pinchart]

The VSP-DL instances have two LIFs, and thus two copies of the
VI6_DISP_IRQ_ENB, VI6_DISP_IRQ_STA and VI6_WPF_WRBCK_CTRL registers. Fix
the corresponding macros accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c  | 4 ++--
 drivers/media/platform/vsp1/vsp1_regs.h | 6 +++---
 drivers/media/platform/vsp1/vsp1_wpf.c  | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 8d86f618ec77..048190fd3a2d 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -700,8 +700,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
 	drm_pipe->du_private = cfg->callback_data;
 
 	/* Disable the display interrupts. */
-	vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
-	vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
+	vsp1_write(vsp1, VI6_DISP_IRQ_STA(pipe_index), 0);
+	vsp1_write(vsp1, VI6_DISP_IRQ_ENB(pipe_index), 0);
 
 	/* Configure all entities in the pipeline. */
 	vsp1_du_pipeline_configure(pipe);
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
index f6e4157095cc..1bb1d39c60d9 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -39,12 +39,12 @@
 #define VI6_WFP_IRQ_STA_DFE		(1 << 1)
 #define VI6_WFP_IRQ_STA_FRE		(1 << 0)
 
-#define VI6_DISP_IRQ_ENB		0x0078
+#define VI6_DISP_IRQ_ENB(n)		(0x0078 + (n) * 60)
 #define VI6_DISP_IRQ_ENB_DSTE		(1 << 8)
 #define VI6_DISP_IRQ_ENB_MAEE		(1 << 5)
 #define VI6_DISP_IRQ_ENB_LNEE(n)	(1 << (n))
 
-#define VI6_DISP_IRQ_STA		0x007c
+#define VI6_DISP_IRQ_STA(n)		(0x007c + (n) * 60)
 #define VI6_DISP_IRQ_STA_DST		(1 << 8)
 #define VI6_DISP_IRQ_STA_MAE		(1 << 5)
 #define VI6_DISP_IRQ_STA_LNE(n)		(1 << (n))
@@ -307,7 +307,7 @@
 #define VI6_WPF_DSTM_ADDR_C0		0x1028
 #define VI6_WPF_DSTM_ADDR_C1		0x102c
 
-#define VI6_WPF_WRBCK_CTRL		0x1034
+#define VI6_WPF_WRBCK_CTRL(n)		(0x1034 + (n) * 0x100)
 #define VI6_WPF_WRBCK_CTRL_WBMD		(1 << 0)
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index a07c5944b598..18c49e3a7875 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -291,7 +291,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
 	vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
 			   VI6_DPR_WPF_FPORCH_FP_WPFN);
 
-	vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL, 0);
+	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,
-- 
Regards,

Laurent Pinchart

[PATCH v4 5/7] media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse

[Laurent Pinchart]

The vsp1_video_complete_buffer() function completes the current buffer
and returns a pointer to the next buffer. Split the code that completes
the buffer to a separate function for later reuse, and rename
vsp1_video_complete_buffer() to vsp1_video_complete_next_buffer().

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_video.c | 35 ++++++++++++++----------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 328d686189be..cfbab16c4820 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -300,8 +300,22 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
  * Pipeline Management
  */
 
+static void vsp1_video_complete_buffer(struct vsp1_video *video,
+				       struct vsp1_vb2_buffer *buffer)
+{
+	struct vsp1_pipeline *pipe = video->rwpf->entity.pipe;
+	unsigned int i;
+
+	buffer->buf.sequence = pipe->sequence;
+	buffer->buf.vb2_buf.timestamp = ktime_get_ns();
+	for (i = 0; i < buffer->buf.vb2_buf.num_planes; ++i)
+		vb2_set_plane_payload(&buffer->buf.vb2_buf, i,
+				      vb2_plane_size(&buffer->buf.vb2_buf, i));
+	vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_DONE);
+}
+
 /*
- * vsp1_video_complete_buffer - Complete the current buffer
+ * vsp1_video_complete_next_buffer - Complete the current buffer
  * @video: the video node
  *
  * This function completes the current buffer by filling its sequence number,
@@ -310,13 +324,11 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
  * Return the next queued buffer or NULL if the queue is empty.
  */
 static struct vsp1_vb2_buffer *
-vsp1_video_complete_buffer(struct vsp1_video *video)
+vsp1_video_complete_next_buffer(struct vsp1_video *video)
 {
-	struct vsp1_pipeline *pipe = video->rwpf->entity.pipe;
-	struct vsp1_vb2_buffer *next = NULL;
+	struct vsp1_vb2_buffer *next;
 	struct vsp1_vb2_buffer *done;
 	unsigned long flags;
-	unsigned int i;
 
 	spin_lock_irqsave(&video->irqlock, flags);
 
@@ -327,21 +339,14 @@ vsp1_video_complete_buffer(struct vsp1_video *video)
 
 	done = list_first_entry(&video->irqqueue,
 				struct vsp1_vb2_buffer, queue);
-
 	list_del(&done->queue);
 
-	if (!list_empty(&video->irqqueue))
-		next = list_first_entry(&video->irqqueue,
+	next = list_first_entry_or_null(&video->irqqueue,
 					struct vsp1_vb2_buffer, queue);
 
 	spin_unlock_irqrestore(&video->irqlock, flags);
 
-	done->buf.sequence = pipe->sequence;
-	done->buf.vb2_buf.timestamp = ktime_get_ns();
-	for (i = 0; i < done->buf.vb2_buf.num_planes; ++i)
-		vb2_set_plane_payload(&done->buf.vb2_buf, i,
-				      vb2_plane_size(&done->buf.vb2_buf, i));
-	vb2_buffer_done(&done->buf.vb2_buf, VB2_BUF_STATE_DONE);
+	vsp1_video_complete_buffer(video, done);
 
 	return next;
 }
@@ -352,7 +357,7 @@ static void vsp1_video_frame_end(struct vsp1_pipeline *pipe,
 	struct vsp1_video *video = rwpf->video;
 	struct vsp1_vb2_buffer *buf;
 
-	buf = vsp1_video_complete_buffer(video);
+	buf = vsp1_video_complete_next_buffer(video);
 	if (buf == NULL)
 		return;
 
-- 
Regards,

Laurent Pinchart

[PATCH v4 6/7] media: vsp1: Replace the display list internal flag with a flags field

[Laurent Pinchart]

To prepare for addition of more flags to the display list, replace the
'internal' flag field by a bitmask 'flags' field.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_dl.c    | 31 +++++++++++++-----------
 drivers/media/platform/vsp1/vsp1_dl.h    |  2 +-
 drivers/media/platform/vsp1/vsp1_drm.c   |  6 ++++-
 drivers/media/platform/vsp1/vsp1_video.c |  2 +-
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index 64af449791b0..886b3a69d329 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -178,7 +178,7 @@ struct vsp1_dl_cmd_pool {
  * @post_cmd: post command to be issued through extended dl header
  * @has_chain: if true, indicates that there's a partition chain
  * @chain: entry in the display list partition chain
- * @internal: whether the display list is used for internal purpose
+ * @flags: display list flags, a combination of VSP1_DL_FRAME_END_*
  */
 struct vsp1_dl_list {
 	struct list_head list;
@@ -197,7 +197,7 @@ struct vsp1_dl_list {
 	bool has_chain;
 	struct list_head chain;
 
-	bool internal;
+	unsigned int flags;
 };
 
 /**
@@ -861,13 +861,15 @@ static void vsp1_dl_list_commit_continuous(struct vsp1_dl_list *dl)
 	 *
 	 * If a display list is already pending we simply drop it as the new
 	 * display list is assumed to contain a more recent configuration. It is
-	 * an error if the already pending list has the internal flag set, as
-	 * there is then a process waiting for that list to complete. This
-	 * shouldn't happen as the waiting process should perform proper
-	 * locking, but warn just in case.
+	 * an error if the already pending list has the
+	 * VSP1_DL_FRAME_END_INTERNAL flag set, as there is then a process
+	 * waiting for that list to complete. This shouldn't happen as the
+	 * waiting process should perform proper locking, but warn just in
+	 * case.
 	 */
 	if (vsp1_dl_list_hw_update_pending(dlm)) {
-		WARN_ON(dlm->pending && dlm->pending->internal);
+		WARN_ON(dlm->pending &&
+			(dlm->pending->flags & VSP1_DL_FRAME_END_INTERNAL));
 		__vsp1_dl_list_put(dlm->pending);
 		dlm->pending = dl;
 		return;
@@ -897,7 +899,7 @@ static void vsp1_dl_list_commit_singleshot(struct vsp1_dl_list *dl)
 	dlm->active = dl;
 }
 
-void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
+void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
 {
 	struct vsp1_dl_manager *dlm = dl->dlm;
 	struct vsp1_dl_list *dl_next;
@@ -912,7 +914,7 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
 		vsp1_dl_list_fill_header(dl_next, last);
 	}
 
-	dl->internal = internal;
+	dl->flags = dl_flags & ~VSP1_DL_FRAME_END_COMPLETED;
 
 	spin_lock_irqsave(&dlm->lock, flags);
 
@@ -941,9 +943,10 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
  * set in single-shot mode as display list processing is then not continuous and
  * races never occur.
  *
- * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the previous display list
- * has completed and had been queued with the internal notification flag.
- * Internal notification is only supported for continuous mode.
+ * The following flags are only supported for continuous mode.
+ *
+ * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
+ * became active had been queued with the internal notification flag.
  */
 unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 {
@@ -986,9 +989,9 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 	 * frame end interrupt. The display list thus becomes active.
 	 */
 	if (dlm->queued) {
-		if (dlm->queued->internal)
+		if (dlm->queued->flags & VSP1_DL_FRAME_END_INTERNAL)
 			flags |= VSP1_DL_FRAME_END_INTERNAL;
-		dlm->queued->internal = false;
+		dlm->queued->flags &= ~VSP1_DL_FRAME_END_INTERNAL;
 
 		__vsp1_dl_list_put(dlm->active);
 		dlm->active = dlm->queued;
diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
index 125750dc8b5c..e0fdb145e6ed 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.h
+++ b/drivers/media/platform/vsp1/vsp1_dl.h
@@ -61,7 +61,7 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
 void vsp1_dl_list_put(struct vsp1_dl_list *dl);
 struct vsp1_dl_body *vsp1_dl_list_get_body0(struct vsp1_dl_list *dl);
 struct vsp1_dl_ext_cmd *vsp1_dl_get_pre_cmd(struct vsp1_dl_list *dl);
-void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal);
+void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags);
 
 struct vsp1_dl_body_pool *
 vsp1_dl_body_pool_create(struct vsp1_device *vsp1, unsigned int num_bodies,
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 048190fd3a2d..9d20ef5cd5f8 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -537,6 +537,10 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
 	struct vsp1_entity *next;
 	struct vsp1_dl_list *dl;
 	struct vsp1_dl_body *dlb;
+	unsigned int dl_flags = 0;
+
+	if (drm_pipe->force_brx_release)
+		dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
 
 	dl = vsp1_dl_list_get(pipe->output->dlm);
 	dlb = vsp1_dl_list_get_body0(dl);
@@ -559,7 +563,7 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
 		vsp1_entity_configure_partition(entity, pipe, dl, dlb);
 	}
 
-	vsp1_dl_list_commit(dl, drm_pipe->force_brx_release);
+	vsp1_dl_list_commit(dl, dl_flags);
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index cfbab16c4820..8feaa8d89fe8 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -426,7 +426,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
 	}
 
 	/* Complete, and commit the head display list. */
-	vsp1_dl_list_commit(dl, false);
+	vsp1_dl_list_commit(dl, 0);
 	pipe->configured = true;
 
 	vsp1_pipeline_run(pipe);
-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
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
- Periods at the end of sentences.

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];
 };
 
 /**
@@ -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
+ * @dlb: The body
+ * @reg: The register address
+ * @data: The register value
+ *
+ * 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().
+ */
+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) {
+		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;
 }
 
 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
+	 * 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

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

[Kieran Bingham]

Hi Laurent,

Thank you for updating the patch,

On 17/02/2019 02:48, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> 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@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

For your changes,

With the documentation updated for vsp1_dl_body_write_patch(),

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
> 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)",
> +		      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

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

[Kieran Bingham]

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@ideasonboard.com>
>>
>> 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@ideasonboard.com>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> For your changes,
> 
> With the documentation updated for vsp1_dl_body_write_patch(),
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
>> ---
>> 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

Re: [PATCH v4 2/7] media: vsp1: wpf: Fix partition configuration for display pipelines

[Kieran Bingham]

Hi Laurent,

On 17/02/2019 02:48, Laurent Pinchart wrote:
> The WPF accesses partition configuration from pipe->partition in the
> partition configuration that is not used for display pipelines.

That sentence is hard to read...

> Writeback support will require full configuration of the WPF while not
> providing a valid pipe->partition. Rework the configuration code to fall
> back to the full image width in that case, as is already done for the
> part of the configuration currently relevant for display pipelines.
> 

Ah yes - this is probably a better route than allocating a table for a
single partition (which is what I had locally).


> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I like that this change also simplifies the flip/rotate handling code to
make that easier to read.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/vsp1/vsp1_wpf.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> index 32bb207b2007..a07c5944b598 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -362,6 +362,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
>  	const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
>  	unsigned int width;
>  	unsigned int height;
> +	unsigned int left;
>  	unsigned int offset;
>  	unsigned int flip;
>  	unsigned int i;
> @@ -371,13 +372,16 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
>  						 RWPF_PAD_SINK);
>  	width = sink_format->width;
>  	height = sink_format->height;
> +	left = 0;
>  
>  	/*
>  	 * Cropping. The partition algorithm can split the image into
>  	 * multiple slices.
>  	 */
> -	if (pipe->partitions > 1)
> +	if (pipe->partitions > 1) {
>  		width = pipe->partition->wpf.width;
> +		left = pipe->partition->wpf.left;
> +	}
>  
>  	vsp1_wpf_write(wpf, dlb, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
>  		       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
> @@ -408,13 +412,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
>  	flip = wpf->flip.active;
>  
>  	if (flip & BIT(WPF_CTRL_HFLIP) && !wpf->flip.rotate)
> -		offset = format->width - pipe->partition->wpf.left
> -			- pipe->partition->wpf.width;
> +		offset = format->width - left - width;
>  	else if (flip & BIT(WPF_CTRL_VFLIP) && wpf->flip.rotate)
> -		offset = format->height - pipe->partition->wpf.left
> -			- pipe->partition->wpf.width;
> +		offset = format->height - left - width;
>  	else
> -		offset = pipe->partition->wpf.left;
> +		offset = left;

This hunk looks a lot simpler :)


>  
>  	for (i = 0; i < format->num_planes; ++i) {
>  		unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
> @@ -436,7 +438,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
>  		 * image height.
>  		 */
>  		if (wpf->flip.rotate)
> -			height = pipe->partition->wpf.width;
> +			height = width;
>  		else
>  			height = format->height;
>  
> 

-- 
Regards
--
Kieran

Re: [PATCH v4 3/7] media: vsp1: Replace leftover occurrence of fragment with body

[Kieran Bingham]

Hi Laurent,

On 17/02/2019 02:48, Laurent Pinchart wrote:
> Display list fragments have been renamed to bodies. Replace one last
> occurrence of the word fragment in the documentation.
> 

Interesting, this must have been a change that was written before the
fragments were renamed, but got in /after/ they were removed or something.

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Regardless, it looks correct to fix this up.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/vsp1/vsp1_dl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> index 26289adaf658..64af449791b0 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -699,8 +699,8 @@ struct vsp1_dl_body *vsp1_dl_list_get_body0(struct vsp1_dl_list *dl)
>   * which bodies are added.
>   *
>   * Adding a body to a display list passes ownership of the body to the list. The
> - * caller retains its reference to the fragment when adding it to the display
> - * list, but is not allowed to add new entries to the body.
> + * caller retains its reference to the body when adding it to the display list,
> + * but is not allowed to add new entries to the body.
>   *
>   * The reference must be explicitly released by a call to vsp1_dl_body_put()
>   * when the body isn't needed anymore.
> 

-- 
Regards
--
Kieran

Re: [PATCH v4 4/7] media: vsp1: Fix addresses of display-related registers for VSP-DL

[Kieran Bingham]

Hi Laurent,

On 17/02/2019 02:48, Laurent Pinchart wrote:
> The VSP-DL instances have two LIFs, and thus two copies of the
> VI6_DISP_IRQ_ENB, VI6_DISP_IRQ_STA and VI6_WPF_WRBCK_CTRL registers. Fix
> the corresponding macros accordingly.
> 

Seep. This could have ended badly if someone used both LIF's :)
 (which I'm sure happens)


> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
>  drivers/media/platform/vsp1/vsp1_drm.c  | 4 ++--
>  drivers/media/platform/vsp1/vsp1_regs.h | 6 +++---
>  drivers/media/platform/vsp1/vsp1_wpf.c  | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 8d86f618ec77..048190fd3a2d 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -700,8 +700,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
>  	drm_pipe->du_private = cfg->callback_data;
>  
>  	/* Disable the display interrupts. */
> -	vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
> -	vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
> +	vsp1_write(vsp1, VI6_DISP_IRQ_STA(pipe_index), 0);
> +	vsp1_write(vsp1, VI6_DISP_IRQ_ENB(pipe_index), 0);
>  
>  	/* Configure all entities in the pipeline. */
>  	vsp1_du_pipeline_configure(pipe);
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
> index f6e4157095cc..1bb1d39c60d9 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -39,12 +39,12 @@
>  #define VI6_WFP_IRQ_STA_DFE		(1 << 1)
>  #define VI6_WFP_IRQ_STA_FRE		(1 << 0)
>  
> -#define VI6_DISP_IRQ_ENB		0x0078
> +#define VI6_DISP_IRQ_ENB(n)		(0x0078 + (n) * 60)
>  #define VI6_DISP_IRQ_ENB_DSTE		(1 << 8)
>  #define VI6_DISP_IRQ_ENB_MAEE		(1 << 5)
>  #define VI6_DISP_IRQ_ENB_LNEE(n)	(1 << (n))
>  
> -#define VI6_DISP_IRQ_STA		0x007c
> +#define VI6_DISP_IRQ_STA(n)		(0x007c + (n) * 60)
>  #define VI6_DISP_IRQ_STA_DST		(1 << 8)
>  #define VI6_DISP_IRQ_STA_MAE		(1 << 5)
>  #define VI6_DISP_IRQ_STA_LNE(n)		(1 << (n))
> @@ -307,7 +307,7 @@
>  #define VI6_WPF_DSTM_ADDR_C0		0x1028
>  #define VI6_WPF_DSTM_ADDR_C1		0x102c
>  
> -#define VI6_WPF_WRBCK_CTRL		0x1034
> +#define VI6_WPF_WRBCK_CTRL(n)		(0x1034 + (n) * 0x100)
>  #define VI6_WPF_WRBCK_CTRL_WBMD		(1 << 0)
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> index a07c5944b598..18c49e3a7875 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -291,7 +291,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
>  	vsp1_dl_body_write(dlb, VI6_DPR_WPF_FPORCH(wpf->entity.index),
>  			   VI6_DPR_WPF_FPORCH_FP_WPFN);
>  
> -	vsp1_dl_body_write(dlb, VI6_WPF_WRBCK_CTRL, 0);
> +	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,
> 

-- 
Regards
--
Kieran

Re: [PATCH v4 5/7] media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse

[Kieran Bingham]

Hi Laurent

On 17/02/2019 02:48, Laurent Pinchart wrote:
> The vsp1_video_complete_buffer() function completes the current buffer
> and returns a pointer to the next buffer. Split the code that completes
> the buffer to a separate function for later reuse, and rename
> vsp1_video_complete_buffer() to vsp1_video_complete_next_buffer().
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

This looks good to me - except perhaps the documentation /might/ need
some refresh. With or without updates there, the code changes look good
to me:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 35 ++++++++++++++----------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> index 328d686189be..cfbab16c4820 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -300,8 +300,22 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
>   * Pipeline Management
>   */
>  
> +static void vsp1_video_complete_buffer(struct vsp1_video *video,
> +				       struct vsp1_vb2_buffer *buffer)
> +{
> +	struct vsp1_pipeline *pipe = video->rwpf->entity.pipe;
> +	unsigned int i;
> +
> +	buffer->buf.sequence = pipe->sequence;
> +	buffer->buf.vb2_buf.timestamp = ktime_get_ns();
> +	for (i = 0; i < buffer->buf.vb2_buf.num_planes; ++i)
> +		vb2_set_plane_payload(&buffer->buf.vb2_buf, i,
> +				      vb2_plane_size(&buffer->buf.vb2_buf, i));
> +	vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
>  /*
> - * vsp1_video_complete_buffer - Complete the current buffer
> + * vsp1_video_complete_next_buffer - Complete the current buffer

Should the brief be updated?
It looks quite odd to call the function "complete next buffer" but with
a brief saying "complete the current buffer".

Technically it's still correct, but it's more like
"Complete the current buffer and return the next buffer"
or such.


>   * @video: the video node
>   *
>   * This function completes the current buffer by filling its sequence number,

Is this still accurate enough to keep the text as is ?



> @@ -310,13 +324,11 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
>   * Return the next queued buffer or NULL if the queue is empty.
>   */
>  static struct vsp1_vb2_buffer *
> -vsp1_video_complete_buffer(struct vsp1_video *video)
> +vsp1_video_complete_next_buffer(struct vsp1_video *video)
>  {
> -	struct vsp1_pipeline *pipe = video->rwpf->entity.pipe;
> -	struct vsp1_vb2_buffer *next = NULL;
> +	struct vsp1_vb2_buffer *next;
>  	struct vsp1_vb2_buffer *done;
>  	unsigned long flags;
> -	unsigned int i;
>  
>  	spin_lock_irqsave(&video->irqlock, flags);
>  
> @@ -327,21 +339,14 @@ vsp1_video_complete_buffer(struct vsp1_video *video)
>  
>  	done = list_first_entry(&video->irqqueue,
>  				struct vsp1_vb2_buffer, queue);
> -
>  	list_del(&done->queue);
>  
> -	if (!list_empty(&video->irqqueue))
> -		next = list_first_entry(&video->irqqueue,
> +	next = list_first_entry_or_null(&video->irqqueue,

That's a nice way to save a line.


>  					struct vsp1_vb2_buffer, queue);
>  
>  	spin_unlock_irqrestore(&video->irqlock, flags);
>  
> -	done->buf.sequence = pipe->sequence;
> -	done->buf.vb2_buf.timestamp = ktime_get_ns();
> -	for (i = 0; i < done->buf.vb2_buf.num_planes; ++i)
> -		vb2_set_plane_payload(&done->buf.vb2_buf, i,
> -				      vb2_plane_size(&done->buf.vb2_buf, i));
> -	vb2_buffer_done(&done->buf.vb2_buf, VB2_BUF_STATE_DONE);
> +	vsp1_video_complete_buffer(video, done);
>  
>  	return next;
>  }
> @@ -352,7 +357,7 @@ static void vsp1_video_frame_end(struct vsp1_pipeline *pipe,
>  	struct vsp1_video *video = rwpf->video;
>  	struct vsp1_vb2_buffer *buf;
>  
> -	buf = vsp1_video_complete_buffer(video);
> +	buf = vsp1_video_complete_next_buffer(video);
>  	if (buf == NULL)
>  		return;
>  
> 

-- 
Regards
--
Kieran

Re: [PATCH v4 6/7] media: vsp1: Replace the display list internal flag with a flags field

[Kieran Bingham]

Hi Laurent,

On 17/02/2019 02:48, Laurent Pinchart wrote:
> To prepare for addition of more flags to the display list, replace the
> 'internal' flag field by a bitmask 'flags' field.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/vsp1/vsp1_dl.c    | 31 +++++++++++++-----------
>  drivers/media/platform/vsp1/vsp1_dl.h    |  2 +-
>  drivers/media/platform/vsp1/vsp1_drm.c   |  6 ++++-
>  drivers/media/platform/vsp1/vsp1_video.c |  2 +-
>  4 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
> index 64af449791b0..886b3a69d329 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -178,7 +178,7 @@ struct vsp1_dl_cmd_pool {
>   * @post_cmd: post command to be issued through extended dl header
>   * @has_chain: if true, indicates that there's a partition chain
>   * @chain: entry in the display list partition chain
> - * @internal: whether the display list is used for internal purpose
> + * @flags: display list flags, a combination of VSP1_DL_FRAME_END_*
>   */
>  struct vsp1_dl_list {
>  	struct list_head list;
> @@ -197,7 +197,7 @@ struct vsp1_dl_list {
>  	bool has_chain;
>  	struct list_head chain;
>  
> -	bool internal;
> +	unsigned int flags;
>  };
>  
>  /**
> @@ -861,13 +861,15 @@ static void vsp1_dl_list_commit_continuous(struct vsp1_dl_list *dl)
>  	 *
>  	 * If a display list is already pending we simply drop it as the new
>  	 * display list is assumed to contain a more recent configuration. It is
> -	 * an error if the already pending list has the internal flag set, as
> -	 * there is then a process waiting for that list to complete. This
> -	 * shouldn't happen as the waiting process should perform proper
> -	 * locking, but warn just in case.
> +	 * an error if the already pending list has the
> +	 * VSP1_DL_FRAME_END_INTERNAL flag set, as there is then a process
> +	 * waiting for that list to complete. This shouldn't happen as the
> +	 * waiting process should perform proper locking, but warn just in
> +	 * case.
>  	 */
>  	if (vsp1_dl_list_hw_update_pending(dlm)) {
> -		WARN_ON(dlm->pending && dlm->pending->internal);
> +		WARN_ON(dlm->pending &&
> +			(dlm->pending->flags & VSP1_DL_FRAME_END_INTERNAL));
>  		__vsp1_dl_list_put(dlm->pending);
>  		dlm->pending = dl;
>  		return;
> @@ -897,7 +899,7 @@ static void vsp1_dl_list_commit_singleshot(struct vsp1_dl_list *dl)
>  	dlm->active = dl;
>  }
>  
> -void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
> +void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags)
>  {
>  	struct vsp1_dl_manager *dlm = dl->dlm;
>  	struct vsp1_dl_list *dl_next;
> @@ -912,7 +914,7 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
>  		vsp1_dl_list_fill_header(dl_next, last);
>  	}
>  
> -	dl->internal = internal;
> +	dl->flags = dl_flags & ~VSP1_DL_FRAME_END_COMPLETED;
>  
>  	spin_lock_irqsave(&dlm->lock, flags);
>  
> @@ -941,9 +943,10 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal)
>   * set in single-shot mode as display list processing is then not continuous and
>   * races never occur.
>   *
> - * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the previous display list
> - * has completed and had been queued with the internal notification flag.
> - * Internal notification is only supported for continuous mode.
> + * The following flags are only supported for continuous mode.
> + *
> + * The VSP1_DL_FRAME_END_INTERNAL flag indicates that the display list that just
> + * became active had been queued with the internal notification flag.
>   */
>  unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
> @@ -986,9 +989,9 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  	 * frame end interrupt. The display list thus becomes active.
>  	 */
>  	if (dlm->queued) {
> -		if (dlm->queued->internal)
> +		if (dlm->queued->flags & VSP1_DL_FRAME_END_INTERNAL)
>  			flags |= VSP1_DL_FRAME_END_INTERNAL;
> -		dlm->queued->internal = false;
> +		dlm->queued->flags &= ~VSP1_DL_FRAME_END_INTERNAL;
>  
>  		__vsp1_dl_list_put(dlm->active);
>  		dlm->active = dlm->queued;
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
> index 125750dc8b5c..e0fdb145e6ed 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -61,7 +61,7 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
>  void vsp1_dl_list_put(struct vsp1_dl_list *dl);
>  struct vsp1_dl_body *vsp1_dl_list_get_body0(struct vsp1_dl_list *dl);
>  struct vsp1_dl_ext_cmd *vsp1_dl_get_pre_cmd(struct vsp1_dl_list *dl);
> -void vsp1_dl_list_commit(struct vsp1_dl_list *dl, bool internal);
> +void vsp1_dl_list_commit(struct vsp1_dl_list *dl, unsigned int dl_flags);
>  
>  struct vsp1_dl_body_pool *
>  vsp1_dl_body_pool_create(struct vsp1_device *vsp1, unsigned int num_bodies,
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
> index 048190fd3a2d..9d20ef5cd5f8 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -537,6 +537,10 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>  	struct vsp1_entity *next;
>  	struct vsp1_dl_list *dl;
>  	struct vsp1_dl_body *dlb;
> +	unsigned int dl_flags = 0;
> +
> +	if (drm_pipe->force_brx_release)
> +		dl_flags |= VSP1_DL_FRAME_END_INTERNAL;
>  
>  	dl = vsp1_dl_list_get(pipe->output->dlm);
>  	dlb = vsp1_dl_list_get_body0(dl);
> @@ -559,7 +563,7 @@ static void vsp1_du_pipeline_configure(struct vsp1_pipeline *pipe)
>  		vsp1_entity_configure_partition(entity, pipe, dl, dlb);
>  	}
>  
> -	vsp1_dl_list_commit(dl, drm_pipe->force_brx_release);
> +	vsp1_dl_list_commit(dl, dl_flags);
>  }
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> index cfbab16c4820..8feaa8d89fe8 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -426,7 +426,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
>  	}
>  
>  	/* Complete, and commit the head display list. */
> -	vsp1_dl_list_commit(dl, false);
> +	vsp1_dl_list_commit(dl, 0);
>  	pipe->configured = true;
>  
>  	vsp1_pipeline_run(pipe);
> 

-- 
Regards
--
Kieran

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Brian Starkey]

Hi Laurent,

On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> Hello,
> 
> This patch series implements display writeback support for the R-Car
> Gen3 platforms in the VSP1 driver.
> 
> DRM/KMS provides a writeback API through a special type of writeback
> connectors. This series takes a different approach by exposing writeback
> as a V4L2 device. While there is nothing fundamentally wrong with
> writeback connectors, display for R-Car Gen3 platforms relies on the
> VSP1 driver behind the scene, which already implements V4L2 support.
> Enabling writeback through V4L2 is thus significantly easier in this
> case.

How does this look to an application? (I'm entirely ignorant about
R-Car). They are interacting with the DRM device, and then need to
open and configure a v4l2 device to get the writeback? Can the process
which isn't controlling the DRM device independently capture the
screen output?

I didn't see any major complication to implementing this as a
writeback connector. If you want/need to use the vb2_queue, couldn't
you just do that entirely from within the kernel?

Honestly (predictably?), to me it seems like a bad idea to introduce a
second, non-compatible interface for display writeback. Any
application interested in display writeback (e.g. compositors) will
need to implement both in order to support all HW. drm_hwcomposer
already supports writeback via DRM writeback connectors.

While I can see the advantages of having writeback exposed via v4l2
for streaming use-cases, I think it would be better to have it done in
such a way that it works for all writeback connectors, rather than
being VSP1-specific. That would allow any application to choose
whichever method is most appropriate for their use-case, without
limiting themselves to a subset of hardware.

> 
> The writeback pixel format is restricted to RGB, due to the VSP1
> outputting RGB to the display and lacking a separate colour space
> conversion unit for writeback. The resolution can be freely picked by
> will result in cropping or composing, not scaling.
> 
> Writeback requests are queued to the hardware on page flip (atomic
> flush), and complete at the next vblank. This means that a queued
> writeback buffer will not be processed until the next page flip, but
> once it starts being written to by the VSP, it will complete at the next
> vblank regardless of whether another page flip occurs at that time.
> 

This sounds the same as mali-dp, and so fits directly with the
semantics of writeback connectors.

Thanks,
-Brian

> The code is based on a merge of the media master branch, the drm-next
> branch and the R-Car DT next branch. For convenience patches can be
> found at
> 
> 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> 
> Kieran Bingham (2):
>   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
>   media: vsp1: Provide a writeback video device
> 
> Laurent Pinchart (5):
>   media: vsp1: wpf: Fix partition configuration for display pipelines
>   media: vsp1: Replace leftover occurrence of fragment with body
>   media: vsp1: Fix addresses of display-related registers for VSP-DL
>   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
>   media: vsp1: Replace the display list internal flag with a flags field
> 
>  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
>  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
>  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
>  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_regs.h  |   6 +-
>  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
>  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
>  drivers/media/platform/vsp1/vsp1_video.h |   6 +
>  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
>  11 files changed, 378 insertions(+), 75 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Laurent Pinchart]

Hi Brian,

On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote:
> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch series implements display writeback support for the R-Car
> > Gen3 platforms in the VSP1 driver.
> > 
> > DRM/KMS provides a writeback API through a special type of writeback
> > connectors. This series takes a different approach by exposing writeback
> > as a V4L2 device. While there is nothing fundamentally wrong with
> > writeback connectors, display for R-Car Gen3 platforms relies on the
> > VSP1 driver behind the scene, which already implements V4L2 support.
> > Enabling writeback through V4L2 is thus significantly easier in this
> > case.
> 
> How does this look to an application? (I'm entirely ignorant about
> R-Car). They are interacting with the DRM device, and then need to
> open and configure a v4l2 device to get the writeback? Can the process
> which isn't controlling the DRM device independently capture the
> screen output?

It can. The V4L2 capture device is independently controlled, and doesn't
affect the DRM/KMS device. The only dependency is that the V4L2 device
will only provide frames on atomic commit, a blocking VIDIOC_DQBUF or a
select()/poll() call will block until the atomic commit.

> I didn't see any major complication to implementing this as a
> writeback connector. If you want/need to use the vb2_queue, couldn't
> you just do that entirely from within the kernel?

In theory yes, in practice possibly, but with a higher complexity. I
didn't go for V4L2 to avoid writeback connectors, but because the
infrastructure was there already in the VSP driver.

> Honestly (predictably?), to me it seems like a bad idea to introduce a
> second, non-compatible interface for display writeback. Any
> application interested in display writeback (e.g. compositors) will
> need to implement both in order to support all HW. drm_hwcomposer
> already supports writeback via DRM writeback connectors.
> 
> While I can see the advantages of having writeback exposed via v4l2
> for streaming use-cases, I think it would be better to have it done in
> such a way that it works for all writeback connectors, rather than
> being VSP1-specific. That would allow any application to choose
> whichever method is most appropriate for their use-case, without
> limiting themselves to a subset of hardware.

I get your point. There's no much use arguing, as I believe you're right
:-) I'll give this another shot using writeback connectors.

> > The writeback pixel format is restricted to RGB, due to the VSP1
> > outputting RGB to the display and lacking a separate colour space
> > conversion unit for writeback. The resolution can be freely picked by
> > will result in cropping or composing, not scaling.
> > 
> > Writeback requests are queued to the hardware on page flip (atomic
> > flush), and complete at the next vblank. This means that a queued
> > writeback buffer will not be processed until the next page flip, but
> > once it starts being written to by the VSP, it will complete at the next
> > vblank regardless of whether another page flip occurs at that time.
> 
> This sounds the same as mali-dp, and so fits directly with the
> semantics of writeback connectors.
> 
> > The code is based on a merge of the media master branch, the drm-next
> > branch and the R-Car DT next branch. For convenience patches can be
> > found at
> > 
> > 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> > 
> > Kieran Bingham (2):
> >   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
> >   media: vsp1: Provide a writeback video device
> > 
> > Laurent Pinchart (5):
> >   media: vsp1: wpf: Fix partition configuration for display pipelines
> >   media: vsp1: Replace leftover occurrence of fragment with body
> >   media: vsp1: Fix addresses of display-related registers for VSP-DL
> >   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
> >   media: vsp1: Replace the display list internal flag with a flags field
> > 
> >  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
> >  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
> >  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
> >  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_regs.h  |   6 +-
> >  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> >  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
> >  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> >  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
> >  11 files changed, 378 insertions(+), 75 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/7] media: vsp1: wpf: Fix partition configuration for display pipelines

[Laurent Pinchart]

Hi Kieran,

On Sun, Feb 17, 2019 at 08:16:27PM +0000, Kieran Bingham wrote:
> On 17/02/2019 02:48, Laurent Pinchart wrote:
> > The WPF accesses partition configuration from pipe->partition in the
> > partition configuration that is not used for display pipelines.
> 
> That sentence is hard to read...

Indeed. I'll rewrite it.

> > Writeback support will require full configuration of the WPF while not
> > providing a valid pipe->partition. Rework the configuration code to fall
> > back to the full image width in that case, as is already done for the
> > part of the configuration currently relevant for display pipelines.
> > 
> 
> Ah yes - this is probably a better route than allocating a table for a
> single partition (which is what I had locally).
> 
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> I like that this change also simplifies the flip/rotate handling code to
> make that easier to read.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> >  drivers/media/platform/vsp1/vsp1_wpf.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
> > index 32bb207b2007..a07c5944b598 100644
> > --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> > +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> > @@ -362,6 +362,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> >  	const struct vsp1_format_info *fmtinfo = wpf->fmtinfo;
> >  	unsigned int width;
> >  	unsigned int height;
> > +	unsigned int left;
> >  	unsigned int offset;
> >  	unsigned int flip;
> >  	unsigned int i;
> > @@ -371,13 +372,16 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> >  						 RWPF_PAD_SINK);
> >  	width = sink_format->width;
> >  	height = sink_format->height;
> > +	left = 0;
> >  
> >  	/*
> >  	 * Cropping. The partition algorithm can split the image into
> >  	 * multiple slices.
> >  	 */
> > -	if (pipe->partitions > 1)
> > +	if (pipe->partitions > 1) {
> >  		width = pipe->partition->wpf.width;
> > +		left = pipe->partition->wpf.left;
> > +	}
> >  
> >  	vsp1_wpf_write(wpf, dlb, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
> >  		       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
> > @@ -408,13 +412,11 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> >  	flip = wpf->flip.active;
> >  
> >  	if (flip & BIT(WPF_CTRL_HFLIP) && !wpf->flip.rotate)
> > -		offset = format->width - pipe->partition->wpf.left
> > -			- pipe->partition->wpf.width;
> > +		offset = format->width - left - width;
> >  	else if (flip & BIT(WPF_CTRL_VFLIP) && wpf->flip.rotate)
> > -		offset = format->height - pipe->partition->wpf.left
> > -			- pipe->partition->wpf.width;
> > +		offset = format->height - left - width;
> >  	else
> > -		offset = pipe->partition->wpf.left;
> > +		offset = left;
> 
> This hunk looks a lot simpler :)
> 
> 
> >  
> >  	for (i = 0; i < format->num_planes; ++i) {
> >  		unsigned int hsub = i > 0 ? fmtinfo->hsub : 1;
> > @@ -436,7 +438,7 @@ static void wpf_configure_partition(struct vsp1_entity *entity,
> >  		 * image height.
> >  		 */
> >  		if (wpf->flip.rotate)
> > -			height = pipe->partition->wpf.width;
> > +			height = width;
> >  		else
> >  			height = format->height;
> >  
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 5/7] media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse

[Laurent Pinchart]

Hi Kieran,

On Sun, Feb 17, 2019 at 08:35:25PM +0000, Kieran Bingham wrote:
> On 17/02/2019 02:48, Laurent Pinchart wrote:
> > The vsp1_video_complete_buffer() function completes the current buffer
> > and returns a pointer to the next buffer. Split the code that completes
> > the buffer to a separate function for later reuse, and rename
> > vsp1_video_complete_buffer() to vsp1_video_complete_next_buffer().
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> This looks good to me - except perhaps the documentation /might/ need
> some refresh. With or without updates there, the code changes look good
> to me:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> >  drivers/media/platform/vsp1/vsp1_video.c | 35 ++++++++++++++----------
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> > index 328d686189be..cfbab16c4820 100644
> > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > @@ -300,8 +300,22 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
> >   * Pipeline Management
> >   */
> >  
> > +static void vsp1_video_complete_buffer(struct vsp1_video *video,
> > +				       struct vsp1_vb2_buffer *buffer)
> > +{
> > +	struct vsp1_pipeline *pipe = video->rwpf->entity.pipe;
> > +	unsigned int i;
> > +
> > +	buffer->buf.sequence = pipe->sequence;
> > +	buffer->buf.vb2_buf.timestamp = ktime_get_ns();
> > +	for (i = 0; i < buffer->buf.vb2_buf.num_planes; ++i)
> > +		vb2_set_plane_payload(&buffer->buf.vb2_buf, i,
> > +				      vb2_plane_size(&buffer->buf.vb2_buf, i));
> > +	vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_DONE);
> > +}
> > +
> >  /*
> > - * vsp1_video_complete_buffer - Complete the current buffer
> > + * vsp1_video_complete_next_buffer - Complete the current buffer
> 
> Should the brief be updated?
> It looks quite odd to call the function "complete next buffer" but with
> a brief saying "complete the current buffer".
> 
> Technically it's still correct, but it's more like
> "Complete the current buffer and return the next buffer"
> or such.

Good point. I'll update the brief to that.

> >   * @video: the video node
> >   *
> >   * This function completes the current buffer by filling its sequence number,
> 
> Is this still accurate enough to keep the text as is ?

It's still true, isn't it ?

> > @@ -310,13 +324,11 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
> >   * Return the next queued buffer or NULL if the queue is empty.
> >   */
> >  static struct vsp1_vb2_buffer *
> > -vsp1_video_complete_buffer(struct vsp1_video *video)
> > +vsp1_video_complete_next_buffer(struct vsp1_video *video)
> >  {
> > -	struct vsp1_pipeline *pipe = video->rwpf->entity.pipe;
> > -	struct vsp1_vb2_buffer *next = NULL;
> > +	struct vsp1_vb2_buffer *next;
> >  	struct vsp1_vb2_buffer *done;
> >  	unsigned long flags;
> > -	unsigned int i;
> >  
> >  	spin_lock_irqsave(&video->irqlock, flags);
> >  
> > @@ -327,21 +339,14 @@ vsp1_video_complete_buffer(struct vsp1_video *video)
> >  
> >  	done = list_first_entry(&video->irqqueue,
> >  				struct vsp1_vb2_buffer, queue);
> > -
> >  	list_del(&done->queue);
> >  
> > -	if (!list_empty(&video->irqqueue))
> > -		next = list_first_entry(&video->irqqueue,
> > +	next = list_first_entry_or_null(&video->irqqueue,
> 
> That's a nice way to save a line.
> 
> >  					struct vsp1_vb2_buffer, queue);
> >  
> >  	spin_unlock_irqrestore(&video->irqlock, flags);
> >  
> > -	done->buf.sequence = pipe->sequence;
> > -	done->buf.vb2_buf.timestamp = ktime_get_ns();
> > -	for (i = 0; i < done->buf.vb2_buf.num_planes; ++i)
> > -		vb2_set_plane_payload(&done->buf.vb2_buf, i,
> > -				      vb2_plane_size(&done->buf.vb2_buf, i));
> > -	vb2_buffer_done(&done->buf.vb2_buf, VB2_BUF_STATE_DONE);
> > +	vsp1_video_complete_buffer(video, done);
> >  
> >  	return next;
> >  }
> > @@ -352,7 +357,7 @@ static void vsp1_video_frame_end(struct vsp1_pipeline *pipe,
> >  	struct vsp1_video *video = rwpf->video;
> >  	struct vsp1_vb2_buffer *buf;
> >  
> > -	buf = vsp1_video_complete_buffer(video);
> > +	buf = vsp1_video_complete_next_buffer(video);
> >  	if (buf == NULL)
> >  		return;
> >  

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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@ideasonboard.com>
> > 
> > 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@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> For your changes,
> 
> With the documentation updated for vsp1_dl_body_write_patch(),
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> > 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

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Laurent Pinchart]

Hi Brian,

On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote:
> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch series implements display writeback support for the R-Car
> > Gen3 platforms in the VSP1 driver.
> > 
> > DRM/KMS provides a writeback API through a special type of writeback
> > connectors. This series takes a different approach by exposing writeback
> > as a V4L2 device. While there is nothing fundamentally wrong with
> > writeback connectors, display for R-Car Gen3 platforms relies on the
> > VSP1 driver behind the scene, which already implements V4L2 support.
> > Enabling writeback through V4L2 is thus significantly easier in this
> > case.
> 
> How does this look to an application? (I'm entirely ignorant about
> R-Car). They are interacting with the DRM device, and then need to
> open and configure a v4l2 device to get the writeback? Can the process
> which isn't controlling the DRM device independently capture the
> screen output?
> 
> I didn't see any major complication to implementing this as a
> writeback connector. If you want/need to use the vb2_queue, couldn't
> you just do that entirely from within the kernel?
> 
> Honestly (predictably?), to me it seems like a bad idea to introduce a
> second, non-compatible interface for display writeback. Any
> application interested in display writeback (e.g. compositors) will
> need to implement both in order to support all HW. drm_hwcomposer
> already supports writeback via DRM writeback connectors.
> 
> While I can see the advantages of having writeback exposed via v4l2
> for streaming use-cases, I think it would be better to have it done in
> such a way that it works for all writeback connectors, rather than
> being VSP1-specific. That would allow any application to choose
> whichever method is most appropriate for their use-case, without
> limiting themselves to a subset of hardware.

So I gave writeback connectors a go, and it wasn't very pretty. There
writeback support in the DRM core leaks jobs, and is missing support for
the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job
driver-specific data. I'm working on these issues and will submit
patches.

In the meantime, I need to test my implementation, so I need a command
line application that will let me exercise the API. I assume you've
tested your code, haven't you ? :-) Could you tell me how I can test
writeback ?

> > The writeback pixel format is restricted to RGB, due to the VSP1
> > outputting RGB to the display and lacking a separate colour space
> > conversion unit for writeback. The resolution can be freely picked by
> > will result in cropping or composing, not scaling.
> > 
> > Writeback requests are queued to the hardware on page flip (atomic
> > flush), and complete at the next vblank. This means that a queued
> > writeback buffer will not be processed until the next page flip, but
> > once it starts being written to by the VSP, it will complete at the next
> > vblank regardless of whether another page flip occurs at that time.
> 
> This sounds the same as mali-dp, and so fits directly with the
> semantics of writeback connectors.
> 
> > The code is based on a merge of the media master branch, the drm-next
> > branch and the R-Car DT next branch. For convenience patches can be
> > found at
> > 
> > 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> > 
> > Kieran Bingham (2):
> >   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
> >   media: vsp1: Provide a writeback video device
> > 
> > Laurent Pinchart (5):
> >   media: vsp1: wpf: Fix partition configuration for display pipelines
> >   media: vsp1: Replace leftover occurrence of fragment with body
> >   media: vsp1: Fix addresses of display-related registers for VSP-DL
> >   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
> >   media: vsp1: Replace the display list internal flag with a flags field
> > 
> >  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
> >  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
> >  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
> >  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_regs.h  |   6 +-
> >  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> >  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
> >  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> >  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
> >  11 files changed, 378 insertions(+), 75 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Brian Starkey]

Hi Laurent,

On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote:
> Hi Brian,
> 
> On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote:
> > On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> > > Hello,
> > > 
> > > This patch series implements display writeback support for the R-Car
> > > Gen3 platforms in the VSP1 driver.
> > > 
> > > DRM/KMS provides a writeback API through a special type of writeback
> > > connectors. This series takes a different approach by exposing writeback
> > > as a V4L2 device. While there is nothing fundamentally wrong with
> > > writeback connectors, display for R-Car Gen3 platforms relies on the
> > > VSP1 driver behind the scene, which already implements V4L2 support.
> > > Enabling writeback through V4L2 is thus significantly easier in this
> > > case.
> > 
> > How does this look to an application? (I'm entirely ignorant about
> > R-Car). They are interacting with the DRM device, and then need to
> > open and configure a v4l2 device to get the writeback? Can the process
> > which isn't controlling the DRM device independently capture the
> > screen output?
> > 
> > I didn't see any major complication to implementing this as a
> > writeback connector. If you want/need to use the vb2_queue, couldn't
> > you just do that entirely from within the kernel?
> > 
> > Honestly (predictably?), to me it seems like a bad idea to introduce a
> > second, non-compatible interface for display writeback. Any
> > application interested in display writeback (e.g. compositors) will
> > need to implement both in order to support all HW. drm_hwcomposer
> > already supports writeback via DRM writeback connectors.
> > 
> > While I can see the advantages of having writeback exposed via v4l2
> > for streaming use-cases, I think it would be better to have it done in
> > such a way that it works for all writeback connectors, rather than
> > being VSP1-specific. That would allow any application to choose
> > whichever method is most appropriate for their use-case, without
> > limiting themselves to a subset of hardware.
> 
> So I gave writeback connectors a go, and it wasn't very pretty.

Sorry you didn't have a good time :-(

> There writeback support in the DRM core leaks jobs,

Is this the cleanup on check fail, or something else?

One possible pitfall is that you must set the job in the connector
state to NULL after you call drm_writeback_queue_job(). The API there
could easily be changed to pass in the connector_state and clear it in
drm_writeback_queue_job() instead of relying on drivers to do it.

> and is missing support for
> the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job
> driver-specific data. I'm working on these issues and will submit
> patches.
> 

Hm, yes that didn't occur to me; we don't have a prepare_fb callback.

> In the meantime, I need to test my implementation, so I need a command
> line application that will let me exercise the API. I assume you've
> tested your code, haven't you ? :-) Could you tell me how I can test
> writeback ?

Indeed, there's igts on the list which I wrote and tested:

https://patchwork.kernel.org/patch/10764975/

And there's support in drm_hwcomposer (though I must admit I haven't
personally run the drm_hwc code):

https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3

I'm afraid I haven't really touched any of the writeback code for a
couple of years - Liviu picked that up. He's on holiday until Monday,
but he should be able to help with the status of the igts.

Hope that helps,

-Brian

> 
> > > The writeback pixel format is restricted to RGB, due to the VSP1
> > > outputting RGB to the display and lacking a separate colour space
> > > conversion unit for writeback. The resolution can be freely picked by
> > > will result in cropping or composing, not scaling.
> > > 
> > > Writeback requests are queued to the hardware on page flip (atomic
> > > flush), and complete at the next vblank. This means that a queued
> > > writeback buffer will not be processed until the next page flip, but
> > > once it starts being written to by the VSP, it will complete at the next
> > > vblank regardless of whether another page flip occurs at that time.
> > 
> > This sounds the same as mali-dp, and so fits directly with the
> > semantics of writeback connectors.
> > 
> > > The code is based on a merge of the media master branch, the drm-next
> > > branch and the R-Car DT next branch. For convenience patches can be
> > > found at
> > > 
> > > 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> > > 
> > > Kieran Bingham (2):
> > >   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
> > >   media: vsp1: Provide a writeback video device
> > > 
> > > Laurent Pinchart (5):
> > >   media: vsp1: wpf: Fix partition configuration for display pipelines
> > >   media: vsp1: Replace leftover occurrence of fragment with body
> > >   media: vsp1: Fix addresses of display-related registers for VSP-DL
> > >   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
> > >   media: vsp1: Replace the display list internal flag with a flags field
> > > 
> > >  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
> > >  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
> > >  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_regs.h  |   6 +-
> > >  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> > >  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
> > >  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> > >  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
> > >  11 files changed, 378 insertions(+), 75 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Laurent Pinchart]

Hi Brian,

On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote:
> On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote:
> > On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote:
> >> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> >>> Hello,
> >>> 
> >>> This patch series implements display writeback support for the R-Car
> >>> Gen3 platforms in the VSP1 driver.
> >>> 
> >>> DRM/KMS provides a writeback API through a special type of writeback
> >>> connectors. This series takes a different approach by exposing writeback
> >>> as a V4L2 device. While there is nothing fundamentally wrong with
> >>> writeback connectors, display for R-Car Gen3 platforms relies on the
> >>> VSP1 driver behind the scene, which already implements V4L2 support.
> >>> Enabling writeback through V4L2 is thus significantly easier in this
> >>> case.
> >> 
> >> How does this look to an application? (I'm entirely ignorant about
> >> R-Car). They are interacting with the DRM device, and then need to
> >> open and configure a v4l2 device to get the writeback? Can the process
> >> which isn't controlling the DRM device independently capture the
> >> screen output?
> >> 
> >> I didn't see any major complication to implementing this as a
> >> writeback connector. If you want/need to use the vb2_queue, couldn't
> >> you just do that entirely from within the kernel?
> >> 
> >> Honestly (predictably?), to me it seems like a bad idea to introduce a
> >> second, non-compatible interface for display writeback. Any
> >> application interested in display writeback (e.g. compositors) will
> >> need to implement both in order to support all HW. drm_hwcomposer
> >> already supports writeback via DRM writeback connectors.
> >> 
> >> While I can see the advantages of having writeback exposed via v4l2
> >> for streaming use-cases, I think it would be better to have it done in
> >> such a way that it works for all writeback connectors, rather than
> >> being VSP1-specific. That would allow any application to choose
> >> whichever method is most appropriate for their use-case, without
> >> limiting themselves to a subset of hardware.
> > 
> > So I gave writeback connectors a go, and it wasn't very pretty.
> 
> Sorry you didn't have a good time :-(

No worries. That was to be expected with such young code :-)

> > There writeback support in the DRM core leaks jobs,
> 
> Is this the cleanup on check fail, or something else?

Yes, that's the problem. I have patches for it that I will post soon.

> One possible pitfall is that you must set the job in the connector
> state to NULL after you call drm_writeback_queue_job(). The API there
> could easily be changed to pass in the connector_state and clear it in
> drm_writeback_queue_job() instead of relying on drivers to do it.

I also have a patch for that :-)

> > and is missing support for
> > the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job
> > driver-specific data. I'm working on these issues and will submit
> > patches.
> 
> Hm, yes that didn't occur to me; we don't have a prepare_fb callback.
> 
> > In the meantime, I need to test my implementation, so I need a command
> > line application that will let me exercise the API. I assume you've
> > tested your code, haven't you ? :-) Could you tell me how I can test
> > writeback ?
> 
> Indeed, there's igts on the list which I wrote and tested:
> 
> https://patchwork.kernel.org/patch/10764975/

Will you get these merged ? Pushing everybody to use the writeback
connector API without any test is mainline isn't nice, it almost makes
me want to go back to V4L2.

igt test cases are nice to have, but what I need now is a tool to
execise the API manually, similar to modetest, with command line
parameters to configure the device, and the ability to capture frames to
disk using writeback. How did you perform such tests when you developed
writeback support ?

> And there's support in drm_hwcomposer (though I must admit I haven't
> personally run the drm_hwc code):
> 
> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3

That won't help me much as I don't have an android port for the R-Car
boards.

> I'm afraid I haven't really touched any of the writeback code for a
> couple of years - Liviu picked that up. He's on holiday until Monday,
> but he should be able to help with the status of the igts.
> 
> Hope that helps,
> 
> >>> The writeback pixel format is restricted to RGB, due to the VSP1
> >>> outputting RGB to the display and lacking a separate colour space
> >>> conversion unit for writeback. The resolution can be freely picked by
> >>> will result in cropping or composing, not scaling.
> >>> 
> >>> Writeback requests are queued to the hardware on page flip (atomic
> >>> flush), and complete at the next vblank. This means that a queued
> >>> writeback buffer will not be processed until the next page flip, but
> >>> once it starts being written to by the VSP, it will complete at the next
> >>> vblank regardless of whether another page flip occurs at that time.
> >> 
> >> This sounds the same as mali-dp, and so fits directly with the
> >> semantics of writeback connectors.
> >> 
> >>> The code is based on a merge of the media master branch, the drm-next
> >>> branch and the R-Car DT next branch. For convenience patches can be
> >>> found at
> >>> 
> >>> 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> >>> 
> >>> Kieran Bingham (2):
> >>>   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
> >>>   media: vsp1: Provide a writeback video device
> >>> 
> >>> Laurent Pinchart (5):
> >>>   media: vsp1: wpf: Fix partition configuration for display pipelines
> >>>   media: vsp1: Replace leftover occurrence of fragment with body
> >>>   media: vsp1: Fix addresses of display-related registers for VSP-DL
> >>>   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
> >>>   media: vsp1: Replace the display list internal flag with a flags field
> >>> 
> >>>  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
> >>>  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
> >>>  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
> >>>  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_regs.h  |   6 +-
> >>>  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> >>>  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
> >>>  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> >>>  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
> >>>  11 files changed, 378 insertions(+), 75 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Brian Starkey]

Hi Laurent,

On Thu, Feb 21, 2019 at 12:02:57PM +0200, Laurent Pinchart wrote:
> Hi Brian,
> 
> On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote:
> > On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote:
> > >> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> > >>> Hello,
> > >>> 
> > >>> This patch series implements display writeback support for the R-Car
> > >>> Gen3 platforms in the VSP1 driver.
> > >>> 
> > >>> DRM/KMS provides a writeback API through a special type of writeback
> > >>> connectors. This series takes a different approach by exposing writeback
> > >>> as a V4L2 device. While there is nothing fundamentally wrong with
> > >>> writeback connectors, display for R-Car Gen3 platforms relies on the
> > >>> VSP1 driver behind the scene, which already implements V4L2 support.
> > >>> Enabling writeback through V4L2 is thus significantly easier in this
> > >>> case.
> > >> 
> > >> How does this look to an application? (I'm entirely ignorant about
> > >> R-Car). They are interacting with the DRM device, and then need to
> > >> open and configure a v4l2 device to get the writeback? Can the process
> > >> which isn't controlling the DRM device independently capture the
> > >> screen output?
> > >> 
> > >> I didn't see any major complication to implementing this as a
> > >> writeback connector. If you want/need to use the vb2_queue, couldn't
> > >> you just do that entirely from within the kernel?
> > >> 
> > >> Honestly (predictably?), to me it seems like a bad idea to introduce a
> > >> second, non-compatible interface for display writeback. Any
> > >> application interested in display writeback (e.g. compositors) will
> > >> need to implement both in order to support all HW. drm_hwcomposer
> > >> already supports writeback via DRM writeback connectors.
> > >> 
> > >> While I can see the advantages of having writeback exposed via v4l2
> > >> for streaming use-cases, I think it would be better to have it done in
> > >> such a way that it works for all writeback connectors, rather than
> > >> being VSP1-specific. That would allow any application to choose
> > >> whichever method is most appropriate for their use-case, without
> > >> limiting themselves to a subset of hardware.
> > > 
> > > So I gave writeback connectors a go, and it wasn't very pretty.
> > 
> > Sorry you didn't have a good time :-(
> 
> No worries. That was to be expected with such young code :-)
> 
> > > There writeback support in the DRM core leaks jobs,
> > 
> > Is this the cleanup on check fail, or something else?
> 
> Yes, that's the problem. I have patches for it that I will post soon.
> 
> > One possible pitfall is that you must set the job in the connector
> > state to NULL after you call drm_writeback_queue_job(). The API there
> > could easily be changed to pass in the connector_state and clear it in
> > drm_writeback_queue_job() instead of relying on drivers to do it.
> 
> I also have a patch for that :-)
> 
> > > and is missing support for
> > > the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job
> > > driver-specific data. I'm working on these issues and will submit
> > > patches.
> > 
> > Hm, yes that didn't occur to me; we don't have a prepare_fb callback.
> > 
> > > In the meantime, I need to test my implementation, so I need a command
> > > line application that will let me exercise the API. I assume you've
> > > tested your code, haven't you ? :-) Could you tell me how I can test
> > > writeback ?
> > 
> > Indeed, there's igts on the list which I wrote and tested:
> > 
> > https://patchwork.kernel.org/patch/10764975/
> 
> Will you get these merged ? Pushing everybody to use the writeback
> connector API without any test is mainline isn't nice, it almost makes
> me want to go back to V4L2.

I wasn't trying to be pushy - I only shared my opinion that I didn't
think it was a good idea to introduce a second display writeback API,
when we already have one. You're entirely entitled to ignore my
opinion.

The tests have been available since the very early versions of the
writeback series. I don't know what's blocking them from merging, I
haven't been tracking it very closely.

If you'd be happy to provide your review and test on them, that may
help the process along?

> 
> igt test cases are nice to have, but what I need now is a tool to
> execise the API manually, similar to modetest, with command line
> parameters to configure the device, and the ability to capture frames to
> disk using writeback. How did you perform such tests when you developed
> writeback support ?
> 

I used a pre-existing internal tool which does exactly that.

I appreciate that we don't have upstream tooling for writeback. As you
say, it's a young API (well, not by date, but certainly by usage).

I also do appreciate you taking the time to consider it, identifying
issues which we did not, and for fixing them. The only way it stops
being a young API, with bugs and no tooling, is if people adopt it.

Thanks,
-Brian

> > And there's support in drm_hwcomposer (though I must admit I haven't
> > personally run the drm_hwc code):
> > 
> > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3
> 
> That won't help me much as I don't have an android port for the R-Car
> boards.
> 
> > I'm afraid I haven't really touched any of the writeback code for a
> > couple of years - Liviu picked that up. He's on holiday until Monday,
> > but he should be able to help with the status of the igts.
> > 
> > Hope that helps,
> > 
> > >>> The writeback pixel format is restricted to RGB, due to the VSP1
> > >>> outputting RGB to the display and lacking a separate colour space
> > >>> conversion unit for writeback. The resolution can be freely picked by
> > >>> will result in cropping or composing, not scaling.
> > >>> 
> > >>> Writeback requests are queued to the hardware on page flip (atomic
> > >>> flush), and complete at the next vblank. This means that a queued
> > >>> writeback buffer will not be processed until the next page flip, but
> > >>> once it starts being written to by the VSP, it will complete at the next
> > >>> vblank regardless of whether another page flip occurs at that time.
> > >> 
> > >> This sounds the same as mali-dp, and so fits directly with the
> > >> semantics of writeback connectors.
> > >> 
> > >>> The code is based on a merge of the media master branch, the drm-next
> > >>> branch and the R-Car DT next branch. For convenience patches can be
> > >>> found at
> > >>> 
> > >>> 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> > >>> 
> > >>> Kieran Bingham (2):
> > >>>   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
> > >>>   media: vsp1: Provide a writeback video device
> > >>> 
> > >>> Laurent Pinchart (5):
> > >>>   media: vsp1: wpf: Fix partition configuration for display pipelines
> > >>>   media: vsp1: Replace leftover occurrence of fragment with body
> > >>>   media: vsp1: Fix addresses of display-related registers for VSP-DL
> > >>>   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
> > >>>   media: vsp1: Replace the display list internal flag with a flags field
> > >>> 
> > >>>  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
> > >>>  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
> > >>>  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
> > >>>  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_regs.h  |   6 +-
> > >>>  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> > >>>  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
> > >>>  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> > >>>  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
> > >>>  11 files changed, 378 insertions(+), 75 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Laurent Pinchart]

Hi Brian,

On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote:
> On Thu, Feb 21, 2019 at 12:02:57PM +0200, Laurent Pinchart wrote:
> > On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote:
> >> On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote:
> >>> On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote:
> >>>> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> >>>>> Hello,
> >>>>> 
> >>>>> This patch series implements display writeback support for the R-Car
> >>>>> Gen3 platforms in the VSP1 driver.
> >>>>> 
> >>>>> DRM/KMS provides a writeback API through a special type of writeback
> >>>>> connectors. This series takes a different approach by exposing writeback
> >>>>> as a V4L2 device. While there is nothing fundamentally wrong with
> >>>>> writeback connectors, display for R-Car Gen3 platforms relies on the
> >>>>> VSP1 driver behind the scene, which already implements V4L2 support.
> >>>>> Enabling writeback through V4L2 is thus significantly easier in this
> >>>>> case.
> >>>> 
> >>>> How does this look to an application? (I'm entirely ignorant about
> >>>> R-Car). They are interacting with the DRM device, and then need to
> >>>> open and configure a v4l2 device to get the writeback? Can the process
> >>>> which isn't controlling the DRM device independently capture the
> >>>> screen output?
> >>>> 
> >>>> I didn't see any major complication to implementing this as a
> >>>> writeback connector. If you want/need to use the vb2_queue, couldn't
> >>>> you just do that entirely from within the kernel?
> >>>> 
> >>>> Honestly (predictably?), to me it seems like a bad idea to introduce a
> >>>> second, non-compatible interface for display writeback. Any
> >>>> application interested in display writeback (e.g. compositors) will
> >>>> need to implement both in order to support all HW. drm_hwcomposer
> >>>> already supports writeback via DRM writeback connectors.
> >>>> 
> >>>> While I can see the advantages of having writeback exposed via v4l2
> >>>> for streaming use-cases, I think it would be better to have it done in
> >>>> such a way that it works for all writeback connectors, rather than
> >>>> being VSP1-specific. That would allow any application to choose
> >>>> whichever method is most appropriate for their use-case, without
> >>>> limiting themselves to a subset of hardware.
> >>> 
> >>> So I gave writeback connectors a go, and it wasn't very pretty.
> >> 
> >> Sorry you didn't have a good time :-(
> > 
> > No worries. That was to be expected with such young code :-)
> > 
> >>> There writeback support in the DRM core leaks jobs,
> >> 
> >> Is this the cleanup on check fail, or something else?
> > 
> > Yes, that's the problem. I have patches for it that I will post soon.
> > 
> >> One possible pitfall is that you must set the job in the connector
> >> state to NULL after you call drm_writeback_queue_job(). The API there
> >> could easily be changed to pass in the connector_state and clear it in
> >> drm_writeback_queue_job() instead of relying on drivers to do it.
> > 
> > I also have a patch for that :-)
> > 
> >>> and is missing support for
> >>> the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job
> >>> driver-specific data. I'm working on these issues and will submit
> >>> patches.
> >> 
> >> Hm, yes that didn't occur to me; we don't have a prepare_fb callback.
> >> 
> >>> In the meantime, I need to test my implementation, so I need a command
> >>> line application that will let me exercise the API. I assume you've
> >>> tested your code, haven't you ? :-) Could you tell me how I can test
> >>> writeback ?
> >> 
> >> Indeed, there's igts on the list which I wrote and tested:
> >> 
> >> https://patchwork.kernel.org/patch/10764975/
> > 
> > Will you get these merged ? Pushing everybody to use the writeback
> > connector API without any test is mainline isn't nice, it almost makes
> > me want to go back to V4L2.
> 
> I wasn't trying to be pushy - I only shared my opinion that I didn't
> think it was a good idea to introduce a second display writeback API,
> when we already have one. You're entirely entitled to ignore my
> opinion.

I agree we should aim for a single API. My preference would have been
for V4L2 universally, but now that we have writeback connectors
upstream, the choice has been made, so we should stick to it.

> The tests have been available since the very early versions of the
> writeback series. I don't know what's blocking them from merging, I
> haven't been tracking it very closely.
> 
> If you'd be happy to provide your review and test on them, that may
> help the process along?

Now that I've sent an entirely untested patch series out, this is next
on my list :-)

> > igt test cases are nice to have, but what I need now is a tool to
> > execise the API manually, similar to modetest, with command line
> > parameters to configure the device, and the ability to capture frames to
> > disk using writeback. How did you perform such tests when you developed
> > writeback support ?
> 
> I used a pre-existing internal tool which does exactly that.

Any hope of sharing the sources ?

> I appreciate that we don't have upstream tooling for writeback. As you
> say, it's a young API (well, not by date, but certainly by usage).
> 
> I also do appreciate you taking the time to consider it, identifying
> issues which we did not, and for fixing them. The only way it stops
> being a young API, with bugs and no tooling, is if people adopt it.

If the developers who initially pushed the API upstream without an
open-source test tool could spend a bit of time on this issue, I'm sure
it would help too :-)

> >> And there's support in drm_hwcomposer (though I must admit I haven't
> >> personally run the drm_hwc code):
> >> 
> >> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3
> > 
> > That won't help me much as I don't have an android port for the R-Car
> > boards.
> > 
> >> I'm afraid I haven't really touched any of the writeback code for a
> >> couple of years - Liviu picked that up. He's on holiday until Monday,
> >> but he should be able to help with the status of the igts.
> >> 
> >> Hope that helps,
> >> 
> >>>>> The writeback pixel format is restricted to RGB, due to the VSP1
> >>>>> outputting RGB to the display and lacking a separate colour space
> >>>>> conversion unit for writeback. The resolution can be freely picked by
> >>>>> will result in cropping or composing, not scaling.
> >>>>> 
> >>>>> Writeback requests are queued to the hardware on page flip (atomic
> >>>>> flush), and complete at the next vblank. This means that a queued
> >>>>> writeback buffer will not be processed until the next page flip, but
> >>>>> once it starts being written to by the VSP, it will complete at the next
> >>>>> vblank regardless of whether another page flip occurs at that time.
> >>>> 
> >>>> This sounds the same as mali-dp, and so fits directly with the
> >>>> semantics of writeback connectors.
> >>>> 
> >>>>> The code is based on a merge of the media master branch, the drm-next
> >>>>> branch and the R-Car DT next branch. For convenience patches can be
> >>>>> found at
> >>>>> 
> >>>>> 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> >>>>> 
> >>>>> Kieran Bingham (2):
> >>>>>   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
> >>>>>   media: vsp1: Provide a writeback video device
> >>>>> 
> >>>>> Laurent Pinchart (5):
> >>>>>   media: vsp1: wpf: Fix partition configuration for display pipelines
> >>>>>   media: vsp1: Replace leftover occurrence of fragment with body
> >>>>>   media: vsp1: Fix addresses of display-related registers for VSP-DL
> >>>>>   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
> >>>>>   media: vsp1: Replace the display list internal flag with a flags field
> >>>>> 
> >>>>>  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
> >>>>>  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
> >>>>>  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
> >>>>>  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_regs.h  |   6 +-
> >>>>>  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> >>>>>  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
> >>>>>  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> >>>>>  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
> >>>>>  11 files changed, 378 insertions(+), 75 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Brian Starkey]

On Thu, Feb 21, 2019 at 02:23:10PM +0200, Laurent Pinchart wrote:
> On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote:

[snip]

> > 
> > I used a pre-existing internal tool which does exactly that.
> 
> Any hope of sharing the sources ?
> 

Not in a timescale or form which would be useful to you. I'm convinced
people only ask questions like this to make us look like Bad Guys.

Opening everything up is a process, and it's going to take us time.
Sure we could be doing better, but I also think there's a lot of
people who do worse.

> > I appreciate that we don't have upstream tooling for writeback. As you
> > say, it's a young API (well, not by date, but certainly by usage).
> > 
> > I also do appreciate you taking the time to consider it, identifying
> > issues which we did not, and for fixing them. The only way it stops
> > being a young API, with bugs and no tooling, is if people adopt it.
> 
> If the developers who initially pushed the API upstream without an
> open-source test tool could spend a bit of time on this issue, I'm sure
> it would help too :-)
> 

No one suggested a test test tool before. In fact, the DRM subsystem
explicitly requires that features land with something that isn't only
a test tool, hence why we did drm_hwcomposer.

That said, yes, we should be trying harder to get the igts landed. I
personally think igts are far more useful than a random example C
file, but I guess opinions differ.

Thanks,
-Brian

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Daniel Vetter]

On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote:
> Hi Laurent,
> 
> On Thu, Feb 21, 2019 at 12:02:57PM +0200, Laurent Pinchart wrote:
> > Hi Brian,
> > 
> > On Thu, Feb 21, 2019 at 09:50:19AM +0000, Brian Starkey wrote:
> > > On Thu, Feb 21, 2019 at 10:23:17AM +0200, Laurent Pinchart wrote:
> > > > On Mon, Feb 18, 2019 at 12:22:58PM +0000, Brian Starkey wrote:
> > > >> On Sun, Feb 17, 2019 at 04:48:45AM +0200, Laurent Pinchart wrote:
> > > >>> Hello,
> > > >>> 
> > > >>> This patch series implements display writeback support for the R-Car
> > > >>> Gen3 platforms in the VSP1 driver.
> > > >>> 
> > > >>> DRM/KMS provides a writeback API through a special type of writeback
> > > >>> connectors. This series takes a different approach by exposing writeback
> > > >>> as a V4L2 device. While there is nothing fundamentally wrong with
> > > >>> writeback connectors, display for R-Car Gen3 platforms relies on the
> > > >>> VSP1 driver behind the scene, which already implements V4L2 support.
> > > >>> Enabling writeback through V4L2 is thus significantly easier in this
> > > >>> case.
> > > >> 
> > > >> How does this look to an application? (I'm entirely ignorant about
> > > >> R-Car). They are interacting with the DRM device, and then need to
> > > >> open and configure a v4l2 device to get the writeback? Can the process
> > > >> which isn't controlling the DRM device independently capture the
> > > >> screen output?
> > > >> 
> > > >> I didn't see any major complication to implementing this as a
> > > >> writeback connector. If you want/need to use the vb2_queue, couldn't
> > > >> you just do that entirely from within the kernel?
> > > >> 
> > > >> Honestly (predictably?), to me it seems like a bad idea to introduce a
> > > >> second, non-compatible interface for display writeback. Any
> > > >> application interested in display writeback (e.g. compositors) will
> > > >> need to implement both in order to support all HW. drm_hwcomposer
> > > >> already supports writeback via DRM writeback connectors.
> > > >> 
> > > >> While I can see the advantages of having writeback exposed via v4l2
> > > >> for streaming use-cases, I think it would be better to have it done in
> > > >> such a way that it works for all writeback connectors, rather than
> > > >> being VSP1-specific. That would allow any application to choose
> > > >> whichever method is most appropriate for their use-case, without
> > > >> limiting themselves to a subset of hardware.
> > > > 
> > > > So I gave writeback connectors a go, and it wasn't very pretty.
> > > 
> > > Sorry you didn't have a good time :-(
> > 
> > No worries. That was to be expected with such young code :-)
> > 
> > > > There writeback support in the DRM core leaks jobs,
> > > 
> > > Is this the cleanup on check fail, or something else?
> > 
> > Yes, that's the problem. I have patches for it that I will post soon.
> > 
> > > One possible pitfall is that you must set the job in the connector
> > > state to NULL after you call drm_writeback_queue_job(). The API there
> > > could easily be changed to pass in the connector_state and clear it in
> > > drm_writeback_queue_job() instead of relying on drivers to do it.
> > 
> > I also have a patch for that :-)
> > 
> > > > and is missing support for
> > > > the equivalent of .prepare_fb()/.cleanup_fb(), which requires per-job
> > > > driver-specific data. I'm working on these issues and will submit
> > > > patches.
> > > 
> > > Hm, yes that didn't occur to me; we don't have a prepare_fb callback.
> > > 
> > > > In the meantime, I need to test my implementation, so I need a command
> > > > line application that will let me exercise the API. I assume you've
> > > > tested your code, haven't you ? :-) Could you tell me how I can test
> > > > writeback ?
> > > 
> > > Indeed, there's igts on the list which I wrote and tested:
> > > 
> > > https://patchwork.kernel.org/patch/10764975/
> > 
> > Will you get these merged ? Pushing everybody to use the writeback
> > connector API without any test is mainline isn't nice, it almost makes
> > me want to go back to V4L2.
> 
> I wasn't trying to be pushy - I only shared my opinion that I didn't
> think it was a good idea to introduce a second display writeback API,
> when we already have one. You're entirely entitled to ignore my
> opinion.
> 
> The tests have been available since the very early versions of the
> writeback series. I don't know what's blocking them from merging, I
> haven't been tracking it very closely.
> 
> If you'd be happy to provide your review and test on them, that may
> help the process along?
> 
> > 
> > igt test cases are nice to have, but what I need now is a tool to
> > execise the API manually, similar to modetest, with command line
> > parameters to configure the device, and the ability to capture frames to
> > disk using writeback. How did you perform such tests when you developed
> > writeback support ?
> > 
> 
> I used a pre-existing internal tool which does exactly that.
> 
> I appreciate that we don't have upstream tooling for writeback. As you
> say, it's a young API (well, not by date, but certainly by usage).
> 
> I also do appreciate you taking the time to consider it, identifying
> issues which we did not, and for fixing them. The only way it stops
> being a young API, with bugs and no tooling, is if people adopt it.

Mentioned on irc already to Laurent, but here for completeness: igt has
pretty decent support for combining that into one utility. We support
piles of standard stuff like --interactive-debug already, plus you can add
whatever additional things you need and feel like. There's quite a few
such igt tests/tools combinations already.
-Daniel

> 
> Thanks,
> -Brian
> 
> > > And there's support in drm_hwcomposer (though I must admit I haven't
> > > personally run the drm_hwc code):
> > > 
> > > https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3
> > 
> > That won't help me much as I don't have an android port for the R-Car
> > boards.
> > 
> > > I'm afraid I haven't really touched any of the writeback code for a
> > > couple of years - Liviu picked that up. He's on holiday until Monday,
> > > but he should be able to help with the status of the igts.
> > > 
> > > Hope that helps,
> > > 
> > > >>> The writeback pixel format is restricted to RGB, due to the VSP1
> > > >>> outputting RGB to the display and lacking a separate colour space
> > > >>> conversion unit for writeback. The resolution can be freely picked by
> > > >>> will result in cropping or composing, not scaling.
> > > >>> 
> > > >>> Writeback requests are queued to the hardware on page flip (atomic
> > > >>> flush), and complete at the next vblank. This means that a queued
> > > >>> writeback buffer will not be processed until the next page flip, but
> > > >>> once it starts being written to by the VSP, it will complete at the next
> > > >>> vblank regardless of whether another page flip occurs at that time.
> > > >> 
> > > >> This sounds the same as mali-dp, and so fits directly with the
> > > >> semantics of writeback connectors.
> > > >> 
> > > >>> The code is based on a merge of the media master branch, the drm-next
> > > >>> branch and the R-Car DT next branch. For convenience patches can be
> > > >>> found at
> > > >>> 
> > > >>> 	git://linuxtv.org/pinchartl/media.git v4l2/vsp1/writeback
> > > >>> 
> > > >>> Kieran Bingham (2):
> > > >>>   Revert "[media] v4l: vsp1: Supply frames to the DU continuously"
> > > >>>   media: vsp1: Provide a writeback video device
> > > >>> 
> > > >>> Laurent Pinchart (5):
> > > >>>   media: vsp1: wpf: Fix partition configuration for display pipelines
> > > >>>   media: vsp1: Replace leftover occurrence of fragment with body
> > > >>>   media: vsp1: Fix addresses of display-related registers for VSP-DL
> > > >>>   media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
> > > >>>   media: vsp1: Replace the display list internal flag with a flags field
> > > >>> 
> > > >>>  drivers/media/platform/vsp1/vsp1_dl.c    | 118 ++++++++++++--
> > > >>>  drivers/media/platform/vsp1/vsp1_dl.h    |   6 +-
> > > >>>  drivers/media/platform/vsp1/vsp1_drm.c   |  24 ++-
> > > >>>  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_regs.h  |   6 +-
> > > >>>  drivers/media/platform/vsp1/vsp1_rwpf.h  |   2 +
> > > >>>  drivers/media/platform/vsp1/vsp1_video.c | 198 +++++++++++++++++++----
> > > >>>  drivers/media/platform/vsp1/vsp1_video.h |   6 +
> > > >>>  drivers/media/platform/vsp1/vsp1_wpf.c   |  65 ++++++--
> > > >>>  11 files changed, 378 insertions(+), 75 deletions(-)
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v4 0/7] VSP1: Display writeback support

[Laurent Pinchart]

Hi Brian,

On Thu, Feb 21, 2019 at 01:44:56PM +0000, Brian Starkey wrote:
> On Thu, Feb 21, 2019 at 02:23:10PM +0200, Laurent Pinchart wrote:
> > On Thu, Feb 21, 2019 at 12:19:13PM +0000, Brian Starkey wrote:
> 
> [snip]
> 
> >> I used a pre-existing internal tool which does exactly that.
> > 
> > Any hope of sharing the sources ?
> 
> Not in a timescale or form which would be useful to you. I'm convinced
> people only ask questions like this to make us look like Bad Guys.

I didn't have big hopes, but it was a honest question.

> Opening everything up is a process, and it's going to take us time.
> Sure we could be doing better, but I also think there's a lot of
> people who do worse.

No question that ARM is neither the worst nor the best. I also value
your and the other graphics developer's efforts more than I value ARM's
open-source policy, as you are fighting against company policies to
drive open-source.

> >> I appreciate that we don't have upstream tooling for writeback. As you
> >> say, it's a young API (well, not by date, but certainly by usage).
> >> 
> >> I also do appreciate you taking the time to consider it, identifying
> >> issues which we did not, and for fixing them. The only way it stops
> >> being a young API, with bugs and no tooling, is if people adopt it.
> > 
> > If the developers who initially pushed the API upstream without an
> > open-source test tool could spend a bit of time on this issue, I'm sure
> > it would help too :-)
> 
> No one suggested a test test tool before. In fact, the DRM subsystem
> explicitly requires that features land with something that isn't only
> a test tool, hence why we did drm_hwcomposer.
> 
> That said, yes, we should be trying harder to get the igts landed. I
> personally think igts are far more useful than a random example C
> file, but I guess opinions differ.

I think both are useful, for different purposes. Automated test cases
are very valuable to test for compliance and catch regressions, while a
manualy tool is very valuable during development.

-- 
Regards,

Laurent Pinchart