Re: [PATCH v1 00/18] Add HANTRO G2/HEVC decoder support for IMX8MQ

[Hans Verkuil]

On 17/02/2021 09:36, Greg KH wrote:
> On Wed, Feb 17, 2021 at 09:28:09AM +0100, Benjamin Gaignard wrote:
>>
>> Le 17/02/2021 à 09:08, Greg KH a écrit :
>>> On Wed, Feb 17, 2021 at 09:02:48AM +0100, Benjamin Gaignard wrote:
>>>> The IMX8MQ got two VPUs but until now only G1 has been enabled.
>>>> This series aim to add the second VPU (aka G2) and provide basic
>>>> HEVC decoding support.
>>> Why are you adding this directly to drivers/staging/media/ and not
>>> drivers/media/?  Why can't this just go to the main location and not
>>> live in staging?
>>
>> G2/HEVC is added inside the already exiting Hantro driver, it is "just"
>> an other codec from Hantro driver point of view.
>> In addition of that v4l2-hevc uAPI is still unstable.
>> One goal of this series is to have one more consumer of this v4l2-hevc
>> uAPI so maybe we can claim it to be stable enough to move away from staging
>> and then do the same for Hantro driver. That would be a great achievement !
> 
> I know I do not like seeing new additions/features/whatever being added
> to staging drivers as that encourages people to do new stuff on them
> without doing the real work needed to get them out of staging.

In order to support a specific codec (MPEG-2, H.264, HEVC, VP8, etc.) for
stateless codec hardware like the hantro, V4L2 controls need to be defined.
The contents of these controls is derived directly from the underlying codec
standards, but it is quite difficult to get this right with the first attempt,
since these standards are very complex.

So we went for the strategy of keeping these drivers in staging to make it
easy to work on, while keeping the APIs for each codec private (i.e., they are
not exposed in include/uapi/linux).

Once we have sufficient confidence in the API for a specific codec we move
it to uapi and thus fix the API. We also renumber the control IDs at that
time to avoid any confusion between the staging version and the final version.

We did that for H.264 and I hope we can soon do the same for MPEG-2 and VP8.

HEVC is definitely not ready for that yet.

The key phrase is 'sufficient confidence': one requirement is that it is supported
by at least two drivers to be reasonably certain the API doesn't contain any HW
specific stuff, and it passes test suites and review by codec experts.

All this is actively being worked on, so this is very much alive, but it is
complex and time consuming.

> So what is preventing the existing driver from getting out of staging
> now?

Once MPEG-2 and VP8 are finalized it is probably time to move these drivers
out of staging, while still keeping the HEVC API part private.

> 
> And how are you all creating new userspace apis for staging drivers to
> the v4l layer?  What happens when you export something new and then
> userspace starts to rely on it and then you change it?

Nothing is exported. So if userspace want to use it they have to manually
copy headers from include/media to their application.

> 
> Anyway, the media staging drivers are on their own, I don't touch them,
> it just feels odd to me...

It's an unusual situation. But putting the drivers in staging and keeping
the codec API headers private turns out to be the most effective way to
develop this.

Regards,

	Hans

PS: stateful vs stateless decoders: stateful decoders are fully supported
today: you just feed the decoder the compressed stream and the decoded frames
are produced by the firmware/hardware. I.e. the HW takes care of the decoder
state. Stateless decoders require you to pass the compressed frame + decoder
state to the hardware, so they do not keep track of the decoder state, that
needs to be done in software. And that requires structures to be created that
store the state, which luckily can be derived from the codec standards.