Re: [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2018-11-26 at 13:14 +0900, Tomasz Figa wrote:
> Hi Ezequiel,
> 
> On Sat, Nov 24, 2018 at 2:20 AM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
> > Make the core set the reserved fields to zero in
> > v4l2_pix_format_mplane and v4l2_plane_pix_format structs,
> > for _MPLANE queue types.
> > 
> > Moving this to the core avoids having to do so in each
> > and every driver.
> > 
> > Suggested-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > ---
> >  drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++++++++++----
> >  1 file changed, 45 insertions(+), 6 deletions(-)
> > 
> 
> Thanks for the patch. Please see my comments inline.
> 
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 10b862dcbd86..3858fffc3e68 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1420,6 +1420,7 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> >  {
> >         struct v4l2_format *p = arg;
> >         int ret = check_fmt(file, p->type);
> > +       int i;
> > 
> >         if (ret)
> >                 return ret;
> > @@ -1458,7 +1459,13 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> >                 p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> >                 return ret;
> >         case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > -               return ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg);
> > +               ret = ops->vidioc_g_fmt_vid_cap_mplane(file, fh, arg);
> > +               memset(p->fmt.pix_mp.reserved, 0,
> > +                      sizeof(p->fmt.pix_mp.reserved));
> > +               for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> > +                       memset(p->fmt.pix_mp.plane_fmt[i].reserved, 0,
> > +                              sizeof(p->fmt.pix_mp.plane_fmt[i].reserved));
> > +               return ret;
> >         case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> >                 return ops->vidioc_g_fmt_vid_overlay(file, fh, arg);
> >         case V4L2_BUF_TYPE_VBI_CAPTURE:
> > @@ -1474,7 +1481,13 @@ static int v4l_g_fmt(const struct v4l2_ioctl_ops *ops,
> >                 p->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC;
> >                 return ret;
> >         case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > -               return ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg);
> > +               ret = ops->vidioc_g_fmt_vid_out_mplane(file, fh, arg);
> > +               memset(p->fmt.pix_mp.reserved, 0,
> > +                      sizeof(p->fmt.pix_mp.reserved));
> > +               for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> > +                       memset(p->fmt.pix_mp.plane_fmt[i].reserved, 0,
> > +                              sizeof(p->fmt.pix_mp.plane_fmt[i].reserved));
> > +               return ret;
> 
> I wonder if we need this for G_FMT. The driver can just memset() the
> whole struct itself and then just initialize the fields it cares
> about, but actually in many cases the driver will just include an
> instance of the pix_fmt(_mp) struct in its internal state (which has
> the reserved fields already zeroed) and just copy it to the target
> struct in the callback.
> 

Perhaps in many cases, but from code inspection it seems not
all of them (randomly opened vicodec & mtk-jpeg and both need
a memset!). 

I'm thinkig it'd best to keep it this way for consistency
and to avoid having the worry at all about this in the drivers.

Should we use CLEAR_AFTER_FIELD here as well?

> >         case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> >                 return ops->vidioc_g_fmt_vid_out_overlay(file, fh, arg);
> >         case V4L2_BUF_TYPE_VBI_OUTPUT:
> > @@ -1512,6 +1525,7 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> >         struct v4l2_format *p = arg;
> >         struct video_device *vfd = video_devdata(file);
> >         int ret = check_fmt(file, p->type);
> > +       int i;
> > 
> >         if (ret)
> >                 return ret;
> > @@ -1536,7 +1550,13 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> >                 if (unlikely(!ops->vidioc_s_fmt_vid_cap_mplane))
> >                         break;
> >                 CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
> > -               return ops->vidioc_s_fmt_vid_cap_mplane(file, fh, arg);
> > +               ret = ops->vidioc_s_fmt_vid_cap_mplane(file, fh, arg);
> > +               memset(p->fmt.pix_mp.reserved, 0,
> > +                      sizeof(p->fmt.pix_mp.reserved));
> 
> Note that we're already zeroing this field before calling driver's callback.
> 

Right, I missed the CLEAR_AFTER_FIELD macro was also covering reserved fields.

> > +               for (i = 0; i < p->fmt.pix_mp.num_planes; i++)
> > +                       memset(p->fmt.pix_mp.plane_fmt[i].reserved, 0,
> > +                              sizeof(p->fmt.pix_mp.plane_fmt[i].reserved));
> 
> Should we use the CLEAR_AFTER_FIELD() macro? Also, should we do before
> calling the driver, as with pix_mp.reserved?

Yeah, that makes more sense.

Thanks for reviewing,
Eze




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux