Re: [PATCH v2] [media] uvcvideo: Add quirk to force the Oculus DK2 IR tracker to grayscale

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Philipp,

On Monday 29 September 2014 21:38:39 Philipp Zabel wrote:
> On Wed, Aug 6, 2014 at 10:50 PM, Philipp Zabel wrote:
> > This patch adds a quirk to force Y8 pixel format even if the camera
> > reports half-width YUYV.
> > 
> > Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> 
> do you have any further comments on this patch?

Sorry for the late reply. Please see below for a couple of small comments.

> > ---
> > 
> >  drivers/media/usb/uvc/uvc_driver.c | 29 ++++++++++++++++++++++++++++-
> >  drivers/media/usb/uvc/uvcvideo.h   |  1 +
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c
> > b/drivers/media/usb/uvc/uvc_driver.c index c3bb250..90a8f10 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -311,6 +311,7 @@ static int uvc_parse_format(struct uvc_device *dev,
> >         struct uvc_format_desc *fmtdesc;
> >         struct uvc_frame *frame;
> >         const unsigned char *start = buffer;
> > +       bool force_yuy2_to_y8 = false;

To keep things a bit more generic, how about an unsigned int width_multiplier 
initialized to 1 and set to 2 when the quirk applies ?

> >         unsigned int interval;
> >         unsigned int i, n;
> >         __u8 ftype;
> > @@ -333,6 +334,22 @@ static int uvc_parse_format(struct uvc_device *dev,
> > 
> >                 /* Find the format descriptor from its GUID. */
> >                 fmtdesc = uvc_format_by_guid(&buffer[5]);
> > +               format->bpp = buffer[21];
> > +
> > +               if (dev->quirks & UVC_QUIRK_FORCE_Y8) {
> > +                       if (fmtdesc && fmtdesc->fcc == V4L2_PIX_FMT_YUYV
> > &&
> > +                           format->bpp == 16) {

I wonder if that check is really needed, all YUYV formats should have 16bpp.

> > +                               force_yuy2_to_y8 = true;
> > +                               fmtdesc = &uvc_fmts[9];

The hardcoded index here is hair-raising :-) How about something like the 
following instead ?

 		}
 
 		format->bpp = buffer[21];
+
+		/* Some devices report a format that doesn't match what they
+		 * really send.
+		 */
+		if (dev->quirks & UVC_QUIRK_FORCE_Y8) {
+			if (format->fcc == V4L2_PIX_FMT_YUYV) {
+				strlcpy(format->name, "Greyscale 8-bit (Y8  )",
+					sizeof(format->name));
+				format->fcc = V4L2_PIX_FMT_GREY;
+				format->bpp = 8;
+				width_multiplier = 2;
+			}
+		}
+
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
 			ftype = UVC_VS_FRAME_UNCOMPRESSED;
 		} else {

I know it duplicates the format string, but as we're trying to move them to 
the V4L2 core anyway, I don't see that as being a big problem.


> > +                               format->bpp = 8;
> > +                       } else {
> > +                               uvc_printk(KERN_WARNING,
> > +                                       "Forcing %d-bit %s to %s not
> > supported",
> > +                                       format->bpp, fmtdesc->name,
> > +                                       uvc_fmts[9].name);
> > +                       }
> > +               }
> > +
> >                 if (fmtdesc != NULL) {
> >                         strlcpy(format->name, fmtdesc->name,
> >                                 sizeof format->name);
> > @@ -345,7 +362,6 @@ static int uvc_parse_format(struct uvc_device *dev,
> >                         format->fcc = 0;
> >                 }
> > 
> > -               format->bpp = buffer[21];
> >                 if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
> >                         ftype = UVC_VS_FRAME_UNCOMPRESSED;
> >                 } else {
> > @@ -455,6 +471,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> >                 frame->bFrameIndex = buffer[3];
> >                 frame->bmCapabilities = buffer[4];
> >                 frame->wWidth = get_unaligned_le16(&buffer[5]);
> > +               if (force_yuy2_to_y8)
> > +                       frame->wWidth *= 2;

This would become

+		frame->wWidth = get_unaligned_le16(&buffer[5])
+			      * width_multiplier;

If you're fine with that there's no need to resubmit, I'll modify the patch 
when applying it to my tree.

> >                 frame->wHeight = get_unaligned_le16(&buffer[7]);
> >                 frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
> >                 frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
> > @@ -2467,6 +2485,15 @@ static struct usb_device_id uvc_ids[] = {
> >           .bInterfaceProtocol   = 0,
> >           .driver_info          = UVC_QUIRK_PROBE_MINMAX
> >                                 | UVC_QUIRK_IGNORE_SELECTOR_UNIT },
> > +       /* Oculus VR Positional Tracker DK2 */
> > +       { .match_flags          = USB_DEVICE_ID_MATCH_DEVICE
> > +                               | USB_DEVICE_ID_MATCH_INT_INFO,
> > +         .idVendor             = 0x2833,
> > +         .idProduct            = 0x0201,
> > +         .bInterfaceClass      = USB_CLASS_VIDEO,
> > +         .bInterfaceSubClass   = 1,
> > +         .bInterfaceProtocol   = 0,
> > +         .driver_info          = UVC_QUIRK_FORCE_Y8 },
> >         /* Generic USB Video Class */
> >         { USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, 0) },
> >         {}
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > b/drivers/media/usb/uvc/uvcvideo.h index 9e35982..1dd78c0 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -137,6 +137,7 @@
> >  #define UVC_QUIRK_FIX_BANDWIDTH                0x00000080
> >  #define UVC_QUIRK_PROBE_DEF            0x00000100
> >  #define UVC_QUIRK_RESTRICT_FRAME_RATE  0x00000200
> > +#define UVC_QUIRK_FORCE_Y8             0x00000400
> > 
> >  /* Format flags */
> >  #define UVC_FMT_FLAG_COMPRESSED                0x00000001

-- 
Regards,

Laurent Pinchart