From: Hans Verkuil <hverkuil@xs4all.nl>
To: Tomasz Figa <tfiga@chromium.org>,
Nicolas Dufresne <nicolas@ndufresne.ca>
Cc: Hirokazu Honda <hiroh@chromium.org>,
Ezequiel Garcia <ezequiel@collabora.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Greg KH <gregkh@linuxfoundation.org>,
Linux Media Mailing List <linux-media@vger.kernel.org>,
devel@driverdev.osuosl.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] media: hantro: Support H264 profile control
Date: Mon, 3 Feb 2020 12:59:54 +0100 [thread overview]
Message-ID: <8a6a371d-17cd-eb32-779c-0669da5a8d5e@xs4all.nl> (raw)
In-Reply-To: <CAAFQd5AZ1DTtz2myuO9YSwjMaxPme0BDNaOBo97tSHLbm4XWPg@mail.gmail.com>
On 2/3/20 12:13 PM, Tomasz Figa wrote:
> On Sat, Jan 18, 2020 at 10:31 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>>
>> Le vendredi 10 janvier 2020 à 13:31 +0100, Hans Verkuil a écrit :
>>> On 11/29/19 1:16 AM, Tomasz Figa wrote:
>>>> On Sat, Nov 23, 2019 at 1:52 AM Nicolas Dufresne <nicolas@ndufresne.ca>
>>>> wrote:
>>>>> Le samedi 23 novembre 2019 à 01:00 +0900, Tomasz Figa a écrit :
>>>>>> On Sat, Nov 23, 2019 at 12:09 AM Nicolas Dufresne <nicolas@ndufresne.ca>
>>>>>> wrote:
>>>>>>> Le vendredi 22 novembre 2019 à 14:16 +0900, Hirokazu Honda a écrit :
>>>>>>>> The Hantro G1 decoder supports H.264 profiles from Baseline to High,
>>>>>>>> with
>>>>>>>> the exception of the Extended profile.
>>>>>>>>
>>>>>>>> Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE control, so that the
>>>>>>>> applications can query the driver for the list of supported
>>>>>>>> profiles.
>>>>>>>
>>>>>>> Thanks for this patch. Do you think we could also add the LEVEL
>>>>>>> control
>>>>>>> so the profile/level enumeration becomes complete ?
>>>>>>>
>>>>>>> I'm thinking it would be nice if the v4l2 compliance test make sure
>>>>>>> that codecs do implement these controls (both stateful and stateless),
>>>>>>> it's essential for stack with software fallback, or multiple capable
>>>>>>> codec hardware but with different capabilities.
>>>>>>>
>>>>>>
>>>>>> Level is a difficult story, because it also specifies the number of
>>>>>> macroblocks per second, but for decoders like this the number of
>>>>>> macroblocks per second it can handle depends on things the driver
>>>>>> might be not aware of - clock frequencies, DDR throughput, system
>>>>>> load, etc.
>>>>>>
>>>>>> My take on this is that the decoder driver should advertise the
>>>>>> highest resolution the decoder can handle due to hardware constraints.
>>>>>> Performance related things depend on the integration details and
>>>>>> should be managed elsewhere. For example Android and Chrome OS manage
>>>>>> expected decoding performance in per-board configuration files.
>>>>>
>>>>> When you read datasheet, the HW is always rated to maximum level (and
>>>>> it's a range) with the assumption of a single stream. It seems much
>>>>> easier to expose this as-is, statically then to start doing some math
>>>>> with data that isn't fully exposed to the user. This is about filtering
>>>>> of multiple CODEC instances, it does not need to be rocket science,
>>>>> specially that the amount of missing data is important (e.g. usage of
>>>>> tiles, compression, IPP all have an impact on the final performance).
>>>>> All we want to know in userspace is if this HW is even possibly capable
>>>>> of LEVEL X, and if not, we'll look for another one.
>>>>>
>>>>
>>>> Agreed, one could potentially define it this way, but would it be
>>>> really useful for the userspace and the users? I guess it could enable
>>>> slightly faster fallback to software decoding in the extreme case of
>>>> the hardware not supporting the level at all, but I suspect that the
>>>> majority of cases would be the hardware just being unusably slow.
>>>>
>>>> Also, as I mentioned before, we already return the range of supported
>>>> resolutions, which in practice should map to the part of the level
>>>> that may depend on hardware capabilities rather than performance, so
>>>> exposing levels as well would add redundancy to the information
>>>> exposed.
>>>
>>> There is a lot of discussion here, but it all revolves around a potential
>>> LEVEL control.
>>>
>>> So I gather everyone is OK with merging this patch?
>>
>> I'm ok with this. For me, the level reflects the real time performance
>> capability as define in the spec, while the width/height constraints usually
>> represent an addressing capabicity, which may or may not operate real-time.
>>
>
> I'd like to see the level control documentation improved before we
> start adding it to the drivers. I'll be okay with that then as long as
> the values are exposed in a consistent and well defined way. :)
>
> As for this patch, Hans, are you going to apply it?
It's in a PR for 5.7. I had hoped it would go in for v5.6, but it was
too late for that.
Regards,
Hans
>
> Best regards,
> Tomasz
>
>>>
>>> If not, let me know asap.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>>>>>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
>>>>>>>> ---
>>>>>>>> drivers/staging/media/hantro/hantro_drv.c | 10 ++++++++++
>>>>>>>> 1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/staging/media/hantro/hantro_drv.c
>>>>>>>> b/drivers/staging/media/hantro/hantro_drv.c
>>>>>>>> index 6d9d41170832..9387619235d8 100644
>>>>>>>> --- a/drivers/staging/media/hantro/hantro_drv.c
>>>>>>>> +++ b/drivers/staging/media/hantro/hantro_drv.c
>>>>>>>> @@ -355,6 +355,16 @@ static const struct hantro_ctrl controls[] = {
>>>>>>>> .def =
>>>>>>>> V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>>>>>>>> .max =
>>>>>>>> V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>>>>>>>> },
>>>>>>>> + }, {
>>>>>>>> + .codec = HANTRO_H264_DECODER,
>>>>>>>> + .cfg = {
>>>>>>>> + .id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
>>>>>>>> + .min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
>>>>>>>> + .max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
>>>>>>>> + .menu_skip_mask =
>>>>>>>> + BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
>>>>>>>> + .def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
>>>>>>>> + }
>>>>>>>> }, {
>>>>>>>> },
>>>>>>>> };
>>
next prev parent reply other threads:[~2020-02-03 12:00 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-22 5:16 [PATCH] media: hantro: Support H264 profile control Hirokazu Honda
2019-11-22 15:09 ` Nicolas Dufresne
2019-11-22 16:00 ` Tomasz Figa
2019-11-22 16:52 ` Nicolas Dufresne
2019-11-29 0:16 ` Tomasz Figa
2020-01-10 12:31 ` Hans Verkuil
2020-01-11 4:59 ` Ezequiel Garcia
2020-01-18 13:31 ` Nicolas Dufresne
2020-02-03 11:13 ` Tomasz Figa
2020-02-03 11:59 ` Hans Verkuil [this message]
2020-02-03 13:48 ` Tomasz Figa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8a6a371d-17cd-eb32-779c-0669da5a8d5e@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=devel@driverdev.osuosl.org \
--cc=ezequiel@collabora.com \
--cc=gregkh@linuxfoundation.org \
--cc=hiroh@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=nicolas@ndufresne.ca \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).