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