linux-kernel - Re: [PATCH 1/2] media: amphion: Support display delay for Hevc format

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86d9eb915432c6b070f7c7f524973e9a363de76e.camel@ndufresne.ca>
Date: Mon, 06 Jan 2025 17:16:52 -0500
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Ming Qian <ming.qian@....nxp.com>, mchehab@...nel.org, 
	hverkuil-cisco@...all.nl
Cc: shawnguo@...nel.org, robh+dt@...nel.org, s.hauer@...gutronix.de, 
	kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
 xiahong.bao@....com, 	eagle.zhou@....com, tao.jiang_2@....com,
 imx@...ts.linux.dev, 	linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, 	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] media: amphion: Support display delay for Hevc
 format

Hi,

nit: use capital HEVC in the subject

Le jeudi 19 décembre 2024 à 10:51 +0900, Ming Qian a écrit :
> The amphion decoder firmware v1.9.0 supports display delay 0 for
> hevc format, then driver can enable this feature.

nit: HEVC

I think this added "feature" hides a bug you haven't fixed in this patch.


        v4l2_ctrl_new_std(&inst->ctrl_handler, &vdec_ctrl_ops,
                          V4L2_CID_MPEG_VIDEO_DEC_DISPLAY_DELAY_ENABLE,
                          0, 1, 1, 0);

With the control registered this way, 0 is the default, and the range of 0-1.
But from your commit message, this is only supported from firmware 1.9.0 and up.
I think the patch should basically adjust the min and def values according to
the detected firmware version.

This might actually be more complex, aka per CODEC, and for that you may want to
use v4l2_ctrl_config structure.

Nicolas


> Signed-off-by: Ming Qian <ming.qian@....nxp.com>
> ---
>  drivers/media/platform/amphion/vpu_malone.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
> index 5c6b2a841b6f..8f4aa48b2d65 100644
> --- a/drivers/media/platform/amphion/vpu_malone.c
> +++ b/drivers/media/platform/amphion/vpu_malone.c
> @@ -332,6 +332,8 @@ struct vpu_dec_ctrl {
>  	u32 buf_addr[VID_API_NUM_STREAMS];
>  };
>  
> +static const struct malone_padding_scode *get_padding_scode(u32 type, u32 fmt);
> +
>  u32 vpu_malone_get_data_size(void)
>  {
>  	return sizeof(struct vpu_dec_ctrl);
> @@ -654,8 +656,10 @@ static int vpu_malone_set_params(struct vpu_shared_addr *shared,
>  		hc->jpg[instance].jpg_mjpeg_interlaced = 0;
>  	}
>  
> -	hc->codec_param[instance].disp_imm = params->display_delay_enable ? 1 : 0;
> -	if (malone_format != MALONE_FMT_AVC)
> +	if (params->display_delay_enable &&
> +	    get_padding_scode(SCODE_PADDING_BUFFLUSH, params->codec_format))
> +		hc->codec_param[instance].disp_imm = 1;
> +	else
>  		hc->codec_param[instance].disp_imm = 0;
>  	hc->codec_param[instance].dbglog_enable = 0;
>  	iface->dbglog_desc.level = 0;
> @@ -1024,6 +1028,7 @@ static const struct malone_padding_scode padding_scodes[] = {
>  	{SCODE_PADDING_EOS,      V4L2_PIX_FMT_JPEG,        {0x0, 0x0}},
>  	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_H264,        {0x15010000, 0x0}},
>  	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_H264_MVC,    {0x15010000, 0x0}},
> +	{SCODE_PADDING_BUFFLUSH, V4L2_PIX_FMT_HEVC,        {0x3e010000, 0x20}},
>  };
>  
>  static const struct malone_padding_scode padding_scode_dft = {0x0, 0x0};
> @@ -1058,8 +1063,11 @@ static int vpu_malone_add_padding_scode(struct vpu_buffer *stream_buffer,
>  	int ret;
>  
>  	ps = get_padding_scode(scode_type, pixelformat);
> -	if (!ps)
> +	if (!ps) {
> +		if (scode_type == SCODE_PADDING_BUFFLUSH)
> +			return 0;
>  		return -EINVAL;
> +	}
>  
>  	wptr = readl(&str_buf->wptr);
>  	if (wptr < stream_buffer->phys || wptr > stream_buffer->phys + stream_buffer->length)