linux-kernel - Re: [PATCH 07/10] media: mtk-vcodec: venc: remove redundant code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 Jun 2020 09:37:21 +0800
From:   Tiffany Lin <tiffany.lin@...iatek.com>
To:     Alexandre Courbot <acourbot@...omium.org>
CC:     Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
        Rui Wang <gtk_ruiwang@...iatek.com>,
        Yunfei Dong <yunfei.dong@...iatek.com>,
        "Pi-Hsun Shih" <pihsun@...omium.org>,
        Maoguang Meng <maoguang.meng@...iatek.com>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 07/10] media: mtk-vcodec: venc: remove redundant code

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@...iatek.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@...omium.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;
> > >
> >