diff mbox series

[v2] media: mediatek: vcodec: fix h264 cavlc bitstream fail

Message ID 20221018114122.26785-1-yunfei.dong@mediatek.com
State Accepted
Commit d555409dd1b7cc9e7e5b9e2924c0ef4bf23f6c9b
Headers show
Series [v2] media: mediatek: vcodec: fix h264 cavlc bitstream fail | expand

Commit Message

Yunfei Dong (董云飞) Oct. 18, 2022, 11:41 a.m. UTC
Some cavlc bistream will decode fail when the frame size is small than
20 bytes. Need to add pending data at the end of the bitstream.

For the minimum size of mapped memory is 256 bytes(16x16), adding four bytes data
won't lead to access unknown virtual memory.

Fixes: 59fba9eed5a7 ("media: mediatek: vcodec: support stateless H.264 decoding for mt8192")
Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
compared with v1:
- add detail comments for function: vdec_h264_insert_startcode.
- re-write commit message.
---
 .../vcodec/vdec/vdec_h264_req_multi_if.c      | 32 +++++++++++++++++--
 1 file changed, 29 insertions(+), 3 deletions(-)

Comments

Chen-Yu Tsai Nov. 14, 2022, 6:12 a.m. UTC | #1
On Tue, Oct 18, 2022 at 7:41 PM Yunfei Dong <yunfei.dong@mediatek.com> wrote:
>
> Some cavlc bistream will decode fail when the frame size is small than
> 20 bytes. Need to add pending data at the end of the bitstream.
>
> For the minimum size of mapped memory is 256 bytes(16x16), adding four bytes data
> won't lead to access unknown virtual memory.
>
> Fixes: 59fba9eed5a7 ("media: mediatek: vcodec: support stateless H.264 decoding for mt8192")
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>

Reviewed-by: Chen-Yu Tsai <wenst@chromium.org>
AngeloGioacchino Del Regno Nov. 14, 2022, 11:08 a.m. UTC | #2
Il 18/10/22 13:41, Yunfei Dong ha scritto:
> Some cavlc bistream will decode fail when the frame size is small than

s/small/smaller/g

> 20 bytes. Need to add pending data at the end of the bitstream.
> 
> For the minimum size of mapped memory is 256 bytes(16x16), adding four bytes data
> won't lead to access unknown virtual memory.
> 
> Fixes: 59fba9eed5a7 ("media: mediatek: vcodec: support stateless H.264 decoding for mt8192")
> Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> ---
> compared with v1:
> - add detail comments for function: vdec_h264_insert_startcode.
> - re-write commit message.
> ---
>   .../vcodec/vdec/vdec_h264_req_multi_if.c      | 32 +++++++++++++++++--
>   1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c
> index 4cc92700692b..18e048755d11 100644
> --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c
> @@ -539,6 +539,29 @@ static int vdec_h264_slice_core_decode(struct vdec_lat_buf *lat_buf)
>   	return 0;
>   }
>   
> +static void vdec_h264_insert_startcode(struct mtk_vcodec_dev *vcodec_dev, unsigned char *buf,
> +				       size_t *bs_size, struct mtk_h264_pps_param *pps)
> +{
> +	struct device *dev = &vcodec_dev->plat_dev->dev;
> +
> +	/* Need to add pending data at the end of bitstream when bs_sz is small than
> +	 * 20 bytes for cavlc bitstream, or lat will decode fail. This pending data is
> +	 * useful for mt8192 and mt8195 platform.

What is the reason why other SoCs don't need this?

> +	 *
> +	 * cavlc bitstream when entropy_coding_mode_flag is false.
> +	 */
> +	if (pps->entropy_coding_mode_flag || *bs_size > 20 ||
> +	    !(of_device_is_compatible(dev->of_node, "mediatek,mt8192-vcodec-dec") ||
> +	    of_device_is_compatible(dev->of_node, "mediatek,mt8195-vcodec-dec")))

I'm not comfortable seeing of_device_is_compatible... this list will grow whenever
a new SoC needing this appears.
Please think about a good name for a flag/quirk, or a bool, in the platform data
for these two SoCs and use it.

Regards,
Angelo
diff mbox series

Patch

diff --git a/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c
index 4cc92700692b..18e048755d11 100644
--- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c
+++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_h264_req_multi_if.c
@@ -539,6 +539,29 @@  static int vdec_h264_slice_core_decode(struct vdec_lat_buf *lat_buf)
 	return 0;
 }
 
+static void vdec_h264_insert_startcode(struct mtk_vcodec_dev *vcodec_dev, unsigned char *buf,
+				       size_t *bs_size, struct mtk_h264_pps_param *pps)
+{
+	struct device *dev = &vcodec_dev->plat_dev->dev;
+
+	/* Need to add pending data at the end of bitstream when bs_sz is small than
+	 * 20 bytes for cavlc bitstream, or lat will decode fail. This pending data is
+	 * useful for mt8192 and mt8195 platform.
+	 *
+	 * cavlc bitstream when entropy_coding_mode_flag is false.
+	 */
+	if (pps->entropy_coding_mode_flag || *bs_size > 20 ||
+	    !(of_device_is_compatible(dev->of_node, "mediatek,mt8192-vcodec-dec") ||
+	    of_device_is_compatible(dev->of_node, "mediatek,mt8195-vcodec-dec")))
+		return;
+
+	buf[*bs_size] = 0;
+	buf[*bs_size + 1] = 0;
+	buf[*bs_size + 2] = 1;
+	buf[*bs_size + 3] = 0xff;
+	(*bs_size) += 4;
+}
+
 static int vdec_h264_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
 				      struct vdec_fb *fb, bool *res_chg)
 {
@@ -582,9 +605,6 @@  static int vdec_h264_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
 	}
 
 	inst->vsi->dec.nal_info = buf[nal_start_idx];
-	inst->vsi->dec.bs_buf_addr = (u64)bs->dma_addr;
-	inst->vsi->dec.bs_buf_size = bs->size;
-
 	lat_buf->src_buf_req = src_buf_info->m2m_buf.vb.vb2_buf.req_obj.req;
 	v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb, &lat_buf->ts_info, true);
 
@@ -592,6 +612,12 @@  static int vdec_h264_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
 	if (err)
 		goto err_free_fb_out;
 
+	vdec_h264_insert_startcode(inst->ctx->dev, buf, &bs->size,
+				   &share_info->h264_slice_params.pps);
+
+	inst->vsi->dec.bs_buf_addr = (uint64_t)bs->dma_addr;
+	inst->vsi->dec.bs_buf_size = bs->size;
+
 	*res_chg = inst->resolution_changed;
 	if (inst->resolution_changed) {
 		mtk_vcodec_debug(inst, "- resolution changed -");