linux-kernel - Re: Re: [PATCH v2 2/9] media: cedrus: h264: Support profile and level controls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <649607da7517ac88f49ff4332d2a5eba585424f9.camel@collabora.com>
Date:   Tue, 17 Nov 2020 16:55:36 -0300
From:   Ezequiel Garcia <ezequiel@...labora.com>
To:     Jernej Škrabec <jernej.skrabec@...l.net>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     kernel@...labora.com, Jonas Karlman <jonas@...boo.se>,
        Hans Verkuil <hverkuil@...all.nl>,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Maxime Ripard <mripard@...nel.org>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Subject: Re: Re: [PATCH v2 2/9] media: cedrus: h264: Support profile and
 level controls

On Tue, 2020-11-17 at 20:55 +0100, Jernej Škrabec wrote:
> Dne torek, 17. november 2020 ob 20:40:09 CET je Ezequiel Garcia napisal(a):
> > On Tue, 2020-11-17 at 20:24 +0100, Jernej Škrabec wrote:
> > > Hi Ezequiel,
> > > 
> > > sorry for late review.
> > > 
> > > First of all, this patch doesn't break anything. However, see comment 
> below.
> > > Dne petek, 13. november 2020 ob 22:51:14 CET je Ezequiel Garcia 
> napisal(a):
> > > > Cedrus supports H.264 profiles from Baseline to High,
> > > > up to Level 5.1, except for the Extended profile
> > > > 
> > > > Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and
> > > > V4L2_CID_MPEG_VIDEO_H264_LEVEL so that userspace can
> > > > query the driver for the supported profiles and levels.
> > > > 
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@...labora.com>
> > > > ---
> > > >  drivers/staging/media/sunxi/cedrus/cedrus.c | 21 +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > > 
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/
> staging/
> > > media/sunxi/cedrus/cedrus.c
> > > > index 9a102b7c1bb9..8b0e97752d27 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > > > @@ -103,6 +103,27 @@ static const struct cedrus_control 
> cedrus_controls[] = 
> > > {
> > > >  		.codec		= CEDRUS_CODEC_H264,
> > > >  		.required	= false,
> > > >  	},
> > > > +	{
> > > > +		.cfg = {
> > > > +			.id	= 
> > > V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > > > +			.min	= 
> > > V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
> > > > +			.def	= 
> > > V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
> > > > +			.max	= 
> > > V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> > > > +			.menu_skip_mask =
> > > > +				
> > > BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> > > > +		},
> > > > +		.codec		= CEDRUS_CODEC_H264,
> > > > +		.required	= false,
> > > > +	},
> > > > +	{
> > > > +		.cfg = {
> > > > +			.id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
> > > > +			.min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
> > > > +			.max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
> > > 
> > > I went through several datasheets and only newer ones (H6, H616) state 
> max. 
> > > supported level, which is 4.2. Please change it in next revision.
> > > 
> > > After that, you can add
> > > Reviewed-by: Jernej Skrabec <jernej.skrabec@...l.net>
> > > 
> > 
> > Note that I used level 5.1 based on a commit from you:
> > """
> >     media: cedrus: h264: Fix 4K decoding on H6
> >     
> >     Due to unknown reason, H6 needs larger intraprediction buffer for 4K
> >     videos than other SoCs. This was discovered by playing 4096x2304 video,
> >     which is maximum what H6 VPU is supposed to support.
> > """
> > 
> > I guessed this meant it supported level 5 or higher.
> > (Now that I think about it, I meant at least H6, does).
> > 
> > According to https://en.wikipedia.org/wiki/Advanced_Video_Coding#Levels,
> > level 4.2 is up to 2,048×1,080@...0.
> 
> Strange, then I guess datasheet is wrong (wouldn't be first time). 
> Unfortunatelly there is no documentation for Cedrus capabilities, so 
> everything is either educated guess or tested on HW. Documentation for older 
> than H6 SoCs always mention only 1080p @ 60fps limit, even though several of 
> them are capable of decoding 4K H264 videos (I'm not sure about max. fps 
> though).
> 
> > Frankly, I'm open to put whatever value makes you happy.
> 
> To be honest, I'm not sure what is correct value here. It may depend on Cedrus 
> core variant.
> 

We can drop the level information as well, and just keep the profile one.

Maximum resolution is already enforced, via TRY_FMT, anyway.

Thanks,
Ezequiel

> Best regards,
> Jernej
> 
> >   
> > Thanks,
> > Ezequiel
> > 
> > > Best regards,
> > > Jernej
> > > 
> > > > +		},
> > > > +		.codec		= CEDRUS_CODEC_H264,
> > > > +		.required	= false,
> > > > +	},
> > > >  	{
> > > >  		.cfg = {
> > > >  			.id	= V4L2_CID_MPEG_VIDEO_HEVC_SPS,
> > > > -- 
> > > > 2.27.0
> > > > 
> > > > 
> > 
> > 
> 
>