| Message ID | 20200520082723.96136-8-acourbot@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: mtk-vcodec: venc: support for MT8183 | expand |
On Wed, 2020-05-20 at 17:27 +0900, Alexandre Courbot wrote: > vidioc_try_fmt() does clamp height and width when called on the OUTPUT > queue, so clamping them prior to calling this function is redundant. Set > the queue's parameters after calling vidioc_try_fmt() so we can use the > values it computed. > vidioc_try_fmt clamps height and width only when f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE Does this cleanup pass v4l2 compliance test? I recall compliance test will try different fmt and make sure driver response enough information? > Signed-off-by: Alexandre Courbot <acourbot@chromium.org> > --- > .../media/platform/mtk-vcodec/mtk_vcodec_enc.c | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > index 05743a745a11..f0af78f112db 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > @@ -449,7 +449,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv, > struct mtk_q_data *q_data; > int ret, i; > const struct mtk_video_fmt *fmt; > - struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp; > > vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); > if (!vq) { > @@ -474,20 +473,13 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv, > f->fmt.pix.pixelformat = fmt->fourcc; > } > > - pix_fmt_mp->height = clamp(pix_fmt_mp->height, > - MTK_VENC_MIN_H, > - MTK_VENC_MAX_H); > - pix_fmt_mp->width = clamp(pix_fmt_mp->width, > - MTK_VENC_MIN_W, > - MTK_VENC_MAX_W); > - > - q_data->visible_width = f->fmt.pix_mp.width; > - q_data->visible_height = f->fmt.pix_mp.height; > - q_data->fmt = fmt; > - ret = vidioc_try_fmt(f, q_data->fmt); > + ret = vidioc_try_fmt(f, fmt); > if (ret) > return ret; > > + q_data->fmt = fmt; > + q_data->visible_width = f->fmt.pix_mp.width; > + q_data->visible_height = f->fmt.pix_mp.height; > q_data->coded_width = f->fmt.pix_mp.width; > q_data->coded_height = f->fmt.pix_mp.height; >
On Fri, Jun 19, 2020 at 3:59 PM Tiffany Lin <tiffany.lin@mediatek.com> wrote: > > On Wed, 2020-05-20 at 17:27 +0900, Alexandre Courbot wrote: > > vidioc_try_fmt() does clamp height and width when called on the OUTPUT > > queue, so clamping them prior to calling this function is redundant. Set > > the queue's parameters after calling vidioc_try_fmt() so we can use the > > values it computed. > > > > vidioc_try_fmt clamps height and width only when f->type == > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > > Does this cleanup pass v4l2 compliance test? It doesn't result in more tests failing at least. :) But although I cannot test with a pristine upstream version, it seems like some tests are not passing to begin with. If you have different results with a true upstream I would like to hear about it. Otherwise I am willing to help with getting all the tests in the green. Regarding this particular patch, you are right that we may end up writing an unclamped size in q_data. It's probably better to drop it for now. > I recall compliance test will try different fmt and make sure driver > response enough information? > > > > Signed-off-by: Alexandre Courbot <acourbot@chromium.org> > > --- > > .../media/platform/mtk-vcodec/mtk_vcodec_enc.c | 16 ++++------------ > > 1 file changed, 4 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > index 05743a745a11..f0af78f112db 100644 > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > @@ -449,7 +449,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv, > > struct mtk_q_data *q_data; > > int ret, i; > > const struct mtk_video_fmt *fmt; > > - struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp; > > > > vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); > > if (!vq) { > > @@ -474,20 +473,13 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv, > > f->fmt.pix.pixelformat = fmt->fourcc; > > } > > > > - pix_fmt_mp->height = clamp(pix_fmt_mp->height, > > - MTK_VENC_MIN_H, > > - MTK_VENC_MAX_H); > > - pix_fmt_mp->width = clamp(pix_fmt_mp->width, > > - MTK_VENC_MIN_W, > > - MTK_VENC_MAX_W); > > - > > - q_data->visible_width = f->fmt.pix_mp.width; > > - q_data->visible_height = f->fmt.pix_mp.height; > > - q_data->fmt = fmt; > > - ret = vidioc_try_fmt(f, q_data->fmt); > > + ret = vidioc_try_fmt(f, fmt); > > if (ret) > > return ret; > > > > + q_data->fmt = fmt; > > + q_data->visible_width = f->fmt.pix_mp.width; > > + q_data->visible_height = f->fmt.pix_mp.height; > > q_data->coded_width = f->fmt.pix_mp.width; > > q_data->coded_height = f->fmt.pix_mp.height; > > >
On Mon, 2020-06-22 at 22:10 +0900, Alexandre Courbot wrote: > On Fri, Jun 19, 2020 at 3:59 PM Tiffany Lin <tiffany.lin@mediatek.com> wrote: > > > > On Wed, 2020-05-20 at 17:27 +0900, Alexandre Courbot wrote: > > > vidioc_try_fmt() does clamp height and width when called on the OUTPUT > > > queue, so clamping them prior to calling this function is redundant. Set > > > the queue's parameters after calling vidioc_try_fmt() so we can use the > > > values it computed. > > > > > > > vidioc_try_fmt clamps height and width only when f->type == > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE > > > > Does this cleanup pass v4l2 compliance test? > > It doesn't result in more tests failing at least. :) But although I > cannot test with a pristine upstream version, it seems like some tests > are not passing to begin with. If you have different results with a > true upstream I would like to hear about it. Otherwise I am willing to > help with getting all the tests in the green. > > Regarding this particular patch, you are right that we may end up > writing an unclamped size in q_data. It's probably better to drop it > for now. > I did attach compliance tests results when I upstream first version. It's how maintainer make sure all v4l2 driver implement interfaces the same way. And by doing this automatically instead review flow to make sure it meet interfaces spec. > > I recall compliance test will try different fmt and make sure driver > > response enough information? > > > > > > > Signed-off-by: Alexandre Courbot <acourbot@chromium.org> > > > --- > > > .../media/platform/mtk-vcodec/mtk_vcodec_enc.c | 16 ++++------------ > > > 1 file changed, 4 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > > index 05743a745a11..f0af78f112db 100644 > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c > > > @@ -449,7 +449,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv, > > > struct mtk_q_data *q_data; > > > int ret, i; > > > const struct mtk_video_fmt *fmt; > > > - struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp; > > > > > > vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); > > > if (!vq) { > > > @@ -474,20 +473,13 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv, > > > f->fmt.pix.pixelformat = fmt->fourcc; > > > } > > > > > > - pix_fmt_mp->height = clamp(pix_fmt_mp->height, > > > - MTK_VENC_MIN_H, > > > - MTK_VENC_MAX_H); > > > - pix_fmt_mp->width = clamp(pix_fmt_mp->width, > > > - MTK_VENC_MIN_W, > > > - MTK_VENC_MAX_W); > > > - > > > - q_data->visible_width = f->fmt.pix_mp.width; > > > - q_data->visible_height = f->fmt.pix_mp.height; > > > - q_data->fmt = fmt; > > > - ret = vidioc_try_fmt(f, q_data->fmt); > > > + ret = vidioc_try_fmt(f, fmt); > > > if (ret) > > > return ret; > > > > > > + q_data->fmt = fmt; > > > + q_data->visible_width = f->fmt.pix_mp.width; > > > + q_data->visible_height = f->fmt.pix_mp.height; > > > q_data->coded_width = f->fmt.pix_mp.width; > > > q_data->coded_height = f->fmt.pix_mp.height; > > > > >
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c index 05743a745a11..f0af78f112db 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c @@ -449,7 +449,6 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv, struct mtk_q_data *q_data; int ret, i; const struct mtk_video_fmt *fmt; - struct v4l2_pix_format_mplane *pix_fmt_mp = &f->fmt.pix_mp; vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type); if (!vq) { @@ -474,20 +473,13 @@ static int vidioc_venc_s_fmt_out(struct file *file, void *priv, f->fmt.pix.pixelformat = fmt->fourcc; } - pix_fmt_mp->height = clamp(pix_fmt_mp->height, - MTK_VENC_MIN_H, - MTK_VENC_MAX_H); - pix_fmt_mp->width = clamp(pix_fmt_mp->width, - MTK_VENC_MIN_W, - MTK_VENC_MAX_W); - - q_data->visible_width = f->fmt.pix_mp.width; - q_data->visible_height = f->fmt.pix_mp.height; - q_data->fmt = fmt; - ret = vidioc_try_fmt(f, q_data->fmt); + ret = vidioc_try_fmt(f, fmt); if (ret) return ret; + q_data->fmt = fmt; + q_data->visible_width = f->fmt.pix_mp.width; + q_data->visible_height = f->fmt.pix_mp.height; q_data->coded_width = f->fmt.pix_mp.width; q_data->coded_height = f->fmt.pix_mp.height;
vidioc_try_fmt() does clamp height and width when called on the OUTPUT queue, so clamping them prior to calling this function is redundant. Set the queue's parameters after calling vidioc_try_fmt() so we can use the values it computed. Signed-off-by: Alexandre Courbot <acourbot@chromium.org> --- .../media/platform/mtk-vcodec/mtk_vcodec_enc.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)