Message-ID: <f05953a93fb8f250f4da7c3384b6e1c43c7b1605.camel@collabora.com>
Date:   Mon, 27 Jun 2022 15:28:42 -0400
From:   Nicolas Dufresne <nicolas.dufresne@...labora.com>
To:     Chen-Yu Tsai <wenst@...omium.org>
Cc:     Tiffany Lin <tiffany.lin@...iatek.com>,
        Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        linux-media@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        linux-kernel@...r.kernel.org,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Yunfei Dong <yunfei.dong@...iatek.com>
Subject: Re: [PATCH 1/4] media: mediatek: vcodec: dec: Fix 4K frame size
 enumeration

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@...labora.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@...omium.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:
> > 

Powered by blists - more mailing lists