Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   John Cox <jc@kynesim.co.uk>
To:     Benjamin Gaignard <benjamin.gaignard@collabora.com>
Cc:     mchehab@kernel.org, ezequiel@vanguardiasur.com.ar,
        p.zabel@pengutronix.de, gregkh@linuxfoundation.org,
        mripard@kernel.org, paul.kocialkowski@bootlin.com, wens@csie.org,
        jernej.skrabec@gmail.com, hverkuil-cisco@xs4all.nl,
        jonas@kwiboo.se, nicolas@ndufresne.ca, 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,
        kernel@collabora.com, knaerzche@gmail.com,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>
Subject: Re: [RFC v2 6/8] media: uapi: Remove bit_size field from v4l2_ctrl_hevc_slice_params
Date:   Tue, 15 Feb 2022 14:17:57 +0000
Message-ID: <t2dn0hddgq22nt6a7sr6kl44irm3c2lj1j@4ax.com>
References: <20220215110103.241297-1-benjamin.gaignard@collabora.com> <20220215110103.241297-7-benjamin.gaignard@collabora.com>
In-Reply-To: <20220215110103.241297-7-benjamin.gaignard@collabora.com>
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

Hi

>The bit size of the slice could be deduced from the buffer payload
>so remove bit_size field to avoid duplicated the information.

I think this is a bad idea. In the future we are (I hope) going to want
to have an array (variable) of slice headers all referring to the same
bit buffer.  When we do that we will need this field.

>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>---
> .../userspace-api/media/v4l/ext-ctrls-codec.rst       |  3 ---
> drivers/staging/media/sunxi/cedrus/cedrus_h265.c      | 11 ++++-------
> include/uapi/linux/v4l2-controls.h                    |  3 +--
> 3 files changed, 5 insertions(+), 12 deletions(-)
>
>diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst =
b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>index 3296ac3b9fca..c3ae97657fa7 100644
>--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>@@ -2965,9 +2965,6 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>     :stub-columns: 0
>     :widths:       1 1 2
>=20
>-    * - __u32
>-      - ``bit_size``
>-      - Size (in bits) of the current slice data.
>     * - __u32
>       - ``data_bit_offset``
>       - Offset (in bits) to the video data in the current slice data.
>diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c =
b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>index 8ab2d9c6f048..db8c7475eeb8 100644
>--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>@@ -312,8 +312,8 @@ static void cedrus_h265_setup(struct cedrus_ctx =
*ctx,
> 	const struct v4l2_hevc_pred_weight_table *pred_weight_table;
> 	unsigned int width_in_ctb_luma, ctb_size_luma;
> 	unsigned int log2_max_luma_coding_block_size;
>+	size_t slice_bytes;
> 	dma_addr_t src_buf_addr;
>-	dma_addr_t src_buf_end_addr;
> 	u32 chroma_log2_weight_denom;
> 	u32 output_pic_list_index;
> 	u32 pic_order_cnt[2];
>@@ -370,8 +370,8 @@ static void cedrus_h265_setup(struct cedrus_ctx =
*ctx,
>=20
> 	cedrus_write(dev, VE_DEC_H265_BITS_OFFSET, 0);
>=20
>-	reg =3D slice_params->bit_size;
>-	cedrus_write(dev, VE_DEC_H265_BITS_LEN, reg);
>+	slice_bytes =3D vb2_get_plane_payload(&run->src->vb2_buf, 0);
>+	cedrus_write(dev, VE_DEC_H265_BITS_LEN, slice_bytes);

I think one of these must be wrong. bit_size is in bits,
vb2_get_plane_payload is in bytes?

Regards

John Cox
=20
> 	/* Source beginning and end addresses. */
>=20
>@@ -384,10 +384,7 @@ static void cedrus_h265_setup(struct cedrus_ctx =
*ctx,
>=20
> 	cedrus_write(dev, VE_DEC_H265_BITS_ADDR, reg);
>=20
>-	src_buf_end_addr =3D src_buf_addr +
>-			   DIV_ROUND_UP(slice_params->bit_size, 8);
>-
>-	reg =3D VE_DEC_H265_BITS_END_ADDR_BASE(src_buf_end_addr);
>+	reg =3D VE_DEC_H265_BITS_END_ADDR_BASE(src_buf_addr + slice_bytes);
> 	cedrus_write(dev, VE_DEC_H265_BITS_END_ADDR, reg);
>=20
> 	/* Coding tree block address */
>diff --git a/include/uapi/linux/v4l2-controls.h =
b/include/uapi/linux/v4l2-controls.h
>index b1a3dc05f02f..27f5d272dc43 100644
>--- a/include/uapi/linux/v4l2-controls.h
>+++ b/include/uapi/linux/v4l2-controls.h
>@@ -2457,7 +2457,6 @@ struct v4l2_hevc_pred_weight_table {
> #define V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT	(1ULL << 9)
>=20
> struct v4l2_ctrl_hevc_slice_params {
>-	__u32	bit_size;
> 	__u32	data_bit_offset;
>=20
> 	/* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */
>@@ -2484,7 +2483,7 @@ struct v4l2_ctrl_hevc_slice_params {
> 	/* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture timing SEI message */
> 	__u8	pic_struct;
>=20
>-	__u8	reserved;
>+	__u8	reserved[5];
>=20
> 	/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
> 	__u32	slice_segment_addr;