Message ID | 1457539401-11515-7-git-send-email-ao2@ao2.it (mailing list archive) |
---|---|
State | Changes Requested, archived |
Delegated to: | Hans de Goede |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84) (envelope-from <linux-media-owner@vger.kernel.org>) id 1adgaa-0008AP-Fn; Wed, 09 Mar 2016 16:03:56 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.76/mailfrontend-6) with esmtp id 1adgaY-0007yh-4s; Wed, 09 Mar 2016 17:03:56 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933172AbcCIQDm (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 9 Mar 2016 11:03:42 -0500 Received: from smtp207.alice.it ([82.57.200.103]:53521 "EHLO smtp207.alice.it" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933171AbcCIQDm (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 9 Mar 2016 11:03:42 -0500 Received: from jcn (87.3.192.69) by smtp207.alice.it (8.6.060.28) id 562CAA691CD6DB37; Wed, 9 Mar 2016 17:03:26 +0100 Received: from ao2 by jcn with local (Exim 4.86_2) (envelope-from <ao2@ao2.it>) id 1adga8-00030s-Bc; Wed, 09 Mar 2016 17:03:28 +0100 From: Antonio Ospite <ao2@ao2.it> To: Linux Media <linux-media@vger.kernel.org> Cc: Hans de Goede <hdegoede@redhat.com>, Hans Verkuil <hverkuil@xs4all.nl>, Antonio Ospite <ao2@ao2.it> Subject: [PATCH 6/7] [media] gspca: fix a v4l2-compliance failure during VIDIOC_REQBUFS Date: Wed, 9 Mar 2016 17:03:20 +0100 Message-Id: <1457539401-11515-7-git-send-email-ao2@ao2.it> X-Mailer: git-send-email 2.7.0 In-Reply-To: <1457539401-11515-1-git-send-email-ao2@ao2.it> References: <1457539401-11515-1-git-send-email-ao2@ao2.it> X-Face: z*RaLf`X<@C75u6Ig9}{oW$H; 1_\2t5)({*|jhM<pyWR#k60!#=#>/Vb; ]yA5<GWI5`6u&+ ; 6b'@y|8w"wB; 4/e!7wYYrcqdJFY,~%Gk_4]cq$Ei/7<j&N3ah(m`ku?pX.&+~:_/wC~dwn^)MizBG !pE^+iDQQ1yC6^,)YDKkxDd!T>\I~93>J<_`<4)A{':UrE Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2016.3.9.160018 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_2000_2999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, NO_URI_HTTPS 0, REFERENCES 0, SINGLE_URI_IN_BODY 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __REFERENCES 0, __SANE_MSGID 0, __SINGLE_URI_TEXT 0, __TO_MALFORMED_2 0, __URI_IN_BODY 0, __URI_NO_WWW 0, __URI_NS , __URI_WITH_PATH 0' |
Commit Message
Antonio Ospite
March 9, 2016, 4:03 p.m. UTC
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(-)
Comments
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) { > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) { > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {