Re: [PATCH v1 05/18] media: controls: Add control for HEVC codec

From: Ezequiel Garcia <ezequiel@collabora.com>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>,
	 p.zabel@pengutronix.de, mchehab@kernel.org, robh+dt@kernel.org,
	 shawnguo@kernel.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de,  festevam@gmail.com, linux-imx@nxp.com,
	gregkh@linuxfoundation.org,  mripard@kernel.org,
	paul.kocialkowski@bootlin.com, wens@csie.org,
	 jernej.skrabec@siol.net, krzk@kernel.org, shengjiu.wang@nxp.com,
	 adrian.ratiu@collabora.com, aisheng.dong@nxp.com,
	peng.fan@nxp.com,  Anson.Huang@nxp.com, hverkuil-cisco@xs4all.nl
Cc: devel@driverdev.osuosl.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
	kernel@collabora.com, linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v1 05/18] media: controls: Add control for HEVC codec
Date: Wed, 17 Feb 2021 16:58:22 -0300	[thread overview]
Message-ID: <f66a65db46d24beb42b2c286de9e6212b6666978.camel@collabora.com> (raw)
In-Reply-To: <20210217080306.157876-6-benjamin.gaignard@collabora.com>

Hi Benjamin,

On Wed, 2021-02-17 at 09:02 +0100, Benjamin Gaignard wrote:
> Add HEVC decode params and scaling matrix controls.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 36 ++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 016cf6204cbb..5e45333fd862 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1028,6 +1028,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>         case V4L2_CID_MPEG_VIDEO_HEVC_SPS:                      return "HEVC Sequence Parameter Set";
>         case V4L2_CID_MPEG_VIDEO_HEVC_PPS:                      return "HEVC Picture Parameter Set";
>         case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS:             return "HEVC Slice Parameters";
> +       case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS:            return "HEVC Decode Parameters";
> +       case V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX:           return "HEVC Scaling Matrix";

I would move all the SCALING_MATRIX changes to their own patches.

>         case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE:              return "HEVC Decode Mode";
>         case V4L2_CID_MPEG_VIDEO_HEVC_START_CODE:               return "HEVC Start Code";
>  
> @@ -1482,6 +1484,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>         case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS:
>                 *type = V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS;
>                 break;
> +       case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_PARAMS:
> +               *type = V4L2_CTRL_TYPE_HEVC_DECODE_PARAMS;
> +               break;
> +       case V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX:
> +               *type = V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX;
> +               break;
>         case V4L2_CID_UNIT_CELL_SIZE:
>                 *type = V4L2_CTRL_TYPE_AREA;
>                 *flags |= V4L2_CTRL_FLAG_READ_ONLY;
> @@ -1833,6 +1841,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>         struct v4l2_ctrl_hevc_sps *p_hevc_sps;
>         struct v4l2_ctrl_hevc_pps *p_hevc_pps;
>         struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params;
> +       struct v4l2_ctrl_hevc_decode_params *p_hevc_decode_params;
>         struct v4l2_area *area;
>         void *p = ptr.p + idx * ctrl->elem_size;
>         unsigned int i;
> @@ -2108,26 +2117,33 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>                 zero_padding(*p_hevc_pps);
>                 break;
>  
> -       case V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS:
> -               p_hevc_slice_params = p;
> +       case V4L2_CTRL_TYPE_HEVC_DECODE_PARAMS:
> +               p_hevc_decode_params = p;
>  
> -               if (p_hevc_slice_params->num_active_dpb_entries >
> +               if (p_hevc_decode_params->num_active_dpb_entries >

I suspect this change should be squashed with the patch that moves
num_active_dpb_entries from the slice control, or otherwise
this code won't compile.

>                     V4L2_HEVC_DPB_ENTRIES_NUM_MAX)
>                         return -EINVAL;
>  
> -               zero_padding(p_hevc_slice_params->pred_weight_table);
> -
> -               for (i = 0; i < p_hevc_slice_params->num_active_dpb_entries;
> +               for (i = 0; i < p_hevc_decode_params->num_active_dpb_entries;
>                      i++) {
>                         struct v4l2_hevc_dpb_entry *dpb_entry =
> -                               &p_hevc_slice_params->dpb[i];
> +                               &p_hevc_decode_params->dpb[i];
>  

Ditto.

Thanks,
Ezequiel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip