Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] media: hantro: Support H264 profile control
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>
References: <20191122051608.128717-1-hiroh@chromium.org>
 <767528be59275265072896e5c679e97575615fdd.camel@ndufresne.ca>
 <CAAFQd5D3OpAAtX7_0ktz4-aAgWN_G4YBQMR=Vwp7JPopjvRkRA@mail.gmail.com>
 <f5341ed837529bd38d466d4b655e261d64065912.camel@ndufresne.ca>
 <CAAFQd5Cpk8qG+VgE6+aznBmXu11YG0gNQyCRanZghds-TPKvyg@mail.gmail.com>
From:   Hans Verkuil <hverkuil@xs4all.nl>
Message-ID: <c88b2822-0dd2-8ea3-eb0b-262df5a30830@xs4all.nl>
Date:   Fri, 10 Jan 2020 13:31:08 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <CAAFQd5Cpk8qG+VgE6+aznBmXu11YG0gNQyCRanZghds-TPKvyg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?

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,
>>>>> +             }
>>>>>       }, {
>>>>>       },
>>>>>  };