Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Jernej =?utf-8?B?xaBrcmFiZWM=?= <jernej.skrabec@gmail.com>
To:     mchehab@kernel.org, ezequiel@vanguardiasur.com.ar,
        p.zabel@pengutronix.de, gregkh@linuxfoundation.org,
        mripard@kernel.org, paul.kocialkowski@bootlin.com, wens@csie.org,
        hverkuil-cisco@xs4all.nl, jonas@kwiboo.se, nicolas@ndufresne.ca,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>
Cc:     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, Alex Bee <knaerzche@gmail.com>,
        jc@kynesim.co.uk
Subject: Re: Re: [RFC] media: uapi: Move HEVC stateless controls out of staging
Date:   Mon, 14 Feb 2022 20:26:34 +0100
Message-ID: <2302767.NG923GbCHz@kista>
In-Reply-To: <903ca214-9576-33aa-8412-7c71c9d8ac09@collabora.com>
References: <20220201123439.353854-1-benjamin.gaignard@collabora.com> <8038233.T7Z3S40VBb@kista> <903ca214-9576-33aa-8412-7c71c9d8ac09@collabora.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Dne ponedeljek, 14. februar 2022 ob 18:25:01 CET je Benjamin Gaignard=20
napisal(a):
>=20
> Le 13/02/2022 =C3=A0 12:33, Jernej =C5=A0krabec a =C3=A9crit :
> > Hi Benjamin,
> >
> > CC: Alex, John
> >
> > Sorry for late response, but I've been very busy last week.
> >
> > First of all, thank you for doing this! It's about time that HEVC moves
> > forward.
> >
> > Dne torek, 01. februar 2022 ob 13:34:39 CET je Benjamin Gaignard=20
napisal(a):
> >> The HEVC stateless 'uAPI' was staging and marked explicitly in the
> >> V4L2 specification that it will change and is unstable.
> >>
> >> Note that these control IDs were never exported as a public API,
> >> they were only defined in kernel-local headers (hevc-ctrls.h).
> >>
> >> While moving the controls out of staging they are renamed and
> >> control IDs get new numbers.
> >> Drivers (Hantro, Cedrus) and Documentation are updated accordaly.
> > accordaly -> accordingly
> >
> >> Additional structures fields has been added for RKVDEC driver usage.
> > You should do separate patch for that, preceding this one. One patch=20
should
> > only do one thing.
>=20
> I will do that in v2
>=20
> >
> > I also suggest that you add additional patch for removing bit_size fiel=
d in
> > struct v4l2_ctrl_hevc_slice_params. Similar fields were already removed=
=20
from
> > MPEG2 and H264 structures. Bit size can be deduced from output buffer s=
ize=20
and
> > it doesn't hurt if bit size in Cedrus is set to bigger value than actua=
l=20
slice
> > bit size.
>=20
> ok
>=20
> >
> >> Hantro dedicated control is moving to hantro-media.h
> >> Since hevc-ctrls.h content has been dispatched in others file, remove =
it.
> >>
> >> fluster tests results on IMX8MQ is 77/147 for HEVC codec.
> >>
> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > Note that Cedrus still needs additional information in order to decode=
=20
some
> > HEVC videos. Missing info is num_entry_point_offsets and list of all
> > entry_point_offset_minus1 (obviously, num_entry_point_offsets in size).
> >
> > I suggest that this is represented in a new control, which would use=20
dynamic
> > array feature, written by Hans. While Cedrus supports max. 256 entries,=
 it=20
can
> > be much bigger in theory, but in reality, it's much smaller (like 4-8
> > entries).
>=20
> I haven't seen yet any user for these fields but I will create a new cont=
rol=20
like
> #define V4L2_CID_STATELESS_HEVC_ENTRY_POINT	(V4L2_CID_CODEC_STATELESS_BAS=
E +=20
407)
>=20
> struct v4l2_ctrl_hevc_entry_point_offset {
> 	__u32	entry_point_offset_minus1;
> };

Yeah, Cedrus is currently the only mainline driver that needs that in order=
 to=20
fully work. I think John used num_entry_point_offsets in his (out of tree) =
RPi=20
HEVC decoding driver too.

Wouldn't be easier to just use u32 directly? This is just array of numbers,=
 so=20
nothing else will be added in that struct...

Anyway, once you add this, I'll quickly update driver to take advantage of =
it.

>=20
> and add it in the documentation:
> ``V4L2_CID_STATELESS_HEVC_ENTRY_POINT (struct)``
>      Specifies the i-th entry point offset in bytes and is represented by
>      offset_len_minus1 plus 1 bits.
>      This control is a dynamically sized array. The number of entry point
>      offsets is reported by the ``elems`` field.
>      This bitstream parameter is defined according to :ref:`hevc`.
>      They are described in section 7.4.7.1 "General slice segment header
>      semantics" of the specification.
>=20
> >
> > Last but not least, data_bit_offset should be better defined. Currently=
 it
> > points right after last header bit, just like Cedrus needs it. However,=
=20
there
> > is padding after that, at least 1 bit and 8 bits at most, so slice data=
=20
always
> > starts from byte aligned address. It probably make sense to rework that=
=20
field
> > to be byte offset, not bit, just like in VA-API. Note that RPi HEVC dri=
ver=20
also
> > uses byte aligned address directly. Cedrus would need some kind of=20
workaround
> > and only one that works is this one:
> > https://github.com/bootlin/libva-v4l2-request/blob/master/src/h265.c#L1=
91-L209
>=20
> If Cedrus driver is happy with this definition I will keep it like that.
> When providing offset in bit is more accurate and any driver can align th=
e=20
value
> if needed, the reverse (byte -> bit) isn't possible.

If I'm not mistaken, HEVC standard actually requires that slice data starts=
 at=20
byte aligned address, so nothing would be lost for correctness of uAPI.

I already had this discussion with John and IIRC conclusion was to have byt=
e=20
aligned value here. John, can you please confirm if my interpretation is=20
correct?

Best regards,
Jernej

>=20
> Regards,
> Benjamin
>=20
> >
> > Best regards,
> > Jernej
> >
> >
>=20