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;
MIME-Version: 1.0
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>
 <c88b2822-0dd2-8ea3-eb0b-262df5a30830@xs4all.nl> <8b10414a1c198a6e3bd5e131a72bd6f68466bea5.camel@ndufresne.ca>
 <CAAFQd5AZ1DTtz2myuO9YSwjMaxPme0BDNaOBo97tSHLbm4XWPg@mail.gmail.com> <8a6a371d-17cd-eb32-779c-0669da5a8d5e@xs4all.nl>
In-Reply-To: <8a6a371d-17cd-eb32-779c-0669da5a8d5e@xs4all.nl>
From:   Tomasz Figa <tfiga@chromium.org>
Date:   Mon, 3 Feb 2020 22:48:47 +0900
Message-ID: <CAAFQd5CXGqv5W+uZurGEv+iwz4=R7crLwqUAe2d5_gXt1e7XzQ@mail.gmail.com>
Subject: Re: [PATCH] media: hantro: Support H264 profile control
To:     Hans Verkuil <hverkuil@xs4all.nl>
Cc:     Nicolas Dufresne <nicolas@ndufresne.ca>,
        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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 3, 2020 at 9:00 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> 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 =C3=A0 13:31 +0100, Hans Verkuil a =C3=A9c=
rit :
> >>> 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 =C3=A0 01:00 +0900, Tomasz Figa a =C3=A9=
crit :
> >>>>>> On Sat, Nov 23, 2019 at 12:09 AM Nicolas Dufresne <nicolas@ndufres=
ne.ca>
> >>>>>> wrote:
> >>>>>>> Le vendredi 22 novembre 2019 =C3=A0 14:16 +0900, Hirokazu Honda a=
 =C3=A9crit :
> >>>>>>>> The Hantro G1 decoder supports H.264 profiles from Baseline to H=
igh,
> >>>>>>>> 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 su=
re
> >>>>>>> that codecs do implement these controls (both stateful and statel=
ess),
> >>>>>>> it's essential for stack with software fallback, or multiple capa=
ble
> >>>>>>> codec hardware but with different capabilities.
> >>>>>>>
> >>>>>>
> >>>>>> Level is a difficult story, because it also specifies the number o=
f
> >>>>>> 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 constrai=
nts.
> >>>>>> Performance related things depend on the integration details and
> >>>>>> should be managed elsewhere. For example Android and Chrome OS man=
age
> >>>>>> expected decoding performance in per-board configuration files.
> >>>>>
> >>>>> When you read datasheet, the HW is always rated to maximum level (a=
nd
> >>>>> 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 ma=
th
> >>>>> with data that isn't fully exposed to the user. This is about filte=
ring
> >>>>> 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 cap=
able
> >>>>> 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 enab=
le
> >>>> 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 supporte=
d
> >>>> 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 poten=
tial
> >>> 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 u=
sually
> >> 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.

Okay, thanks!

>
> 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[] =
=3D {
> >>>>>>>>                       .def =3D
> >>>>>>>> V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
> >>>>>>>>                       .max =3D
> >>>>>>>> V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
> >>>>>>>>               },
> >>>>>>>> +     }, {
> >>>>>>>> +             .codec =3D HANTRO_H264_DECODER,
> >>>>>>>> +             .cfg =3D {
> >>>>>>>> +                     .id =3D V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> >>>>>>>> +                     .min =3D V4L2_MPEG_VIDEO_H264_PROFILE_BASE=
LINE,
> >>>>>>>> +                     .max =3D V4L2_MPEG_VIDEO_H264_PROFILE_HIGH=
,
> >>>>>>>> +                     .menu_skip_mask =3D
> >>>>>>>> +                     BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED)=
,
> >>>>>>>> +                     .def =3D V4L2_MPEG_VIDEO_H264_PROFILE_MAIN=
,
> >>>>>>>> +             }
> >>>>>>>>       }, {
> >>>>>>>>       },
> >>>>>>>>  };
> >>
>