* [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields
@ 2018-11-23 17:19 Ezequiel Garcia
2018-11-26 4:14 ` Tomasz Figa
0 siblings, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2018-11-23 17:19 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, Tomasz Figa, Ezequiel Garcia
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@chromium.org>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++++++++++----
1 file changed, 45 insertions(+), 6 deletions(-)
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;
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));
+ 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:
if (unlikely(!ops->vidioc_s_fmt_vid_overlay))
break;
@@ -1564,7 +1584,13 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
if (unlikely(!ops->vidioc_s_fmt_vid_out_mplane))
break;
CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
- return ops->vidioc_s_fmt_vid_out_mplane(file, fh, arg);
+ ret = ops->vidioc_s_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;
case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
if (unlikely(!ops->vidioc_s_fmt_vid_out_overlay))
break;
@@ -1604,6 +1630,7 @@ static int v4l_try_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;
@@ -1623,7 +1650,13 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
if (unlikely(!ops->vidioc_try_fmt_vid_cap_mplane))
break;
CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
- return ops->vidioc_try_fmt_vid_cap_mplane(file, fh, arg);
+ ret = ops->vidioc_try_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:
if (unlikely(!ops->vidioc_try_fmt_vid_overlay))
break;
@@ -1651,7 +1684,13 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
if (unlikely(!ops->vidioc_try_fmt_vid_out_mplane))
break;
CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
- return ops->vidioc_try_fmt_vid_out_mplane(file, fh, arg);
+ ret = ops->vidioc_try_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;
case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
if (unlikely(!ops->vidioc_try_fmt_vid_out_overlay))
break;
--
2.19.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields
2018-11-23 17:19 [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields Ezequiel Garcia
@ 2018-11-26 4:14 ` Tomasz Figa
2018-11-26 23:29 ` Ezequiel Garcia
0 siblings, 1 reply; 5+ messages in thread
From: Tomasz Figa @ 2018-11-26 4:14 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: Linux Media Mailing List, Hans Verkuil
Hi Ezequiel,
On Sat, Nov 24, 2018 at 2:20 AM Ezequiel Garcia <ezequiel@collabora.com> 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@chromium.org>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> 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.
> 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.
> + 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?
> + return ret;
> case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> if (unlikely(!ops->vidioc_s_fmt_vid_overlay))
> break;
> @@ -1564,7 +1584,13 @@ static int v4l_s_fmt(const struct v4l2_ioctl_ops *ops,
> if (unlikely(!ops->vidioc_s_fmt_vid_out_mplane))
> break;
> CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
> - return ops->vidioc_s_fmt_vid_out_mplane(file, fh, arg);
> + ret = ops->vidioc_s_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;
Ditto.
> case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> if (unlikely(!ops->vidioc_s_fmt_vid_out_overlay))
> break;
> @@ -1604,6 +1630,7 @@ static int v4l_try_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;
> @@ -1623,7 +1650,13 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
> if (unlikely(!ops->vidioc_try_fmt_vid_cap_mplane))
> break;
> CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
> - return ops->vidioc_try_fmt_vid_cap_mplane(file, fh, arg);
> + ret = ops->vidioc_try_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;
Ditto.
> case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> if (unlikely(!ops->vidioc_try_fmt_vid_overlay))
> break;
> @@ -1651,7 +1684,13 @@ static int v4l_try_fmt(const struct v4l2_ioctl_ops *ops,
> if (unlikely(!ops->vidioc_try_fmt_vid_out_mplane))
> break;
> CLEAR_AFTER_FIELD(p, fmt.pix_mp.xfer_func);
> - return ops->vidioc_try_fmt_vid_out_mplane(file, fh, arg);
> + ret = ops->vidioc_try_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;
Ditto.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields
2018-11-26 4:14 ` Tomasz Figa
@ 2018-11-26 23:29 ` Ezequiel Garcia
2018-11-27 7:59 ` Tomasz Figa
0 siblings, 1 reply; 5+ messages in thread
From: Ezequiel Garcia @ 2018-11-26 23:29 UTC (permalink / raw)
To: Tomasz Figa; +Cc: Linux Media Mailing List, Hans Verkuil
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@collabora.com> 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@chromium.org>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields
2018-11-26 23:29 ` Ezequiel Garcia
@ 2018-11-27 7:59 ` Tomasz Figa
2018-11-27 13:24 ` Ezequiel Garcia
0 siblings, 1 reply; 5+ messages in thread
From: Tomasz Figa @ 2018-11-27 7:59 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: Linux Media Mailing List, Hans Verkuil
On Tue, Nov 27, 2018 at 8:29 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> 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@collabora.com> 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@chromium.org>
> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > ---
> > > 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.
I guess it makes sense indeed. The structure isn't terribly big, so
there shouldn't be any significant performance penalty I suppose.
Perhaps the next step would be to have someone clean up the existing drivers.
>
> Should we use CLEAR_AFTER_FIELD here as well?
>
I think so.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields
2018-11-27 7:59 ` Tomasz Figa
@ 2018-11-27 13:24 ` Ezequiel Garcia
0 siblings, 0 replies; 5+ messages in thread
From: Ezequiel Garcia @ 2018-11-27 13:24 UTC (permalink / raw)
To: Tomasz Figa; +Cc: Linux Media Mailing List, Hans Verkuil
On Tue, 2018-11-27 at 16:59 +0900, Tomasz Figa wrote:
> On Tue, Nov 27, 2018 at 8:29 AM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > 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@collabora.com> 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@chromium.org>
> > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > ---
> > > > 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.
>
> I guess it makes sense indeed. The structure isn't terribly big, so
> there shouldn't be any significant performance penalty I suppose.
Actually, after some experiments, I realized the entire v4l2_format struct
is already being memset to 0 in v4l_g_fmt.
Thanks,
Eze
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-28 0:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-23 17:19 [PATCH] v4l2-ioctl: Zero v4l2_pix_format_mplane reserved fields Ezequiel Garcia
2018-11-26 4:14 ` Tomasz Figa
2018-11-26 23:29 ` Ezequiel Garcia
2018-11-27 7:59 ` Tomasz Figa
2018-11-27 13:24 ` Ezequiel Garcia
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.