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

[Ezequiel Garcia <ezequiel@collabora.com>]

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@collabora.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@siol.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@60.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
> > > > 
> > > > 
> > 
> > 
> 
>