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;
Message-ID: <903ca214-9576-33aa-8412-7c71c9d8ac09@collabora.com>
Date:   Mon, 14 Feb 2022 18:25:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [RFC] media: uapi: Move HEVC stateless controls out of staging
Content-Language: en-US
To:     =?UTF-8?Q?Jernej_=c5=a0krabec?= <jernej.skrabec@gmail.com>,
        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
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
References: <20220201123439.353854-1-benjamin.gaignard@collabora.com>
 <8038233.T7Z3S40VBb@kista>
From:   Benjamin Gaignard <benjamin.gaignard@collabora.com>
In-Reply-To: <8038233.T7Z3S40VBb@kista>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


Le 13/02/2022 à 12:33, Jernej Škrabec a écrit :
> 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 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 should
> only do one thing.

I will do that in v2

>
> I also suggest that you add additional patch for removing bit_size field in
> struct v4l2_ctrl_hevc_slice_params. Similar fields were already removed from
> MPEG2 and H264 structures. Bit size can be deduced from output buffer size and
> it doesn't hurt if bit size in Cedrus is set to bigger value than actual slice
> bit size.

ok

>
>> 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 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 dynamic
> array feature, written by Hans. While Cedrus supports max. 256 entries, it can
> be much bigger in theory, but in reality, it's much smaller (like 4-8
> entries).

I haven't seen yet any user for these fields but I will create a new control like
#define V4L2_CID_STATELESS_HEVC_ENTRY_POINT	(V4L2_CID_CODEC_STATELESS_BASE + 407)

struct v4l2_ctrl_hevc_entry_point_offset {
	__u32	entry_point_offset_minus1;
};

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.

>
> 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, there
> is padding after that, at least 1 bit and 8 bits at most, so slice data always
> starts from byte aligned address. It probably make sense to rework that field
> to be byte offset, not bit, just like in VA-API. Note that RPi HEVC driver also
> uses byte aligned address directly. Cedrus would need some kind of workaround
> and only one that works is this one:
> https://github.com/bootlin/libva-v4l2-request/blob/master/src/h265.c#L191-L209

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 the value
if needed, the reverse (byte -> bit) isn't possible.

Regards,
Benjamin

>
> Best regards,
> Jernej
>
>