[PATCH 0/4] media: mediatek: vcodec: Fix 4K decoding support

[PATCH 0/4] media: mediatek: vcodec: Fix 4K decoding support

[Chen-Yu Tsai]

While testing a backport of recent mtk-vcodec developments on ChromeOS
v5.10 kernel [1], it was found that 4K decoding support had regressed.
The decoder was not correctly reporting 4K frame sizes when queried,
and ChromeOS then determined that the hardware did not support it.

This turned out to be a mix of different bugs:

1. Frame size enumeration on the output side should not depend on the
   currently set format, or any other derived state. This is fixed in
   patch 1.

2. The default resolution limit was not set according to the default
   output format determined at runtime, but hard-coded to 1080p. An
   S_FMT call is needed to override this. This is fixed in patch 2.

3. TRY_FMT on the output side was incorrectly clamping the resolution
   based on the current maximum values. It should not. Fixed in patch
   3.

The last patch fixes an odd error in the bug, where the maximum
resolution restriction could be lifted to 4K, even if the output format
doesn't allow it. In practice this wouldn't cause any issues given the
other fixes in this series and other existing checks in both the driver
and V4L2 core, but it seemed easy to fix.

This series is based on next-20220627 with media-staging at

     d8e8aa866ed8 ("media: mediatek: vcodec: Report supported bitrate modes")

merged in.

This was only tested on the backport kernel [1] on MT8195, which is the
only currently supported SoC that does 4K decoding. Hopefully the folks
at Collabora can give this a test on their mainline MT8195 integration
branch.


Regards
ChenYu

[1] https://crrev.com/c/3713491

Chen-Yu Tsai (4):
  media: mediatek: vcodec: dec: Fix 4K frame size enumeration
  media: mediatek: vcodec: dec: Set default max resolution based on
    format
  media: mediatek: vcodec: dec: Fix resolution clamping in TRY_FMT
  media: mediatek: vcodec: dec: Set maximum resolution when S_FMT on
    output only

 .../platform/mediatek/vcodec/mtk_vcodec_dec.c | 45 ++++++++++++++-----
 .../vcodec/mtk_vcodec_dec_stateless.c         |  7 +++
 2 files changed, 41 insertions(+), 11 deletions(-)

-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH 1/4] media: mediatek: vcodec: dec: Fix 4K frame size enumeration

[Chen-Yu Tsai]

This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec:
Read max resolution from dec_capability"). In this commit, the maximum
resolution ended up being a function of both the firmware capability and
the current set format.

However, frame size enumeration for output (coded) formats should not
depend on the format set, but should return supported resolutions for
the format requested by userspace.

Fix this so that the driver returns the supported resolutions correctly,
even if the instance only has default settings, or if the output format
is currently set to VP8F, which does not support 4K.

Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c    | 2 --
 .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c    | 7 +++++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
index 5d6fdf18c3a6..fcb4b8131c49 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
@@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
 		fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
 		fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise;
 
-		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,
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
index 16d55785d84b..9a4d3e3658aa 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
@@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc,
 
 		mtk_vdec_framesizes[count_framesizes].fourcc = fourcc;
 		mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd;
+		if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
+		    fourcc != V4L2_PIX_FMT_VP8_FRAME) {
+			mtk_vdec_framesizes[count_framesizes].stepwise.max_width =
+				VCODEC_DEC_4K_CODED_WIDTH;
+			mtk_vdec_framesizes[count_framesizes].stepwise.max_height =
+				VCODEC_DEC_4K_CODED_HEIGHT;
+		}
 		num_framesizes++;
 		break;
 	case V4L2_PIX_FMT_MM21:
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH 2/4] media: mediatek: vcodec: dec: Set default max resolution based on format

[Chen-Yu Tsai]

In commit b018be06f3c7 ("media: mediatek: vcodec: Read max resolution
from dec_capability") the driver ended up blindly setting the maximum
resolution to 1080p, even if the hardware and default output format
supported up to 4K. This would only be overridden by a subsequent
S_FMT call.

Correctly initialize the maximum resolution based on the default output
format.

Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 .../media/platform/mediatek/vcodec/mtk_vcodec_dec.c  | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
index fcb4b8131c49..f1b82d4c5be5 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
@@ -169,6 +169,16 @@ void mtk_vcodec_dec_set_default_params(struct mtk_vcodec_ctx *ctx)
 	q_data->sizeimage[0] = DFT_CFG_WIDTH * DFT_CFG_HEIGHT;
 	q_data->bytesperline[0] = 0;
 
+	if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
+	    q_data->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;
+	} else {
+		ctx->max_width = MTK_VDEC_MAX_W;
+		ctx->max_height = MTK_VDEC_MAX_H;
+	}
+
 	q_data = &ctx->q_data[MTK_Q_DATA_DST];
 	memset(q_data, 0, sizeof(struct mtk_q_data));
 	q_data->visible_width = DFT_CFG_WIDTH;
@@ -177,8 +187,6 @@ 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,
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH 3/4] media: mediatek: vcodec: dec: Fix resolution clamping in TRY_FMT

[Chen-Yu Tsai]

In commit b018be06f3c7 ("media: mediatek: vcodec: Read max resolution
from dec_capability"), TRY_FMT clamps the resolution to the maximum
that was previously set either by default 1080p or the limit set by a
previous S_FMT call. This does not make sense when doing TRY_FMT for
the output side, which may have different capabilities.

Instead, for the output side, find the maximum resolution based on the
pixel format requested. For the capture side, continue to use the
maximum resolution set by default or by a previous S_FMT call.

The maximum resolution is found from the list of per-format frame
sizes, so the patch "media: mediatek: vcodec: dec: Fix 4K frame size
enumeration" is needed.

Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 .../platform/mediatek/vcodec/mtk_vcodec_dec.c | 28 +++++++++++++++----
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
index f1b82d4c5be5..ea951cb3b927 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
@@ -282,13 +282,29 @@ 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;
+	const struct mtk_vcodec_dec_pdata *dec_pdata = ctx->dev->vdec_pdata;
+	unsigned int max_width, max_height;
+	int i;
+
+	if (V4L2_TYPE_IS_OUTPUT(f->type)) {
+		max_width = MTK_VDEC_MAX_W;
+		max_height = MTK_VDEC_MAX_H;
+		for (i = 0; i < *dec_pdata->num_framesizes; ++i)
+			if (f->fmt.pix_mp.pixelformat == dec_pdata->vdec_framesizes[i].fourcc) {
+				max_width = dec_pdata->vdec_framesizes[i].stepwise.max_width;
+				max_height = dec_pdata->vdec_framesizes[i].stepwise.max_height;
+			}
+	} else {
+		max_width = ctx->max_width;
+		max_height = ctx->max_height;
+	}
 
 	pix_fmt_mp->field = V4L2_FIELD_NONE;
 
 	pix_fmt_mp->width =
-		clamp(pix_fmt_mp->width, MTK_VDEC_MIN_W, ctx->max_width);
+		clamp(pix_fmt_mp->width, MTK_VDEC_MIN_W, max_width);
 	pix_fmt_mp->height =
-		clamp(pix_fmt_mp->height, MTK_VDEC_MIN_H, ctx->max_height);
+		clamp(pix_fmt_mp->height, MTK_VDEC_MIN_H, max_height);
 
 	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		pix_fmt_mp->num_planes = 1;
@@ -306,16 +322,16 @@ static int vidioc_try_fmt(struct mtk_vcodec_ctx *ctx, struct v4l2_format *f,
 		tmp_h = pix_fmt_mp->height;
 		v4l_bound_align_image(&pix_fmt_mp->width,
 					MTK_VDEC_MIN_W,
-					ctx->max_width, 6,
+					max_width, 6,
 					&pix_fmt_mp->height,
 					MTK_VDEC_MIN_H,
-					ctx->max_height, 6, 9);
+					max_height, 6, 9);
 
 		if (pix_fmt_mp->width < tmp_w &&
-			(pix_fmt_mp->width + 64) <= ctx->max_width)
+			(pix_fmt_mp->width + 64) <= max_width)
 			pix_fmt_mp->width += 64;
 		if (pix_fmt_mp->height < tmp_h &&
-			(pix_fmt_mp->height + 64) <= ctx->max_height)
+			(pix_fmt_mp->height + 64) <= max_height)
 			pix_fmt_mp->height += 64;
 
 		mtk_v4l2_debug(0,
-- 
2.37.0.rc0.161.g10f37bed90-goog

[PATCH 4/4] media: mediatek: vcodec: dec: Set maximum resolution when S_FMT on output only

[Chen-Yu Tsai]

Only the coded format impacts whether 4K resolution is supported: the
decoder does not support 4K for VP8, but does for other formats.

Update the maximum resolution in S_FMT only when called for the output
format. Otherwise we could set output format for VP8, then set capture
format, and then the resolution limit becomes inconsistent.

Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
index ea951cb3b927..995c61d6a40e 100644
--- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
@@ -521,7 +521,8 @@ static int vidioc_vdec_s_fmt(struct file *file, void *priv,
 	if (fmt == NULL)
 		return -EINVAL;
 
-	if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
+	if (V4L2_TYPE_IS_OUTPUT(f->type) &&
+	    !(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;
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH 1/4] media: mediatek: vcodec: dec: Fix 4K frame size enumeration

[Nicolas Dufresne]

Hi Chen-Yu,

Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit :
> This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec:
> Read max resolution from dec_capability"). In this commit, the maximum
> resolution ended up being a function of both the firmware capability and
> the current set format.
> 
> However, frame size enumeration for output (coded) formats should not
> depend on the format set, but should return supported resolutions for
> the format requested by userspace.

Good point. Though, I don't see any special casing for the CAPTURE case. As this
HW does not include a scaler, it must only return 1 resolution when being
enumerated for CAPTURE side (or not implement that enumeration, but its
complicated to half implement something in m2m). The return unique size should
match what G_FMT(CAPTURE) would return.

> 
> Fix this so that the driver returns the supported resolutions correctly,
> even if the instance only has default settings, or if the output format
> is currently set to VP8F, which does not support 4K.
> 
> Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c    | 2 --
>  .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c    | 7 +++++++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> index 5d6fdf18c3a6..fcb4b8131c49 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
>  		fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
>  		fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise;
>  
> -		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,
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> index 16d55785d84b..9a4d3e3658aa 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc,
>  
>  		mtk_vdec_framesizes[count_framesizes].fourcc = fourcc;
>  		mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd;
> +		if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
> +		    fourcc != V4L2_PIX_FMT_VP8_FRAME) {
> +			mtk_vdec_framesizes[count_framesizes].stepwise.max_width =
> +				VCODEC_DEC_4K_CODED_WIDTH;
> +			mtk_vdec_framesizes[count_framesizes].stepwise.max_height =
> +				VCODEC_DEC_4K_CODED_HEIGHT;
> +		}

I don't particularly like to see this special cased check being added into
multiple places. Its also in your patch 2, and I think it exist in a third
place. Could it be possible to have an internal helper to ensure we don't
duplicate this logic ? Somehow, it seems there is something in common between
set_default, try_fmt and this code.

>  		num_framesizes++;
>  		break;
>  	case V4L2_PIX_FMT_MM21:

Re: [PATCH 1/4] media: mediatek: vcodec: dec: Fix 4K frame size enumeration

[Nicolas Dufresne]

Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit :
> This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec:
> Read max resolution from dec_capability"). In this commit, the maximum
> resolution ended up being a function of both the firmware capability and
> the current set format.
> 
> However, frame size enumeration for output (coded) formats should not
> depend on the format set, but should return supported resolutions for
> the format requested by userspace.
> 
> Fix this so that the driver returns the supported resolutions correctly,
> even if the instance only has default settings, or if the output format
> is currently set to VP8F, which does not support 4K.
> 
> Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c    | 2 --
>  .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c    | 7 +++++++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> index 5d6fdf18c3a6..fcb4b8131c49 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
>  		fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
>  		fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise;
>  
> -		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,
> diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> index 16d55785d84b..9a4d3e3658aa 100644
> --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc,
>  
>  		mtk_vdec_framesizes[count_framesizes].fourcc = fourcc;
>  		mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd;

While at it, can you constify stepwise_fhd, it is scary to think that someone
could modify it and cause big headache.

> +		if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
> +		    fourcc != V4L2_PIX_FMT_VP8_FRAME) {
> +			mtk_vdec_framesizes[count_framesizes].stepwise.max_width =
> +				VCODEC_DEC_4K_CODED_WIDTH;
> +			mtk_vdec_framesizes[count_framesizes].stepwise.max_height =
> +				VCODEC_DEC_4K_CODED_HEIGHT;
> +		}
>  		num_framesizes++;
>  		break;
>  	case V4L2_PIX_FMT_MM21:

Re: [PATCH 1/4] media: mediatek: vcodec: dec: Fix 4K frame size enumeration

[Chen-Yu Tsai]

On Mon, Jun 27, 2022 at 11:32 PM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Hi Chen-Yu,
>
> Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit :
> > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec:
> > Read max resolution from dec_capability"). In this commit, the maximum
> > resolution ended up being a function of both the firmware capability and
> > the current set format.
> >
> > However, frame size enumeration for output (coded) formats should not
> > depend on the format set, but should return supported resolutions for
> > the format requested by userspace.
>
> Good point. Though, I don't see any special casing for the CAPTURE case. As this
> HW does not include a scaler, it must only return 1 resolution when being
> enumerated for CAPTURE side (or not implement that enumeration, but its
> complicated to half implement something in m2m). The return unique size should
> match what G_FMT(CAPTURE) would return.

There are no frame sizes added for the capture formats, so this function
effectively returns -EINVAL for any of them. This is also what rkvdec
does: it only looks through the list of coded formats.

Also, struct v4l2_frmsizeenum does not have a field saying whether it's
capture or output side; it simply specifies a pixel format.

>
> >
> > Fix this so that the driver returns the supported resolutions correctly,
> > even if the instance only has default settings, or if the output format
> > is currently set to VP8F, which does not support 4K.
> >
> > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c    | 2 --
> >  .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c    | 7 +++++++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > index 5d6fdf18c3a6..fcb4b8131c49 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
> >               fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> >               fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise;
> >
> > -             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,
> > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > index 16d55785d84b..9a4d3e3658aa 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc,
> >
> >               mtk_vdec_framesizes[count_framesizes].fourcc = fourcc;
> >               mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd;
> > +             if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
> > +                 fourcc != V4L2_PIX_FMT_VP8_FRAME) {
> > +                     mtk_vdec_framesizes[count_framesizes].stepwise.max_width =
> > +                             VCODEC_DEC_4K_CODED_WIDTH;
> > +                     mtk_vdec_framesizes[count_framesizes].stepwise.max_height =
> > +                             VCODEC_DEC_4K_CODED_HEIGHT;
> > +             }
>
> I don't particularly like to see this special cased check being added into
> multiple places. Its also in your patch 2, and I think it exist in a third
> place. Could it be possible to have an internal helper to ensure we don't

It's also in s_fmt(), so touched on in patch 4. I could also rewrite it so
only this spot has the special case, and all the other places look though
mtk_vdec_framesizes to get the maximum, like what I did for try_fmt in
patch 3. What do you think?

Ultimately I think it would be better to move framesizes into the
(driver-specific) pixel format data structure. That is a bigger refactoring
than a simple fix though.

> duplicate this logic ? Somehow, it seems there is something in common between
> set_default, try_fmt and this code.

Yes. That is what I mentioned in chat about refactoring the ioctls and format
handling code. set_default should really not set anything format specific,
but instead call set_fmt with a default format.


Regards
ChenYu

>
> >               num_framesizes++;
> >               break;
> >       case V4L2_PIX_FMT_MM21:
>

Re: [PATCH 1/4] media: mediatek: vcodec: dec: Fix 4K frame size enumeration

[Chen-Yu Tsai]

On Mon, Jun 27, 2022 at 11:33 PM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit :
> > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec:
> > Read max resolution from dec_capability"). In this commit, the maximum
> > resolution ended up being a function of both the firmware capability and
> > the current set format.
> >
> > However, frame size enumeration for output (coded) formats should not
> > depend on the format set, but should return supported resolutions for
> > the format requested by userspace.
> >
> > Fix this so that the driver returns the supported resolutions correctly,
> > even if the instance only has default settings, or if the output format
> > is currently set to VP8F, which does not support 4K.
> >
> > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c    | 2 --
> >  .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c    | 7 +++++++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > index 5d6fdf18c3a6..fcb4b8131c49 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
> >               fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> >               fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise;
> >
> > -             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,
> > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > index 16d55785d84b..9a4d3e3658aa 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc,
> >
> >               mtk_vdec_framesizes[count_framesizes].fourcc = fourcc;
> >               mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd;
>
> While at it, can you constify stepwise_fhd, it is scary to think that someone
> could modify it and cause big headache.

I noticed that as well, but since a planned refactoring would get rid of it,
I didn't fix it in this series.

I can add a patch for that.


ChenYu

> > +             if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
> > +                 fourcc != V4L2_PIX_FMT_VP8_FRAME) {
> > +                     mtk_vdec_framesizes[count_framesizes].stepwise.max_width =
> > +                             VCODEC_DEC_4K_CODED_WIDTH;
> > +                     mtk_vdec_framesizes[count_framesizes].stepwise.max_height =
> > +                             VCODEC_DEC_4K_CODED_HEIGHT;
> > +             }
> >               num_framesizes++;
> >               break;
> >       case V4L2_PIX_FMT_MM21:
>

Re: [PATCH 1/4] media: mediatek: vcodec: dec: Fix 4K frame size enumeration

[Nicolas Dufresne]

Le mardi 28 juin 2022 à 00:08 +0800, Chen-Yu Tsai a écrit :
> On Mon, Jun 27, 2022 at 11:32 PM Nicolas Dufresne
> <nicolas.dufresne@collabora.com> wrote:
> > 
> > Hi Chen-Yu,
> > 
> > Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit :
> > > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec:
> > > Read max resolution from dec_capability"). In this commit, the maximum
> > > resolution ended up being a function of both the firmware capability and
> > > the current set format.
> > > 
> > > However, frame size enumeration for output (coded) formats should not
> > > depend on the format set, but should return supported resolutions for
> > > the format requested by userspace.
> > 
> > Good point. Though, I don't see any special casing for the CAPTURE case. As this
> > HW does not include a scaler, it must only return 1 resolution when being
> > enumerated for CAPTURE side (or not implement that enumeration, but its
> > complicated to half implement something in m2m). The return unique size should
> > match what G_FMT(CAPTURE) would return.
> 
> There are no frame sizes added for the capture formats, so this function
> effectively returns -EINVAL for any of them. This is also what rkvdec
> does: it only looks through the list of coded formats.

This is effectively against the spec, ENOTTY would be the only alternative to
not implementing both sides. Though, I'll agree with you, this bugs predates
anything here. Perhaps you could at add MM21 to the switch and returns ENOTTY
there ?

> 
> Also, struct v4l2_frmsizeenum does not have a field saying whether it's
> capture or output side; it simply specifies a pixel format.

Acked.

> 
> > 
> > > 
> > > Fix this so that the driver returns the supported resolutions correctly,
> > > even if the instance only has default settings, or if the output format
> > > is currently set to VP8F, which does not support 4K.
> > > 
> > > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c    | 2 --
> > >  .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c    | 7 +++++++
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > index 5d6fdf18c3a6..fcb4b8131c49 100644
> > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
> > >               fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> > >               fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise;
> > > 
> > > -             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,
> > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > > index 16d55785d84b..9a4d3e3658aa 100644
> > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc,
> > > 
> > >               mtk_vdec_framesizes[count_framesizes].fourcc = fourcc;
> > >               mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd;
> > > +             if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
> > > +                 fourcc != V4L2_PIX_FMT_VP8_FRAME) {
> > > +                     mtk_vdec_framesizes[count_framesizes].stepwise.max_width =
> > > +                             VCODEC_DEC_4K_CODED_WIDTH;
> > > +                     mtk_vdec_framesizes[count_framesizes].stepwise.max_height =
> > > +                             VCODEC_DEC_4K_CODED_HEIGHT;
> > > +             }
> > 
> > I don't particularly like to see this special cased check being added into
> > multiple places. Its also in your patch 2, and I think it exist in a third
> > place. Could it be possible to have an internal helper to ensure we don't
> 
> It's also in s_fmt(), so touched on in patch 4. I could also rewrite it so
> only this spot has the special case, and all the other places look though
> mtk_vdec_framesizes to get the maximum, like what I did for try_fmt in
> patch 3. What do you think?

I don't have a strong opinion, could be a totally internal (and unrelated to any
ioctl naming) helper that does the right thing.

> 
> Ultimately I think it would be better to move framesizes into the
> (driver-specific) pixel format data structure. That is a bigger refactoring
> than a simple fix though.

Agreed.

> 
> > duplicate this logic ? Somehow, it seems there is something in common between
> > set_default, try_fmt and this code.
> 
> Yes. That is what I mentioned in chat about refactoring the ioctls and format
> handling code. set_default should really not set anything format specific,
> but instead call set_fmt with a default format.

So if this could have a simple helper that returns the max width/height for the
specified format and HW capability, I'm then fine with the series. If you can
change the EINVAL (which means nothing is supported) into ENOTTY for the MM21
case, I'd also be more confortable (even though still a bit odd, but no longer a
lie).

regards,
Nicolas

> 
> 
> Regards
> ChenYu
> 
> > 
> > >               num_framesizes++;
> > >               break;
> > >       case V4L2_PIX_FMT_MM21:
> > 

Re: [PATCH 1/4] media: mediatek: vcodec: dec: Fix 4K frame size enumeration

[Chen-Yu Tsai]

On Tue, Jun 28, 2022 at 3:28 AM Nicolas Dufresne
<nicolas.dufresne@collabora.com> wrote:
>
> Le mardi 28 juin 2022 à 00:08 +0800, Chen-Yu Tsai a écrit :
> > On Mon, Jun 27, 2022 at 11:32 PM Nicolas Dufresne
> > <nicolas.dufresne@collabora.com> wrote:
> > >
> > > Hi Chen-Yu,
> > >
> > > Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit :
> > > > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec:
> > > > Read max resolution from dec_capability"). In this commit, the maximum
> > > > resolution ended up being a function of both the firmware capability and
> > > > the current set format.
> > > >
> > > > However, frame size enumeration for output (coded) formats should not
> > > > depend on the format set, but should return supported resolutions for
> > > > the format requested by userspace.
> > >
> > > Good point. Though, I don't see any special casing for the CAPTURE case. As this
> > > HW does not include a scaler, it must only return 1 resolution when being
> > > enumerated for CAPTURE side (or not implement that enumeration, but its
> > > complicated to half implement something in m2m). The return unique size should
> > > match what G_FMT(CAPTURE) would return.
> >
> > There are no frame sizes added for the capture formats, so this function
> > effectively returns -EINVAL for any of them. This is also what rkvdec
> > does: it only looks through the list of coded formats.
>
> This is effectively against the spec, ENOTTY would be the only alternative to
> not implementing both sides. Though, I'll agree with you, this bugs predates
> anything here. Perhaps you could at add MM21 to the switch and returns ENOTTY
> there ?

I think you are slightly misreading the code? The switch/case is in the
function that adds supported formats, not the ioctl callback.

But yeah, I can have the enum_framesizes callback match against capture
formats and return -ENOTTY for them.

For unmatched formats, either capture or output, is it still correct
to return -EINVAL?

> >
> > Also, struct v4l2_frmsizeenum does not have a field saying whether it's
> > capture or output side; it simply specifies a pixel format.
>
> Acked.
>
> >
> > >
> > > >
> > > > Fix this so that the driver returns the supported resolutions correctly,
> > > > even if the instance only has default settings, or if the output format
> > > > is currently set to VP8F, which does not support 4K.
> > > >
> > > > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability")
> > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > > ---
> > > >  drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c    | 2 --
> > > >  .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c    | 7 +++++++
> > > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > index 5d6fdf18c3a6..fcb4b8131c49 100644
> > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c
> > > > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
> > > >               fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> > > >               fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise;
> > > >
> > > > -             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,
> > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > > > index 16d55785d84b..9a4d3e3658aa 100644
> > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c
> > > > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc,
> > > >
> > > >               mtk_vdec_framesizes[count_framesizes].fourcc = fourcc;
> > > >               mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd;
> > > > +             if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) &&
> > > > +                 fourcc != V4L2_PIX_FMT_VP8_FRAME) {
> > > > +                     mtk_vdec_framesizes[count_framesizes].stepwise.max_width =
> > > > +                             VCODEC_DEC_4K_CODED_WIDTH;
> > > > +                     mtk_vdec_framesizes[count_framesizes].stepwise.max_height =
> > > > +                             VCODEC_DEC_4K_CODED_HEIGHT;
> > > > +             }
> > >
> > > I don't particularly like to see this special cased check being added into
> > > multiple places. Its also in your patch 2, and I think it exist in a third
> > > place. Could it be possible to have an internal helper to ensure we don't
> >
> > It's also in s_fmt(), so touched on in patch 4. I could also rewrite it so
> > only this spot has the special case, and all the other places look though
> > mtk_vdec_framesizes to get the maximum, like what I did for try_fmt in
> > patch 3. What do you think?
>
> I don't have a strong opinion, could be a totally internal (and unrelated to any
> ioctl naming) helper that does the right thing.

According to offline discussions in chat, and looking at the code again,
it seems we can get rid of ctx->max_{width,height} altogether.

The check will only exist in mtk_vcodec_add_formats(), and the resolution
clamping will only happen in the try_fmt callback.

ChenYu

> >
> > Ultimately I think it would be better to move framesizes into the
> > (driver-specific) pixel format data structure. That is a bigger refactoring
> > than a simple fix though.
>
> Agreed.
>
> >
> > > duplicate this logic ? Somehow, it seems there is something in common between
> > > set_default, try_fmt and this code.
> >
> > Yes. That is what I mentioned in chat about refactoring the ioctls and format
> > handling code. set_default should really not set anything format specific,
> > but instead call set_fmt with a default format.
>
> So if this could have a simple helper that returns the max width/height for the
> specified format and HW capability, I'm then fine with the series. If you can
> change the EINVAL (which means nothing is supported) into ENOTTY for the MM21
> case, I'd also be more confortable (even though still a bit odd, but no longer a
> lie).
>
> regards,
> Nicolas
>
> >
> >
> > Regards
> > ChenYu
> >
> > >
> > > >               num_framesizes++;
> > > >               break;
> > > >       case V4L2_PIX_FMT_MM21:
> > >
>