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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > ---
> > > >  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:
> > >
>