Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   John Cox <jc@kynesim.co.uk>
To:     Jernej Skrabec <jernej.skrabec@siol.net>
Cc:     mchehab@kernel.org, hverkuil-cisco@xs4all.nl, mripard@kernel.org,
        paul.kocialkowski@bootlin.com, wens@csie.org,
        ezequiel@collabora.com, benjamin.gaignard@collabora.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-staging@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev
Subject: Re: [PATCH] media: hevc: Fix dependent slice segment flags
Date:   Thu, 29 Apr 2021 15:51:00 +0100
Message-ID: <lrhl8gl392tcpss5rcpkqr1cahujnlqq9k@4ax.com>
References: <20210427071554.2222625-1-jernej.skrabec@siol.net>
In-Reply-To: <20210427071554.2222625-1-jernej.skrabec@siol.net>
User-Agent: ForteAgent/8.00.32.1272
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

>Dependent slice segment flag for PPS control is misnamed. It should have
>"enabled" at the end. It only tells if this flag is present in slice
>header or not and not the actual value.
>
>Fix this by renaming the PPS flag and introduce another flag for slice
>control which tells actual value.
>
>Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
>---
> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 5 ++++-
> drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 4 ++--
> include/media/hevc-ctrls.h                                | 3 ++-
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
>diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst =
b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>index 7b90cb939e9d..5ed343ddd1ea 100644
>--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>@@ -3059,7 +3059,7 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>     :stub-columns: 0
>     :widths:       1 1 2
>=20
>-    * - ``V4L2_HEVC_PPS_FLAG_DEPENDENT_SLICE_SEGMENT``
>+    * - ``V4L2_HEVC_PPS_FLAG_DEPENDENT_SLICE_SEGMENT_ENABLED``
>       - 0x00000001
>       -
>     * - ``V4L2_HEVC_PPS_FLAG_OUTPUT_FLAG_PRESENT``
>@@ -3274,6 +3274,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>     * - =
``V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_LOOP_FILTER_ACROSS_SLICES_ENABLED``
>       - 0x00000100
>       -
>+    * - ``V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT``
>+      - 0x00000200
>+      -
>=20
> .. raw:: latex
>=20
>diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c =
b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>index 397a4ba5df4c..6821e3d05d34 100644
>--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>@@ -479,8 +479,8 @@ static void cedrus_h265_setup(struct cedrus_ctx =
*ctx,
> 				slice_params->flags);
>=20
> 	reg |=3D =
VE_DEC_H265_FLAG(VE_DEC_H265_DEC_SLICE_HDR_INFO0_FLAG_DEPENDENT_SLICE_SEG=
MENT,
>-				V4L2_HEVC_PPS_FLAG_DEPENDENT_SLICE_SEGMENT,
>-				pps->flags);
>+				V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT,
>+				slice_params->flags);
>=20
> 	/* FIXME: For multi-slice support. */
> 	reg |=3D =
VE_DEC_H265_DEC_SLICE_HDR_INFO0_FLAG_FIRST_SLICE_SEGMENT_IN_PIC;
>diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
>index b713eeed1915..dc964ff7cd29 100644
>--- a/include/media/hevc-ctrls.h
>+++ b/include/media/hevc-ctrls.h
>@@ -83,7 +83,7 @@ struct v4l2_ctrl_hevc_sps {
> 	__u64	flags;
> };
>=20
>-#define V4L2_HEVC_PPS_FLAG_DEPENDENT_SLICE_SEGMENT		(1ULL << 0)
>+#define V4L2_HEVC_PPS_FLAG_DEPENDENT_SLICE_SEGMENT_ENABLED	(1ULL << 0)
> #define V4L2_HEVC_PPS_FLAG_OUTPUT_FLAG_PRESENT			(1ULL << 1)
> #define V4L2_HEVC_PPS_FLAG_SIGN_DATA_HIDING_ENABLED		(1ULL << 2)
> #define V4L2_HEVC_PPS_FLAG_CABAC_INIT_PRESENT			(1ULL << 3)
>@@ -166,6 +166,7 @@ struct v4l2_hevc_pred_weight_table {
> #define V4L2_HEVC_SLICE_PARAMS_FLAG_USE_INTEGER_MV		(1ULL << 6)
> #define V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_DEBLOCKING_FILTER_DISABLED =
(1ULL << 7)
> #define =
V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_LOOP_FILTER_ACROSS_SLICES_ENABLED (1ULL=
 << 8)
>+#define V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT	(1ULL << 9)
>=20
> struct v4l2_ctrl_hevc_slice_params {
> 	__u32	bit_size;

Happy to see this flag finally making it

Many thanks

John Cox