DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] media: hantro: Support H264 profile control
@ 2019-11-22  5:16 Hirokazu Honda
  2019-11-22 15:09 ` Nicolas Dufresne
  0 siblings, 1 reply; 8+ messages in thread
From: Hirokazu Honda @ 2019-11-22  5:16 UTC (permalink / raw)
  To: ezequiel, mchehab, gregkh, linux-media, devel, linux-kernel, tfiga
  Cc: Hirokazu Honda

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.

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,
+		}
 	}, {
 	},
 };
-- 
2.24.0.432.g9d3f5f5b63-goog

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] media: hantro: Support H264 profile control
  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
  0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Dufresne @ 2019-11-22 15:09 UTC (permalink / raw)
  To: Hirokazu Honda, ezequiel, mchehab, gregkh, linux-media, devel,
	linux-kernel, tfiga

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.

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] media: hantro: Support H264 profile control
  2019-11-22 15:09 ` Nicolas Dufresne
@ 2019-11-22 16:00   ` Tomasz Figa
  2019-11-22 16:52     ` Nicolas Dufresne
  0 siblings, 1 reply; 8+ messages in thread
From: Tomasz Figa @ 2019-11-22 16:00 UTC (permalink / raw)
  To: Nicolas Dufresne
  Cc: devel, Greg KH, Linux Kernel Mailing List, Hirokazu Honda,
	Mauro Carvalho Chehab, Ezequiel Garcia, Linux Media Mailing List

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.

> >
> > 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,
> > +             }
> >       }, {
> >       },
> >  };
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] media: hantro: Support H264 profile control
  2019-11-22 16:00   ` Tomasz Figa
@ 2019-11-22 16:52     ` Nicolas Dufresne
  2019-11-29  0:16       ` Tomasz Figa
  0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Dufresne @ 2019-11-22 16:52 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: devel, Greg KH, Linux Kernel Mailing List, Hirokazu Honda,
	Mauro Carvalho Chehab, Ezequiel Garcia, Linux Media Mailing List

[-- Attachment #1.1: Type: text/plain, Size: 3488 bytes --]

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.

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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] media: hantro: Support H264 profile control
  2019-11-22 16:52     ` Nicolas Dufresne
@ 2019-11-29  0:16       ` Tomasz Figa
  2020-01-10 12:31         ` Hans Verkuil
  0 siblings, 1 reply; 8+ messages in thread
From: Tomasz Figa @ 2019-11-29  0:16 UTC (permalink / raw)
  To: Nicolas Dufresne
  Cc: devel, Greg KH, Linux Kernel Mailing List, Hirokazu Honda,
	Mauro Carvalho Chehab, Ezequiel Garcia, Linux Media Mailing List

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.

> >
> > > > 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,
> > > > +             }
> > > >       }, {
> > > >       },
> > > >  };
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] media: hantro: Support H264 profile control
  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
  0 siblings, 2 replies; 8+ messages in thread
From: Hans Verkuil @ 2020-01-10 12:31 UTC (permalink / raw)
  To: Tomasz Figa, Nicolas Dufresne
  Cc: devel, Greg KH, Linux Kernel Mailing List, Hirokazu Honda,
	Mauro Carvalho Chehab, Ezequiel Garcia, Linux Media Mailing List

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] media: hantro: Support H264 profile control
  2020-01-10 12:31         ` Hans Verkuil
@ 2020-01-11  4:59           ` Ezequiel Garcia
  2020-01-18 13:31           ` Nicolas Dufresne
  1 sibling, 0 replies; 8+ messages in thread
From: Ezequiel Garcia @ 2020-01-11  4:59 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Nicolas Dufresne, Hirokazu Honda
  Cc: devel, Greg KH, Mauro Carvalho Chehab, Linux Kernel Mailing List,
	Linux Media Mailing List

Hello Hirokazu, Hans,

On Fri, 2020-01-10 at 13:31 +0100, Hans Verkuil wrote:
> 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 fine with this.

[..]
> > > > > > 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,

I'd like to see the .def here ^^^ for consistency
with the other controls.

But I know this is my OCD speaking, so it's fine
as-is as well.

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

> > > > > > +                     .max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> > > > > > +                     .menu_skip_mask =
> > > > > > +                     BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> > > > > > +                     .def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,

Thanks,
Eze


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] media: hantro: Support H264 profile control
  2020-01-10 12:31         ` Hans Verkuil
  2020-01-11  4:59           ` Ezequiel Garcia
@ 2020-01-18 13:31           ` Nicolas Dufresne
  1 sibling, 0 replies; 8+ messages in thread
From: Nicolas Dufresne @ 2020-01-18 13:31 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa
  Cc: devel, Greg KH, Linux Kernel Mailing List, Hirokazu Honda,
	Mauro Carvalho Chehab, Ezequiel Garcia, Linux Media Mailing List

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.

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

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org
	public-inbox-index driverdev-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git