From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Chen-Yu Tsai <wenst@chromium.org>
Cc: Yunfei Dong <yunfei.dong@mediatek.com>,
Alexandre Courbot <acourbot@chromium.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Tzung-Bi Shih <tzungbi@chromium.org>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com>,
Benjamin Gaignard <benjamin.gaignard@collabora.com>,
Tiffany Lin <tiffany.lin@mediatek.com>,
Andrew-CT Chen <andrew-ct.chen@mediatek.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Tomasz Figa <tfiga@google.com>,
George Sun <george.sun@mediatek.com>,
Xiaoyong Lu <xiaoyong.lu@mediatek.com>,
Hsin-Yi Wang <hsinyi@chromium.org>,
Fritz Koenig <frkoenig@chromium.org>,
Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
Daniel Vetter <daniel@ffwll.ch>,
dri-devel <dri-devel@lists.freedesktop.org>,
Irui Wang <irui.wang@mediatek.com>,
Steve Cho <stevecho@chromium.org>,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
srv_heupstream@mediatek.com, linux-mediatek@lists.infradead.org,
Project_Global_Chrome_Upstream_Group@mediatek.com
Subject: Re: [PATCH v7, 04/15] media: mtk-vcodec: Read max resolution from dec_capability
Date: Tue, 21 Jun 2022 11:33:14 -0400 [thread overview]
Message-ID: <e95f2b43c131927a488f86081683c15b885efea7.camel@ndufresne.ca> (raw)
In-Reply-To: <YqwjOurt2DCV6snP@google.com>
Le vendredi 17 juin 2022 à 14:46 +0800, Chen-Yu Tsai a écrit :
> Hi,
>
> On Mon, Feb 28, 2022 at 04:29:15PM -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
> > > @@ -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:
> >
> >
> > 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;
> > +
>
> Recent testing on ChromeOS suggests this doesn't work. The spec implies
> that querying capabilities could happen before the output format is set.
> And also, supported frame sizes are detected for each given format,
> which may not be the one current set.
In v4l2, formats are always set. Perhaps the problem is that we don't
automatically set ctx->max_width/height for the default format when the firmware
is up. I noticed recently the chromium always do G_FMT before S_FMT, so perhaps
it can skip S_FMT if the default format is appropriate, and that endup avoiding
the code I've just suggested. At the time I wrote that, I only had GStreamer
available to test, and it always calls S_FMT, which is mandatory, see 4.5.3.2.
Initialization step 1. But I cannot say userland would be wrong to skip if that
format was "initially" correct.
If my understanding is not correct, then perhaps you should provide a tad more
details on how this failed for you, and we can then better judge an appropriate
fix.
regards,
Nicolas
>
> So the if block above has to be reintroduced in some form. I'll take a
> look at this.
>
>
> Regards
> ChenYu
>
> > 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;
> > > }
> > >
>
> [...]
>
next prev parent reply other threads:[~2022-06-21 15:35 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-23 3:39 [PATCH v7, 00/15] media: mtk-vcodec: support for M8192 decoder Yunfei Dong
2022-02-23 3:39 ` [PATCH v7, 01/15] media: mtk-vcodec: Add vdec enable/disable hardware helpers Yunfei Dong
2022-02-25 9:23 ` AngeloGioacchino Del Regno
2022-02-23 3:39 ` [PATCH v7, 02/15] media: mtk-vcodec: Using firmware type to separate different firmware architecture Yunfei Dong
2022-02-23 3:39 ` [PATCH v7, 03/15] media: mtk-vcodec: get capture queue buffer size from scp Yunfei Dong
2022-03-01 14:44 ` Nicolas Dufresne
2022-03-02 2:26 ` yunfei.dong
2022-02-23 3:39 ` [PATCH v7, 04/15] media: mtk-vcodec: Read max resolution from dec_capability Yunfei Dong
2022-02-25 9:23 ` AngeloGioacchino Del Regno
2022-02-28 21:29 ` Nicolas Dufresne
2022-03-02 1:47 ` yunfei.dong
2022-06-17 6:46 ` Chen-Yu Tsai
2022-06-21 15:33 ` Nicolas Dufresne [this message]
2022-02-23 3:39 ` [PATCH v7, 05/15] media: mtk-vcodec: Call v4l2_m2m_set_dst_buffered() set capture buffer buffered Yunfei Dong
2022-03-01 18:50 ` Nicolas Dufresne
2022-02-23 3:39 ` [PATCH v7, 06/15] media: mtk-vcodec: Refactor get and put capture buffer flow Yunfei Dong
2022-03-01 19:00 ` Nicolas Dufresne
2022-02-23 3:40 ` [PATCH v7, 07/15] media: mtk-vcodec: Refactor supported vdec formats and framesizes Yunfei Dong
2022-02-25 9:24 ` AngeloGioacchino Del Regno
2022-03-01 14:34 ` Nicolas Dufresne
2022-03-04 7:27 ` yunfei.dong
2022-02-23 3:40 ` [PATCH v7, 08/15] media: mtk-vcodec: Add format to support MT21C Yunfei Dong
2022-02-25 9:24 ` AngeloGioacchino Del Regno
2022-02-23 3:40 ` [PATCH v7, 09/15] media: mtk-vcodec: disable vp8 4K capability Yunfei Dong
2022-03-01 19:02 ` Nicolas Dufresne
2022-02-23 3:40 ` [PATCH v7, 10/15] media: mtk-vcodec: Fix v4l2-compliance fail Yunfei Dong
2022-02-23 3:40 ` [PATCH v7, 11/15] media: mtk-vcodec: record capture queue format type Yunfei Dong
2022-02-25 9:24 ` AngeloGioacchino Del Regno
2022-02-23 3:40 ` [PATCH v7, 12/15] media: mtk-vcodec: Extract H264 common code Yunfei Dong
2022-03-01 21:30 ` Nicolas Dufresne
2022-02-23 3:40 ` [PATCH v7, 13/15] media: mtk-vcodec: support stateless H.264 decoding for mt8192 Yunfei Dong
2022-03-01 22:01 ` Nicolas Dufresne
2022-02-23 3:40 ` [PATCH v7, 14/15] media: mtk-vcodec: support stateless VP8 decoding Yunfei Dong
2022-03-01 22:15 ` Nicolas Dufresne
2022-02-23 3:40 ` [PATCH v7, 15/15] media: mtk-vcodec: support stateless VP9 decoding Yunfei Dong
2022-03-01 22:22 ` Nicolas Dufresne
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e95f2b43c131927a488f86081683c15b885efea7.camel@ndufresne.ca \
--to=nicolas@ndufresne.ca \
--cc=Project_Global_Chrome_Upstream_Group@mediatek.com \
--cc=acourbot@chromium.org \
--cc=andrew-ct.chen@mediatek.com \
--cc=angelogioacchino.delregno@collabora.com \
--cc=benjamin.gaignard@collabora.com \
--cc=dafna.hirschfeld@collabora.com \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=frkoenig@chromium.org \
--cc=george.sun@mediatek.com \
--cc=hsinyi@chromium.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=irui.wang@mediatek.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=mchehab@kernel.org \
--cc=robh+dt@kernel.org \
--cc=srv_heupstream@mediatek.com \
--cc=stevecho@chromium.org \
--cc=tfiga@google.com \
--cc=tiffany.lin@mediatek.com \
--cc=tzungbi@chromium.org \
--cc=wenst@chromium.org \
--cc=xiaoyong.lu@mediatek.com \
--cc=yunfei.dong@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).