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:   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,
        Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        Alex Bee <knaerzche@gmail.com>, jc@kynesim.co.uk
Subject: Re: [RFC] media: uapi: Move HEVC stateless controls out of staging
Date:   Sun, 13 Feb 2022 12:33:53 +0100
Message-ID: <8038233.T7Z3S40VBb@kista>
In-Reply-To: <20220201123439.353854-1-benjamin.gaignard@collabora.com>
References: <20220201123439.353854-1-benjamin.gaignard@collabora.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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 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.

> 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).

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

Best regards,
Jernej