Re: [PATCH v2 4/5] media: vcodec: Implement manual request completion
From: Nicolas Dufresne
Date: Mon Apr 14 2025 - 15:20:11 EST
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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > Co-developed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
thanks,
--
Nicolas Dufresne
Principal Engineer at Collabora