| Message ID | 20200106161417.19150-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 06/01/2020 16:14, Laurent Pinchart wrote: > The VIDIOC_G_FMT implementation overwrites the v4l2_format type field > with 0. Fix it. Could you clarify this please? You mean the kernel changes the arg->type? The whole function reads as: int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg) { LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt"; if (!validateBufferType(arg->type)) return -EINVAL; memset(&arg->fmt, 0, sizeof(arg->fmt)); > + arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; /* LP-Addition */ arg->fmt.pix = curV4L2Format_.fmt.pix; return 0; } I see usage of arg->type in the validateBufferType before the memset. Does this need to be set correctly before that ? or otherwise what is the purpose of the call to validateBufferType(arg->type)? -- Kieran > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/v4l2/v4l2_camera_proxy.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index 2eeb12396d90..dd3ee3e6c6ff 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -229,6 +229,7 @@ int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg) > return -EINVAL; > > memset(&arg->fmt, 0, sizeof(arg->fmt)); > + arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > arg->fmt.pix = curV4L2Format_.fmt.pix; > > return 0; >
Hi Kieran, On Mon, Jan 06, 2020 at 04:58:48PM +0000, Kieran Bingham wrote: > On 06/01/2020 16:14, Laurent Pinchart wrote: > > The VIDIOC_G_FMT implementation overwrites the v4l2_format type field > > with 0. Fix it. > > Could you clarify this please? > > You mean the kernel changes the arg->type? > > The whole function reads as: > > int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg) > { > LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt"; > > if (!validateBufferType(arg->type)) > return -EINVAL; > > memset(&arg->fmt, 0, sizeof(arg->fmt)); > > + arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; /* LP-Addition */ > arg->fmt.pix = curV4L2Format_.fmt.pix; > > return 0; > } > > > I see usage of arg->type in the validateBufferType before the memset. > > Does this need to be set correctly before that ? or otherwise what is > the purpose of the call to validateBufferType(arg->type)? I meant that *our* implementation of VIDIOC_G_FMT unconditionally sets arg->type to 0 (due to the memset). It's incorrect, the field should retain its value. And now that I've written this, I realize that we only memset arg->fmt, not arg... I'll drop this patch :-) > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/v4l2/v4l2_camera_proxy.cpp | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > index 2eeb12396d90..dd3ee3e6c6ff 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -229,6 +229,7 @@ int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg) > > return -EINVAL; > > > > memset(&arg->fmt, 0, sizeof(arg->fmt)); > > + arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > > arg->fmt.pix = curV4L2Format_.fmt.pix; > > > > return 0;
Hi Laurent, On 06/01/2020 17:11, Laurent Pinchart wrote: > Hi Kieran, > > On Mon, Jan 06, 2020 at 04:58:48PM +0000, Kieran Bingham wrote: >> On 06/01/2020 16:14, Laurent Pinchart wrote: >>> The VIDIOC_G_FMT implementation overwrites the v4l2_format type field >>> with 0. Fix it. >> >> Could you clarify this please? >> >> You mean the kernel changes the arg->type? >> >> The whole function reads as: >> >> int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg) >> { >> LOG(V4L2Compat, Debug) << "Servicing vidioc_g_fmt"; >> >> if (!validateBufferType(arg->type)) >> return -EINVAL; >> >> memset(&arg->fmt, 0, sizeof(arg->fmt)); >>> + arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; /* LP-Addition */ >> arg->fmt.pix = curV4L2Format_.fmt.pix; >> >> return 0; >> } >> >> >> I see usage of arg->type in the validateBufferType before the memset. >> >> Does this need to be set correctly before that ? or otherwise what is >> the purpose of the call to validateBufferType(arg->type)? > > I meant that *our* implementation of VIDIOC_G_FMT unconditionally sets > arg->type to 0 (due to the memset). It's incorrect, the field should but .... > retain its value. And now that I've written this, I realize that we only > memset arg->fmt, not arg... I'll drop this patch :-) Aha - there we go then :-D I thought it was an odd looking patch! -- Kieran > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> src/v4l2/v4l2_camera_proxy.cpp | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp >>> index 2eeb12396d90..dd3ee3e6c6ff 100644 >>> --- a/src/v4l2/v4l2_camera_proxy.cpp >>> +++ b/src/v4l2/v4l2_camera_proxy.cpp >>> @@ -229,6 +229,7 @@ int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg) >>> return -EINVAL; >>> >>> memset(&arg->fmt, 0, sizeof(arg->fmt)); >>> + arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >>> arg->fmt.pix = curV4L2Format_.fmt.pix; >>> >>> return 0; >
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index 2eeb12396d90..dd3ee3e6c6ff 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -229,6 +229,7 @@ int V4L2CameraProxy::vidioc_g_fmt(struct v4l2_format *arg) return -EINVAL; memset(&arg->fmt, 0, sizeof(arg->fmt)); + arg->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; arg->fmt.pix = curV4L2Format_.fmt.pix; return 0;
The VIDIOC_G_FMT implementation overwrites the v4l2_format type field with 0. Fix it. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/v4l2/v4l2_camera_proxy.cpp | 1 + 1 file changed, 1 insertion(+)