* Re: [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode
2022-07-27 6:13 [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode Mingjia Zhang
@ 2022-07-27 8:06 ` AngeloGioacchino Del Regno
2022-07-27 8:31 ` yunfei.dong
2022-08-24 13:02 ` Hans Verkuil
2 siblings, 0 replies; 6+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-27 8:06 UTC (permalink / raw)
To: Mingjia Zhang, Yunfei Dong, Alexandre Courbot, Nicolas Dufresne,
Hans Verkuil, Benjamin Gaignard, Tiffany Lin, Andrew-CT Chen,
Mauro Carvalho Chehab, Rob Herring, Matthias Brugger, Tomasz Figa
Cc: George Sun, Xiaoyong Lu, Hsin-Yi Wang, Fritz Koenig,
Daniel Vetter, dri-devel, Irui Wang, Steve Cho, linux-media,
devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group
Il 27/07/22 08:13, Mingjia Zhang ha scritto:
> In order to reduce decoder latency, enable VP9 inner racing mode.
> Send lat trans buffer information to core when trigger lat to work,
> need not to wait until lat decode done.
>
> Signed-off-by: mingjia zhang <mingjia.zhang@mediatek.com>
For MT8195:
Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode
2022-07-27 6:13 [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode Mingjia Zhang
2022-07-27 8:06 ` AngeloGioacchino Del Regno
@ 2022-07-27 8:31 ` yunfei.dong
2022-08-24 13:02 ` Hans Verkuil
2 siblings, 0 replies; 6+ messages in thread
From: yunfei.dong @ 2022-07-27 8:31 UTC (permalink / raw)
To: Mingjia Zhang, Alexandre Courbot, Nicolas Dufresne, Hans Verkuil,
AngeloGioacchino Del Regno, Benjamin Gaignard, Tiffany Lin,
Andrew-CT Chen, Mauro Carvalho Chehab, Rob Herring,
Matthias Brugger, Tomasz Figa
Cc: George Sun, Xiaoyong Lu, Hsin-Yi Wang, Fritz Koenig,
Daniel Vetter, dri-devel, Irui Wang, Steve Cho, linux-media,
devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group
Hi mingjia,
Thanks for your patch.
Reviewed-by: Yunfei Dong <yunfei.dong@mediatek.com>
On Wed, 2022-07-27 at 14:13 +0800, Mingjia Zhang wrote:
> In order to reduce decoder latency, enable VP9 inner racing mode.
> Send lat trans buffer information to core when trigger lat to work,
> need not to wait until lat decode done.
>
> Signed-off-by: mingjia zhang <mingjia.zhang@mediatek.com>
> ---
> 1. CTS/GTS test pass
> 2. Fluster result: Ran 240/303 tests successfully
> ---
> .../vcodec/vdec/vdec_vp9_req_lat_if.c | 64 ++++++++++++-----
> --
> 1 file changed, 40 insertions(+), 24 deletions(-)
>
> diff --git
> a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index fb1c36a3592d..92b47f0fdf40 100644
> ---
> a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++
> b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -436,6 +436,7 @@ struct vdec_vp9_slice_ref {
> * @frame_ctx: 4 frame context according to VP9 Spec
> * @frame_ctx_helper: 4 frame context according to newest
> kernel spec
> * @dirty: state of each frame context
> + * @local_vsi: local instance vsi information
> * @init_vsi: vsi used for initialized VP9 instance
> * @vsi: vsi used for decoding/flush ...
> * @core_vsi: vsi used for Core stage
> @@ -482,6 +483,8 @@ struct vdec_vp9_slice_instance {
> struct v4l2_vp9_frame_context frame_ctx_helper;
> unsigned char dirty[4];
>
> + struct vdec_vp9_slice_vsi local_vsi;
> +
> /* MicroP vsi */
> union {
> struct vdec_vp9_slice_init_vsi *init_vsi;
> @@ -1616,16 +1619,10 @@ static int
> vdec_vp9_slice_update_single(struct vdec_vp9_slice_instance *instance
> }
>
> static int vdec_vp9_slice_update_lat(struct vdec_vp9_slice_instance
> *instance,
> - struct vdec_lat_buf *lat_buf,
> - struct vdec_vp9_slice_pfc *pfc)
> + struct vdec_vp9_slice_vsi *vsi)
> {
> - struct vdec_vp9_slice_vsi *vsi;
> -
> - vsi = &pfc->vsi;
> - memcpy(&pfc->state[0], &vsi->state, sizeof(vsi->state));
> -
> mtk_vcodec_debug(instance, "Frame %u LAT CRC 0x%08x %lx %lx\n",
> - pfc->seq, vsi->state.crc[0],
> + (instance->seq - 1), vsi->state.crc[0],
> (unsigned long)vsi->trans.dma_addr,
> (unsigned long)vsi->trans.dma_addr_end);
>
> @@ -2090,6 +2087,13 @@ static int vdec_vp9_slice_lat_decode(void
> *h_vdec, struct mtk_vcodec_mem *bs,
> return ret;
> }
>
> + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability)) {
> + vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> + memcpy(&instance->local_vsi, vsi, sizeof(*vsi));
> + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx,
> lat_buf);
> + vsi = &instance->local_vsi;
> + }
> +
> if (instance->irq) {
> ret = mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IRQ_
> RECEIVED,
> WAIT_INTR_TIMEOUT_MS
> , MTK_VDEC_LAT0);
> @@ -2102,22 +2106,25 @@ static int vdec_vp9_slice_lat_decode(void
> *h_vdec, struct mtk_vcodec_mem *bs,
> }
>
> vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> - ret = vdec_vp9_slice_update_lat(instance, lat_buf, pfc);
> + ret = vdec_vp9_slice_update_lat(instance, vsi);
>
> - /* LAT trans full, no more UBE or decode timeout */
> - if (ret) {
> - mtk_vcodec_err(instance, "VP9 decode error: %d\n",
> ret);
> - return ret;
> - }
> + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> + /* LAT trans full, no more UBE or decode timeout */
> + if (ret) {
> + mtk_vcodec_err(instance, "frame[%d] decode
> error: %d\n",
> + ret, (instance->seq - 1));
> + return ret;
> + }
>
> - mtk_vcodec_debug(instance, "lat dma addr: 0x%lx 0x%lx\n",
> - (unsigned long)pfc->vsi.trans.dma_addr,
> - (unsigned long)pfc->vsi.trans.dma_addr_end);
>
> - vdec_msg_queue_update_ube_wptr(&ctx->msg_queue,
> - vsi->trans.dma_addr_end +
> - ctx-
> >msg_queue.wdma_addr.dma_addr);
> - vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf);
> + vsi->trans.dma_addr_end += ctx->msg_queue.wdma_addr.dma_addr;
> + vdec_msg_queue_update_ube_wptr(&ctx->msg_queue, vsi-
> >trans.dma_addr_end);
> + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx,
> lat_buf);
> +
> + mtk_vcodec_debug(instance, "lat trans end addr(0x%lx), ube
> start addr(0x%lx)\n",
> + (unsigned long)vsi->trans.dma_addr_end,
> + (unsigned long)ctx-
> >msg_queue.wdma_addr.dma_addr);
>
> return 0;
> }
> @@ -2193,10 +2200,14 @@ static int vdec_vp9_slice_core_decode(struct
> vdec_lat_buf *lat_buf)
> goto err;
> }
>
> - pfc->vsi.trans.dma_addr_end += ctx-
> >msg_queue.wdma_addr.dma_addr;
> mtk_vcodec_debug(instance, "core dma_addr_end 0x%lx\n",
> (unsigned long)pfc->vsi.trans.dma_addr_end);
> - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> >vsi.trans.dma_addr_end);
> +
> + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> >vsi.trans.dma_addr);
> + else
> + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> >vsi.trans.dma_addr_end);
> +
> ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf-
> >src_buf_req);
>
> return 0;
> @@ -2204,7 +2215,12 @@ static int vdec_vp9_slice_core_decode(struct
> vdec_lat_buf *lat_buf)
> err:
> if (ctx && pfc) {
> /* always update read pointer */
> - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> >vsi.trans.dma_addr_end);
> + if (IS_VDEC_INNER_RACING(instance->ctx->dev-
> >dec_capability))
> + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue,
> + pfc-
> >vsi.trans.dma_addr);
> + else
> + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue,
> + pfc-
> >vsi.trans.dma_addr_end);
>
> if (fb)
> ctx->dev->vdec_pdata->cap_to_disp(ctx, 1,
> lat_buf->src_buf_req);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode
2022-07-27 6:13 [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode Mingjia Zhang
2022-07-27 8:06 ` AngeloGioacchino Del Regno
2022-07-27 8:31 ` yunfei.dong
@ 2022-08-24 13:02 ` Hans Verkuil
2022-10-10 6:14 ` mingjia.zhang
2022-10-24 6:13 ` mingjia.zhang
2 siblings, 2 replies; 6+ messages in thread
From: Hans Verkuil @ 2022-08-24 13:02 UTC (permalink / raw)
To: Mingjia Zhang, Yunfei Dong, Alexandre Courbot, Nicolas Dufresne,
AngeloGioacchino Del Regno, Benjamin Gaignard, Tiffany Lin,
Andrew-CT Chen, Mauro Carvalho Chehab, Rob Herring,
Matthias Brugger, Tomasz Figa
Cc: Irui Wang, George Sun, Steve Cho, devicetree,
Project_Global_Chrome_Upstream_Group, linux-kernel, dri-devel,
Xiaoyong Lu, linux-mediatek, Hsin-Yi Wang, Fritz Koenig,
linux-arm-kernel, linux-media
Hi Mingjia,
On 27/07/2022 08:13, Mingjia Zhang wrote:
> In order to reduce decoder latency, enable VP9 inner racing mode.
> Send lat trans buffer information to core when trigger lat to work,
> need not to wait until lat decode done.
>
> Signed-off-by: mingjia zhang <mingjia.zhang@mediatek.com>
I'm getting this compile warning:
drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c: In function 'vdec_vp9_slice_core_decode':
drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c:2218:50: warning: 'instance' may be used uninitialized in this function [-Wmaybe-uninitialized]
2218 | if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
| ^~
I think you need to take a close look at the error handling in vdec_vp9_slice_core_decode().
After each error there is a 'goto err;' and that will run the new code, and that doesn't
feel right.
Regards,
Hans
> ---
> 1. CTS/GTS test pass
> 2. Fluster result: Ran 240/303 tests successfully
> ---
> .../vcodec/vdec/vdec_vp9_req_lat_if.c | 64 ++++++++++++-------
> 1 file changed, 40 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> index fb1c36a3592d..92b47f0fdf40 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> @@ -436,6 +436,7 @@ struct vdec_vp9_slice_ref {
> * @frame_ctx: 4 frame context according to VP9 Spec
> * @frame_ctx_helper: 4 frame context according to newest kernel spec
> * @dirty: state of each frame context
> + * @local_vsi: local instance vsi information
> * @init_vsi: vsi used for initialized VP9 instance
> * @vsi: vsi used for decoding/flush ...
> * @core_vsi: vsi used for Core stage
> @@ -482,6 +483,8 @@ struct vdec_vp9_slice_instance {
> struct v4l2_vp9_frame_context frame_ctx_helper;
> unsigned char dirty[4];
>
> + struct vdec_vp9_slice_vsi local_vsi;
> +
> /* MicroP vsi */
> union {
> struct vdec_vp9_slice_init_vsi *init_vsi;
> @@ -1616,16 +1619,10 @@ static int vdec_vp9_slice_update_single(struct vdec_vp9_slice_instance *instance
> }
>
> static int vdec_vp9_slice_update_lat(struct vdec_vp9_slice_instance *instance,
> - struct vdec_lat_buf *lat_buf,
> - struct vdec_vp9_slice_pfc *pfc)
> + struct vdec_vp9_slice_vsi *vsi)
> {
> - struct vdec_vp9_slice_vsi *vsi;
> -
> - vsi = &pfc->vsi;
> - memcpy(&pfc->state[0], &vsi->state, sizeof(vsi->state));
> -
> mtk_vcodec_debug(instance, "Frame %u LAT CRC 0x%08x %lx %lx\n",
> - pfc->seq, vsi->state.crc[0],
> + (instance->seq - 1), vsi->state.crc[0],
> (unsigned long)vsi->trans.dma_addr,
> (unsigned long)vsi->trans.dma_addr_end);
>
> @@ -2090,6 +2087,13 @@ static int vdec_vp9_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
> return ret;
> }
>
> + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability)) {
> + vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> + memcpy(&instance->local_vsi, vsi, sizeof(*vsi));
> + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf);
> + vsi = &instance->local_vsi;
> + }
> +
> if (instance->irq) {
> ret = mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IRQ_RECEIVED,
> WAIT_INTR_TIMEOUT_MS, MTK_VDEC_LAT0);
> @@ -2102,22 +2106,25 @@ static int vdec_vp9_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
> }
>
> vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> - ret = vdec_vp9_slice_update_lat(instance, lat_buf, pfc);
> + ret = vdec_vp9_slice_update_lat(instance, vsi);
>
> - /* LAT trans full, no more UBE or decode timeout */
> - if (ret) {
> - mtk_vcodec_err(instance, "VP9 decode error: %d\n", ret);
> - return ret;
> - }
> + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> + /* LAT trans full, no more UBE or decode timeout */
> + if (ret) {
> + mtk_vcodec_err(instance, "frame[%d] decode error: %d\n",
> + ret, (instance->seq - 1));
> + return ret;
> + }
>
> - mtk_vcodec_debug(instance, "lat dma addr: 0x%lx 0x%lx\n",
> - (unsigned long)pfc->vsi.trans.dma_addr,
> - (unsigned long)pfc->vsi.trans.dma_addr_end);
>
> - vdec_msg_queue_update_ube_wptr(&ctx->msg_queue,
> - vsi->trans.dma_addr_end +
> - ctx->msg_queue.wdma_addr.dma_addr);
> - vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf);
> + vsi->trans.dma_addr_end += ctx->msg_queue.wdma_addr.dma_addr;
> + vdec_msg_queue_update_ube_wptr(&ctx->msg_queue, vsi->trans.dma_addr_end);
> + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf);
> +
> + mtk_vcodec_debug(instance, "lat trans end addr(0x%lx), ube start addr(0x%lx)\n",
> + (unsigned long)vsi->trans.dma_addr_end,
> + (unsigned long)ctx->msg_queue.wdma_addr.dma_addr);
>
> return 0;
> }
> @@ -2193,10 +2200,14 @@ static int vdec_vp9_slice_core_decode(struct vdec_lat_buf *lat_buf)
> goto err;
> }
>
> - pfc->vsi.trans.dma_addr_end += ctx->msg_queue.wdma_addr.dma_addr;
> mtk_vcodec_debug(instance, "core dma_addr_end 0x%lx\n",
> (unsigned long)pfc->vsi.trans.dma_addr_end);
> - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc->vsi.trans.dma_addr_end);
> +
> + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc->vsi.trans.dma_addr);
> + else
> + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc->vsi.trans.dma_addr_end);
> +
> ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf->src_buf_req);
>
> return 0;
> @@ -2204,7 +2215,12 @@ static int vdec_vp9_slice_core_decode(struct vdec_lat_buf *lat_buf)
> err:
> if (ctx && pfc) {
> /* always update read pointer */
> - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc->vsi.trans.dma_addr_end);
> + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue,
> + pfc->vsi.trans.dma_addr);
> + else
> + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue,
> + pfc->vsi.trans.dma_addr_end);
>
> if (fb)
> ctx->dev->vdec_pdata->cap_to_disp(ctx, 1, lat_buf->src_buf_req);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode
2022-08-24 13:02 ` Hans Verkuil
@ 2022-10-10 6:14 ` mingjia.zhang
2022-10-24 6:13 ` mingjia.zhang
1 sibling, 0 replies; 6+ messages in thread
From: mingjia.zhang @ 2022-10-10 6:14 UTC (permalink / raw)
To: Hans Verkuil, Yunfei Dong, Alexandre Courbot, Nicolas Dufresne,
AngeloGioacchino Del Regno, Benjamin Gaignard, Tiffany Lin,
Andrew-CT Chen, Mauro Carvalho Chehab, Rob Herring,
Matthias Brugger, Tomasz Figa
Cc: Irui Wang, George Sun, Steve Cho, devicetree,
Project_Global_Chrome_Upstream_Group, linux-kernel, dri-devel,
Xiaoyong Lu, linux-mediatek, Hsin-Yi Wang, Fritz Koenig,
linux-arm-kernel, linux-media
Hi Hans,
Thanks for you reply and useful comments.
After reviewing the error handling in vdec_vp9_slice_core_decode(), I
found that the problem you pointed out does exist in error handle part.
'instance' may be used uninitialized in this function.
I have fixed this build warning and reviewed other code, thanks.
Thanks,
mingjia
On Wed, 2022-08-24 at 15:02 +0200, Hans Verkuil wrote:
> Hi Mingjia,
>
> On 27/07/2022 08:13, Mingjia Zhang wrote:
> > In order to reduce decoder latency, enable VP9 inner racing mode.
> > Send lat trans buffer information to core when trigger lat to work,
> > need not to wait until lat decode done.
> >
> > Signed-off-by: mingjia zhang <mingjia.zhang@mediatek.com>
>
> I'm getting this compile warning:
>
> drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c: In
> function 'vdec_vp9_slice_core_decode':
> drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c:221
> 8:50: warning: 'instance' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 2218 | if (IS_VDEC_INNER_RACING(instance->ctx->dev-
> >dec_capability))
> | ^~
>
> I think you need to take a close look at the error handling in
> vdec_vp9_slice_core_decode().
>
> After each error there is a 'goto err;' and that will run the new
> code, and that doesn't
> feel right.
>
> Regards,
>
> Hans
>
> > ---
> > 1. CTS/GTS test pass
> > 2. Fluster result: Ran 240/303 tests successfully
> > ---
> > .../vcodec/vdec/vdec_vp9_req_lat_if.c | 64 ++++++++++++---
> > ----
> > 1 file changed, 40 insertions(+), 24 deletions(-)
> >
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > index fb1c36a3592d..92b47f0fdf40 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > @@ -436,6 +436,7 @@ struct vdec_vp9_slice_ref {
> > * @frame_ctx: 4 frame context according to VP9 Spec
> > * @frame_ctx_helper: 4 frame context according to newest
> > kernel spec
> > * @dirty: state of each frame context
> > + * @local_vsi: local instance vsi information
> > * @init_vsi: vsi used for initialized VP9 instance
> > * @vsi: vsi used for decoding/flush ...
> > * @core_vsi: vsi used for Core stage
> > @@ -482,6 +483,8 @@ struct vdec_vp9_slice_instance {
> > struct v4l2_vp9_frame_context frame_ctx_helper;
> > unsigned char dirty[4];
> >
> > + struct vdec_vp9_slice_vsi local_vsi;
> > +
> > /* MicroP vsi */
> > union {
> > struct vdec_vp9_slice_init_vsi *init_vsi;
> > @@ -1616,16 +1619,10 @@ static int
> > vdec_vp9_slice_update_single(struct vdec_vp9_slice_instance
> > *instance
> > }
> >
> > static int vdec_vp9_slice_update_lat(struct
> > vdec_vp9_slice_instance *instance,
> > - struct vdec_lat_buf *lat_buf,
> > - struct vdec_vp9_slice_pfc *pfc)
> > + struct vdec_vp9_slice_vsi *vsi)
> > {
> > - struct vdec_vp9_slice_vsi *vsi;
> > -
> > - vsi = &pfc->vsi;
> > - memcpy(&pfc->state[0], &vsi->state, sizeof(vsi->state));
> > -
> > mtk_vcodec_debug(instance, "Frame %u LAT CRC 0x%08x %lx %lx\n",
> > - pfc->seq, vsi->state.crc[0],
> > + (instance->seq - 1), vsi->state.crc[0],
> > (unsigned long)vsi->trans.dma_addr,
> > (unsigned long)vsi->trans.dma_addr_end);
> >
> > @@ -2090,6 +2087,13 @@ static int vdec_vp9_slice_lat_decode(void
> > *h_vdec, struct mtk_vcodec_mem *bs,
> > return ret;
> > }
> >
> > + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability)) {
> > + vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> > + memcpy(&instance->local_vsi, vsi, sizeof(*vsi));
> > + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx,
> > lat_buf);
> > + vsi = &instance->local_vsi;
> > + }
> > +
> > if (instance->irq) {
> > ret = mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IR
> > Q_RECEIVED,
> > WAIT_INTR_TIMEOUT_MS
> > , MTK_VDEC_LAT0);
> > @@ -2102,22 +2106,25 @@ static int vdec_vp9_slice_lat_decode(void
> > *h_vdec, struct mtk_vcodec_mem *bs,
> > }
> >
> > vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> > - ret = vdec_vp9_slice_update_lat(instance, lat_buf, pfc);
> > + ret = vdec_vp9_slice_update_lat(instance, vsi);
> >
> > - /* LAT trans full, no more UBE or decode timeout */
> > - if (ret) {
> > - mtk_vcodec_err(instance, "VP9 decode error: %d\n",
> > ret);
> > - return ret;
> > - }
> > + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> > + /* LAT trans full, no more UBE or decode timeout */
> > + if (ret) {
> > + mtk_vcodec_err(instance, "frame[%d] decode
> > error: %d\n",
> > + ret, (instance->seq - 1));
> > + return ret;
> > + }
> >
> > - mtk_vcodec_debug(instance, "lat dma addr: 0x%lx 0x%lx\n",
> > - (unsigned long)pfc->vsi.trans.dma_addr,
> > - (unsigned long)pfc->vsi.trans.dma_addr_end);
> >
> > - vdec_msg_queue_update_ube_wptr(&ctx->msg_queue,
> > - vsi->trans.dma_addr_end +
> > - ctx-
> > >msg_queue.wdma_addr.dma_addr);
> > - vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf);
> > + vsi->trans.dma_addr_end += ctx->msg_queue.wdma_addr.dma_addr;
> > + vdec_msg_queue_update_ube_wptr(&ctx->msg_queue, vsi-
> > >trans.dma_addr_end);
> > + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> > + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx,
> > lat_buf);
> > +
> > + mtk_vcodec_debug(instance, "lat trans end addr(0x%lx), ube
> > start addr(0x%lx)\n",
> > + (unsigned long)vsi->trans.dma_addr_end,
> > + (unsigned long)ctx-
> > >msg_queue.wdma_addr.dma_addr);
> >
> > return 0;
> > }
> > @@ -2193,10 +2200,14 @@ static int
> > vdec_vp9_slice_core_decode(struct vdec_lat_buf *lat_buf)
> > goto err;
> > }
> >
> > - pfc->vsi.trans.dma_addr_end += ctx-
> > >msg_queue.wdma_addr.dma_addr;
> > mtk_vcodec_debug(instance, "core dma_addr_end 0x%lx\n",
> > (unsigned long)pfc->vsi.trans.dma_addr_end);
> > - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> > >vsi.trans.dma_addr_end);
> > +
> > + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> > >vsi.trans.dma_addr);
> > + else
> > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> > >vsi.trans.dma_addr_end);
> > +
> > ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf-
> > >src_buf_req);
> >
> > return 0;
> > @@ -2204,7 +2215,12 @@ static int vdec_vp9_slice_core_decode(struct
> > vdec_lat_buf *lat_buf)
> > err:
> > if (ctx && pfc) {
> > /* always update read pointer */
> > - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> > >vsi.trans.dma_addr_end);
> > + if (IS_VDEC_INNER_RACING(instance->ctx->dev-
> > >dec_capability))
> > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue,
> > + pfc-
> > >vsi.trans.dma_addr);
> > + else
> > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue,
> > + pfc-
> > >vsi.trans.dma_addr_end);
> >
> > if (fb)
> > ctx->dev->vdec_pdata->cap_to_disp(ctx, 1,
> > lat_buf->src_buf_req);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH, v2] media: mediatek: vcodec: Add to support VP9 inner racing mode
2022-08-24 13:02 ` Hans Verkuil
2022-10-10 6:14 ` mingjia.zhang
@ 2022-10-24 6:13 ` mingjia.zhang
1 sibling, 0 replies; 6+ messages in thread
From: mingjia.zhang @ 2022-10-24 6:13 UTC (permalink / raw)
To: Hans Verkuil, Yunfei Dong, Alexandre Courbot, Nicolas Dufresne,
AngeloGioacchino Del Regno, Benjamin Gaignard, Tiffany Lin,
Andrew-CT Chen, Mauro Carvalho Chehab, Rob Herring,
Matthias Brugger, Tomasz Figa
Cc: Irui Wang, George Sun, Steve Cho, devicetree,
Project_Global_Chrome_Upstream_Group, linux-kernel, dri-devel,
Xiaoyong Lu, linux-mediatek, Hsin-Yi Wang, Fritz Koenig,
linux-arm-kernel, linux-media
Hi Hans,
Thanks for you reply and useful suggestions.
The previous patch v3 may not fully modified. Now I push patch v4.
Please help to review it, thanks.
Thanks,
mingjia
On Wed, 2022-08-24 at 15:02 +0200, Hans Verkuil wrote:
> Hi Mingjia,
>
> On 27/07/2022 08:13, Mingjia Zhang wrote:
> > In order to reduce decoder latency, enable VP9 inner racing mode.
> > Send lat trans buffer information to core when trigger lat to work,
> > need not to wait until lat decode done.
> >
> > Signed-off-by: mingjia zhang <mingjia.zhang@mediatek.com>
>
> I'm getting this compile warning:
>
> drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c: In
> function 'vdec_vp9_slice_core_decode':
> drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c:221
> 8:50: warning: 'instance' may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 2218 | if (IS_VDEC_INNER_RACING(instance->ctx->dev-
> >dec_capability))
> | ^~
>
After reviewing the error handling in vdec_vp9_slice_core_decode(), I
found that the problem you pointed out does exist in error handle part.
'instance' may be used uninitialized in this function. I have fixed
this build warning and reviewed other code, thanks.
> I think you need to take a close look at the error handling in
> vdec_vp9_slice_core_decode().
>
> After each error there is a 'goto err;' and that will run the new
> code, and that doesn't
> feel right.
>
I have modified the code logic, and reduced some redundant code,
thanks.
> Regards,
>
> Hans
>
> > ---
> > 1. CTS/GTS test pass
> > 2. Fluster result: Ran 240/303 tests successfully
> > ---
> > .../vcodec/vdec/vdec_vp9_req_lat_if.c | 64 ++++++++++++---
> > ----
> > 1 file changed, 40 insertions(+), 24 deletions(-)
> >
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > index fb1c36a3592d..92b47f0fdf40 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp9_req_lat_if.c
> > @@ -436,6 +436,7 @@ struct vdec_vp9_slice_ref {
> > * @frame_ctx: 4 frame context according to VP9 Spec
> > * @frame_ctx_helper: 4 frame context according to newest
> > kernel spec
> > * @dirty: state of each frame context
> > + * @local_vsi: local instance vsi information
> > * @init_vsi: vsi used for initialized VP9 instance
> > * @vsi: vsi used for decoding/flush ...
> > * @core_vsi: vsi used for Core stage
> > @@ -482,6 +483,8 @@ struct vdec_vp9_slice_instance {
> > struct v4l2_vp9_frame_context frame_ctx_helper;
> > unsigned char dirty[4];
> >
> > + struct vdec_vp9_slice_vsi local_vsi;
> > +
> > /* MicroP vsi */
> > union {
> > struct vdec_vp9_slice_init_vsi *init_vsi;
> > @@ -1616,16 +1619,10 @@ static int
> > vdec_vp9_slice_update_single(struct vdec_vp9_slice_instance
> > *instance
> > }
> >
> > static int vdec_vp9_slice_update_lat(struct
> > vdec_vp9_slice_instance *instance,
> > - struct vdec_lat_buf *lat_buf,
> > - struct vdec_vp9_slice_pfc *pfc)
> > + struct vdec_vp9_slice_vsi *vsi)
> > {
> > - struct vdec_vp9_slice_vsi *vsi;
> > -
> > - vsi = &pfc->vsi;
> > - memcpy(&pfc->state[0], &vsi->state, sizeof(vsi->state));
> > -
> > mtk_vcodec_debug(instance, "Frame %u LAT CRC 0x%08x %lx %lx\n",
> > - pfc->seq, vsi->state.crc[0],
> > + (instance->seq - 1), vsi->state.crc[0],
> > (unsigned long)vsi->trans.dma_addr,
> > (unsigned long)vsi->trans.dma_addr_end);
> >
> > @@ -2090,6 +2087,13 @@ static int vdec_vp9_slice_lat_decode(void
> > *h_vdec, struct mtk_vcodec_mem *bs,
> > return ret;
> > }
> >
> > + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability)) {
> > + vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> > + memcpy(&instance->local_vsi, vsi, sizeof(*vsi));
> > + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx,
> > lat_buf);
> > + vsi = &instance->local_vsi;
> > + }
> > +
> > if (instance->irq) {
> > ret = mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IR
> > Q_RECEIVED,
> > WAIT_INTR_TIMEOUT_MS
> > , MTK_VDEC_LAT0);
> > @@ -2102,22 +2106,25 @@ static int vdec_vp9_slice_lat_decode(void
> > *h_vdec, struct mtk_vcodec_mem *bs,
> > }
> >
> > vdec_vp9_slice_vsi_from_remote(vsi, instance->vsi, 0);
> > - ret = vdec_vp9_slice_update_lat(instance, lat_buf, pfc);
> > + ret = vdec_vp9_slice_update_lat(instance, vsi);
> >
> > - /* LAT trans full, no more UBE or decode timeout */
> > - if (ret) {
> > - mtk_vcodec_err(instance, "VP9 decode error: %d\n",
> > ret);
> > - return ret;
> > - }
> > + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> > + /* LAT trans full, no more UBE or decode timeout */
> > + if (ret) {
> > + mtk_vcodec_err(instance, "frame[%d] decode
> > error: %d\n",
> > + ret, (instance->seq - 1));
> > + return ret;
> > + }
> >
> > - mtk_vcodec_debug(instance, "lat dma addr: 0x%lx 0x%lx\n",
> > - (unsigned long)pfc->vsi.trans.dma_addr,
> > - (unsigned long)pfc->vsi.trans.dma_addr_end);
> >
> > - vdec_msg_queue_update_ube_wptr(&ctx->msg_queue,
> > - vsi->trans.dma_addr_end +
> > - ctx-
> > >msg_queue.wdma_addr.dma_addr);
> > - vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx, lat_buf);
> > + vsi->trans.dma_addr_end += ctx->msg_queue.wdma_addr.dma_addr;
> > + vdec_msg_queue_update_ube_wptr(&ctx->msg_queue, vsi-
> > >trans.dma_addr_end);
> > + if (!IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> > + vdec_msg_queue_qbuf(&ctx->dev->msg_queue_core_ctx,
> > lat_buf);
> > +
> > + mtk_vcodec_debug(instance, "lat trans end addr(0x%lx), ube
> > start addr(0x%lx)\n",
> > + (unsigned long)vsi->trans.dma_addr_end,
> > + (unsigned long)ctx-
> > >msg_queue.wdma_addr.dma_addr);
> >
> > return 0;
> > }
> > @@ -2193,10 +2200,14 @@ static int
> > vdec_vp9_slice_core_decode(struct vdec_lat_buf *lat_buf)
> > goto err;
> > }
> >
> > - pfc->vsi.trans.dma_addr_end += ctx-
> > >msg_queue.wdma_addr.dma_addr;
> > mtk_vcodec_debug(instance, "core dma_addr_end 0x%lx\n",
> > (unsigned long)pfc->vsi.trans.dma_addr_end);
> > - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> > >vsi.trans.dma_addr_end);
> > +
> > + if (IS_VDEC_INNER_RACING(instance->ctx->dev->dec_capability))
> > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> > >vsi.trans.dma_addr);
> > + else
> > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> > >vsi.trans.dma_addr_end);
> > +
> > ctx->dev->vdec_pdata->cap_to_disp(ctx, 0, lat_buf-
> > >src_buf_req);
> >
> > return 0;
> > @@ -2204,7 +2215,12 @@ static int vdec_vp9_slice_core_decode(struct
> > vdec_lat_buf *lat_buf)
> > err:
> > if (ctx && pfc) {
> > /* always update read pointer */
> > - vdec_msg_queue_update_ube_rptr(&ctx->msg_queue, pfc-
> > >vsi.trans.dma_addr_end);
> > + if (IS_VDEC_INNER_RACING(instance->ctx->dev-
> > >dec_capability))
> > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue,
> > + pfc-
> > >vsi.trans.dma_addr);
> > + else
> > + vdec_msg_queue_update_ube_rptr(&ctx->msg_queue,
> > + pfc-
> > >vsi.trans.dma_addr_end);
> >
> > if (fb)
> > ctx->dev->vdec_pdata->cap_to_disp(ctx, 1,
> > lat_buf->src_buf_req);
^ permalink raw reply [flat|nested] 6+ messages in thread