Re: [PATCH 4/5] media: vcodec: Implement manual request completion

[Nicolas Dufresne <nicolas@ndufresne.ca>]

Le vendredi 14 mars 2025 à 14:26 +0100, Sebastian Fricke a écrit :
> Rework how requests are completed in the MediaTek VCodec driver, by
> implementing the new manual request completion feature, which allows to
> keep a request open while allowing to add new bitstream data.
> This is useful in this case, because the hardware has a LAT and a core
> decode work, after the LAT decode the bitstream isn't required anymore
> so the source buffer can be set done and the request stays open until
> the core decode work finishes.
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
>  .../mediatek/vcodec/common/mtk_vcodec_cmn_drv.h    | 13 ++++++
>  .../mediatek/vcodec/decoder/mtk_vcodec_dec.c       |  4 +-
>  .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c   | 52 ++++++++++++++++++++++
>  .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   |  4 ++
>  .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 46 +++++++++----------
>  5 files changed, 94 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> index 6087e27bd604d24e5d37b48de5bb37eab86fc1ab..81ec5beecfaed239ed70bee3460aae27e476231c 100644
> --- a/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> +++ b/drivers/media/platform/mediatek/vcodec/common/mtk_vcodec_cmn_drv.h
> @@ -105,6 +105,19 @@ enum mtk_instance_state {
>  	MTK_STATE_ABORT = 4,
>  };
>  
> +/**
> + * enum mtk_request_state - Stages of processing a request
> + * MTK_REQUEST_RECEIVED: Hardware prepared for the LAT decode
> + * MTK_REQUEST_DONE_WITH_BITSTREAM: LAT decode finished, the bitstream is not
> + *				    needed anymore
> + * MTK_REQUEST_COMPLETE: CORE decode finished
> + */
> +enum mtk_request_state {
> +	MTK_REQUEST_RECEIVED = 0,
> +	MTK_REQUEST_LAT_DONE = 1,
> +	MTK_REQUEST_CORE_DONE = 2,
> +};
> +
>  enum mtk_fmt_type {
>  	MTK_FMT_DEC = 0,
>  	MTK_FMT_ENC = 1,
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> index 98838217b97d45ed2b5431fdf87c94e0ff79fc57..036ad191a9c3e644fe99b4ce25d6a089292f1e57 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec.c
> @@ -889,8 +889,10 @@ void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
>  					src_buf->vb2_buf.req_obj.req;
>  				v4l2_m2m_buf_done(src_buf,
>  						VB2_BUF_STATE_ERROR);
> -				if (req)
> +				if (req) {
>  					v4l2_ctrl_request_complete(req, &ctx->ctrl_hdl);
> +					media_request_manual_complete(req);
> +				}
>  			}
>  		}
>  		return;
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> index 9247d92d431d8570609423156b989878f7901f1c..d9c7aaec0c4515cb73b80c913b1ad5b08392dd18 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.c
> @@ -26,6 +26,58 @@
>  #include "mtk_vcodec_dec_pm.h"
>  #include "../common/mtk_vcodec_intr.h"
>  
> +const char *state_to_str(enum mtk_request_state state)
> +{
> +	switch (state) {
> +	case MTK_REQUEST_RECEIVED:
> +		return "RECEIVED";
> +	case MTK_REQUEST_LAT_DONE:
> +		return "LAT_DONE";
> +	case MTK_REQUEST_CORE_DONE:
> +		return "CORE_DONE";
> +	default:
> +		return "UNKNOWN";
> +	}
> +}
> +
> +int mtk_vcodec_complete(struct mtk_vcodec_dec_ctx *ctx, enum mtk_request_state state,
> +			enum vb2_buffer_state buffer_state, struct media_request *src_buf_req)
> +{
> +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> +
> +	mutex_lock(&ctx->lock);
> +
> +	if (ctx->req_state >= state) {
> +		mutex_unlock(&ctx->lock);
> +		return -EINVAL;
> +	}
> +
> +	switch (ctx->req_state) {
> +	case MTK_REQUEST_RECEIVED:
> +		v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
> +		src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> +		v4l2_m2m_buf_done(src_buf, buffer_state);
> +		ctx->req_state = MTK_REQUEST_LAT_DONE;
> +		if (state == MTK_REQUEST_LAT_DONE)
> +			break;
> +		fallthrough;
> +	case MTK_REQUEST_LAT_DONE:
> +		dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> +		v4l2_m2m_buf_done(dst_buf, buffer_state);
> +		media_request_manual_complete(src_buf_req);
> +		ctx->req_state = MTK_REQUEST_CORE_DONE;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	mtk_v4l2_vdec_dbg(3, ctx, "Switch state from %s to %s.\n",
> +			  state_to_str(ctx->req_state), state_to_str(state));
> +	ctx->req_state = state;
> +	mutex_unlock(&ctx->lock);
> +	return 0;
> +}
> +
>  static int mtk_vcodec_get_hw_count(struct mtk_vcodec_dec_ctx *ctx, struct mtk_vcodec_dec_dev *dev)
>  {
>  	switch (dev->vdec_pdata->hw_arch) {
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> index ac568ed14fa257d25b533b6fd6b3cd341227ecc2..21c2301363d0151ba3cf384a2a32fff9f5d46034 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h
> @@ -185,6 +185,7 @@ struct mtk_vcodec_dec_ctx {
>  	struct mtk_q_data q_data[2];
>  	int id;
>  	enum mtk_instance_state state;
> +	enum mtk_request_state req_state;

As you discovered before I came to do that final review, this is the
wrong version of the patchset. The request state must be per-request.
Looking forward your v2.

Nicolas

>  
>  	const struct vdec_common_if *dec_if;
>  	void *drv_handle;
> @@ -326,6 +327,9 @@ wake_up_dec_ctx(struct mtk_vcodec_dec_ctx *ctx, unsigned int reason, unsigned in
>  	wake_up_interruptible(&ctx->queue[hw_id]);
>  }
>  
> +int mtk_vcodec_complete(struct mtk_vcodec_dec_ctx *ctx, enum mtk_request_state state,
> +			enum vb2_buffer_state buffer_state, struct media_request *src_buf_req);
> +
>  #define mtk_vdec_err(ctx, fmt, args...)                               \
>  	mtk_vcodec_err((ctx)->id, (ctx)->dev->plat_dev, fmt, ##args)
>  
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
> index afa224da0f4165cf5701d6861f1f787c6317bfe4..9187d7bcfc8aea17f3fc98d94419777d8026db51 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
> @@ -247,7 +247,6 @@ static const struct v4l2_frmsize_stepwise stepwise_fhd = {
>  static void mtk_vdec_stateless_cap_to_disp(struct mtk_vcodec_dec_ctx *ctx, int error,
>  					   struct media_request *src_buf_req)
>  {
> -	struct vb2_v4l2_buffer *vb2_dst;
>  	enum vb2_buffer_state state;
>  
>  	if (error)
> @@ -255,17 +254,7 @@ static void mtk_vdec_stateless_cap_to_disp(struct mtk_vcodec_dec_ctx *ctx, int e
>  	else
>  		state = VB2_BUF_STATE_DONE;
>  
> -	vb2_dst = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx);
> -	if (vb2_dst) {
> -		v4l2_m2m_buf_done(vb2_dst, state);
> -		mtk_v4l2_vdec_dbg(2, ctx, "free frame buffer id:%d to done list",
> -				  vb2_dst->vb2_buf.index);
> -	} else {
> -		mtk_v4l2_vdec_err(ctx, "dst buffer is NULL");
> -	}
> -
> -	if (src_buf_req)
> -		v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
> +	mtk_vcodec_complete(ctx, MTK_REQUEST_CORE_DONE, state, src_buf_req);
>  }
>  
>  static struct vdec_fb *vdec_get_cap_buffer(struct mtk_vcodec_dec_ctx *ctx)
> @@ -308,6 +297,7 @@ static void vb2ops_vdec_buf_request_complete(struct vb2_buffer *vb)
>  	struct mtk_vcodec_dec_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
>  
>  	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->ctrl_hdl);
> +	media_request_manual_complete(vb->req_obj.req);
>  }
>  
>  static void mtk_vdec_worker(struct work_struct *work)
> @@ -358,12 +348,17 @@ static void mtk_vdec_worker(struct work_struct *work)
>  	else
>  		mtk_v4l2_vdec_err(ctx, "vb2 buffer media request is NULL");
>  
> +	mutex_lock(&ctx->lock);
> +	ctx->req_state = MTK_REQUEST_RECEIVED;
> +	mutex_unlock(&ctx->lock);
> +
>  	ret = vdec_if_decode(ctx, bs_src, NULL, &res_chg);
> +
>  	if (ret && ret != -EAGAIN) {
>  		mtk_v4l2_vdec_err(ctx,
> -				  "[%d] decode src_buf[%d] sz=0x%zx pts=%llu ret=%d res_chg=%d",
> +				  "[%d] decode src_buf[%d] sz=0x%zx pts=%llu res_chg=%d ret=%d",
>  				  ctx->id, vb2_src->index, bs_src->size,
> -				  vb2_src->timestamp, ret, res_chg);
> +				  vb2_src->timestamp, res_chg, ret);
>  		if (ret == -EIO) {
>  			mutex_lock(&ctx->lock);
>  			dec_buf_src->error = true;
> @@ -372,18 +367,15 @@ static void mtk_vdec_worker(struct work_struct *work)
>  	}
>  
>  	state = ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE;
> +	if (ret != -EAGAIN)
> +		mtk_vcodec_complete(ctx, MTK_REQUEST_LAT_DONE, state, src_buf_req);
> +
>  	if (!IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch) ||
>  	    ctx->current_codec == V4L2_PIX_FMT_VP8_FRAME) {
> -		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx, state);
> -		if (src_buf_req)
> -			v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
> -	} else {
> -		if (ret != -EAGAIN) {
> -			v4l2_m2m_src_buf_remove(ctx->m2m_ctx);
> -			v4l2_m2m_buf_done(vb2_v4l2_src, state);
> -		}
> -		v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> +		mtk_vcodec_complete(ctx, MTK_REQUEST_CORE_DONE, state, src_buf_req);
>  	}
> +
> +	v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
>  }
>  
>  static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb)
> @@ -731,9 +723,15 @@ static int fops_media_request_validate(struct media_request *mreq)
>  	return vb2_request_validate(mreq);
>  }
>  
> +static void fops_media_request_queue(struct media_request *mreq)
> +{
> +	media_request_mark_manual_completion(mreq);
> +	v4l2_m2m_request_queue(mreq);
> +}
> +
>  const struct media_device_ops mtk_vcodec_media_ops = {
>  	.req_validate	= fops_media_request_validate,
> -	.req_queue	= v4l2_m2m_request_queue,
> +	.req_queue	= fops_media_request_queue,
>  };
>  
>  static void mtk_vcodec_add_formats(unsigned int fourcc,