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

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Lucas,

On Wed, 2017-04-05 at 15:09 +0200, Lucas Stach wrote:
> This implements a simple handler for the case where decode did not finish
> sucessfully. 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>

Just a naming nitpick below.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

> ---
>  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 36062fc494e3..6a088f9343bb 100644
> --- a/drivers/media/platform/coda/coda-bit.c
> +++ b/drivers/media/platform/coda/coda-bit.c
> @@ -2113,12 +2113,32 @@ static void coda_finish_decode(struct coda_ctx *ctx)
>  	ctx->display_idx = display_idx;
>  }
>  
> +static void coda_error_decode(struct coda_ctx *ctx)

This sounds a bit like we are decoding an error code. Could we maybe
rename this any of coda_fail_decode or coda_decode_error/failure  or
similar?

> +{
> +	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,
> +	.error_run = coda_error_decode,

How about .fail_run to follow the <verb>_run pattern, or
.run_error/failure to break it?

>  	.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 eb6548f46cba..0bbf155f9783 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -1100,6 +1100,9 @@ static void coda_pic_run_work(struct work_struct *work)
>  		ctx->hold = true;
>  
>  		coda_hw_reset(ctx);
> +
> +		if (ctx->ops->error_run)
> +			ctx->ops->error_run(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 4b831c91ae4a..799ffca72203 100644
> --- a/drivers/media/platform/coda/coda.h
> +++ b/drivers/media/platform/coda/coda.h
> @@ -180,6 +180,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 (*error_run)(struct coda_ctx *ctx);
>  	void (*seq_end_work)(struct work_struct *work);
>  	void (*release)(struct coda_ctx *ctx);
>  };

regards
Philipp