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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 10/04/25 17:39, Nicolas Dufresne ha scritto:
> From: Sebastian Fricke <sebastian.fricke@collabora.com>
> 
> 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>
> Co-developed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

This patch is great - but looks like it's worsening naming consistency across the
driver.

Please look below.

> ---
>   .../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   | 50 +++++++++++++++++
>   .../mediatek/vcodec/decoder/mtk_vcodec_dec_drv.h   | 19 +++++++
>   .../vcodec/decoder/mtk_vcodec_dec_stateless.c      | 63 +++++++++++++---------
>   5 files changed, 124 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..c5fd37cb60ca0cc5fd09c9243b36fbc716c56454 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_LAT_DONE: LAT decode finished, the bitstream is not
> + *		      needed anymore
> + * @MTK_REQUEST_CORE_DONE: 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..c80c1db509eaadd449bfd183c5eb9db0a1fc22bd 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,56 @@
>   #include "mtk_vcodec_dec_pm.h"
>   #include "../common/mtk_vcodec_intr.h"
>   
> +static const char *state_to_str(enum mtk_request_state state)

static const char *mtk_vcodec_req_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";
> +	}
> +}
> +
> +void mtk_request_complete(struct mtk_vcodec_dec_ctx *ctx, enum mtk_request_state state,

void mtk_vcodec_request_complete( ....)


> +			  enum vb2_buffer_state buffer_state, struct media_request *src_buf_req)
> +{
> +	struct mtk_request *req = req_to_mtk_req(src_buf_req);
> +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> +
> +	mutex_lock(&ctx->lock);
> +
> +	if (req->req_state >= state) {
> +		mutex_unlock(&ctx->lock);
> +		return;
> +	}
> +
> +	switch (req->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);
> +		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);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	mtk_v4l2_vdec_dbg(3, ctx, "Switch state from %s to %s.\n",
> +			  state_to_str(req->req_state), state_to_str(state));
> +	req->req_state = state;
> +	mutex_unlock(&ctx->lock);
> +}
> +
>   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..cd61bf46de6918c27ed39ba64162e5f2637f93b2 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
> @@ -126,6 +126,17 @@ struct mtk_vcodec_dec_pdata {
>   	bool uses_stateless_api;
>   };
>   
> +/**
> + * struct mtk_request - Media request private data.
> + *
> + * @req_state: Request completion state
> + * @req: Media Request structure
> + */
> +struct mtk_request {

Maybe mtk_vcodec_dec_request? :-)

> +	enum mtk_request_state req_state;
> +	struct media_request req;
> +};
> +
>   /**
>    * struct mtk_vcodec_dec_ctx - Context (instance) private data.
>    *
> @@ -317,6 +328,11 @@ static inline struct mtk_vcodec_dec_ctx *ctrl_to_dec_ctx(struct v4l2_ctrl *ctrl)
>   	return container_of(ctrl->handler, struct mtk_vcodec_dec_ctx, ctrl_hdl);
>   }
>   
> +static inline struct mtk_request *req_to_mtk_req(struct media_request *req)

...and this could become req_to_dec_req ... but no strong opinions on this one
specifically, so feel free to keep this as it is.

> +{
> +	return container_of(req, struct mtk_request, req);
> +}
> +
>   /* Wake up context wait_queue */
>   static inline void
>   wake_up_dec_ctx(struct mtk_vcodec_dec_ctx *ctx, unsigned int reason, unsigned int hw_id)

After which....

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>