Re: [PATCH v3 2/3] media: mediatek: vcodec: support extended h264 decode

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

Il 07/11/24 08:45, Yunfei Dong ha scritto:
> The address end of working buffer can't be calculated directly with buffer
> size in kernel for some special architecture. Adding new extend vsi_ex to
> calculate the address end in firmware.
> Adding capability to separate extend and non extend driver for different
> platform.
> At last, hardware can parse the syntax to get nal information in firmware
> for extend architecture, needn't to parse it again in kernel.
> 
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> ---
>   .../vcodec/decoder/mtk_vcodec_dec_drv.h       |   2 +
>   .../decoder/vdec/vdec_h264_req_multi_if.c     | 487 +++++++++++++++++-
>   2 files changed, 472 insertions(+), 17 deletions(-)
> 
> 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 886fa385e2e6..1e697bc810b0 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
> @@ -17,6 +17,7 @@
>   
>   #define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >= MTK_VDEC_LAT_SINGLE_CORE)
>   #define IS_VDEC_INNER_RACING(capability) ((capability) & MTK_VCODEC_INNER_RACING)
> +#define IS_VDEC_SUPPORT_EX(capability) ((capability) & MTK_VDEC_IS_SUPPORT_EX)
>   
>   enum mtk_vcodec_dec_chip_name {
>   	MTK_VDEC_INVAL = 0,
> @@ -42,6 +43,7 @@ enum mtk_vdec_format_types {
>   	MTK_VDEC_FORMAT_HEVC_FRAME = 0x1000,
>   	MTK_VCODEC_INNER_RACING = 0x20000,
>   	MTK_VDEC_IS_SUPPORT_10BIT = 0x40000,
> +	MTK_VDEC_IS_SUPPORT_EX = 0x80000,
>   };
>   
>   /*
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> index 851a8490b828..d0aecd9621d9 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c

..snip..

>   	inst->vsi_ctx.dec.y_fb_dma = y_fb_dma;
> @@ -816,8 +1260,17 @@ static int vdec_h264_slice_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
>   	if (!h_vdec)
>   		return -EINVAL;
>   
> -	if (inst->ctx->dev->vdec_pdata->hw_arch == MTK_VDEC_PURE_SINGLE_CORE)
> -		ret = vdec_h264_slice_single_decode(h_vdec, bs, unused, res_chg);
> +	if (inst->ctx->dev->vdec_pdata->hw_arch == MTK_VDEC_PURE_SINGLE_CORE) {
> +		if (IS_VDEC_SUPPORT_EX(inst->ctx->dev->dec_capability))
> +			ret = vdec_h264_slice_single_decode_ex(h_vdec, bs, unused, res_chg);

I wonder if we can use function pointers here, as I feel like vcodec is becoming
a bit "full of branches here and there"...

The rough idea is:

/* there, or somewhere that's called only once in the driver lifetime anyway */
static int vdec_h264_slice_init(.....)
{
	........

	if (hw_arch == MTK_VDEC_PURE_SINGLE_CORE) {
		if (inst->ctx->dev->dec_capability & MTK_VDEC_IS_SUPPORT_EX)
			inst->decode = vdec_h264_slice_single_decode_ex;
		else
			inst->decode = vdec_h264_slice_single_decode;
	}  else {
		if (inst->ctx->dev->dec_capability & MTK_VDEC_IS_SUPPORT_EX)
			inst->decode = vdec_h264_slice_lat_decode_ex;
		else
			inst->decode = vdec_h264_slice_lat_decode;
	}

	......
}

static int vdec_h264_slice_decode(...)
{
	if (!inst)
		return -EINVAL;

	return inst->decode( .... )
}

...less branches during decoding *of each frame* :-)

Cheers,
Angelo