diff mbox

uvcvideo: logitech C920 resets controls during VIDIOC_STREAMON

Message ID 5317ACAC.8000008@williammanley.net (mailing list archive)
State Superseded, archived
Delegated to: Laurent Pinchart
Headers

Commit Message

William Manley March 5, 2014, 11:01 p.m. UTC 
  Hi All

I've been attempting to use the Logitech C920 with the uvcvideo driver.
 I set the controls with v4l2-ctl but some of them change during
VIDIOC_STREAMON.  My understanding is that the values of controls should
be preserved.

Minimal test case:

    #include <linux/videodev2.h>
    #include <stdio.h>
    #include <stdlib.h>
    #include <fcntl.h>
    #include <errno.h>
    #include <string.h>
    #include <unistd.h>
    #include <sys/ioctl.h>

    #define DEVICE "/dev/video2"

    int main()
    {
            int fd, type;

            fd = open(DEVICE, O_RDWR | O_CLOEXEC);
            if (fd < 0) {
                    perror("Failed to open " DEVICE "\n");
                    return 1;
            }

            struct v4l2_requestbuffers reqbuf = {
                    .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
                    .memory = V4L2_MEMORY_MMAP,
                    .count = 1,
            };

            if (-1 == ioctl (fd, VIDIOC_REQBUFS, &reqbuf)) {
                    perror("VIDIOC_REQBUFS");
                    return 1;
            }

            system("v4l2-ctl -d" DEVICE " -l | grep exposure_absolute");

            type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
            if (ioctl (fd, VIDIOC_STREAMON, &type) != 0) {
                    perror("VIDIOC_STREAMON");
                    return 1;
            }

            printf("VIDIOC_STREAMON\n");

            usleep(100000);
            system("v4l2-ctl -d" DEVICE " -l | grep exposure_absolute");

            return 0;
    }

None of the other controls seem to be affected.  Note: to get the C920
to report exposure_absolute correctly I also had to make this change to
the uvcvideo kernel driver:



At this point the backtrace looks something like:

    uvc_init_video
    uvc_video_enable
    uvc_v4l2_do_ioctl (in the case VIDIOC_STREAMON:)

The call to uvc_ctrl_resume_device() has the effect that the v4l2 ctrls
which are cached in the kernel get resubmitted to the camera as if it
were coming out of suspend/resume.

I've looked at the wireshark capture of USB traffic and I can't find
anywhere where the host causes exposure_auto to change.  The camera does
have a mode where it would change by itself but that is disabled
(exposure_auto=1).  I've uploaded the wireshark trace to:

http://williammanley.net/usb-wireshark-streamon.pcapng

I'm guessing that this is a hardware bug.  One fix would be modify the
driver to save all values at the beginning of STREAMON and then restore
them at the end.  What do you think?

Thanks

Will
--
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

Comments

Paulo Assis March 6, 2014, 12:09 p.m. UTC | #1 
Hi,

2014-03-05 23:01 GMT+00:00 William Manley <will@williammanley.net>:
> Hi All
>
> I've been attempting to use the Logitech C920 with the uvcvideo driver.
>  I set the controls with v4l2-ctl but some of them change during
> VIDIOC_STREAMON.  My understanding is that the values of controls should
> be preserved.
>
> Minimal test case:
>
>     #include <linux/videodev2.h>
>     #include <stdio.h>
>     #include <stdlib.h>
>     #include <fcntl.h>
>     #include <errno.h>
>     #include <string.h>
>     #include <unistd.h>
>     #include <sys/ioctl.h>
>
>     #define DEVICE "/dev/video2"
>
>     int main()
>     {
>             int fd, type;
>
>             fd = open(DEVICE, O_RDWR | O_CLOEXEC);
>             if (fd < 0) {
>                     perror("Failed to open " DEVICE "\n");
>                     return 1;
>             }
>
>             struct v4l2_requestbuffers reqbuf = {
>                     .type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
>                     .memory = V4L2_MEMORY_MMAP,
>                     .count = 1,
>             };
>
>             if (-1 == ioctl (fd, VIDIOC_REQBUFS, &reqbuf)) {
>                     perror("VIDIOC_REQBUFS");
>                     return 1;
>             }
>
>             system("v4l2-ctl -d" DEVICE " -l | grep exposure_absolute");
>
>             type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>             if (ioctl (fd, VIDIOC_STREAMON, &type) != 0) {
>                     perror("VIDIOC_STREAMON");
>                     return 1;
>             }
>
>             printf("VIDIOC_STREAMON\n");
>
>             usleep(100000);
>             system("v4l2-ctl -d" DEVICE " -l | grep exposure_absolute");
>
>             return 0;
>     }
>
> None of the other controls seem to be affected.  Note: to get the C920
> to report exposure_absolute correctly I also had to make this change to
> the uvcvideo kernel driver:
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c
> index a2f4501..e7c805b 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -227,7 +227,8 @@ static struct uvc_control_info uvc_ctrls[] = {
>                 .size           = 4,
>                 .flags          = UVC_CTRL_FLAG_SET_CUR
>                                 | UVC_CTRL_FLAG_GET_RANGE
> -                               | UVC_CTRL_FLAG_RESTORE,
> +                               | UVC_CTRL_FLAG_RESTORE
> +                               | UVC_CTRL_FLAG_AUTO_UPDATE,
>         },
>         {
>                 .entity         = UVC_GUID_UVC_CAMERA,
>
>
> The variables seem to be changed when the URBs are Submitted.  To
> investigate I made the following change to the uvc driver:
>
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c
> index 3394c34..f2f66f6 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1649,17 +1649,23 @@ static int uvc_init_video(struct uvc_streaming
> *stream, gfp_t gfp_flags)
>         if (ret < 0)
>                 return ret;
>
> +       /* No effect: */
> +       uvc_ctrl_resume_device(stream->dev);
> +
>         /* Submit the URBs. */
>         for (i = 0; i < UVC_URBS; ++i) {
>                 ret = usb_submit_urb(stream->urb[i], gfp_flags);
>                 if (ret < 0) {
>                         uvc_printk(KERN_ERR, "Failed to submit URB %u "
>                                         "(%d).\n", i, ret);
>                         uvc_uninit_video(stream, 1);
>                         return ret;
>                 }
>         }
>
> +       /* "Fixes" the issue: */
> +       uvc_ctrl_resume_device(stream->dev);
> +
>         return 0;
>  }
>
>
> At this point the backtrace looks something like:
>
>     uvc_init_video
>     uvc_video_enable
>     uvc_v4l2_do_ioctl (in the case VIDIOC_STREAMON:)
>
> The call to uvc_ctrl_resume_device() has the effect that the v4l2 ctrls
> which are cached in the kernel get resubmitted to the camera as if it
> were coming out of suspend/resume.
>
> I've looked at the wireshark capture of USB traffic and I can't find
> anywhere where the host causes exposure_auto to change.  The camera does
> have a mode where it would change by itself but that is disabled
> (exposure_auto=1).

This alone doesn't guarantee that exposure absolute is untouched.
To do so you need to set V4L2_CID_EXPOSURE_AUTO_PRIORITY to 0 and
V4L2_CID_EXPOSURE_AUTO to V4L2_EXPOSURE_MANUAL

> I've uploaded the wireshark trace to:
>
> http://williammanley.net/usb-wireshark-streamon.pcapng
>
> I'm guessing that this is a hardware bug.  One fix would be modify the
> driver to save all values at the beginning of STREAMON and then restore
> them at the end.  What do you think?
>
> Thanks
>
> Will
> --
> 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

Regards,
Paulo
--
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
William Manley March 6, 2014, 1:04 p.m. UTC | #2 
On 06/03/14 12:09, Paulo Assis wrote:
> Hi,
> 
> 2014-03-05 23:01 GMT+00:00 William Manley <will@williammanley.net>:
>> Hi All
>>
>> I've been attempting to use the Logitech C920 with the uvcvideo driver.
>>  I set the controls with v4l2-ctl but some of them change during
>> VIDIOC_STREAMON.  My understanding is that the values of controls should
>> be preserved.
>>

[snip]

>> The camera does
>> have a mode where it would change by itself but that is disabled
>> (exposure_auto=1).
> 
> This alone doesn't guarantee that exposure absolute is untouched.
> To do so you need to set V4L2_CID_EXPOSURE_AUTO_PRIORITY to 0 and
> V4L2_CID_EXPOSURE_AUTO to V4L2_EXPOSURE_MANUAL

The behaviour is the same whether exposure_auto_priority is set to 1 or
0.  I don't think I've explained myself clearly - the exposure time does
not change by itself, apart from during VIDIOC_STREAMON:

* When I'm streaming video from the camera it's constant.
* When no data is coming from the camera it's constant

It's only modified during STREAMON and when I explicitly set it with
v4l2-ctl.

This doesn't seem like correct behaviour as it breaks the use-case of
setting the controls as you want before starting streaming.  My
workaround is to reset all the controls after calling VIDIOC_STREAMON
but this is ugly and you get a few frames at the beginning of the video
stream where the settings are set correctly.

Thanks

Will
--
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
William Manley March 7, 2014, 3:36 p.m. UTC | #3 
On 06/03/14 13:04, William Manley wrote:
> On 06/03/14 12:09, Paulo Assis wrote:
>> Hi,
>>
>> 2014-03-05 23:01 GMT+00:00 William Manley <will@williammanley.net>:
>>> Hi All
>>>
>>> I've been attempting to use the Logitech C920 with the uvcvideo driver.
>>>  I set the controls with v4l2-ctl but some of them change during
>>> VIDIOC_STREAMON.  My understanding is that the values of controls should
>>> be preserved.
>>>
> 
> [snip]
> 
>>> The camera does
>>> have a mode where it would change by itself but that is disabled
>>> (exposure_auto=1).
>>
>> This alone doesn't guarantee that exposure absolute is untouched.
>> To do so you need to set V4L2_CID_EXPOSURE_AUTO_PRIORITY to 0 and
>> V4L2_CID_EXPOSURE_AUTO to V4L2_EXPOSURE_MANUAL
> 
> The behaviour is the same whether exposure_auto_priority is set to 1 or
> 0.  I don't think I've explained myself clearly - the exposure time does
> not change by itself, apart from during VIDIOC_STREAMON:
> 
> * When I'm streaming video from the camera it's constant.
> * When no data is coming from the camera it's constant
> 
> It's only modified during STREAMON and when I explicitly set it with
> v4l2-ctl.
> 
> This doesn't seem like correct behaviour as it breaks the use-case of
> setting the controls as you want before starting streaming.  My
> workaround is to reset all the controls after calling VIDIOC_STREAMON
> but this is ugly and you get a few frames at the beginning of the video
> stream where the settings are set correctly.

It would be really useful for me to hear definitively if this is
expected behaviour WRT the v4l2 API (in which case I'll fix the bug in
GStreamer) or a bug in the C920/uvcvideo driver (in which case I'll work
further to fix the bug in v4l).

To me it feels like it should be fixed here but I'd like to know so I
don't waste my time coming up with patches.

Thanks

Will
--
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 mbox

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
b/drivers/media/usb/uvc/uvc_ctrl.c
index a2f4501..e7c805b 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -227,7 +227,8 @@  static struct uvc_control_info uvc_ctrls[] = {
                .size           = 4,
                .flags          = UVC_CTRL_FLAG_SET_CUR
                                | UVC_CTRL_FLAG_GET_RANGE
-                               | UVC_CTRL_FLAG_RESTORE,
+                               | UVC_CTRL_FLAG_RESTORE
+                               | UVC_CTRL_FLAG_AUTO_UPDATE,
        },
        {
                .entity         = UVC_GUID_UVC_CAMERA,


The variables seem to be changed when the URBs are Submitted.  To
investigate I made the following change to the uvc driver:


diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 3394c34..f2f66f6 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1649,17 +1649,23 @@  static int uvc_init_video(struct uvc_streaming
*stream, gfp_t gfp_flags)
        if (ret < 0)
                return ret;

+       /* No effect: */
+       uvc_ctrl_resume_device(stream->dev);
+
        /* Submit the URBs. */
        for (i = 0; i < UVC_URBS; ++i) {
                ret = usb_submit_urb(stream->urb[i], gfp_flags);
                if (ret < 0) {
                        uvc_printk(KERN_ERR, "Failed to submit URB %u "
                                        "(%d).\n", i, ret);
                        uvc_uninit_video(stream, 1);
                        return ret;
                }
        }

+       /* "Fixes" the issue: */
+       uvc_ctrl_resume_device(stream->dev);
+
        return 0;
 }