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

["yunfei.dong@mediatek.com" <yunfei.dong@mediatek.com>]

Hi Nicolas,

Thanks for your comments, I will fix this patch according your
suggestion.

On Mon, 2022-02-28 at 16:29 -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@mediatek.com>
> > Reviewed-by: Tzung-Bi Shih<tzungbi@google.com>
> > ---
> >  .../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
> > @@ -152,13 +152,15 @@ void mtk_vcodec_dec_set_default_params(struct
> > mtk_vcodec_ctx *ctx)
> >  	q_data->coded_height = DFT_CFG_HEIGHT;
> >  	q_data->fmt = ctx->dev->vdec_pdata->default_cap_fmt;
> >  	q_data->field = V4L2_FIELD_NONE;
> > +	ctx->max_width = MTK_VDEC_MAX_W;
> > +	ctx->max_height = MTK_VDEC_MAX_H;
> >  
> >  	v4l_bound_align_image(&q_data->coded_width,
> >  				MTK_VDEC_MIN_W,
> > -				MTK_VDEC_MAX_W, 4,
> > +				ctx->max_width, 4,
> >  				&q_data->coded_height,
> >  				MTK_VDEC_MIN_H,
> > -				MTK_VDEC_MAX_H, 5, 6);
> > +				ctx->max_height, 5, 6);
> >  
> >  	q_data->sizeimage[0] = q_data->coded_width * q_data-
> > >coded_height;
> >  	q_data->bytesperline[0] = q_data->coded_width;
> > @@ -217,7 +219,7 @@ static int vidioc_vdec_subscribe_evt(struct
> > v4l2_fh *fh,
> >  	}
> >  }
> >  
> > -static int vidioc_try_fmt(struct v4l2_format *f,
> > +static int vidioc_try_fmt(struct mtk_vcodec_ctx *ctx, struct
> > v4l2_format *f,
> >  			  const struct mtk_video_fmt *fmt)
> >  {
> >  	struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp;
> > @@ -225,9 +227,9 @@ static int vidioc_try_fmt(struct v4l2_format
> > *f,
> >  	pix_fmt_mp->field = V4L2_FIELD_NONE;
> >  
> >  	pix_fmt_mp->width =
> > -		clamp(pix_fmt_mp->width, MTK_VDEC_MIN_W,
> > MTK_VDEC_MAX_W);
> > +		clamp(pix_fmt_mp->width, MTK_VDEC_MIN_W, ctx-
> > >max_width);
> >  	pix_fmt_mp->height =
> > -		clamp(pix_fmt_mp->height, MTK_VDEC_MIN_H,
> > MTK_VDEC_MAX_H);
> > +		clamp(pix_fmt_mp->height, MTK_VDEC_MIN_H, ctx-
> > >max_height);
> >  
> >  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> >  		pix_fmt_mp->num_planes = 1;
> > @@ -245,16 +247,16 @@ static int vidioc_try_fmt(struct v4l2_format
> > *f,
> >  		tmp_h = pix_fmt_mp->height;
> >  		v4l_bound_align_image(&pix_fmt_mp->width,
> >  					MTK_VDEC_MIN_W,
> > -					MTK_VDEC_MAX_W, 6,
> > +					ctx->max_width, 6,
> >  					&pix_fmt_mp->height,
> >  					MTK_VDEC_MIN_H,
> > -					MTK_VDEC_MAX_H, 6, 9);
> > +					ctx->max_height, 6, 9);
> >  
> >  		if (pix_fmt_mp->width < tmp_w &&
> > -			(pix_fmt_mp->width + 64) <= MTK_VDEC_MAX_W)
> > +			(pix_fmt_mp->width + 64) <= ctx->max_width)
> >  			pix_fmt_mp->width += 64;
> >  		if (pix_fmt_mp->height < tmp_h &&
> > -			(pix_fmt_mp->height + 64) <= MTK_VDEC_MAX_H)
> > +			(pix_fmt_mp->height + 64) <= ctx->max_height)
> >  			pix_fmt_mp->height += 64;
> >  
> >  		mtk_v4l2_debug(0,
> > @@ -294,7 +296,7 @@ static int vidioc_try_fmt_vid_cap_mplane(struct
> > file *file, void *priv,
> >  		fmt = mtk_vdec_find_format(f, dec_pdata);
> >  	}
> >  
> > -	return vidioc_try_fmt(f, fmt);
> > +	return vidioc_try_fmt(ctx, f, fmt);
> >  }
> >  
> >  static int vidioc_try_fmt_vid_out_mplane(struct file *file, void
> > *priv,
> > @@ -317,7 +319,7 @@ static int vidioc_try_fmt_vid_out_mplane(struct
> > file *file, void *priv,
> >  		return -EINVAL;
> >  	}
> >  
> > -	return vidioc_try_fmt(f, fmt);
> > +	return vidioc_try_fmt(ctx, f, fmt);
> >  }
> >  
> >  static int vidioc_vdec_g_selection(struct file *file, void *priv,
> > @@ -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:
> 
I will fix it in your suggestion, thanks.

Best Regards,
Yunfei Dong
> 
> 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;
> +
>  		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;
> >  	}
> >  
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > index bb7b8e914d24..6d27e4d41ede 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > @@ -284,6 +284,8 @@ struct vdec_pic_info {
> >   *	  mtk_video_dec_buf.
> >   * @hw_id: hardware index used to identify different hardware.
> >   *
> > + * @max_width: hardware supported max width
> > + * @max_height: hardware supported max height
> >   * @msg_queue: msg queue used to store lat buffer information.
> >   */
> >  struct mtk_vcodec_ctx {
> > @@ -329,6 +331,8 @@ struct mtk_vcodec_ctx {
> >  	struct mutex lock;
> >  	int hw_id;
> >  
> > +	unsigned int max_width;
> > +	unsigned int max_height;
> >  	struct vdec_msg_queue msg_queue;
> >  };
> >  
> 
>