Re: [PATCH v7, 04/15] media: mtk-vcodec: Read max resolution from dec_capability

[Nicolas Dufresne]

Le vendredi 17 juin 2022 à 14:46 +0800, Chen-Yu Tsai a écrit :
> Hi,
> 
> On Mon, Feb 28, 2022 at 04:29:15PM -0500, Nicolas Dufresne wrote:
> > Hi Yunfei,
> > 
> > this patch does not work unless userland calls enum_framesizes, which is
> > completely optional. See comment and suggestion below.
> > 
> > Le mercredi 23 février 2022 à 11:39 +0800, Yunfei Dong a écrit :
> > > Supported max resolution for different platforms are not the same: 2K
> > > or 4K, getting it according to dec_capability.
> > > 
> > > Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > > Reviewed-by: Tzung-Bi Shih<tzungbi@xxxxxxxxxx>
> > > ---
> > >  .../platform/mtk-vcodec/mtk_vcodec_dec.c      | 29 +++++++++++--------
> > >  .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  4 +++
> > >  2 files changed, 21 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> > > index 130ecef2e766..304f5afbd419 100644
> > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> > > @@ -445,7 +447,7 @@ static int vidioc_vdec_s_fmt(struct file *file, void *priv,
> > >  		return -EINVAL;
> > >  
> > >  	q_data->fmt = fmt;
> > > -	vidioc_try_fmt(f, q_data->fmt);
> > > +	vidioc_try_fmt(ctx, f, q_data->fmt);
> > >  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > >  		q_data->sizeimage[0] = pix_mp->plane_fmt[0].sizeimage;
> > >  		q_data->coded_width = pix_mp->width;
> > > @@ -545,6 +547,9 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
> > >  				fsize->stepwise.min_height,
> > >  				fsize->stepwise.max_height,
> > >  				fsize->stepwise.step_height);
> > > +
> > > +		ctx->max_width = fsize->stepwise.max_width;
> > > +		ctx->max_height = fsize->stepwise.max_height;
> > 
> > The spec does not require calling enum_fmt, so changing the maximum here is
> > incorrect (and fail with GStreamer). If userland never enum the framesizes, the
> > resolution get limited to 1080p.
> > 
> > As this only depends and the OUTPUT format and the device being open()
> > (condition being dev_capability being set and OUTPUT format being known / not
> > VP8), you could initialize the cxt max inside s_fmt(OUTPUT) instead, which is a
> > mandatory call. I have tested this change to verify this:
> > 
> > 
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> > index 044e3dfbdd8c..3e7c571526a4 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> > @@ -484,6 +484,14 @@ static int vidioc_vdec_s_fmt(struct file *file, void *priv,
> >  	if (fmt == NULL)
> >  		return -EINVAL;
> >  
> > +	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
> > +	    !(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
> > +	    fmt->fourcc != V4L2_PIX_FMT_VP8_FRAME) {
> > +		mtk_v4l2_debug(3, "4K is enabled");
> > +		ctx->max_width = VCODEC_DEC_4K_CODED_WIDTH;
> > +		ctx->max_height = VCODEC_DEC_4K_CODED_HEIGHT;
> > +	}
> > +
> >  	q_data->fmt = fmt;
> >  	vidioc_try_fmt(ctx, f, q_data->fmt);
> >  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > @@ -574,15 +582,9 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
> >  
> >  		fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> >  		fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise;
> > -		if (!(ctx->dev->dec_capability &
> > -				VCODEC_CAPABILITY_4K_DISABLED) &&
> > -				fsize->pixel_format != V4L2_PIX_FMT_VP8_FRAME) {
> > -			mtk_v4l2_debug(3, "4K is enabled");
> > -			fsize->stepwise.max_width =
> > -					VCODEC_DEC_4K_CODED_WIDTH;
> > -			fsize->stepwise.max_height =
> > -					VCODEC_DEC_4K_CODED_HEIGHT;
> > -		}
> > +		fsize->stepwise.max_width = ctx->max_width;
> > +		fsize->stepwise.max_height = ctx->max_height;
> > +
> 
> Recent testing on ChromeOS suggests this doesn't work. The spec implies
> that querying capabilities could happen before the output format is set.
> And also, supported frame sizes are detected for each given format,
> which may not be the one current set.

In v4l2, formats are always set. Perhaps the problem is that we don't
automatically set ctx->max_width/height for the default format when the firmware
is up. I noticed recently the chromium always do G_FMT before S_FMT, so perhaps
it can skip S_FMT if the default format is appropriate, and that endup avoiding
the code I've just suggested. At the time I wrote that, I only had GStreamer
available to test, and it always calls S_FMT, which is mandatory, see 4.5.3.2.
Initialization step 1. But I cannot say userland would be wrong to skip if that
format was "initially" correct.

If my understanding is not correct, then perhaps you should provide a tad more
details on how this failed for you, and we can then better judge an appropriate
fix.

regards,
Nicolas

> 
> So the if block above has to be reintroduced in some form. I'll take a
> look at this.
> 
> 
> Regards
> ChenYu
> 
> >  		mtk_v4l2_debug(1, "%x, %d %d %d %d %d %d",
> >  				ctx->dev->dec_capability,
> >  				fsize->stepwise.min_width,
> > @@ -592,8 +594,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
> >  				fsize->stepwise.max_height,
> >  				fsize->stepwise.step_height);
> >  
> > -		ctx->max_width = fsize->stepwise.max_width;
> > -		ctx->max_height = fsize->stepwise.max_height;
> >  		return 0;
> >  	}
> >  
> > 
> > 
> > >  		return 0;
> > >  	}
> > >  
> 
> [...]
>