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

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

Le lundi 14 avril 2025 à 11:11 +0200, AngeloGioacchino Del Regno a écrit :
> 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)

I'd keep this one an exception, its local to this C file.

> 
> > +{
> > +	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( ....)

Ack, though the namspace here seems to be "mtk_vcodec_dec", so I'll opt
for that.

> 
> 
> > +			  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? :-)

Ack.

> 
> > +	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.

Something like that, very subtle at this point.

> 
> > +{
> > +	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>

thanks,

-- 
Nicolas Dufresne
Principal Engineer at Collabora