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 <[email protected]>
> Reviewed-by: Tzung-Bi Shih<[email protected]>
> ---
> .../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
> @@ -152,13 +152,15 @@ void mtk_vcodec_dec_set_default_params(struct mtk_vcodec_ctx *ctx)
> q_data->coded_height = DFT_CFG_HEIGHT;
> q_data->fmt = ctx->dev->vdec_pdata->default_cap_fmt;
> q_data->field = V4L2_FIELD_NONE;
> + ctx->max_width = MTK_VDEC_MAX_W;
> + ctx->max_height = MTK_VDEC_MAX_H;
>
> v4l_bound_align_image(&q_data->coded_width,
> MTK_VDEC_MIN_W,
> - MTK_VDEC_MAX_W, 4,
> + ctx->max_width, 4,
> &q_data->coded_height,
> MTK_VDEC_MIN_H,
> - MTK_VDEC_MAX_H, 5, 6);
> + ctx->max_height, 5, 6);
>
> q_data->sizeimage[0] = q_data->coded_width * q_data->coded_height;
> q_data->bytesperline[0] = q_data->coded_width;
> @@ -217,7 +219,7 @@ static int vidioc_vdec_subscribe_evt(struct v4l2_fh *fh,
> }
> }
>
> -static int vidioc_try_fmt(struct v4l2_format *f,
> +static int vidioc_try_fmt(struct mtk_vcodec_ctx *ctx, struct v4l2_format *f,
> const struct mtk_video_fmt *fmt)
> {
> struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp;
> @@ -225,9 +227,9 @@ static int vidioc_try_fmt(struct v4l2_format *f,
> pix_fmt_mp->field = V4L2_FIELD_NONE;
>
> pix_fmt_mp->width =
> - clamp(pix_fmt_mp->width, MTK_VDEC_MIN_W, MTK_VDEC_MAX_W);
> + clamp(pix_fmt_mp->width, MTK_VDEC_MIN_W, ctx->max_width);
> pix_fmt_mp->height =
> - clamp(pix_fmt_mp->height, MTK_VDEC_MIN_H, MTK_VDEC_MAX_H);
> + clamp(pix_fmt_mp->height, MTK_VDEC_MIN_H, ctx->max_height);
>
> if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> pix_fmt_mp->num_planes = 1;
> @@ -245,16 +247,16 @@ static int vidioc_try_fmt(struct v4l2_format *f,
> tmp_h = pix_fmt_mp->height;
> v4l_bound_align_image(&pix_fmt_mp->width,
> MTK_VDEC_MIN_W,
> - MTK_VDEC_MAX_W, 6,
> + ctx->max_width, 6,
> &pix_fmt_mp->height,
> MTK_VDEC_MIN_H,
> - MTK_VDEC_MAX_H, 6, 9);
> + ctx->max_height, 6, 9);
>
> if (pix_fmt_mp->width < tmp_w &&
> - (pix_fmt_mp->width + 64) <= MTK_VDEC_MAX_W)
> + (pix_fmt_mp->width + 64) <= ctx->max_width)
> pix_fmt_mp->width += 64;
> if (pix_fmt_mp->height < tmp_h &&
> - (pix_fmt_mp->height + 64) <= MTK_VDEC_MAX_H)
> + (pix_fmt_mp->height + 64) <= ctx->max_height)
> pix_fmt_mp->height += 64;
>
> mtk_v4l2_debug(0,
> @@ -294,7 +296,7 @@ static int vidioc_try_fmt_vid_cap_mplane(struct file *file, void *priv,
> fmt = mtk_vdec_find_format(f, dec_pdata);
> }
>
> - return vidioc_try_fmt(f, fmt);
> + return vidioc_try_fmt(ctx, f, fmt);
> }
>
> static int vidioc_try_fmt_vid_out_mplane(struct file *file, void *priv,
> @@ -317,7 +319,7 @@ static int vidioc_try_fmt_vid_out_mplane(struct file *file, void *priv,
> return -EINVAL;
> }
>
> - return vidioc_try_fmt(f, fmt);
> + return vidioc_try_fmt(ctx, f, fmt);
> }
>
> static int vidioc_vdec_g_selection(struct file *file, void *priv,
> @@ -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;
+
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;
> }
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> index bb7b8e914d24..6d27e4d41ede 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> @@ -284,6 +284,8 @@ struct vdec_pic_info {
> * mtk_video_dec_buf.
> * @hw_id: hardware index used to identify different hardware.
> *
> + * @max_width: hardware supported max width
> + * @max_height: hardware supported max height
> * @msg_queue: msg queue used to store lat buffer information.
> */
> struct mtk_vcodec_ctx {
> @@ -329,6 +331,8 @@ struct mtk_vcodec_ctx {
> struct mutex lock;
> int hw_id;
>
> + unsigned int max_width;
> + unsigned int max_height;
> struct vdec_msg_queue msg_queue;
> };
>
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 <[email protected]>
> > Reviewed-by: Tzung-Bi Shih<[email protected]>
> > ---
> > .../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.
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;
> > }
> >
[...]
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 <[email protected]>
> > > Reviewed-by: Tzung-Bi Shih<[email protected]>
> > > ---
> > > .../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;
> > > }
> > >
>
> [...]
>