diff mbox series

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

Message ID 20220627112402.2332046-2-wenst@chromium.org (mailing list archive)
State Changes Requested
Delegated to: Hans Verkuil
Headers
Series media: mediatek: vcodec: Fix 4K decoding support |

Commit Message

Chen-Yu Tsai June 27, 2022, 11:23 a.m. UTC 
  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(-)

Comments

Nicolas Dufresne June 27, 2022, 3:32 p.m. UTC | #1 
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:
Nicolas Dufresne June 27, 2022, 3:33 p.m. UTC | #2 
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:
Chen-Yu Tsai June 27, 2022, 4:08 p.m. UTC | #3 
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:
>
Chen-Yu Tsai June 27, 2022, 4:13 p.m. UTC | #4 
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:
>
Nicolas Dufresne June 27, 2022, 7:28 p.m. UTC | #5 
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:
> >
Chen-Yu Tsai June 28, 2022, 7:04 a.m. UTC | #6 
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:
> > >
>
diff mbox series

Patch

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: