uvcvideo: add quirk to force Phytec CAM 004H to GBRG

This patch adds a quirk to force Phytec CAM 004H to format GBRG because it is announcing its format wrong.

Signed-off-by Christoph Fritz <[email protected]>

Tested-by Norbert Wesp <[email protected]>

---
 drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 17 insertions(+)

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644

drivers/media/usb/uvc/uvc_driver.c +16 -0

@@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device *dev,
 				width_multiplier = 2;
 			}
 		}
+		if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
+			if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
+				strlcpy(format->name, "GBRG Bayer (GBRG)",
+					sizeof(format->name));
+				format->fcc = V4L2_PIX_FMT_SGBRG8;
+			}
+		}
 
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
 			ftype = UVC_VS_FRAME_UNCOMPRESSED;

@@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0 },
+	/* PHYTEC CAM 004H cameras */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x199e,
+	  .idProduct		= 0x8302,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_FORCE_GBRG },
 	/* Bodelin ProScopeHR */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 

drivers/media/usb/uvc/uvc_driver.c +0 -0

 				| USB_DEVICE_ID_MATCH_DEV_HI

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644

drivers/media/usb/uvc/uvcvideo.h +0 -0

 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001

Could you please send me the output of 'lsusb -d 199e:8302 -v' (if possible running as root) ?

Quoted 43 lines

Signed-off-by Christoph Fritz <[email protected]>

Tested-by Norbert Wesp <[email protected]>

---
 drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 17 insertions(+)

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644

drivers/media/usb/uvc/uvc_driver.c +16 -0

@@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device *dev,
 				width_multiplier = 2;
 			}
 		}
+		if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
+			if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
+				strlcpy(format->name, "GBRG Bayer (GBRG)",
+					sizeof(format->name));
+				format->fcc = V4L2_PIX_FMT_SGBRG8;
+			}
+		}
 
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
 			ftype = UVC_VS_FRAME_UNCOMPRESSED;

@@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0 },
+	/* PHYTEC CAM 004H cameras */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x199e,
+	  .idProduct		= 0x8302,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_FORCE_GBRG },
 	/* Bodelin ProScopeHR */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 

| USB_DEVICE_ID_MATCH_DEV_HI

drivers/media/usb/uvc/uvcvideo.h line 167

Quoted 9 lines

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644

drivers/media/usb/uvc/uvcvideo.h +1 -0

@@ -164,6 +164,7 @@
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 #define UVC_QUIRK_FORCE_Y8		0x00000800
+#define UVC_QUIRK_FORCE_GBRG		0x00001000

I don't think we should add a quirk flag for every format that needs to be forced. Instead, now that we have a new way to store per-device parameters since commit 3bc85817d798 ("media: uvcvideo: Add extensible device information"), how about making use of it and adding a field to the uvc_device_info structure to store the forced format ?

Hi Laurent

Quoted 2 lines

Could you please send me the output of 'lsusb -d 199e:8302 -v' (if possible running as root) ?

please see bottom of this mail

Quoted 50 lines

Quoted 50 lines

Signed-off-by Christoph Fritz <[email protected]>

Tested-by Norbert Wesp <[email protected]>

---

 drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 17 insertions(+)

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644

drivers/media/usb/uvc/uvc_driver.c +7 -0

@@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device *dev,
 				width_multiplier = 2;
 			}
 		}
 
+		if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
+			if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
+				strlcpy(format->name, "GBRG Bayer (GBRG)",
+					sizeof(format->name));
+				format->fcc = V4L2_PIX_FMT_SGBRG8;
+			}
+		}
 
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {

drivers/media/usb/uvc/uvc_driver.c +9 -0

 			ftype = UVC_VS_FRAME_UNCOMPRESSED;

@@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0 },
+	/* PHYTEC CAM 004H cameras */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x199e,
+	  .idProduct		= 0x8302,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_FORCE_GBRG },
 	/* Bodelin ProScopeHR */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_DEV_HI

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644

drivers/media/usb/uvc/uvcvideo.h +0 -0

@@ -164,6 +164,7 @@
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400

Quoted 8 lines

drivers/media/usb/uvc/uvcvideo.h line 167

Quoted 2 lines

drivers/media/usb/uvc/uvcvideo.h +1 -0

 #define UVC_QUIRK_FORCE_Y8		0x00000800
+#define UVC_QUIRK_FORCE_GBRG		0x00001000

I don't think we should add a quirk flag for every format that needs to be forced. Instead, now that we have a new way to store per-device parameters since commit 3bc85817d798 ("media: uvcvideo: Add extensible device information"), how about making use of it and adding a field to the uvc_device_info structure to store the forced format ?

I'm currently stuck on kernel 4.9, but for mainline I'll use extensible device information and send a v2 in the next weeks.

output of 'lsusb -d 199e:8302 -v':

Bus 001 Device 010: ID 199e:8302 The Imaging Source Europe GmbH Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2 ?
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
idVendor 0x199e The Imaging Source Europe GmbH
idProduct 0x8302
bcdDevice 8.13

  iManufacturer           2 ?
  iProduct                1 ?
  iSerial                 3 ?
  bNumConfigurations      1

Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 349
bNumInterfaces 2
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Association:
bLength 8
bDescriptorType 11
bFirstInterface 0
bInterfaceCount 2
bFunctionClass 14 Video
bFunctionSubClass 3 Video Interface Collection
bFunctionProtocol 0
iFunction 0
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 1 Video Control
bInterfaceProtocol 0
iInterface 0
VideoControl Interface Descriptor:
bLength 13
bDescriptorType 36
bDescriptorSubtype 1 (HEADER)
bcdUVC 1.00
wTotalLength 78
dwClockFrequency 6.000000MHz
bInCollection 1
baInterfaceNr( 0) 1
VideoControl Interface Descriptor:
bLength 18
bDescriptorType 36
bDescriptorSubtype 2 (INPUT_TERMINAL)
bTerminalID 1
wTerminalType 0x0201 Camera Sensor
bAssocTerminal 0
iTerminal 0
wObjectiveFocalLengthMin 0
wObjectiveFocalLengthMax 0
wOcularFocalLength 0
bControlSize 3
bmControls 0x0004001a
Auto-Exposure Mode
Exposure Time (Absolute)
Exposure Time (Relative)
Privacy
VideoControl Interface Descriptor:
bLength 12
bDescriptorType 36
bDescriptorSubtype 5 (PROCESSING_UNIT)
Warning: Descriptor too short
bUnitID 3
bSourceID 1
wMaxMultiplier 0
bControlSize 3
bmControls 0x00000200
Gain
iProcessing 0
bmVideoStandards 0x1a
NTSC - 525/60
SECAM - 625/50
NTSC - 625/50
VideoControl Interface Descriptor:
bLength 26
bDescriptorType 36
bDescriptorSubtype 6 (EXTENSION_UNIT)
bUnitID 4
guidExtensionCode {95d63e92-f256-e111-83f7-37704824019b}
bNumControl 0
bNrPins 1
baSourceID( 0) 3
bControlSize 1
bmControls( 0) 0x0f
iExtension 0
VideoControl Interface Descriptor:
bLength 9
bDescriptorType 36
bDescriptorSubtype 3 (OUTPUT_TERMINAL)
bTerminalID 2
wTerminalType 0x0101 USB Streaming
bAssocTerminal 0
bSourceID 4
iTerminal 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 3
Transfer Type Interrupt
Synch Type None
Usage Type Data
wMaxPacketSize 0x0008 1x 8 bytes
bInterval 9
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 14 Video
bInterfaceSubClass 2 Video Streaming
bInterfaceProtocol 0
iInterface 0
VideoStreaming Interface Descriptor:
bLength 15
bDescriptorType 36
bDescriptorSubtype 1 (INPUT_HEADER)
Warning: Descriptor too short
bNumFormats 3
wTotalLength 169
bEndPointAddress 130
bmInfo 0
bTerminalLink 2
bStillCaptureMethod 2
bTriggerSupport 1
bTriggerUsage 0
bControlSize 2
bmaControls( 0) 5
bmaControls( 1) 5
bmaControls( 2) 5

VideoStreaming Interface Descriptor:
bLength 28
bDescriptorType 36
bDescriptorSubtype 16 (FORMAT_FRAME_BASED)
bFormatIndex 1
bNumFrameDescriptors 5
guidFormat
{47524247-0000-1000-8000-00aa00389b71}

bBitsPerPixel 8
bDefaultFrameIndex 1
bAspectRatioX 16
bAspectRatioY 9
bmInterlaceFlags 0x86
Interlaced stream or variable: No
Fields per frame: 1 fields
Field 1 first: Yes
Field pattern: Field 1 only
bCopyProtect 0
bVariableSize 0
VideoStreaming Interface Descriptor:
bLength 42
bDescriptorType 36
bDescriptorSubtype 17 (FRAME_FRAME_BASED)
bFrameIndex 1
bmCapabilities 0x00
Still image unsupported
wWidth 744
wHeight 480
dwMinBitRate 4294950144
dwMaxBitRate 4294964224
dwDefaultFrameInterval 333333
bFrameIntervalType 4
dwBytesPerLine 744
dwFrameInterval( 0) 166667
dwFrameInterval( 1) 333333
dwFrameInterval( 2) 400000
dwFrameInterval( 3) 666667
VideoStreaming Interface Descriptor:
bLength 42
bDescriptorType 36
bDescriptorSubtype 17 (FRAME_FRAME_BASED)
bFrameIndex 4
bmCapabilities 0x00
Still image unsupported
wWidth 640
wHeight 480
dwMinBitRate 4294946816
dwMaxBitRate 4294942720
dwDefaultFrameInterval 333333
bFrameIntervalType 4
dwBytesPerLine 640
dwFrameInterval( 0) 166667
dwFrameInterval( 1) 333333
dwFrameInterval( 2) 400000
dwFrameInterval( 3) 666667
VideoStreaming Interface Descriptor:
bLength 6
bDescriptorType 36
bDescriptorSubtype 13 (COLORFORMAT)
bColorPrimaries 0 (Unspecified)
bTransferCharacteristics 0 (Unspecified)
bMatrixCoefficients 0 (Unspecified)
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 1
** UNRECOGNIZED: 02 23
** UNRECOGNIZED: 20 ac 75 10 02 75 11 e8 02 1d 4b 12 20 5e 7d 40 7c
01 7f 04 12 28 33 7d f0 7c 00 7f 03 12 28 33 Device Qualifier (for other device speed):
bLength 10
bDescriptorType 6

bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2 ?
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
bNumConfigurations 1
Device Status: 0x0000
(Bus Powered)

Quoted 88 lines · On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote:

Hi Laurent

Quoted 2 lines

Could you please send me the output of 'lsusb -d 199e:8302 -v' (if possible running as root) ?

please see bottom of this mail

Quoted 50 lines

Quoted 50 lines

Signed-off-by Christoph Fritz <[email protected]>

Tested-by Norbert Wesp <[email protected]>

---

 drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 17 insertions(+)

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644

drivers/media/usb/uvc/uvc_driver.c +7 -0

@@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device
 *dev,
 				width_multiplier = 2;
 			}
 		}
+		if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
+			if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
+				strlcpy(format->name, "GBRG Bayer (GBRG)",
+					sizeof(format->name));
+				format->fcc = V4L2_PIX_FMT_SGBRG8;
+			}
+		}
 
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {

drivers/media/usb/uvc/uvc_driver.c +9 -0

 			ftype = UVC_VS_FRAME_UNCOMPRESSED;

@@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0 },
+	/* PHYTEC CAM 004H cameras */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x199e,
+	  .idProduct		= 0x8302,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_FORCE_GBRG },
 	/* Bodelin ProScopeHR */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_DEV_HI

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644

drivers/media/usb/uvc/uvcvideo.h +0 -0

@@ -164,6 +164,7 @@
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400

Quoted 8 lines

drivers/media/usb/uvc/uvcvideo.h line 167

Quoted 2 lines

drivers/media/usb/uvc/uvcvideo.h +1 -0

 #define UVC_QUIRK_FORCE_Y8		0x00000800
+#define UVC_QUIRK_FORCE_GBRG		0x00001000

I don't think we should add a quirk flag for every format that needs to be forced. Instead, now that we have a new way to store per-device parameters since commit 3bc85817d798 ("media: uvcvideo: Add extensible device information"), how about making use of it and adding a field to the uvc_device_info structure to store the forced format ?

I'm currently stuck on kernel 4.9, but for mainline I'll use extensible device information and send a v2 in the next weeks.

output of 'lsusb -d 199e:8302 -v':

Bus 001 Device 010: ID 199e:8302 The Imaging Source Europe GmbH

Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 239 Miscellaneous Device
bDeviceSubClass 2 ?
bDeviceProtocol 1 Interface Association
bMaxPacketSize0 64
idVendor 0x199e The Imaging Source Europe GmbH
idProduct 0x8302
bcdDevice 8.13

  iManufacturer           2 ?
  iProduct                1 ?
  iSerial                 3 ?
  bNumConfigurations      1

Quoted 8 lines

VideoStreaming Interface Descriptor:
bLength 28
bDescriptorType 36
bDescriptorSubtype 16 (FORMAT_FRAME_BASED)
bFormatIndex 1
bNumFrameDescriptors 5
guidFormat
{47524247-0000-1000-8000-00aa00389b71}

I only know of two devices supporting this format, this one and the DFx 72BUC02. The only two devices I'm aware of that use other Bayer formats are the DFK 23UV024 and DFK 23UX249, and they both report a GBRG format.

I wonder if the DFx 72BUC02 is affected too. In any case, as your device exposes a single format, you can force the format to V4L2_PIX_FMT_SGBRG8 without first checking whether it is equal to V4L2_PIX_FMT_SGRBG8. Feel free to patch the existing FORCE_Y8 code to also remove the format check, I've discussed it today with Philipp Zabel who implemented the FORCE_Y8 quirk and he's fine with that.

Quoted 47 lines · On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote:

Quoted 46 lines

Quoted 46 lines

Quoted 46 lines

 drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 17 insertions(+)

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644

drivers/media/usb/uvc/uvc_driver.c +7 -0

@@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device
 *dev,
 				width_multiplier = 2;
 			}
 		}
+		if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
+			if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
+				strlcpy(format->name, "GBRG Bayer (GBRG)",
+					sizeof(format->name));
+				format->fcc = V4L2_PIX_FMT_SGBRG8;
+			}
+		}
 
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {

drivers/media/usb/uvc/uvc_driver.c +9 -0

 			ftype = UVC_VS_FRAME_UNCOMPRESSED;

@@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0 },
+	/* PHYTEC CAM 004H cameras */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x199e,
+	  .idProduct		= 0x8302,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_FORCE_GBRG },
 	/* Bodelin ProScopeHR */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_DEV_HI

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644

drivers/media/usb/uvc/uvcvideo.h +0 -0

@@ -164,6 +164,7 @@
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/media/usb/uvc/uvcvideo.h line 167

Quoted 2 lines

drivers/media/usb/uvc/uvcvideo.h +1 -0

 #define UVC_QUIRK_FORCE_Y8		0x00000800
+#define UVC_QUIRK_FORCE_GBRG		0x00001000

I don't think we should add a quirk flag for every format that needs to be forced. Instead, now that we have a new way to store per-device parameters since commit 3bc85817d798 ("media: uvcvideo: Add extensible device information"), how about making use of it and adding a field to the uvc_device_info structure to store the forced format ?

Quoted 48 lines · On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote:

Quoted 47 lines · On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote:

Quoted 46 lines

Quoted 46 lines

Quoted 46 lines

 drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 17 insertions(+)

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644

drivers/media/usb/uvc/uvc_driver.c +7 -0

@@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device
 *dev,
 				width_multiplier = 2;
 			}
 		}
+		if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
+			if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
+				strlcpy(format->name, "GBRG Bayer (GBRG)",
+					sizeof(format->name));
+				format->fcc = V4L2_PIX_FMT_SGBRG8;
+			}
+		}
 
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {

drivers/media/usb/uvc/uvc_driver.c +9 -0

 			ftype = UVC_VS_FRAME_UNCOMPRESSED;

@@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0 },
+	/* PHYTEC CAM 004H cameras */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x199e,
+	  .idProduct		= 0x8302,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_FORCE_GBRG },
 	/* Bodelin ProScopeHR */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_DEV_HI

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644

drivers/media/usb/uvc/uvcvideo.h +0 -0

@@ -164,6 +164,7 @@
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/media/usb/uvc/uvcvideo.h line 167

Quoted 2 lines

drivers/media/usb/uvc/uvcvideo.h +1 -0

 #define UVC_QUIRK_FORCE_Y8		0x00000800
+#define UVC_QUIRK_FORCE_GBRG		0x00001000

I don't think we should add a quirk flag for every format that needs to be forced. Instead, now that we have a new way to store per-device parameters since commit 3bc85817d798 ("media: uvcvideo: Add extensible device information"), how about making use of it and adding a field to the uvc_device_info structure to store the forced format ?

you mean something like:

 struct uvc_device_info {
        u32     quirks;
+       u32     forced_color_format;
        u32     meta_format;

};

and

(quoted diff) +3 -0

+static const struct uvc_device_info uvc_forced_color_sgbrg8 = {
+       .forced_color_format = V4L2_PIX_FMT_SGBRG8,
+};

and

(quoted diff) +1 -1

@@ -2817,7 +2820,7 @@ static const struct usb_device_id uvc_ids[] = {
          .bInterfaceClass      = USB_CLASS_VENDOR_SPEC,
          .bInterfaceSubClass   = 1,
          .bInterfaceProtocol   = 0,
-         .driver_info          = (kernel_ulong_t)&uvc_quirk_force_y8 },
+         .driver_info          = (kernel_ulong_t)&uvc_forced_color_y8 },
 

?

If yes:

- there would be a need for forced_color_format in struct uvc_device

- module-parameter quirk would not test force color format any more
- the actual force/quirk changes not only format->fcc:

                if (dev->forced_color_format == V4L2_PIX_FMT_SGBRG8) {

                        strlcpy(format->name, "Greyscale 8-bit (Y8  )",
                                sizeof(format->name));

forced_color_format.

                        format->fcc = dev->forced_color_format;
                        format->bpp = 8;
                        width_multiplier = 2;

                }

Is this the way you want me to go?

Hi Christoph,

(Philipp, there's a question for you at the end)

Quoted 80 lines · On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote:

Quoted 48 lines · On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote:

Quoted 47 lines · On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote:

Quoted 46 lines

Quoted 46 lines

Quoted 46 lines

 drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 17 insertions(+)

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644

drivers/media/usb/uvc/uvc_driver.c +7 -0

@@ -406,6 +406,13 @@ static int uvc_parse_format(struct uvc_device
 *dev,
 				width_multiplier = 2;
 			}
 		}
+		if (dev->quirks & UVC_QUIRK_FORCE_GBRG) {
+			if (format->fcc == V4L2_PIX_FMT_SGRBG8) {
+				strlcpy(format->name, "GBRG Bayer (GBRG)",
+					sizeof(format->name));
+				format->fcc = V4L2_PIX_FMT_SGBRG8;
+			}
+		}
 
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {

drivers/media/usb/uvc/uvc_driver.c +9 -0

 			ftype = UVC_VS_FRAME_UNCOMPRESSED;

@@ -2631,6 +2638,15 @@ static struct usb_device_id uvc_ids[] = {
 	  .bInterfaceClass	= USB_CLASS_VENDOR_SPEC,
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0 },
+	/* PHYTEC CAM 004H cameras */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x199e,
+	  .idProduct		= 0x8302,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_QUIRK_FORCE_GBRG },
 	/* Bodelin ProScopeHR */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_DEV_HI

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644

drivers/media/usb/uvc/uvcvideo.h +0 -0

@@ -164,6 +164,7 @@
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/media/usb/uvc/uvcvideo.h line 167

Quoted 2 lines

drivers/media/usb/uvc/uvcvideo.h +1 -0

 #define UVC_QUIRK_FORCE_Y8		0x00000800
+#define UVC_QUIRK_FORCE_GBRG		0x00001000

I don't think we should add a quirk flag for every format that needs to be forced. Instead, now that we have a new way to store per-device parameters since commit 3bc85817d798 ("media: uvcvideo: Add extensible device information"), how about making use of it and adding a field to the uvc_device_info structure to store the forced format ?

you mean something like:

 struct uvc_device_info {
        u32     quirks;
+       u32     forced_color_format;
        u32     meta_format;

};

and

(quoted diff) +3 -0

+static const struct uvc_device_info uvc_forced_color_sgbrg8 = {
+       .forced_color_format = V4L2_PIX_FMT_SGBRG8,
+};

and

(quoted diff) +1 -1

@@ -2817,7 +2820,7 @@ static const struct usb_device_id uvc_ids[] = {
          .bInterfaceClass      = USB_CLASS_VENDOR_SPEC,
          .bInterfaceSubClass   = 1,
          .bInterfaceProtocol   = 0,
-         .driver_info          = (kernel_ulong_t)&uvc_quirk_force_y8 },
+         .driver_info          = (kernel_ulong_t)&uvc_forced_color_y8 },
 

Quoted 1 line

?

With an additional

static const struct uvc_device_info uvc_forced_color_y8 = {
.forced_color_format = V4L2_PIX_FMT_GREY,
};

Quoted 3 lines

If yes:

- there would be a need for forced_color_format in struct uvc_device

Why so ?

Quoted 5 lines

- module-parameter quirk would not test force color format any more
- the actual force/quirk changes not only format->fcc:

                if (dev->forced_color_format == V4L2_PIX_FMT_SGBRG8) {

The test should be if (dev->forced_color_format) to cover both the Y8 and SGBRG8 cases.

Quoted 2 lines

                        strlcpy(format->name, "Greyscale 8-bit (Y8  )",
                                sizeof(format->name));

You can get the name from the uvc_fmts entry corresponding to dev-

Quoted 5 lines

forced_color_format.

                        format->fcc = dev->forced_color_format;
                        format->bpp = 8;
                        width_multiplier = 2;

bpp and multiplier are more annoying. bpp is a property of the format, which we could add to the uvc_fmts array.

I believe the multiplier could be computed by device bpp / bpp from uvc_fmts. That would work at least for the Oculus VR Positional Tracker DK2, but I don't have the Oculus VR Rift Sensor descriptors to check that. Philipp, if you still have access to the device, could you send that to me ?

Quoted 3 lines

                }

Is this the way you want me to go?

Quoted 3 lines · Laurent Pinchart

Quoted 2 lines · On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote:

Quoted 2 lines · On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote:

Quoted 1 line · On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote:

Quoted 49 lines

Quoted 41 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644

drivers/media/usb/uvc/uvcvideo.h +0 -0

@@ -164,6 +164,7 @@
 #define UVC_QUIRK_RESTRICT_FRAME_RATE	0x00000200
 #define UVC_QUIRK_RESTORE_CTRLS_ON_INIT	0x00000400
 #define UVC_QUIRK_FORCE_Y8		0x00000800

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/media/usb/uvc/uvcvideo.h line 167

Quoted 1 line

drivers/media/usb/uvc/uvcvideo.h +1 -0

+#define UVC_QUIRK_FORCE_GBRG		0x00001000

I don't think we should add a quirk flag for every format that needs to be forced. Instead, now that we have a new way to store per-device parameters since commit 3bc85817d798 ("media: uvcvideo:
Add extensible device information"), how about making use of it and adding a field to the uvc_device_info structure to store the forced format ?

you mean something like:

 struct uvc_device_info {
        u32     quirks;
+       u32     forced_color_format;
        u32     meta_format;

};

and

(quoted diff) +3 -0

+static const struct uvc_device_info uvc_forced_color_sgbrg8 = {
+       .forced_color_format = V4L2_PIX_FMT_SGBRG8,
+};

and

(quoted diff) +1 -1

@@ -2817,7 +2820,7 @@ static const struct usb_device_id uvc_ids[] = {
          .bInterfaceClass      = USB_CLASS_VENDOR_SPEC,
          .bInterfaceSubClass   = 1,
          .bInterfaceProtocol   = 0,
-         .driver_info          = (kernel_ulong_t)&uvc_quirk_force_y8 },
+         .driver_info          = (kernel_ulong_t)&uvc_forced_color_y8
 },
 

Quoted 1 line

?

With an additional

static const struct uvc_device_info uvc_forced_color_y8 = {
.forced_color_format = V4L2_PIX_FMT_GREY,
};

Quoted 4 lines

Quoted 2 lines

If yes:
- there would be a need for forced_color_format in struct uvc_device

Why so ?

What is the alternative?
Is there a better way to access "uvc_device_info->forced_color_format" from within function uvc_parse_format()?

Quoted 30 lines

Quoted 4 lines

- module-parameter quirk would not test force color format any more

- the actual force/quirk changes not only format->fcc:

                if (dev->forced_color_format == V4L2_PIX_FMT_SGBRG8) {

The test should be if (dev->forced_color_format) to cover both the Y8 and SGBRG8 cases.

Quoted 2 lines

                        strlcpy(format->name, "Greyscale 8-bit (Y8  )",
                                sizeof(format->name));

You can get the name from the uvc_fmts entry corresponding to dev-> forced_color_format.

Quoted 4 lines

                        format->fcc = dev->forced_color_format;
                        format->bpp = 8;
                        width_multiplier = 2;

bpp and multiplier are more annoying. bpp is a property of the format, which we could add to the uvc_fmts array.

I believe the multiplier could be computed by device bpp / bpp from uvc_fmts. That would work at least for the Oculus VR Positional Tracker DK2, but I don't have the Oculus VR Rift Sensor descriptors to check that. Philipp, if you still have access to the device, could you send that to me ?

Quoted 3 lines

                }

Is this the way you want me to go?

Hi Christoph,

(Philipp, there's a question for you at the end)

On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote:

Quoted 3 lines

                        format->fcc = dev->forced_color_format;
                        format->bpp = 8;
                        width_multiplier = 2;

bpp and multiplier are more annoying. bpp is a property of the format, which we could add to the uvc_fmts array.

I believe the multiplier could be computed by device bpp / bpp from uvc_fmts. That would work at least for the Oculus VR Positional Tracker DK2, but I don't have the Oculus VR Rift Sensor descriptors to check that. Philipp, if you still have access to the device, could you send that to me ?

Am Donnerstag, den 16.08.2018, 19:39 +0300 schrieb Laurent Pinchart:

Quoted 5 lines

Hi Christoph,

(Philipp, there's a question for you at the end)

On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote:

[...]

Quoted 4 lines

Quoted 3 lines

                        format->fcc = dev->forced_color_format;
                        format->bpp = 8;
                        width_multiplier = 2;

Quoted 8 lines

bpp and multiplier are more annoying. bpp is a property of the format, which we could add to the uvc_fmts array.

I believe the multiplier could be computed by device bpp / bpp from uvc_fmts. That would work at least for the Oculus VR Positional Tracker DK2, but I don't have the Oculus VR Rift Sensor descriptors to check that. Philipp, if you still have access to the device, could you send that to me ?

Full lsusb -v output below, the UVC descriptors are not decoded because bFunctionClass is set to 255. The YUY2 uncompressed format descriptor looks like this:

               ___guidFormat__________________________________

1b 24 04 01 04 59 55 59 32 00 00 10 00 80 00 00 aa 00 38 9b 71 10 01 00 00 00 00 ^^
so, bBitsPerPixel == 16.