[07/10] media: mtk-vcodec: venc: remove redundant code
diff mbox series

Message ID 20200520082723.96136-8-acourbot@chromium.org
State New
Headers show
Series
  • media: mtk-vcodec: venc: support for MT8183
Related show

Commit Message

Alexandre Courbot May 20, 2020, 8:27 a.m. UTC
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(-)

Comments

Tiffany Lin June 19, 2020, 6:59 a.m. UTC | #1
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;
>
Alexandre Courbot June 22, 2020, 1:10 p.m. UTC | #2
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;
> >
>
Tiffany Lin June 23, 2020, 1:37 a.m. UTC | #3
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;
> > >
> >

Patch
diff mbox series

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;