[PATCH v4 0/4] v4l: vsp1: Fix suspend/resume and race on M2M pipelines

[PATCH v4 0/4] v4l: vsp1: Fix suspend/resume and race on M2M pipelines

[Kieran Bingham]

This small patchset helps rework the VSP1 driver to repair an issue on
suspend/resume operations whereby the pipeline does not get reconfigured after
it has been re-initialised following a resume operation.

Along side this, there was an intrinsic race in the vsp1_video_start_streaming()
function whereby multiple streams operating through a BRU, could find themselves
commencing an operation before the pipeline has been configured, or worse -
commencing, just as the pipeline is being configured resulting in a null pointer
dereference on pipe->dl.

Patch [1/4] fixes the multiple stream BRU race
Patch [2/4] is a code move only, with no functional change.
Patch [3/4] fixes the suspend/resume operations for video pipelines by marking
            the new pipe configured flag as false, and configuring the pipe
            during the vsp1_video_pipeline_run() call.
Patch [4/4] removes the context scoped 'pipe->dl' from vsp1_drm.c which is only
            used in a single function

v4:
 - Rework and separate out the BRU race back to v1 style implementation
 - Split BRU race and Suspend Resume fixes into separate commits.

v3:
 - Move configured=false from vsp1_device_init to vsp1_reset_wpf()
 - Clean up flag dereferencing with a local struct *

v2:
 - Refactor video pipeline configuration implementation to solve both suspend
   resume and the VSP BRU race in a single change

v1:
 - Original pipeline configuration rework

Kieran Bingham (4):
  v4l: vsp1: Prevent multiple streamon race commencing pipeline early
  v4l: vsp1: Move vsp1_video_setup_pipeline()
  v4l: vsp1: Repair suspend resume operations for video pipelines
  v4l: vsp1: Remove redundant pipe->dl usage from drm

 drivers/media/platform/vsp1/vsp1_drm.c   |  20 ++--
 drivers/media/platform/vsp1/vsp1_drv.c   |   4 +-
 drivers/media/platform/vsp1/vsp1_pipe.c  |   1 +-
 drivers/media/platform/vsp1/vsp1_pipe.h  |   4 +-
 drivers/media/platform/vsp1/vsp1_video.c | 133 ++++++++++++------------
 5 files changed, 86 insertions(+), 76 deletions(-)

base-commit: 16b6839d4e6f0c3fe6d5db2b4c90fb39dabc8640
-- 
git-series 0.9.1

[PATCH v4 1/4] v4l: vsp1: Prevent multiple streamon race commencing pipeline early

[Kieran Bingham]

With multiple inputs through the BRU it is feasible for the streams to
race each other at stream-on.

Multiple VIDIOC_STREAMON calls racing each other could have process
N-1 skipping over the pipeline setup section and then start the pipeline
early, if videobuf2 has already enqueued buffers to the driver for
process N but not called the .start_streaming() operation yet

In the case of the video pipelines, this
can present two serious issues.

 1) A null-dereference if the pipe->dl is committed at the same time as
    the vsp1_video_setup_pipeline() is processing

 2) A hardware hang, where a display list is committed without having
    called vsp1_video_setup_pipeline() first

Repair this issue, by ensuring that only the stream which configures the
pipeline is able to start it.

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

---

v4:
 - Revert and rework back to v1 implementation style
 - Provide detailed comments on the race

v3:
 - Move 'flag reset' to be inside the vsp1_reset_wpf() function call
 - Tidy up the wpf->pipe reference for the configured flag

To test this race, I have used the vsp-unit-test-0007.sh from Laurent's
VSP-Tests [0] in iteration. Without this patch, failures can be seen be
seen anywhere up to the 150 iterations mark.

With this patch in place, tests have successfully iterated over 1500
loops.

The function affected by this change appears to have been around since
v4.6-rc2-105-g351bbf99f245 and thus could be included in stable trees
from that point forward. The issue may have been prevalent before that
but the solution would need reworking for earlier version.

[0] http://git.ideasonboard.com/renesas/vsp-tests.git
---
 drivers/media/platform/vsp1/vsp1_video.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index e6592b576ca3..f7dc249eb398 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -797,6 +797,7 @@ static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct vsp1_video *video = vb2_get_drv_priv(vq);
 	struct vsp1_pipeline *pipe = video->rwpf->pipe;
+	bool start_pipeline = false;
 	unsigned long flags;
 	int ret;
 
@@ -807,11 +808,23 @@ static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count)
 			mutex_unlock(&pipe->lock);
 			return ret;
 		}
+
+		start_pipeline = true;
 	}
 
 	pipe->stream_count++;
 	mutex_unlock(&pipe->lock);
 
+	/*
+	 * vsp1_pipeline_ready() is not sufficient to establish that all streams
+	 * are prepared and the pipeline is configured, as multiple streams
+	 * can race through streamon with buffers already queued; Therefore we
+	 * don't even attempt to start the pipeline until the last stream has
+	 * called through here.
+	 */
+	if (!start_pipeline)
+		return 0;
+
 	spin_lock_irqsave(&pipe->irqlock, flags);
 	if (vsp1_pipeline_ready(pipe))
 		vsp1_video_pipeline_run(pipe);
-- 
git-series 0.9.1

[PATCH v4 2/4] v4l: vsp1: Move vsp1_video_setup_pipeline()

[Kieran Bingham]

Move the static vsp1_video_setup_pipeline() function in preparation for
the callee updates so that the vsp1_video_pipeline_run() call can
configure pipelines following suspend resume actions.

This commit is just a code move for clarity performing no functional
change.

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

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index f7dc249eb398..938ecc2766ed 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -355,6 +355,47 @@ static void vsp1_video_frame_end(struct vsp1_pipeline *pipe,
 	pipe->buffers_ready |= 1 << video->pipe_index;
 }
 
+static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
+{
+	struct vsp1_entity *entity;
+
+	/* Determine this pipelines sizes for image partitioning support. */
+	vsp1_video_pipeline_setup_partitions(pipe);
+
+	/* Prepare the display list. */
+	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
+	if (!pipe->dl)
+		return -ENOMEM;
+
+	if (pipe->uds) {
+		struct vsp1_uds *uds = to_uds(&pipe->uds->subdev);
+
+		/* If a BRU is present in the pipeline before the UDS, the alpha
+		 * component doesn't need to be scaled as the BRU output alpha
+		 * value is fixed to 255. Otherwise we need to scale the alpha
+		 * component only when available at the input RPF.
+		 */
+		if (pipe->uds_input->type == VSP1_ENTITY_BRU) {
+			uds->scale_alpha = false;
+		} else {
+			struct vsp1_rwpf *rpf =
+				to_rwpf(&pipe->uds_input->subdev);
+
+			uds->scale_alpha = rpf->fmtinfo->alpha;
+		}
+	}
+
+	list_for_each_entry(entity, &pipe->entities, list_pipe) {
+		vsp1_entity_route_setup(entity, pipe->dl);
+
+		if (entity->ops->configure)
+			entity->ops->configure(entity, pipe, pipe->dl,
+					       VSP1_ENTITY_PARAMS_INIT);
+	}
+
+	return 0;
+}
+
 static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe,
 					      struct vsp1_dl_list *dl)
 {
@@ -752,47 +793,6 @@ static void vsp1_video_buffer_queue(struct vb2_buffer *vb)
 	spin_unlock_irqrestore(&pipe->irqlock, flags);
 }
 
-static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
-{
-	struct vsp1_entity *entity;
-
-	/* Determine this pipelines sizes for image partitioning support. */
-	vsp1_video_pipeline_setup_partitions(pipe);
-
-	/* Prepare the display list. */
-	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
-	if (!pipe->dl)
-		return -ENOMEM;
-
-	if (pipe->uds) {
-		struct vsp1_uds *uds = to_uds(&pipe->uds->subdev);
-
-		/* If a BRU is present in the pipeline before the UDS, the alpha
-		 * component doesn't need to be scaled as the BRU output alpha
-		 * value is fixed to 255. Otherwise we need to scale the alpha
-		 * component only when available at the input RPF.
-		 */
-		if (pipe->uds_input->type == VSP1_ENTITY_BRU) {
-			uds->scale_alpha = false;
-		} else {
-			struct vsp1_rwpf *rpf =
-				to_rwpf(&pipe->uds_input->subdev);
-
-			uds->scale_alpha = rpf->fmtinfo->alpha;
-		}
-	}
-
-	list_for_each_entry(entity, &pipe->entities, list_pipe) {
-		vsp1_entity_route_setup(entity, pipe->dl);
-
-		if (entity->ops->configure)
-			entity->ops->configure(entity, pipe, pipe->dl,
-					       VSP1_ENTITY_PARAMS_INIT);
-	}
-
-	return 0;
-}
-
 static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct vsp1_video *video = vb2_get_drv_priv(vq);
-- 
git-series 0.9.1

[PATCH v4 3/4] v4l: vsp1: Repair suspend resume operations for video pipelines

[Kieran Bingham]

When a suspend/resume action is taken, the pipeline is reset and never
reconfigured.

To correct this, we establish a new flag pipe->configured and utilise
this to establish when we write a full configuration set to the current
display list.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drv.c   |  4 ++-
 drivers/media/platform/vsp1/vsp1_pipe.c  |  1 +-
 drivers/media/platform/vsp1/vsp1_pipe.h  |  2 +-
 drivers/media/platform/vsp1/vsp1_video.c | 56 ++++++++++---------------
 4 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index aa237b48ad55..d596cdead1c1 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -413,6 +413,7 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
 
 int vsp1_reset_wpf(struct vsp1_device *vsp1, unsigned int index)
 {
+	struct vsp1_rwpf *wpf = vsp1->wpf[index];
 	unsigned int timeout;
 	u32 status;
 
@@ -429,6 +430,9 @@ int vsp1_reset_wpf(struct vsp1_device *vsp1, unsigned int index)
 		usleep_range(1000, 2000);
 	}
 
+	if (wpf->pipe)
+		wpf->pipe->configured = false;
+
 	if (!timeout) {
 		dev_err(vsp1->dev, "failed to reset wpf.%u\n", index);
 		return -ETIMEDOUT;
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c
index 280ba0804699..c568db193fba 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -216,6 +216,7 @@ void vsp1_pipeline_init(struct vsp1_pipeline *pipe)
 
 	INIT_LIST_HEAD(&pipe->entities);
 	pipe->state = VSP1_PIPELINE_STOPPED;
+	pipe->configured = false;
 }
 
 /* Must be called with the pipe irqlock held. */
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index ac4ad2655551..fff122b4874d 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -61,6 +61,7 @@ enum vsp1_pipeline_state {
  * @pipe: the media pipeline
  * @irqlock: protects the pipeline state
  * @state: current state
+ * @configured: true if the pipeline has been set up for video streaming
  * @wq: wait queue to wait for state change completion
  * @frame_end: frame end interrupt handler
  * @lock: protects the pipeline use count and stream count
@@ -86,6 +87,7 @@ struct vsp1_pipeline {
 
 	spinlock_t irqlock;
 	enum vsp1_pipeline_state state;
+	bool configured;
 	wait_queue_head_t wq;
 
 	void (*frame_end)(struct vsp1_pipeline *pipe);
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 938ecc2766ed..414303442e7c 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -355,18 +355,14 @@ static void vsp1_video_frame_end(struct vsp1_pipeline *pipe,
 	pipe->buffers_ready |= 1 << video->pipe_index;
 }
 
-static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
+static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe,
+				     struct vsp1_dl_list *dl)
 {
 	struct vsp1_entity *entity;
 
 	/* Determine this pipelines sizes for image partitioning support. */
 	vsp1_video_pipeline_setup_partitions(pipe);
 
-	/* Prepare the display list. */
-	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
-	if (!pipe->dl)
-		return -ENOMEM;
-
 	if (pipe->uds) {
 		struct vsp1_uds *uds = to_uds(&pipe->uds->subdev);
 
@@ -386,13 +382,15 @@ static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
 	}
 
 	list_for_each_entry(entity, &pipe->entities, list_pipe) {
-		vsp1_entity_route_setup(entity, pipe->dl);
+		vsp1_entity_route_setup(entity, dl);
 
 		if (entity->ops->configure)
-			entity->ops->configure(entity, pipe, pipe->dl,
+			entity->ops->configure(entity, pipe, dl,
 					       VSP1_ENTITY_PARAMS_INIT);
 	}
 
+	pipe->configured = true;
+
 	return 0;
 }
 
@@ -415,9 +413,16 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
 {
 	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
 	struct vsp1_entity *entity;
+	struct vsp1_dl_list *dl;
 
-	if (!pipe->dl)
-		pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
+	dl = vsp1_dl_list_get(pipe->output->dlm);
+	if (!dl) {
+		dev_err(vsp1->dev, "Failed to obtain a dl list\n");
+		return;
+	}
+
+	if (!pipe->configured)
+		vsp1_video_setup_pipeline(pipe, dl);
 
 	/*
 	 * Start with the runtime parameters as the configure operation can
@@ -426,45 +431,43 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
 	 */
 	list_for_each_entry(entity, &pipe->entities, list_pipe) {
 		if (entity->ops->configure)
-			entity->ops->configure(entity, pipe, pipe->dl,
+			entity->ops->configure(entity, pipe, dl,
 					       VSP1_ENTITY_PARAMS_RUNTIME);
 	}
 
 	/* Run the first partition */
 	pipe->current_partition = 0;
-	vsp1_video_pipeline_run_partition(pipe, pipe->dl);
+	vsp1_video_pipeline_run_partition(pipe, dl);
 
 	/* Process consecutive partitions as necessary */
 	for (pipe->current_partition = 1;
 	     pipe->current_partition < pipe->partitions;
 	     pipe->current_partition++) {
-		struct vsp1_dl_list *dl;
+		struct vsp1_dl_list *child;
 
 		/*
 		 * Partition configuration operations will utilise
 		 * the pipe->current_partition variable to determine
 		 * the work they should complete.
 		 */
-		dl = vsp1_dl_list_get(pipe->output->dlm);
+		child = vsp1_dl_list_get(pipe->output->dlm);
 
 		/*
 		 * An incomplete chain will still function, but output only
 		 * the partitions that had a dl available. The frame end
 		 * interrupt will be marked on the last dl in the chain.
 		 */
-		if (!dl) {
+		if (!child) {
 			dev_err(vsp1->dev, "Failed to obtain a dl list. Frame will be incomplete\n");
 			break;
 		}
 
-		vsp1_video_pipeline_run_partition(pipe, dl);
-		vsp1_dl_list_add_chain(pipe->dl, dl);
+		vsp1_video_pipeline_run_partition(pipe, child);
+		vsp1_dl_list_add_chain(dl, child);
 	}
 
 	/* Complete, and commit the head display list. */
-	vsp1_dl_list_commit(pipe->dl);
-	pipe->dl = NULL;
-
+	vsp1_dl_list_commit(dl);
 	vsp1_pipeline_run(pipe);
 }
 
@@ -799,18 +802,10 @@ static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct vsp1_pipeline *pipe = video->rwpf->pipe;
 	bool start_pipeline = false;
 	unsigned long flags;
-	int ret;
 
 	mutex_lock(&pipe->lock);
-	if (pipe->stream_count == pipe->num_inputs) {
-		ret = vsp1_video_setup_pipeline(pipe);
-		if (ret < 0) {
-			mutex_unlock(&pipe->lock);
-			return ret;
-		}
-
+	if (pipe->stream_count == pipe->num_inputs)
 		start_pipeline = true;
-	}
 
 	pipe->stream_count++;
 	mutex_unlock(&pipe->lock);
@@ -855,9 +850,6 @@ static void vsp1_video_stop_streaming(struct vb2_queue *vq)
 		ret = vsp1_pipeline_stop(pipe);
 		if (ret == -ETIMEDOUT)
 			dev_err(video->vsp1->dev, "pipeline stop timeout\n");
-
-		vsp1_dl_list_put(pipe->dl);
-		pipe->dl = NULL;
 	}
 	mutex_unlock(&pipe->lock);
 
-- 
git-series 0.9.1

[PATCH v4 4/4] v4l: vsp1: Remove redundant pipe->dl usage from drm

[Kieran Bingham]

The pipe->dl is used only inside vsp1_du_atomic_flush(), and can be
obtained and stored locally to simplify the code.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c  | 20 ++++++++++----------
 drivers/media/platform/vsp1/vsp1_pipe.h |  2 --
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index b4b583f7137a..d7ec980300dd 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -220,9 +220,6 @@ void vsp1_du_atomic_begin(struct device *dev)
 	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
 
 	vsp1->drm->num_inputs = pipe->num_inputs;
-
-	/* Prepare the display list. */
-	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
 }
 EXPORT_SYMBOL_GPL(vsp1_du_atomic_begin);
 
@@ -426,10 +423,14 @@ void vsp1_du_atomic_flush(struct device *dev)
 	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
 	struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
 	struct vsp1_entity *entity;
+	struct vsp1_dl_list *dl;
 	unsigned long flags;
 	unsigned int i;
 	int ret;
 
+	/* Prepare the display list. */
+	dl = vsp1_dl_list_get(pipe->output->dlm);
+
 	/* Count the number of enabled inputs and sort them by Z-order. */
 	pipe->num_inputs = 0;
 
@@ -484,26 +485,25 @@ void vsp1_du_atomic_flush(struct device *dev)
 			struct vsp1_rwpf *rpf = to_rwpf(&entity->subdev);
 
 			if (!pipe->inputs[rpf->entity.index]) {
-				vsp1_dl_list_write(pipe->dl, entity->route->reg,
+				vsp1_dl_list_write(dl, entity->route->reg,
 						   VI6_DPR_NODE_UNUSED);
 				continue;
 			}
 		}
 
-		vsp1_entity_route_setup(entity, pipe->dl);
+		vsp1_entity_route_setup(entity, dl);
 
 		if (entity->ops->configure) {
-			entity->ops->configure(entity, pipe, pipe->dl,
+			entity->ops->configure(entity, pipe, dl,
 					       VSP1_ENTITY_PARAMS_INIT);
-			entity->ops->configure(entity, pipe, pipe->dl,
+			entity->ops->configure(entity, pipe, dl,
 					       VSP1_ENTITY_PARAMS_RUNTIME);
-			entity->ops->configure(entity, pipe, pipe->dl,
+			entity->ops->configure(entity, pipe, dl,
 					       VSP1_ENTITY_PARAMS_PARTITION);
 		}
 	}
 
-	vsp1_dl_list_commit(pipe->dl);
-	pipe->dl = NULL;
+	vsp1_dl_list_commit(dl);
 
 	/* Start or stop the pipeline if needed. */
 	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index fff122b4874d..e59bef2653f6 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -108,8 +108,6 @@ struct vsp1_pipeline {
 
 	struct list_head entities;
 
-	struct vsp1_dl_list *dl;
-
 	unsigned int div_size;
 	unsigned int partitions;
 	struct v4l2_rect partition;
-- 
git-series 0.9.1

Re: [PATCH v4 1/4] v4l: vsp1: Prevent multiple streamon race commencing pipeline early

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 06 Jan 2017 12:15:28 Kieran Bingham wrote:
> With multiple inputs through the BRU it is feasible for the streams to
> race each other at stream-on.
> 
> Multiple VIDIOC_STREAMON calls racing each other could have process
> N-1 skipping over the pipeline setup section and then start the pipeline
> early, if videobuf2 has already enqueued buffers to the driver for
> process N but not called the .start_streaming() operation yet
> 
> In the case of the video pipelines, this
> can present two serious issues.
> 
>  1) A null-dereference if the pipe->dl is committed at the same time as
>     the vsp1_video_setup_pipeline() is processing
> 
>  2) A hardware hang, where a display list is committed without having
>     called vsp1_video_setup_pipeline() first
> 
> Repair this issue, by ensuring that only the stream which configures the
> pipeline is able to start it.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 
> ---
> 
> v4:
>  - Revert and rework back to v1 implementation style
>  - Provide detailed comments on the race
> 
> v3:
>  - Move 'flag reset' to be inside the vsp1_reset_wpf() function call
>  - Tidy up the wpf->pipe reference for the configured flag
> 
> To test this race, I have used the vsp-unit-test-0007.sh from Laurent's
> VSP-Tests [0] in iteration. Without this patch, failures can be seen be
> seen anywhere up to the 150 iterations mark.
> 
> With this patch in place, tests have successfully iterated over 1500
> loops.
> 
> The function affected by this change appears to have been around since
> v4.6-rc2-105-g351bbf99f245 and thus could be included in stable trees
> from that point forward. The issue may have been prevalent before that
> but the solution would need reworking for earlier version.
> 
> [0] http://git.ideasonboard.com/renesas/vsp-tests.git
> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index e6592b576ca3..f7dc249eb398
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -797,6 +797,7 @@ static int vsp1_video_start_streaming(struct vb2_queue
> *vq, unsigned int count) {
>  	struct vsp1_video *video = vb2_get_drv_priv(vq);
>  	struct vsp1_pipeline *pipe = video->rwpf->pipe;
> +	bool start_pipeline = false;
>  	unsigned long flags;
>  	int ret;
> 
> @@ -807,11 +808,23 @@ static int vsp1_video_start_streaming(struct vb2_queue
> *vq, unsigned int count) mutex_unlock(&pipe->lock);
>  			return ret;
>  		}
> +
> +		start_pipeline = true;
>  	}
> 
>  	pipe->stream_count++;
>  	mutex_unlock(&pipe->lock);
> 
> +	/*
> +	 * vsp1_pipeline_ready() is not sufficient to establish that all 
streams
> +	 * are prepared and the pipeline is configured, as multiple streams
> +	 * can race through streamon with buffers already queued; Therefore we
> +	 * don't even attempt to start the pipeline until the last stream has
> +	 * called through here.
> +	 */
> +	if (!start_pipeline)
> +		return 0;
> +
>  	spin_lock_irqsave(&pipe->irqlock, flags);
>  	if (vsp1_pipeline_ready(pipe))
>  		vsp1_video_pipeline_run(pipe);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/4] v4l: vsp1: Move vsp1_video_setup_pipeline()

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 06 Jan 2017 12:15:29 Kieran Bingham wrote:
> Move the static vsp1_video_setup_pipeline() function in preparation for
> the callee updates so that the vsp1_video_pipeline_run() call can
> configure pipelines following suspend resume actions.
> 
> This commit is just a code move for clarity performing no functional
> change.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 82 ++++++++++++-------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index f7dc249eb398..938ecc2766ed
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -355,6 +355,47 @@ static void vsp1_video_frame_end(struct vsp1_pipeline
> *pipe, pipe->buffers_ready |= 1 << video->pipe_index;
>  }
> 
> +static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
> +{
> +	struct vsp1_entity *entity;
> +
> +	/* Determine this pipelines sizes for image partitioning support. */
> +	vsp1_video_pipeline_setup_partitions(pipe);
> +
> +	/* Prepare the display list. */
> +	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
> +	if (!pipe->dl)
> +		return -ENOMEM;
> +
> +	if (pipe->uds) {
> +		struct vsp1_uds *uds = to_uds(&pipe->uds->subdev);
> +
> +		/* If a BRU is present in the pipeline before the UDS, the 
alpha
> +		 * component doesn't need to be scaled as the BRU output alpha
> +		 * value is fixed to 255. Otherwise we need to scale the alpha
> +		 * component only when available at the input RPF.
> +		 */
> +		if (pipe->uds_input->type == VSP1_ENTITY_BRU) {
> +			uds->scale_alpha = false;
> +		} else {
> +			struct vsp1_rwpf *rpf =
> +				to_rwpf(&pipe->uds_input->subdev);
> +
> +			uds->scale_alpha = rpf->fmtinfo->alpha;
> +		}
> +	}
> +
> +	list_for_each_entry(entity, &pipe->entities, list_pipe) {
> +		vsp1_entity_route_setup(entity, pipe->dl);
> +
> +		if (entity->ops->configure)
> +			entity->ops->configure(entity, pipe, pipe->dl,
> +					       VSP1_ENTITY_PARAMS_INIT);
> +	}
> +
> +	return 0;
> +}
> +
>  static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe,
>  					      struct vsp1_dl_list *dl)
>  {
> @@ -752,47 +793,6 @@ static void vsp1_video_buffer_queue(struct vb2_buffer
> *vb) spin_unlock_irqrestore(&pipe->irqlock, flags);
>  }
> 
> -static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
> -{
> -	struct vsp1_entity *entity;
> -
> -	/* Determine this pipelines sizes for image partitioning support. */
> -	vsp1_video_pipeline_setup_partitions(pipe);
> -
> -	/* Prepare the display list. */
> -	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
> -	if (!pipe->dl)
> -		return -ENOMEM;
> -
> -	if (pipe->uds) {
> -		struct vsp1_uds *uds = to_uds(&pipe->uds->subdev);
> -
> -		/* If a BRU is present in the pipeline before the UDS, the 
alpha
> -		 * component doesn't need to be scaled as the BRU output alpha
> -		 * value is fixed to 255. Otherwise we need to scale the alpha
> -		 * component only when available at the input RPF.
> -		 */
> -		if (pipe->uds_input->type == VSP1_ENTITY_BRU) {
> -			uds->scale_alpha = false;
> -		} else {
> -			struct vsp1_rwpf *rpf =
> -				to_rwpf(&pipe->uds_input->subdev);
> -
> -			uds->scale_alpha = rpf->fmtinfo->alpha;
> -		}
> -	}
> -
> -	list_for_each_entry(entity, &pipe->entities, list_pipe) {
> -		vsp1_entity_route_setup(entity, pipe->dl);
> -
> -		if (entity->ops->configure)
> -			entity->ops->configure(entity, pipe, pipe->dl,
> -					       VSP1_ENTITY_PARAMS_INIT);
> -	}
> -
> -	return 0;
> -}
> -
>  static int vsp1_video_start_streaming(struct vb2_queue *vq, unsigned int
> count) {
>  	struct vsp1_video *video = vb2_get_drv_priv(vq);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/4] v4l: vsp1: Remove redundant pipe->dl usage from drm

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 06 Jan 2017 12:15:31 Kieran Bingham wrote:
> The pipe->dl is used only inside vsp1_du_atomic_flush(), and can be
> obtained and stored locally to simplify the code.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

As I still have issues with patch 3/4, I propose moving this one earlier in 
the series.

> ---
>  drivers/media/platform/vsp1/vsp1_drm.c  | 20 ++++++++++----------
>  drivers/media/platform/vsp1/vsp1_pipe.h |  2 --
>  2 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index b4b583f7137a..d7ec980300dd
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -220,9 +220,6 @@ void vsp1_du_atomic_begin(struct device *dev)
>  	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
> 
>  	vsp1->drm->num_inputs = pipe->num_inputs;
> -
> -	/* Prepare the display list. */
> -	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
>  }
>  EXPORT_SYMBOL_GPL(vsp1_du_atomic_begin);
> 
> @@ -426,10 +423,14 @@ void vsp1_du_atomic_flush(struct device *dev)
>  	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
>  	struct vsp1_rwpf *inputs[VSP1_MAX_RPF] = { NULL, };
>  	struct vsp1_entity *entity;
> +	struct vsp1_dl_list *dl;
>  	unsigned long flags;
>  	unsigned int i;
>  	int ret;
> 
> +	/* Prepare the display list. */
> +	dl = vsp1_dl_list_get(pipe->output->dlm);
> +
>  	/* Count the number of enabled inputs and sort them by Z-order. */
>  	pipe->num_inputs = 0;
> 
> @@ -484,26 +485,25 @@ void vsp1_du_atomic_flush(struct device *dev)
>  			struct vsp1_rwpf *rpf = to_rwpf(&entity->subdev);
> 
>  			if (!pipe->inputs[rpf->entity.index]) {
> -				vsp1_dl_list_write(pipe->dl, entity->route-
>reg,
> +				vsp1_dl_list_write(dl, entity->route->reg,
>  						   VI6_DPR_NODE_UNUSED);
>  				continue;
>  			}
>  		}
> 
> -		vsp1_entity_route_setup(entity, pipe->dl);
> +		vsp1_entity_route_setup(entity, dl);
> 
>  		if (entity->ops->configure) {
> -			entity->ops->configure(entity, pipe, pipe->dl,
> +			entity->ops->configure(entity, pipe, dl,
>  					       VSP1_ENTITY_PARAMS_INIT);
> -			entity->ops->configure(entity, pipe, pipe->dl,
> +			entity->ops->configure(entity, pipe, dl,
>  					       VSP1_ENTITY_PARAMS_RUNTIME);
> -			entity->ops->configure(entity, pipe, pipe->dl,
> +			entity->ops->configure(entity, pipe, dl,
>  					       VSP1_ENTITY_PARAMS_PARTITION);
>  		}
>  	}
> 
> -	vsp1_dl_list_commit(pipe->dl);
> -	pipe->dl = NULL;
> +	vsp1_dl_list_commit(dl);
> 
>  	/* Start or stop the pipeline if needed. */
>  	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index fff122b4874d..e59bef2653f6
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -108,8 +108,6 @@ struct vsp1_pipeline {
> 
>  	struct list_head entities;
> 
> -	struct vsp1_dl_list *dl;
> -
>  	unsigned int div_size;
>  	unsigned int partitions;
>  	struct v4l2_rect partition;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/4] v4l: vsp1: Repair suspend resume operations for video pipelines

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 06 Jan 2017 12:15:30 Kieran Bingham wrote:
> When a suspend/resume action is taken, the pipeline is reset and never
> reconfigured.
> 
> To correct this, we establish a new flag pipe->configured and utilise
> this to establish when we write a full configuration set to the current
> display list.

As discussed face to face, I think a better approach would be to cache the 
display list instead of recomputing it a resume time. However, given that this 
will require more work, I could be convinced to merge this patch in the 
meantime. Let's check in a couple of weeks whether we will have moved forward 
with display list caching in time for v4.12, and merge this patch otherwise.

> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drv.c   |  4 ++-
>  drivers/media/platform/vsp1/vsp1_pipe.c  |  1 +-
>  drivers/media/platform/vsp1/vsp1_pipe.h  |  2 +-
>  drivers/media/platform/vsp1/vsp1_video.c | 56 ++++++++++---------------
>  4 files changed, 31 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c index aa237b48ad55..d596cdead1c1
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -413,6 +413,7 @@ static int vsp1_create_entities(struct vsp1_device
> *vsp1)
> 
>  int vsp1_reset_wpf(struct vsp1_device *vsp1, unsigned int index)
>  {
> +	struct vsp1_rwpf *wpf = vsp1->wpf[index];
>  	unsigned int timeout;
>  	u32 status;
> 
> @@ -429,6 +430,9 @@ int vsp1_reset_wpf(struct vsp1_device *vsp1, unsigned
> int index) usleep_range(1000, 2000);
>  	}
> 
> +	if (wpf->pipe)
> +		wpf->pipe->configured = false;
> +
>  	if (!timeout) {
>  		dev_err(vsp1->dev, "failed to reset wpf.%u\n", index);
>  		return -ETIMEDOUT;
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 280ba0804699..c568db193fba
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -216,6 +216,7 @@ void vsp1_pipeline_init(struct vsp1_pipeline *pipe)
> 
>  	INIT_LIST_HEAD(&pipe->entities);
>  	pipe->state = VSP1_PIPELINE_STOPPED;
> +	pipe->configured = false;
>  }
> 
>  /* Must be called with the pipe irqlock held. */
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index ac4ad2655551..fff122b4874d
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -61,6 +61,7 @@ enum vsp1_pipeline_state {
>   * @pipe: the media pipeline
>   * @irqlock: protects the pipeline state
>   * @state: current state
> + * @configured: true if the pipeline has been set up for video streaming
>   * @wq: wait queue to wait for state change completion
>   * @frame_end: frame end interrupt handler
>   * @lock: protects the pipeline use count and stream count
> @@ -86,6 +87,7 @@ struct vsp1_pipeline {
> 
>  	spinlock_t irqlock;
>  	enum vsp1_pipeline_state state;
> +	bool configured;
>  	wait_queue_head_t wq;
> 
>  	void (*frame_end)(struct vsp1_pipeline *pipe);
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 938ecc2766ed..414303442e7c
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -355,18 +355,14 @@ static void vsp1_video_frame_end(struct vsp1_pipeline
> *pipe, pipe->buffers_ready |= 1 << video->pipe_index;
>  }
> 
> -static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe)
> +static int vsp1_video_setup_pipeline(struct vsp1_pipeline *pipe,
> +				     struct vsp1_dl_list *dl)
>  {
>  	struct vsp1_entity *entity;
> 
>  	/* Determine this pipelines sizes for image partitioning support. */
>  	vsp1_video_pipeline_setup_partitions(pipe);
> 
> -	/* Prepare the display list. */
> -	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
> -	if (!pipe->dl)
> -		return -ENOMEM;
> -
>  	if (pipe->uds) {
>  		struct vsp1_uds *uds = to_uds(&pipe->uds->subdev);
> 
> @@ -386,13 +382,15 @@ static int vsp1_video_setup_pipeline(struct
> vsp1_pipeline *pipe) }
> 
>  	list_for_each_entry(entity, &pipe->entities, list_pipe) {
> -		vsp1_entity_route_setup(entity, pipe->dl);
> +		vsp1_entity_route_setup(entity, dl);
> 
>  		if (entity->ops->configure)
> -			entity->ops->configure(entity, pipe, pipe->dl,
> +			entity->ops->configure(entity, pipe, dl,
>  					       VSP1_ENTITY_PARAMS_INIT);
>  	}
> 
> +	pipe->configured = true;
> +
>  	return 0;
>  }
> 
> @@ -415,9 +413,16 @@ static void vsp1_video_pipeline_run(struct
> vsp1_pipeline *pipe) {
>  	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
>  	struct vsp1_entity *entity;
> +	struct vsp1_dl_list *dl;
> 
> -	if (!pipe->dl)
> -		pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
> +	dl = vsp1_dl_list_get(pipe->output->dlm);
> +	if (!dl) {
> +		dev_err(vsp1->dev, "Failed to obtain a dl list\n");
> +		return;
> +	}
> +
> +	if (!pipe->configured)
> +		vsp1_video_setup_pipeline(pipe, dl);
> 
>  	/*
>  	 * Start with the runtime parameters as the configure operation can
> @@ -426,45 +431,43 @@ static void vsp1_video_pipeline_run(struct
> vsp1_pipeline *pipe) */
>  	list_for_each_entry(entity, &pipe->entities, list_pipe) {
>  		if (entity->ops->configure)
> -			entity->ops->configure(entity, pipe, pipe->dl,
> +			entity->ops->configure(entity, pipe, dl,
>  					       VSP1_ENTITY_PARAMS_RUNTIME);
>  	}
> 
>  	/* Run the first partition */
>  	pipe->current_partition = 0;
> -	vsp1_video_pipeline_run_partition(pipe, pipe->dl);
> +	vsp1_video_pipeline_run_partition(pipe, dl);
> 
>  	/* Process consecutive partitions as necessary */
>  	for (pipe->current_partition = 1;
>  	     pipe->current_partition < pipe->partitions;
>  	     pipe->current_partition++) {
> -		struct vsp1_dl_list *dl;
> +		struct vsp1_dl_list *child;
> 
>  		/*
>  		 * Partition configuration operations will utilise
>  		 * the pipe->current_partition variable to determine
>  		 * the work they should complete.
>  		 */
> -		dl = vsp1_dl_list_get(pipe->output->dlm);
> +		child = vsp1_dl_list_get(pipe->output->dlm);
> 
>  		/*
>  		 * An incomplete chain will still function, but output only
>  		 * the partitions that had a dl available. The frame end
>  		 * interrupt will be marked on the last dl in the chain.
>  		 */
> -		if (!dl) {
> +		if (!child) {
>  			dev_err(vsp1->dev, "Failed to obtain a dl list. Frame 
will be
> incomplete\n"); break;
>  		}
> 
> -		vsp1_video_pipeline_run_partition(pipe, dl);
> -		vsp1_dl_list_add_chain(pipe->dl, dl);
> +		vsp1_video_pipeline_run_partition(pipe, child);
> +		vsp1_dl_list_add_chain(dl, child);
>  	}
> 
>  	/* Complete, and commit the head display list. */
> -	vsp1_dl_list_commit(pipe->dl);
> -	pipe->dl = NULL;
> -
> +	vsp1_dl_list_commit(dl);
>  	vsp1_pipeline_run(pipe);
>  }
> 
> @@ -799,18 +802,10 @@ static int vsp1_video_start_streaming(struct vb2_queue
> *vq, unsigned int count) struct vsp1_pipeline *pipe = video->rwpf->pipe;
>  	bool start_pipeline = false;
>  	unsigned long flags;
> -	int ret;
> 
>  	mutex_lock(&pipe->lock);
> -	if (pipe->stream_count == pipe->num_inputs) {
> -		ret = vsp1_video_setup_pipeline(pipe);
> -		if (ret < 0) {
> -			mutex_unlock(&pipe->lock);
> -			return ret;
> -		}
> -
> +	if (pipe->stream_count == pipe->num_inputs)
>  		start_pipeline = true;
> -	}
> 
>  	pipe->stream_count++;
>  	mutex_unlock(&pipe->lock);
> @@ -855,9 +850,6 @@ static void vsp1_video_stop_streaming(struct vb2_queue
> *vq) ret = vsp1_pipeline_stop(pipe);
>  		if (ret == -ETIMEDOUT)
>  			dev_err(video->vsp1->dev, "pipeline stop timeout\n");
> -
> -		vsp1_dl_list_put(pipe->dl);
> -		pipe->dl = NULL;
>  	}
>  	mutex_unlock(&pipe->lock);

-- 
Regards,

Laurent Pinchart