[PATCH v2 1/3] [media] coda: use correct offset for mvcol buffer

[PATCH v2 1/3] [media] coda: use correct offset for mvcol buffer

[Philipp Zabel]

From: Lucas Stach <l.stach@pengutronix.de>

The mvcol buffer needs to be placed behind the chroma plane(s), so
use the real offset including any required rounding.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
No changes since v1 [1].

[1] https://patchwork.linuxtv.org/patch/40604
---
 drivers/media/platform/coda/coda-bit.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index 2ec41375a896f..325035bb0a777 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -427,14 +427,16 @@ static int coda_alloc_framebuffers(struct coda_ctx *ctx,
 
 	/* Register frame buffers in the parameter buffer */
 	for (i = 0; i < ctx->num_internal_frames; i++) {
-		u32 y, cb, cr;
+		u32 y, cb, cr, mvcol;
 
 		/* Start addresses of Y, Cb, Cr planes */
 		y = ctx->internal_frames[i].paddr;
 		cb = y + ysize;
 		cr = y + ysize + ysize/4;
+		mvcol = y + ysize + ysize/4 + ysize/4;
 		if (ctx->tiled_map_type == GDI_TILED_FRAME_MB_RASTER_MAP) {
 			cb = round_up(cb, 4096);
+			mvcol = cb + ysize/2;
 			cr = 0;
 			/* Packed 20-bit MSB of base addresses */
 			/* YYYYYCCC, CCyyyyyc, cccc.... */
@@ -448,9 +450,7 @@ static int coda_alloc_framebuffers(struct coda_ctx *ctx,
 		/* mvcol buffer for h.264 */
 		if (ctx->codec->src_fourcc == V4L2_PIX_FMT_H264 &&
 		    dev->devtype->product != CODA_DX6)
-			coda_parabuf_write(ctx, 96 + i,
-					   ctx->internal_frames[i].paddr +
-					   ysize + ysize/4 + ysize/4);
+			coda_parabuf_write(ctx, 96 + i, mvcol);
 	}
 
 	/* mvcol buffer for mpeg4 */
-- 
2.11.0

[PATCH v2 2/3] [media] coda: first step at error recovery

[Philipp Zabel]

From: Lucas Stach <l.stach@pengutronix.de>

This implements a simple handler for the case where decode did not finish
successfully. This might be helpful during normal streaming, but for now it
only handles the case where the context would deadlock with userspace,
i.e. userspace issued DEC_CMD_STOP and waits for EOS, but after the failed
decode run we would hold the context and wait for userspace to queue more
buffers.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
[p.zabel@pengutronix.de: renamed error_decode/run to run/decode_timeout]
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1 [1]:
- Rename error_run/decode callback to run/decode_timeout, as
  this error handler is called on device_run timeouts only.

[1] https://patchwork.linuxtv.org/patch/40603
---
 drivers/media/platform/coda/coda-bit.c    | 20 ++++++++++++++++++++
 drivers/media/platform/coda/coda-common.c |  3 +++
 drivers/media/platform/coda/coda.h        |  1 +
 3 files changed, 24 insertions(+)

diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
index 325035bb0a777..795b6d7584320 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -2198,12 +2198,32 @@ static void coda_finish_decode(struct coda_ctx *ctx)
 	ctx->display_idx = display_idx;
 }
 
+static void coda_decode_timeout(struct coda_ctx *ctx)
+{
+	struct vb2_v4l2_buffer *dst_buf;
+
+	/*
+	 * For now this only handles the case where we would deadlock with
+	 * userspace, i.e. userspace issued DEC_CMD_STOP and waits for EOS,
+	 * but after a failed decode run we would hold the context and wait for
+	 * userspace to queue more buffers.
+	 */
+	if (!(ctx->bit_stream_param & CODA_BIT_STREAM_END_FLAG))
+		return;
+
+	dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
+	dst_buf->sequence = ctx->qsequence - 1;
+
+	coda_m2m_buf_done(ctx, dst_buf, VB2_BUF_STATE_ERROR);
+}
+
 const struct coda_context_ops coda_bit_decode_ops = {
 	.queue_init = coda_decoder_queue_init,
 	.reqbufs = coda_decoder_reqbufs,
 	.start_streaming = coda_start_decoding,
 	.prepare_run = coda_prepare_decode,
 	.finish_run = coda_finish_decode,
+	.run_timeout = coda_decode_timeout,
 	.seq_end_work = coda_seq_end_work,
 	.release = coda_bit_release,
 };
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
index 78bd9a4ace0e4..829c7895a98a2 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1163,6 +1163,9 @@ static void coda_pic_run_work(struct work_struct *work)
 		ctx->hold = true;
 
 		coda_hw_reset(ctx);
+
+		if (ctx->ops->run_timeout)
+			ctx->ops->run_timeout(ctx);
 	} else if (!ctx->aborting) {
 		ctx->ops->finish_run(ctx);
 	}
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index 76d059431ca13..c5f504d8cf67f 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -183,6 +183,7 @@ struct coda_context_ops {
 	int (*start_streaming)(struct coda_ctx *ctx);
 	int (*prepare_run)(struct coda_ctx *ctx);
 	void (*finish_run)(struct coda_ctx *ctx);
+	void (*run_timeout)(struct coda_ctx *ctx);
 	void (*seq_end_work)(struct work_struct *work);
 	void (*release)(struct coda_ctx *ctx);
 };
-- 
2.11.0

[PATCH v2 3/3] [media] coda/imx-vdoa: always wait for job completion

[Philipp Zabel]

From: Lucas Stach <l.stach@pengutronix.de>

As long as only one CODA context is running we get alternating
device_run() and wait_for_completion() calls, but when more then one
CODA context is active, other VDOA slots can be inserted between those
calls for one context.

Make sure to wait on job completion before running a different context
and before destroying the currently active context.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
No changes since v1 [1].

[1] https://patchwork.linuxtv.org/patch/40605
---
 drivers/media/platform/coda/imx-vdoa.c | 49 +++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/coda/imx-vdoa.c b/drivers/media/platform/coda/imx-vdoa.c
index 669a4c82f1ffa..df9b71621420c 100644
--- a/drivers/media/platform/coda/imx-vdoa.c
+++ b/drivers/media/platform/coda/imx-vdoa.c
@@ -101,6 +101,8 @@ struct vdoa_ctx {
 	struct vdoa_data	*vdoa;
 	struct completion	completion;
 	struct vdoa_q_data	q_data[2];
+	unsigned int		submitted_job;
+	unsigned int		completed_job;
 };
 
 static irqreturn_t vdoa_irq_handler(int irq, void *data)
@@ -114,7 +116,7 @@ static irqreturn_t vdoa_irq_handler(int irq, void *data)
 
 	curr_ctx = vdoa->curr_ctx;
 	if (!curr_ctx) {
-		dev_dbg(vdoa->dev,
+		dev_warn(vdoa->dev,
 			"Instance released before the end of transaction\n");
 		return IRQ_HANDLED;
 	}
@@ -127,19 +129,44 @@ static irqreturn_t vdoa_irq_handler(int irq, void *data)
 	} else if (!(val & VDOAIST_EOT)) {
 		dev_warn(vdoa->dev, "Spurious interrupt\n");
 	}
+	curr_ctx->completed_job++;
 	complete(&curr_ctx->completion);
 
 	return IRQ_HANDLED;
 }
 
+int vdoa_wait_for_completion(struct vdoa_ctx *ctx)
+{
+	struct vdoa_data *vdoa = ctx->vdoa;
+
+	if (ctx->submitted_job == ctx->completed_job)
+		return 0;
+
+	if (!wait_for_completion_timeout(&ctx->completion,
+					 msecs_to_jiffies(300))) {
+		dev_err(vdoa->dev,
+			"Timeout waiting for transfer result\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(vdoa_wait_for_completion);
+
 void vdoa_device_run(struct vdoa_ctx *ctx, dma_addr_t dst, dma_addr_t src)
 {
 	struct vdoa_q_data *src_q_data, *dst_q_data;
 	struct vdoa_data *vdoa = ctx->vdoa;
 	u32 val;
 
+	if (vdoa->curr_ctx)
+		vdoa_wait_for_completion(vdoa->curr_ctx);
+
 	vdoa->curr_ctx = ctx;
 
+	reinit_completion(&ctx->completion);
+	ctx->submitted_job++;
+
 	src_q_data = &ctx->q_data[V4L2_M2M_SRC];
 	dst_q_data = &ctx->q_data[V4L2_M2M_DST];
 
@@ -177,21 +204,6 @@ void vdoa_device_run(struct vdoa_ctx *ctx, dma_addr_t dst, dma_addr_t src)
 }
 EXPORT_SYMBOL(vdoa_device_run);
 
-int vdoa_wait_for_completion(struct vdoa_ctx *ctx)
-{
-	struct vdoa_data *vdoa = ctx->vdoa;
-
-	if (!wait_for_completion_timeout(&ctx->completion,
-					 msecs_to_jiffies(300))) {
-		dev_err(vdoa->dev,
-			"Timeout waiting for transfer result\n");
-		return -ETIMEDOUT;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL(vdoa_wait_for_completion);
-
 struct vdoa_ctx *vdoa_context_create(struct vdoa_data *vdoa)
 {
 	struct vdoa_ctx *ctx;
@@ -218,6 +230,11 @@ void vdoa_context_destroy(struct vdoa_ctx *ctx)
 {
 	struct vdoa_data *vdoa = ctx->vdoa;
 
+	if (vdoa->curr_ctx == ctx) {
+		vdoa_wait_for_completion(vdoa->curr_ctx);
+		vdoa->curr_ctx = NULL;
+	}
+
 	clk_disable_unprepare(vdoa->vdoa_clk);
 	kfree(ctx);
 }
-- 
2.11.0

Re: [PATCH v2 2/3] [media] coda: first step at error recovery

[Hans Verkuil]

On 06/08/17 10:55, Philipp Zabel wrote:
> From: Lucas Stach <l.stach@pengutronix.de>
> 
> This implements a simple handler for the case where decode did not finish
> successfully. This might be helpful during normal streaming, but for now it
> only handles the case where the context would deadlock with userspace,
> i.e. userspace issued DEC_CMD_STOP and waits for EOS, but after the failed
> decode run we would hold the context and wait for userspace to queue more
> buffers.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> [p.zabel@pengutronix.de: renamed error_decode/run to run/decode_timeout]
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1 [1]:
> - Rename error_run/decode callback to run/decode_timeout, as
>   this error handler is called on device_run timeouts only.
> 
> [1] https://patchwork.linuxtv.org/patch/40603

It appears the v1 version was merged, not the v2. I'm not sure if the v2 version
was posted after v1 was already merged, or if I missed this v2 series.

I'm marking this as Obsoleted, and if you want to still get these v2 changes
in, then please post a new patch.

Sorry for the mix up,

	Hans

> ---
>  drivers/media/platform/coda/coda-bit.c    | 20 ++++++++++++++++++++
>  drivers/media/platform/coda/coda-common.c |  3 +++
>  drivers/media/platform/coda/coda.h        |  1 +
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/media/platform/coda/coda-bit.c b/drivers/media/platform/coda/coda-bit.c
> index 325035bb0a777..795b6d7584320 100644
> --- a/drivers/media/platform/coda/coda-bit.c
> +++ b/drivers/media/platform/coda/coda-bit.c
> @@ -2198,12 +2198,32 @@ static void coda_finish_decode(struct coda_ctx *ctx)
>  	ctx->display_idx = display_idx;
>  }
>  
> +static void coda_decode_timeout(struct coda_ctx *ctx)
> +{
> +	struct vb2_v4l2_buffer *dst_buf;
> +
> +	/*
> +	 * For now this only handles the case where we would deadlock with
> +	 * userspace, i.e. userspace issued DEC_CMD_STOP and waits for EOS,
> +	 * but after a failed decode run we would hold the context and wait for
> +	 * userspace to queue more buffers.
> +	 */
> +	if (!(ctx->bit_stream_param & CODA_BIT_STREAM_END_FLAG))
> +		return;
> +
> +	dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +	dst_buf->sequence = ctx->qsequence - 1;
> +
> +	coda_m2m_buf_done(ctx, dst_buf, VB2_BUF_STATE_ERROR);
> +}
> +
>  const struct coda_context_ops coda_bit_decode_ops = {
>  	.queue_init = coda_decoder_queue_init,
>  	.reqbufs = coda_decoder_reqbufs,
>  	.start_streaming = coda_start_decoding,
>  	.prepare_run = coda_prepare_decode,
>  	.finish_run = coda_finish_decode,
> +	.run_timeout = coda_decode_timeout,
>  	.seq_end_work = coda_seq_end_work,
>  	.release = coda_bit_release,
>  };
> diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c
> index 78bd9a4ace0e4..829c7895a98a2 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -1163,6 +1163,9 @@ static void coda_pic_run_work(struct work_struct *work)
>  		ctx->hold = true;
>  
>  		coda_hw_reset(ctx);
> +
> +		if (ctx->ops->run_timeout)
> +			ctx->ops->run_timeout(ctx);
>  	} else if (!ctx->aborting) {
>  		ctx->ops->finish_run(ctx);
>  	}
> diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
> index 76d059431ca13..c5f504d8cf67f 100644
> --- a/drivers/media/platform/coda/coda.h
> +++ b/drivers/media/platform/coda/coda.h
> @@ -183,6 +183,7 @@ struct coda_context_ops {
>  	int (*start_streaming)(struct coda_ctx *ctx);
>  	int (*prepare_run)(struct coda_ctx *ctx);
>  	void (*finish_run)(struct coda_ctx *ctx);
> +	void (*run_timeout)(struct coda_ctx *ctx);
>  	void (*seq_end_work)(struct work_struct *work);
>  	void (*release)(struct coda_ctx *ctx);
>  };
> 

Re: [PATCH v2 2/3] [media] coda: first step at error recovery

[Philipp Zabel]

On Fri, 2017-06-23 at 11:29 +0200, Hans Verkuil wrote:
> On 06/08/17 10:55, Philipp Zabel wrote:
> > From: Lucas Stach <l.stach@pengutronix.de>
> > 
> > This implements a simple handler for the case where decode did not finish
> > successfully. This might be helpful during normal streaming, but for now it
> > only handles the case where the context would deadlock with userspace,
> > i.e. userspace issued DEC_CMD_STOP and waits for EOS, but after the failed
> > decode run we would hold the context and wait for userspace to queue more
> > buffers.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> > [p.zabel@pengutronix.de: renamed error_decode/run to run/decode_timeout]
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v1 [1]:
> > - Rename error_run/decode callback to run/decode_timeout, as
> >   this error handler is called on device_run timeouts only.
> > 
> > [1] https://patchwork.linuxtv.org/patch/40603
> 
> It appears the v1 version was merged, not the v2. I'm not sure if the v2 version
> was posted after v1 was already merged, or if I missed this v2 series.
> 
> I'm marking this as Obsoleted, and if you want to still get these v2 changes
> in, then please post a new patch.
> 
> Sorry for the mix up,

Thank you for the heads up, I'll send a patch.

regards
Philipp