[PATCH 0/7] gspca: pass all v4l2-compliance tests + minor fixes

[PATCH 0/7] gspca: pass all v4l2-compliance tests + minor fixes

[Antonio Ospite]

Hi,

after applying this patchset gspca passes all v4l2-compliance tests, at
least it does with a PS3 Eye.

 - Patch 1 removes some magic numbers in subdrivers.
 - Patch 2 is a correctness fix, but it does not bring any functional
   changes.
 - Patch 3 is a readability improvement by itself, but it's also
   a preliminary change for patch 4.
 - Patches from 4 to 7 are the actual compliance fixes.

More details are in the commit messages.

Thanks,
   Antonio

Antonio Ospite (7):
  [media] gspca: ov534/topro: use a define for the default framerate
  [media] gspca: fix setting frame interval type in
    vidioc_enum_frameintervals()
  [media] gspca: rename wxh_to_mode() to wxh_to_nearest_mode()
  [media] gspca: fix a v4l2-compliance failure about
    VIDIOC_ENUM_FRAMEINTERVALS
  [media] gspca: fix a v4l2-compliance failure about buffer timestamp
  [media] gspca: fix a v4l2-compliance failure during VIDIOC_REQBUFS
  [media] gspca: fix a v4l2-compliance failure during read()

 drivers/media/usb/gspca/gspca.c | 36 +++++++++++++++++++++++-------------
 drivers/media/usb/gspca/ov534.c |  7 +++----
 drivers/media/usb/gspca/topro.c |  6 ++++--
 3 files changed, 30 insertions(+), 19 deletions(-)

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH 1/7] [media] gspca: ov534/topro: use a define for the default framerate

[Antonio Ospite]

When writing the change in commit dcc7fdbec53a ("[media] gspca:
ov534/topro: prevent a division by 0") I used magic numbers for the
default framerate to minimize the code footprint to make it easier to
backport the patch to the stable trees.

However it's better if the default framerate has its own define to avoid
risking using different values in different places, and for readability.

While at it also remove some trivial comments about the framerates which
don't add much to the code anymore.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 drivers/media/usb/gspca/ov534.c | 7 +++----
 drivers/media/usb/gspca/topro.c | 6 ++++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/gspca/ov534.c b/drivers/media/usb/gspca/ov534.c
index bfff1d1..9266a5c 100644
--- a/drivers/media/usb/gspca/ov534.c
+++ b/drivers/media/usb/gspca/ov534.c
@@ -51,6 +51,7 @@
 #define OV534_OP_READ_2		0xf9
 
 #define CTRL_TIMEOUT 500
+#define DEFAULT_FRAME_RATE 30
 
 MODULE_AUTHOR("Antonio Ospite <ospite@studenti.unina.it>");
 MODULE_DESCRIPTION("GSPCA/OV534 USB Camera Driver");
@@ -1061,7 +1062,7 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	cam->cam_mode = ov772x_mode;
 	cam->nmodes = ARRAY_SIZE(ov772x_mode);
 
-	sd->frame_rate = 30;
+	sd->frame_rate = DEFAULT_FRAME_RATE;
 
 	return 0;
 }
@@ -1492,10 +1493,8 @@ static void sd_set_streamparm(struct gspca_dev *gspca_dev,
 	struct sd *sd = (struct sd *) gspca_dev;
 
 	if (tpf->numerator == 0 || tpf->denominator == 0)
-		/* Set default framerate */
-		sd->frame_rate = 30;
+		sd->frame_rate = DEFAULT_FRAME_RATE;
 	else
-		/* Set requested framerate */
 		sd->frame_rate = tpf->denominator / tpf->numerator;
 
 	if (gspca_dev->streaming)
diff --git a/drivers/media/usb/gspca/topro.c b/drivers/media/usb/gspca/topro.c
index c028a5c..15eb069 100644
--- a/drivers/media/usb/gspca/topro.c
+++ b/drivers/media/usb/gspca/topro.c
@@ -175,6 +175,8 @@ static const u8 jpeg_q[17] = {
 #error "USB buffer too small"
 #endif
 
+#define DEFAULT_FRAME_RATE 30
+
 static const u8 rates[] = {30, 20, 15, 10, 7, 5};
 static const struct framerates framerates[] = {
 	{
@@ -4020,7 +4022,7 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	gspca_dev->cam.mode_framerates = sd->bridge == BRIDGE_TP6800 ?
 			framerates : framerates_6810;
 
-	sd->framerate = 30;		/* default: 30 fps */
+	sd->framerate = DEFAULT_FRAME_RATE;
 	return 0;
 }
 
@@ -4803,7 +4805,7 @@ static void sd_set_streamparm(struct gspca_dev *gspca_dev,
 	int fr, i;
 
 	if (tpf->numerator == 0 || tpf->denominator == 0)
-		sd->framerate = 30;
+		sd->framerate = DEFAULT_FRAME_RATE;
 	else
 		sd->framerate = tpf->denominator / tpf->numerator;
 
-- 
2.7.0

[PATCH 2/7] [media] gspca: fix setting frame interval type in vidioc_enum_frameintervals()

[Antonio Ospite]

Set the frame _interval_ type to V4L2_FRMIVAL_TYPE_DISCRETE instead of
using V4L2_FRMSIZE_TYPE_DISCRETE which is meant for frame _size_.

The old and new values happen to be the same so there is no functional
change.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 drivers/media/usb/gspca/gspca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index af5cd82..9c8990f 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -1246,7 +1246,7 @@ static int vidioc_enum_frameintervals(struct file *filp, void *priv,
 
 	for (i = 0; i < gspca_dev->cam.mode_framerates[mode].nrates; i++) {
 		if (fival->index == i) {
-			fival->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+			fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
 			fival->discrete.numerator = 1;
 			fival->discrete.denominator =
 				gspca_dev->cam.mode_framerates[mode].rates[i];
-- 
2.7.0

[PATCH 3/7] [media] gspca: rename wxh_to_mode() to wxh_to_nearest_mode()

[Antonio Ospite]

The name wxh_to_nearest_mode() reflects better what the function does.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 drivers/media/usb/gspca/gspca.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 9c8990f..1bb7901 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -991,7 +991,7 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
 	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
 }
 
-static int wxh_to_mode(struct gspca_dev *gspca_dev,
+static int wxh_to_nearest_mode(struct gspca_dev *gspca_dev,
 			int width, int height)
 {
 	int i;
@@ -1125,8 +1125,8 @@ static int try_fmt_vid_cap(struct gspca_dev *gspca_dev,
 	PDEBUG_MODE(gspca_dev, D_CONF, "try fmt cap",
 		    fmt->fmt.pix.pixelformat, w, h);
 
-	/* search the closest mode for width and height */
-	mode = wxh_to_mode(gspca_dev, w, h);
+	/* search the nearest mode for width and height */
+	mode = wxh_to_nearest_mode(gspca_dev, w, h);
 
 	/* OK if right palette */
 	if (gspca_dev->cam.cam_mode[mode].pixelformat
@@ -1233,7 +1233,7 @@ static int vidioc_enum_frameintervals(struct file *filp, void *priv,
 				      struct v4l2_frmivalenum *fival)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(filp);
-	int mode = wxh_to_mode(gspca_dev, fival->width, fival->height);
+	int mode = wxh_to_nearest_mode(gspca_dev, fival->width, fival->height);
 	__u32 i;
 
 	if (gspca_dev->cam.mode_framerates == NULL ||
-- 
2.7.0

[PATCH 4/7] [media] gspca: fix a v4l2-compliance failure about VIDIOC_ENUM_FRAMEINTERVALS

[Antonio Ospite]

According to v4l2-compliance VIDIOC_ENUM_FRAMEINTERVALS should fail for
unsupported frame sizes, but gspca is too tolerant and tries to find
the frame intervals for the frame size nearest to the requested one.

This makes v4l2-compliance fail with this message:

  fail: v4l2-test-formats.cpp(123): \
      found frame intervals for invalid size 321x240
  test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL

Fix this by using an exact match for the frame size when enumerating
frame intervals, and retuning an error if the frame size for which the
frame intervals have been asked is not supported.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 drivers/media/usb/gspca/gspca.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 1bb7901..61cb16d 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -991,6 +991,19 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
 	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
 }
 
+static int wxh_to_mode(struct gspca_dev *gspca_dev,
+			int width, int height)
+{
+	int i;
+
+	for (i = 0; i < gspca_dev->cam.nmodes; i++) {
+		if (width == gspca_dev->cam.cam_mode[i].width
+		    && height == gspca_dev->cam.cam_mode[i].height)
+			return i;
+	}
+	return -EINVAL;
+}
+
 static int wxh_to_nearest_mode(struct gspca_dev *gspca_dev,
 			int width, int height)
 {
@@ -1233,9 +1246,13 @@ static int vidioc_enum_frameintervals(struct file *filp, void *priv,
 				      struct v4l2_frmivalenum *fival)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(filp);
-	int mode = wxh_to_nearest_mode(gspca_dev, fival->width, fival->height);
+	int mode;
 	__u32 i;
 
+	mode = wxh_to_mode(gspca_dev, fival->width, fival->height);
+	if (mode < 0)
+		return -EINVAL;
+
 	if (gspca_dev->cam.mode_framerates == NULL ||
 			gspca_dev->cam.mode_framerates[mode].nrates == 0)
 		return -EINVAL;
-- 
2.7.0

[PATCH 5/7] [media] gspca: fix a v4l2-compliance failure about buffer timestamp

[Antonio Ospite]

v4l2-compliance fails with this message:

  fail: v4l2-test-buffers.cpp(250): \
      timestamp != V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC && \
      timestamp != V4L2_BUF_FLAG_TIMESTAMP_COPY
  ...
  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL

When setting the frame time, gspca uses v4l2_get_timestamp() which uses
ktime_get_ts() which uses ktime_get_ts64() which returns a monotonic
timestamp, so it's safe to initialize the buffer flags to
V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC to fix the failure.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 drivers/media/usb/gspca/gspca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 61cb16d..84b0d6a 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -522,7 +522,7 @@ static int frame_alloc(struct gspca_dev *gspca_dev, struct file *file,
 		frame = &gspca_dev->frame[i];
 		frame->v4l2_buf.index = i;
 		frame->v4l2_buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-		frame->v4l2_buf.flags = 0;
+		frame->v4l2_buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		frame->v4l2_buf.field = V4L2_FIELD_NONE;
 		frame->v4l2_buf.length = frsz;
 		frame->v4l2_buf.memory = memory;
-- 
2.7.0

[PATCH 6/7] [media] gspca: fix a v4l2-compliance failure during VIDIOC_REQBUFS

[Antonio Ospite]

When calling VIDIOC_REQBUFS v4l2-compliance fails with this message:

  fail: v4l2-test-buffers.cpp(476): q.reqbufs(node, 1)
  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL

By looking at the v4l2-compliance code the failure happens when trying
to request V4L2_MEMORY_USERPTR buffers without freeing explicitly the
previously allocated V4L2_MEMORY_MMAP buffers.

This would suggest that when changing the memory field in struct
v4l2_requestbuffers the driver is supposed to free automatically any
previous allocated buffers, and looking for inspiration at the code in
drivers/media/v4l2-core/videobuf2-core.c::vb2_core_reqbufs() seems to
confirm this interpretation; however gspca is just returning -EBUSY in
this case.

Removing the special handling for the case of a different memory value
fixes the compliance failure.

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---

This should be safe, but I'd really like a comment from someone with a more
global knowledge of v4l2.

If my interpretation about how drivers should behave when the value of the
memory field changes is correct, I could send also a documentation update for
Documentation/DocBook/media/v4l/vidioc-reqbufs.xml

Just let me know.

Thanks,
   Antonio


 drivers/media/usb/gspca/gspca.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 84b0d6a..915b6c7 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -1402,13 +1402,6 @@ static int vidioc_reqbufs(struct file *file, void *priv,
 	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
 		return -ERESTARTSYS;
 
-	if (gspca_dev->memory != GSPCA_MEMORY_NO
-	    && gspca_dev->memory != GSPCA_MEMORY_READ
-	    && gspca_dev->memory != rb->memory) {
-		ret = -EBUSY;
-		goto out;
-	}
-
 	/* only one file may do the capture */
 	if (gspca_dev->capt_file != NULL
 	    && gspca_dev->capt_file != file) {
-- 
2.7.0

Re: [PATCH 6/7] [media] gspca: fix a v4l2-compliance failure during VIDIOC_REQBUFS

[Hans Verkuil]

On 03/09/16 17:03, Antonio Ospite wrote:
> When calling VIDIOC_REQBUFS v4l2-compliance fails with this message:
> 
>   fail: v4l2-test-buffers.cpp(476): q.reqbufs(node, 1)
>   test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
> 
> By looking at the v4l2-compliance code the failure happens when trying
> to request V4L2_MEMORY_USERPTR buffers without freeing explicitly the
> previously allocated V4L2_MEMORY_MMAP buffers.
> 
> This would suggest that when changing the memory field in struct
> v4l2_requestbuffers the driver is supposed to free automatically any
> previous allocated buffers, and looking for inspiration at the code in
> drivers/media/v4l2-core/videobuf2-core.c::vb2_core_reqbufs() seems to
> confirm this interpretation; however gspca is just returning -EBUSY in
> this case.
> 
> Removing the special handling for the case of a different memory value
> fixes the compliance failure.
> 
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
> 
> This should be safe, but I'd really like a comment from someone with a more
> global knowledge of v4l2.
> 
> If my interpretation about how drivers should behave when the value of the
> memory field changes is correct, I could send also a documentation update for
> Documentation/DocBook/media/v4l/vidioc-reqbufs.xml

Your interpretation is correct. Calling REQBUFS again should discard the old
buffers and re-allocate new ones. Except, of course, if the old buffers are
in use, then -EBUSY should be returned.

Regards,

	Hans

> 
> Just let me know.
> 
> Thanks,
>    Antonio
> 
> 
>  drivers/media/usb/gspca/gspca.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
> index 84b0d6a..915b6c7 100644
> --- a/drivers/media/usb/gspca/gspca.c
> +++ b/drivers/media/usb/gspca/gspca.c
> @@ -1402,13 +1402,6 @@ static int vidioc_reqbufs(struct file *file, void *priv,
>  	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
>  		return -ERESTARTSYS;
>  
> -	if (gspca_dev->memory != GSPCA_MEMORY_NO
> -	    && gspca_dev->memory != GSPCA_MEMORY_READ
> -	    && gspca_dev->memory != rb->memory) {
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -
>  	/* only one file may do the capture */
>  	if (gspca_dev->capt_file != NULL
>  	    && gspca_dev->capt_file != file) {
> 

Re: [PATCH 6/7] [media] gspca: fix a v4l2-compliance failure during VIDIOC_REQBUFS

[Hans de Goede]

Hi,

On 09-03-16 17:03, Antonio Ospite wrote:
> When calling VIDIOC_REQBUFS v4l2-compliance fails with this message:
>
>    fail: v4l2-test-buffers.cpp(476): q.reqbufs(node, 1)
>    test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
>
> By looking at the v4l2-compliance code the failure happens when trying
> to request V4L2_MEMORY_USERPTR buffers without freeing explicitly the
> previously allocated V4L2_MEMORY_MMAP buffers.
>
> This would suggest that when changing the memory field in struct
> v4l2_requestbuffers the driver is supposed to free automatically any
> previous allocated buffers, and looking for inspiration at the code in
> drivers/media/v4l2-core/videobuf2-core.c::vb2_core_reqbufs() seems to
> confirm this interpretation; however gspca is just returning -EBUSY in
> this case.
>
> Removing the special handling for the case of a different memory value
> fixes the compliance failure.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> This should be safe, but I'd really like a comment from someone with a more
> global knowledge of v4l2.
>
> If my interpretation about how drivers should behave when the value of the
> memory field changes is correct, I could send also a documentation update for
> Documentation/DocBook/media/v4l/vidioc-reqbufs.xml
>
> Just let me know.
>
> Thanks,
>     Antonio
>
>
>   drivers/media/usb/gspca/gspca.c | 7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
> index 84b0d6a..915b6c7 100644
> --- a/drivers/media/usb/gspca/gspca.c
> +++ b/drivers/media/usb/gspca/gspca.c
> @@ -1402,13 +1402,6 @@ static int vidioc_reqbufs(struct file *file, void *priv,
>   	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
>   		return -ERESTARTSYS;
>
> -	if (gspca_dev->memory != GSPCA_MEMORY_NO
> -	    && gspca_dev->memory != GSPCA_MEMORY_READ
> -	    && gspca_dev->memory != rb->memory) {
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -

reqbufs is used internally and this change will allow changing
gspca_dev->memory from USERPTR / MMAP to GSPCA_MEMORY_READ
please replace this check with a check to only allow
rb->memory to be GSPCA_MEMORY_READ when coming from GSPCA_MEMORY_NO
or GSPCA_MEMORY_READ

Regards,

Hans

>   	/* only one file may do the capture */
>   	if (gspca_dev->capt_file != NULL
>   	    && gspca_dev->capt_file != file) {
>

Re: [PATCH 6/7] [media] gspca: fix a v4l2-compliance failure during VIDIOC_REQBUFS

[Antonio Ospite]

On Thu, 10 Mar 2016 15:54:37 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 09-03-16 17:03, Antonio Ospite wrote:
> > When calling VIDIOC_REQBUFS v4l2-compliance fails with this message:
> >
> >    fail: v4l2-test-buffers.cpp(476): q.reqbufs(node, 1)
> >    test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
> >
> > By looking at the v4l2-compliance code the failure happens when trying
> > to request V4L2_MEMORY_USERPTR buffers without freeing explicitly the
> > previously allocated V4L2_MEMORY_MMAP buffers.
> >
> > This would suggest that when changing the memory field in struct
> > v4l2_requestbuffers the driver is supposed to free automatically any
> > previous allocated buffers, and looking for inspiration at the code in
> > drivers/media/v4l2-core/videobuf2-core.c::vb2_core_reqbufs() seems to
> > confirm this interpretation; however gspca is just returning -EBUSY in
> > this case.
> >
> > Removing the special handling for the case of a different memory value
> > fixes the compliance failure.
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >
> > This should be safe, but I'd really like a comment from someone with a more
> > global knowledge of v4l2.
> >
> > If my interpretation about how drivers should behave when the value of the
> > memory field changes is correct, I could send also a documentation update for
> > Documentation/DocBook/media/v4l/vidioc-reqbufs.xml
> >
> > Just let me know.
> >
> > Thanks,
> >     Antonio
> >
> >
> >   drivers/media/usb/gspca/gspca.c | 7 -------
> >   1 file changed, 7 deletions(-)
> >
> > diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
> > index 84b0d6a..915b6c7 100644
> > --- a/drivers/media/usb/gspca/gspca.c
> > +++ b/drivers/media/usb/gspca/gspca.c
> > @@ -1402,13 +1402,6 @@ static int vidioc_reqbufs(struct file *file, void *priv,
> >   	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
> >   		return -ERESTARTSYS;
> >
> > -	if (gspca_dev->memory != GSPCA_MEMORY_NO
> > -	    && gspca_dev->memory != GSPCA_MEMORY_READ
> > -	    && gspca_dev->memory != rb->memory) {
> > -		ret = -EBUSY;
> > -		goto out;
> > -	}
> > -
> 
> reqbufs is used internally and this change will allow changing
> gspca_dev->memory from USERPTR / MMAP to GSPCA_MEMORY_READ
> please replace this check with a check to only allow
> rb->memory to be GSPCA_MEMORY_READ when coming from GSPCA_MEMORY_NO
> or GSPCA_MEMORY_READ
> 

OK, thanks, I'll take a look again later this week.

In the meantime, if patches from 1 to 5 are OK, can we have them merged
so I will just resubmit the last two in the set?

Ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [PATCH 6/7] [media] gspca: fix a v4l2-compliance failure during VIDIOC_REQBUFS

[Hans de Goede]

Hi,

On 14-03-16 16:02, Antonio Ospite wrote:
> On Thu, 10 Mar 2016 15:54:37 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>
>> Hi,
>>
>> On 09-03-16 17:03, Antonio Ospite wrote:
>>> When calling VIDIOC_REQBUFS v4l2-compliance fails with this message:
>>>
>>>     fail: v4l2-test-buffers.cpp(476): q.reqbufs(node, 1)
>>>     test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
>>>
>>> By looking at the v4l2-compliance code the failure happens when trying
>>> to request V4L2_MEMORY_USERPTR buffers without freeing explicitly the
>>> previously allocated V4L2_MEMORY_MMAP buffers.
>>>
>>> This would suggest that when changing the memory field in struct
>>> v4l2_requestbuffers the driver is supposed to free automatically any
>>> previous allocated buffers, and looking for inspiration at the code in
>>> drivers/media/v4l2-core/videobuf2-core.c::vb2_core_reqbufs() seems to
>>> confirm this interpretation; however gspca is just returning -EBUSY in
>>> this case.
>>>
>>> Removing the special handling for the case of a different memory value
>>> fixes the compliance failure.
>>>
>>> Signed-off-by: Antonio Ospite <ao2@ao2.it>
>>> ---
>>>
>>> This should be safe, but I'd really like a comment from someone with a more
>>> global knowledge of v4l2.
>>>
>>> If my interpretation about how drivers should behave when the value of the
>>> memory field changes is correct, I could send also a documentation update for
>>> Documentation/DocBook/media/v4l/vidioc-reqbufs.xml
>>>
>>> Just let me know.
>>>
>>> Thanks,
>>>      Antonio
>>>
>>>
>>>    drivers/media/usb/gspca/gspca.c | 7 -------
>>>    1 file changed, 7 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
>>> index 84b0d6a..915b6c7 100644
>>> --- a/drivers/media/usb/gspca/gspca.c
>>> +++ b/drivers/media/usb/gspca/gspca.c
>>> @@ -1402,13 +1402,6 @@ static int vidioc_reqbufs(struct file *file, void *priv,
>>>    	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
>>>    		return -ERESTARTSYS;
>>>
>>> -	if (gspca_dev->memory != GSPCA_MEMORY_NO
>>> -	    && gspca_dev->memory != GSPCA_MEMORY_READ
>>> -	    && gspca_dev->memory != rb->memory) {
>>> -		ret = -EBUSY;
>>> -		goto out;
>>> -	}
>>> -
>>
>> reqbufs is used internally and this change will allow changing
>> gspca_dev->memory from USERPTR / MMAP to GSPCA_MEMORY_READ
>> please replace this check with a check to only allow
>> rb->memory to be GSPCA_MEMORY_READ when coming from GSPCA_MEMORY_NO
>> or GSPCA_MEMORY_READ
>>
>
> OK, thanks, I'll take a look again later this week.
>
> In the meantime, if patches from 1 to 5 are OK, can we have them merged
> so I will just resubmit the last two in the set?

Not sure when I'll have time to do this, I would prefer to also take
v2 of patch 6 and 7 while at it. But I agree that there is no need
to pick op patches 1 - 5. I'll pick them up from patchwork when I've
time.

Regards,

Hans

[PATCH 7/7] [media] gspca: fix a v4l2-compliance failure during read()

[Antonio Ospite]

v4l2-compliance fails with this message:

  fail: v4l2-test-buffers.cpp(512): Expected EBUSY, got 22
  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL

Looking at the v4l2-compliance code reveals that this failure is about
the read() callback.

In gspca, dev_read() is calling vidioc_dqbuf() which calls
frame_ready_nolock() but the latter returns -EINVAL in a case when
v4l2-compliance expects -EBUSY.

Fix the failure by changing the return value in frame_ready_nolock().

Signed-off-by: Antonio Ospite <ao2@ao2.it>
---
 drivers/media/usb/gspca/gspca.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 915b6c7..de7e300 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -1664,7 +1664,7 @@ static int frame_ready_nolock(struct gspca_dev *gspca_dev, struct file *file,
 		return -ENODEV;
 	if (gspca_dev->capt_file != file || gspca_dev->memory != memory ||
 			!gspca_dev->streaming)
-		return -EINVAL;
+		return -EBUSY;
 
 	/* check if a frame is ready */
 	return gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i);
-- 
2.7.0

Re: [PATCH 7/7] [media] gspca: fix a v4l2-compliance failure during read()

[Hans de Goede]

Hi,

On 09-03-16 17:03, Antonio Ospite wrote:
> v4l2-compliance fails with this message:
>
>    fail: v4l2-test-buffers.cpp(512): Expected EBUSY, got 22
>    test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
>
> Looking at the v4l2-compliance code reveals that this failure is about
> the read() callback.
>
> In gspca, dev_read() is calling vidioc_dqbuf() which calls
> frame_ready_nolock() but the latter returns -EINVAL in a case when
> v4l2-compliance expects -EBUSY.
>
> Fix the failure by changing the return value in frame_ready_nolock().
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>   drivers/media/usb/gspca/gspca.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
> index 915b6c7..de7e300 100644
> --- a/drivers/media/usb/gspca/gspca.c
> +++ b/drivers/media/usb/gspca/gspca.c
> @@ -1664,7 +1664,7 @@ static int frame_ready_nolock(struct gspca_dev *gspca_dev, struct file *file,
>   		return -ENODEV;
>   	if (gspca_dev->capt_file != file || gspca_dev->memory != memory ||
>   			!gspca_dev->streaming)
> -		return -EINVAL;
> +		return -EBUSY;
>
>   	/* check if a frame is ready */
>   	return gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i);

I'm not sure that this is the right fix:


1) !gspca_dev->streaming should result in -EINVAL, this is the same as what videobuf2 is doing
2) gspca_dev->memory != memory should result in -EINVAL
3) gspca_dev->capt_file != file means calling dqbuf without having done reqbufs (through the same fd)
    which certainly seemes like -EINVAL to me.

The actual problem is that dev_read() is not catching that mmap is being in use:

static ssize_t dev_read(struct file *file, char __user *data,
                     size_t count, loff_t *ppos)
{
...
         if (gspca_dev->memory == GSPCA_MEMORY_NO) { /* first time ? */
                 ret = read_alloc(gspca_dev, file);
                 if (ret != 0)
                         return ret;
         }

It will skip the read_alloc since gspca_dev->memory is USERPTR or MMAP
and then do a dqbuf with memory == GSPCA_MEMORY_READ, triggering the
gspca_dev->memory != memory check.

There are a couple of issues here:

1) gspca_dev->memory check without holding usb_lock, the taking and
releasing of usb_lock should be moved from read_alloc() into dev_read()
itself.

2) dev_read() should not assume that reading is ok if
    gspca_dev->memory == GSPCA_MEMORY_NO, it needs a:

if (gspca_dev->memory != GSPCA_MEMORY_NO &&
     gspca_dev->memory != GSPCA_MEMORY_READ)
     return -EBUSY;

(while holding the usb_lock so the above is wrong)

3) If gspca_dev->memory == GSPCA_MEMORY_READ already the
    stream could have been stopped. so we need to check
    gspca_dev->streaming (while holding the usb_lock)
    and do a streamon if it is not set (and then we can
    remove the streamon from read_alloc())

So TL;DR: dev_read needs some love.

Regards,

Hans


p.s.

If you've time to work on v4l2 stuff what gspca really needs
is to just have its buffer handling ripped out and be rewritten
to use videobuf2. I would certainly love to see a patch for that.

Re: [PATCH 7/7] [media] gspca: fix a v4l2-compliance failure during read()

[Antonio Ospite]

On Thu, 10 Mar 2016 16:59:45 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 09-03-16 17:03, Antonio Ospite wrote:
> > v4l2-compliance fails with this message:
> >
> >    fail: v4l2-test-buffers.cpp(512): Expected EBUSY, got 22
> >    test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
> >
> > Looking at the v4l2-compliance code reveals that this failure is about
> > the read() callback.
> >
> > In gspca, dev_read() is calling vidioc_dqbuf() which calls
> > frame_ready_nolock() but the latter returns -EINVAL in a case when
> > v4l2-compliance expects -EBUSY.
> >
> > Fix the failure by changing the return value in frame_ready_nolock().
> >
> > Signed-off-by: Antonio Ospite <ao2@ao2.it>
> > ---
> >   drivers/media/usb/gspca/gspca.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
> > index 915b6c7..de7e300 100644
> > --- a/drivers/media/usb/gspca/gspca.c
> > +++ b/drivers/media/usb/gspca/gspca.c
> > @@ -1664,7 +1664,7 @@ static int frame_ready_nolock(struct gspca_dev *gspca_dev, struct file *file,
> >   		return -ENODEV;
> >   	if (gspca_dev->capt_file != file || gspca_dev->memory != memory ||
> >   			!gspca_dev->streaming)
> > -		return -EINVAL;
> > +		return -EBUSY;
> >
> >   	/* check if a frame is ready */
> >   	return gspca_dev->fr_o != atomic_read(&gspca_dev->fr_i);
> 
> I'm not sure that this is the right fix:
>
> 
> 1) !gspca_dev->streaming should result in -EINVAL, this is the same as what videobuf2 is doing
> 2) gspca_dev->memory != memory should result in -EINVAL
> 3) gspca_dev->capt_file != file means calling dqbuf without having done reqbufs (through the same fd)
>     which certainly seemes like -EINVAL to me.
> 
> The actual problem is that dev_read() is not catching that mmap is being in use:
> 
> static ssize_t dev_read(struct file *file, char __user *data,
>                      size_t count, loff_t *ppos)
> {
> ...
>          if (gspca_dev->memory == GSPCA_MEMORY_NO) { /* first time ? */
>                  ret = read_alloc(gspca_dev, file);
>                  if (ret != 0)
>                          return ret;
>          }
> 
> It will skip the read_alloc since gspca_dev->memory is USERPTR or MMAP
> and then do a dqbuf with memory == GSPCA_MEMORY_READ, triggering the
> gspca_dev->memory != memory check.
> 
> There are a couple of issues here:
> 
> 1) gspca_dev->memory check without holding usb_lock, the taking and
> releasing of usb_lock should be moved from read_alloc() into dev_read()
> itself.
> 
> 2) dev_read() should not assume that reading is ok if
>     gspca_dev->memory == GSPCA_MEMORY_NO, it needs a:
> 
> if (gspca_dev->memory != GSPCA_MEMORY_NO &&
>      gspca_dev->memory != GSPCA_MEMORY_READ)
>      return -EBUSY;
> 
> (while holding the usb_lock so the above is wrong)
> 
> 3) If gspca_dev->memory == GSPCA_MEMORY_READ already the
>     stream could have been stopped. so we need to check
>     gspca_dev->streaming (while holding the usb_lock)
>     and do a streamon if it is not set (and then we can
>     remove the streamon from read_alloc())
> 
> So TL;DR: dev_read needs some love.
>

I'll try to take a look at this too later this week.

> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> If you've time to work on v4l2 stuff what gspca really needs
> is to just have its buffer handling ripped out and be rewritten
> to use videobuf2. I would certainly love to see a patch for that.
> 

It'd be an interesting tasklet but I don't know when I'll be able to do
that.

Ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?