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

2016-03-14 Thread 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  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 
---

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(_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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-03-14 Thread Antonio Ospite 
On Thu, 10 Mar 2016 15:54:37 +0100
Hans de Goede  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 
> > ---
> >
> > 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(_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?
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-03-10 Thread 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 
---

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(_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) {


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-03-09 Thread 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 
> ---
> 
> 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(_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) {
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-03-09 Thread 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 
---

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(_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

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html