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@xxxxxxxxxxxx>
> > > Reviewed-by: Tzung-Bi Shih<tzungbi@xxxxxxxxxx>
> > > ---
> > > .../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;
> > > }
> > >
>
> [...]
>
