Hi Philipp,

Thank you for the patch.

On Friday, 5 January 2018 00:51:29 EET Philipp Zabel wrote:
> The Microsoft HoloLens Sensors device has two separate frame descriptors
> with the same dimensions, each with a single different frame interval:
> 
>       VideoStreaming Interface Descriptor:
>         bLength                            30
>         bDescriptorType                    36
>         bDescriptorSubtype                  5 (FRAME_UNCOMPRESSED)
>         bFrameIndex                         1
>         bmCapabilities                   0x00
>           Still image unsupported
>         wWidth                           1280
>         wHeight                           481
>         dwMinBitRate                147763200
>         dwMaxBitRate                147763200
>         dwMaxVideoFrameBufferSize      615680
>         dwDefaultFrameInterval         333333
>         bFrameIntervalType                  1
>         dwFrameInterval( 0)            333333
>       VideoStreaming Interface Descriptor:
>         bLength                            30
>         bDescriptorType                    36
>         bDescriptorSubtype                  5 (FRAME_UNCOMPRESSED)
>         bFrameIndex                         2
>         bmCapabilities                   0x00
>           Still image unsupported
>         wWidth                           1280
>         wHeight                           481
>         dwMinBitRate                443289600
>         dwMaxBitRate                443289600
>         dwMaxVideoFrameBufferSize      615680
>         dwDefaultFrameInterval         111111
>         bFrameIntervalType                  1
>         dwFrameInterval( 0)            111111
> 
> Skip duplicate dimensions in enum_framesizes, let enum_frameintervals list
> the intervals from both frame descriptors. Change set_streamparm to switch
> to the correct frame index when changing the interval. This enables 90 fps
> capture on a Lenovo Explorer Windows Mixed Reality headset.
> 
> Signed-off-by: Philipp Zabel <philipp.za...@gmail.com>
> ---
> Changes since v1 [1]:
> - Break out of frame size loop if maxd == 0 in uvc_v4l2_set_streamparm.
> - Moved d and tmp variables in uvc_v4l2_set_streamparm into loop,
>   renamed tmp variable to tmp_ival.
> - Changed i loop variables to unsigned int.
> - Changed index variables to unsigned int.
> - One line per variable declaration.
> 
> [1] https://patchwork.linuxtv.org/patch/46109/
> ---
>  drivers/media/usb/uvc/uvc_v4l2.c | 71
> +++++++++++++++++++++++++++++++--------- 1 file changed, 55 insertions(+),
> 16 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index f5ab8164bca5..d9ee400bf47c 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -373,7 +373,10 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming
> *stream, {
>       struct uvc_streaming_control probe;
>       struct v4l2_fract timeperframe;
> -     uint32_t interval;
> +     struct uvc_format *format;
> +     struct uvc_frame *frame;
> +     __u32 interval, maxd;
> +     unsigned int i;
>       int ret;
> 
>       if (parm->type != stream->type)
> @@ -396,9 +399,33 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming
> *stream, return -EBUSY;
>       }
> 
> +     format = stream->cur_format;
> +     frame = stream->cur_frame;
>       probe = stream->ctrl;
> -     probe.dwFrameInterval =
> -             uvc_try_frame_interval(stream->cur_frame, interval);
> +     probe.dwFrameInterval = uvc_try_frame_interval(frame, interval);
> +     maxd = abs((__s32)probe.dwFrameInterval - interval);
> +
> +     /* Try frames with matching size to find the best frame interval. */
> +     for (i = 0; i < format->nframes && maxd != 0; i++) {
> +             __u32 d, tmp_ival;

How about ival instead of tmp_ival ?

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>

If you're fine with the change there's no need to resubmit.

> +
> +             if (&format->frame[i] == stream->cur_frame)
> +                     continue;
> +
> +             if (format->frame[i].wWidth != stream->cur_frame->wWidth ||
> +                 format->frame[i].wHeight != stream->cur_frame->wHeight)
> +                     continue;
> +
> +             tmp_ival = uvc_try_frame_interval(&format->frame[i], interval);
> +             d = abs((__s32)tmp_ival - interval);
> +             if (d >= maxd)
> +                     continue;
> +
> +             frame = &format->frame[i];
> +             probe.bFrameIndex = frame->bFrameIndex;
> +             probe.dwFrameInterval = tmp_ival;
> +             maxd = d;
> +     }
> 
>       /* Probe the device with the new settings. */
>       ret = uvc_probe_video(stream, &probe);
> @@ -408,6 +435,7 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming
> *stream, }
> 
>       stream->ctrl = probe;
> +     stream->cur_frame = frame;
>       mutex_unlock(&stream->mutex);
> 
>       /* Return the actual frame period. */
> @@ -1209,7 +1237,8 @@ static int uvc_ioctl_enum_framesizes(struct file
> *file, void *fh, struct uvc_streaming *stream = handle->stream;
>       struct uvc_format *format = NULL;
>       struct uvc_frame *frame;
> -     int i;
> +     unsigned int index;
> +     unsigned int i;
> 
>       /* Look for the given pixel format */
>       for (i = 0; i < stream->nformats; i++) {
> @@ -1221,10 +1250,20 @@ static int uvc_ioctl_enum_framesizes(struct file
> *file, void *fh, if (format == NULL)
>               return -EINVAL;
> 
> -     if (fsize->index >= format->nframes)
> +     /* Skip duplicate frame sizes */
> +     for (i = 0, index = 0; i < format->nframes; i++) {
> +             if (i && frame->wWidth == format->frame[i].wWidth &&
> +                 frame->wHeight == format->frame[i].wHeight)
> +                     continue;
> +             frame = &format->frame[i];
> +             if (index == fsize->index)
> +                     break;
> +             index++;
> +     }
> +
> +     if (i == format->nframes)
>               return -EINVAL;
> 
> -     frame = &format->frame[fsize->index];
>       fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>       fsize->discrete.width = frame->wWidth;
>       fsize->discrete.height = frame->wHeight;
> @@ -1238,7 +1277,9 @@ static int uvc_ioctl_enum_frameintervals(struct file
> *file, void *fh, struct uvc_streaming *stream = handle->stream;
>       struct uvc_format *format = NULL;
>       struct uvc_frame *frame = NULL;
> -     int i;
> +     unsigned int nintervals;
> +     unsigned int index;
> +     unsigned int i;
> 
>       /* Look for the given pixel format and frame size */
>       for (i = 0; i < stream->nformats; i++) {
> @@ -1250,30 +1291,28 @@ static int uvc_ioctl_enum_frameintervals(struct file
> *file, void *fh, if (format == NULL)
>               return -EINVAL;
> 
> +     index = fival->index;
>       for (i = 0; i < format->nframes; i++) {
>               if (format->frame[i].wWidth == fival->width &&
>                   format->frame[i].wHeight == fival->height) {
>                       frame = &format->frame[i];
> -                     break;
> +                     nintervals = frame->bFrameIntervalType ?: 1;
> +                     if (index < nintervals)
> +                             break;
> +                     index -= nintervals;
>               }
>       }
> -     if (frame == NULL)
> +     if (i == format->nframes)
>               return -EINVAL;
> 
>       if (frame->bFrameIntervalType) {
> -             if (fival->index >= frame->bFrameIntervalType)
> -                     return -EINVAL;
> -
>               fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>               fival->discrete.numerator =
> -                     frame->dwFrameInterval[fival->index];
> +                     frame->dwFrameInterval[index];
>               fival->discrete.denominator = 10000000;
>               uvc_simplify_fraction(&fival->discrete.numerator,
>                       &fival->discrete.denominator, 8, 333);
>       } else {
> -             if (fival->index)
> -                     return -EINVAL;
> -
>               fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
>               fival->stepwise.min.numerator = frame->dwFrameInterval[0];
>               fival->stepwise.min.denominator = 10000000;

-- 
Regards,

Laurent Pinchart

Reply via email to