| Message ID | 1519212389.11643.13.camel@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Christoph, Thank you for the patch. On Wednesday, 21 February 2018 13:26:29 EET Christoph Fritz wrote: > This patch adds a quirk to force Phytec CAM 004H to format GBRG because > it is announcing its format wrong. Could you please send me the output of 'lsusb -d 199e:8302 -v' (if possible running as root) ? > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > Tested-by: Norbert Wesp <n.wesp@phytec.de> > --- > drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++ > drivers/media/usb/uvc/uvcvideo.h | 1 + > 2 files changed, 17 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c > b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -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 > > diff --git a/drivers/media/usb/uvc/uvcvideo.h > b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -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 ? > /* Format flags */ > #define UVC_FMT_FLAG_COMPRESSED 0x00000001
Hi Laurent > Could you please send me the output of 'lsusb -d 199e:8302 -v' (if possible > running as root) ? please see bottom of this mail > > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > > Tested-by: Norbert Wesp <n.wesp@phytec.de> > > --- > > drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++ > > drivers/media/usb/uvc/uvcvideo.h | 1 + > > 2 files changed, 17 insertions(+) > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c > > b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644 > > --- a/drivers/media/usb/uvc/uvc_driver.c > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > @@ -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 > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h > > b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -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 ? 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) Thanks -- Christoph
Hi Christoph, (CC'ing Philipp Zabel) On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote: > Hi Laurent > > > Could you please send me the output of 'lsusb -d 199e:8302 -v' (if > > possible running as root) ? > > please see bottom of this mail > > >> Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > >> Tested-by: Norbert Wesp <n.wesp@phytec.de> > >> --- > >> > >> drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++ > >> drivers/media/usb/uvc/uvcvideo.h | 1 + > >> 2 files changed, 17 insertions(+) > >> > >> diff --git a/drivers/media/usb/uvc/uvc_driver.c > >> b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644 > >> --- a/drivers/media/usb/uvc/uvc_driver.c > >> +++ b/drivers/media/usb/uvc/uvc_driver.c > >> @@ -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 > >> diff --git a/drivers/media/usb/uvc/uvcvideo.h > >> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 > >> --- a/drivers/media/usb/uvc/uvcvideo.h > >> +++ b/drivers/media/usb/uvc/uvcvideo.h > >> @@ -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 ? > > 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} 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. > 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) Thank you.
Hi Christoph, On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote: > On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote: > > Hi Laurent > > > >> Could you please send me the output of 'lsusb -d 199e:8302 -v' (if > >> possible running as root) ? > > > > please see bottom of this mail > > > >>> Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > >>> Tested-by: Norbert Wesp <n.wesp@phytec.de> > >>> --- > >>> > >>> drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++ > >>> drivers/media/usb/uvc/uvcvideo.h | 1 + > >>> 2 files changed, 17 insertions(+) > >>> > >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c > >>> b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644 > >>> --- a/drivers/media/usb/uvc/uvc_driver.c > >>> +++ b/drivers/media/usb/uvc/uvc_driver.c > >>> @@ -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 > >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h > >>> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 > >>> --- a/drivers/media/usb/uvc/uvcvideo.h > >>> +++ b/drivers/media/usb/uvc/uvcvideo.h > >>> @@ -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 ? > > > > 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 [snip] > > 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. Any update on a v2 for this patch ?
Hi Laurent > On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote: > > On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote: > > >>> drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++ > > >>> drivers/media/usb/uvc/uvcvideo.h | 1 + > > >>> 2 files changed, 17 insertions(+) > > >>> > > >>> diff --git a/drivers/media/usb/uvc/uvc_driver.c > > >>> b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644 > > >>> --- a/drivers/media/usb/uvc/uvc_driver.c > > >>> +++ b/drivers/media/usb/uvc/uvc_driver.c > > >>> @@ -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 > > >>> diff --git a/drivers/media/usb/uvc/uvcvideo.h > > >>> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 > > >>> --- a/drivers/media/usb/uvc/uvcvideo.h > > >>> +++ b/drivers/media/usb/uvc/uvcvideo.h > > >>> @@ -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 ? you mean something like: struct uvc_device_info { u32 quirks; + u32 forced_color_format; u32 meta_format; }; and +static const struct uvc_device_info uvc_forced_color_sgbrg8 = { + .forced_color_format = V4L2_PIX_FMT_SGBRG8, +}; and @@ -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)); format->fcc = dev->forced_color_format; format->bpp = 8; width_multiplier = 2; } Is this the way you want me to go? Thanks -- Christoph
Hi Christoph, (Philipp, there's a question for you at the end) On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote: > > On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote: > >> On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote: > >>>>> drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++ > >>>>> drivers/media/usb/uvc/uvcvideo.h | 1 + > >>>>> 2 files changed, 17 insertions(+) > >>>>> > >>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c > >>>>> b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644 > >>>>> --- a/drivers/media/usb/uvc/uvc_driver.c > >>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c > >>>>> @@ -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 > >>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h > >>>>> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 > >>>>> --- a/drivers/media/usb/uvc/uvcvideo.h > >>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h > >>>>> @@ -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 ? > > you mean something like: > > struct uvc_device_info { > u32 quirks; > + u32 forced_color_format; > u32 meta_format; > }; > > and > > +static const struct uvc_device_info uvc_forced_color_sgbrg8 = { > + .forced_color_format = V4L2_PIX_FMT_SGBRG8, > +}; > > and > > @@ -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 }, > > ? With an additional static const struct uvc_device_info uvc_forced_color_y8 = { .forced_color_format = V4L2_PIX_FMT_GREY, }; > If yes: > > - there would be a need for forced_color_format in struct uvc_device Why so ? > - 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. > 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. > 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 ? > } > > Is this the way you want me to go?
On Thu, 2018-08-16 at 19:39 +0300, Laurent Pinchart wrote: > Hi Christoph, > > (Philipp, there's a question for you at the end) > > On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote: > > > On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote: > > >> On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote: > > >>>>> drivers/media/usb/uvc/uvc_driver.c | 16 ++++++++++++++++ > > >>>>> drivers/media/usb/uvc/uvcvideo.h | 1 + > > >>>>> 2 files changed, 17 insertions(+) > > >>>>> > > >>>>> diff --git a/drivers/media/usb/uvc/uvc_driver.c > > >>>>> b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644 > > >>>>> --- a/drivers/media/usb/uvc/uvc_driver.c > > >>>>> +++ b/drivers/media/usb/uvc/uvc_driver.c > > >>>>> @@ -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 > > >>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h > > >>>>> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 > > >>>>> --- a/drivers/media/usb/uvc/uvcvideo.h > > >>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h > > >>>>> @@ -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 ? > > > > you mean something like: > > > > struct uvc_device_info { > > u32 quirks; > > + u32 forced_color_format; > > u32 meta_format; > > }; > > > > and > > > > +static const struct uvc_device_info uvc_forced_color_sgbrg8 = { > > + .forced_color_format = V4L2_PIX_FMT_SGBRG8, > > +}; > > > > and > > > > @@ -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 }, > > > > ? > > With an additional > > static const struct uvc_device_info uvc_forced_color_y8 = { > .forced_color_format = V4L2_PIX_FMT_GREY, > }; > > > > 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()? > > > - 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. > > > 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. > > > 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 ? > > > } > > > > Is this the way you want me to go? >
Hi Christoph, On Friday, 17 August 2018 10:09:12 EEST Christoph Fritz wrote: > On Thu, 2018-08-16 at 19:39 +0300, Laurent Pinchart wrote: > > On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote: > >>> On Wednesday, 21 February 2018 23:24:36 EEST Laurent Pinchart wrote: > >>>> On Wednesday, 21 February 2018 22:42:45 EET Christoph Fritz wrote: [snip] > >>>>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h > >>>>>>> b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 > >>>>>>> --- a/drivers/media/usb/uvc/uvcvideo.h > >>>>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h > >>>>>>> @@ -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 ? > >> > >> you mean something like: > >> > >> struct uvc_device_info { > >> u32 quirks; > >> + u32 forced_color_format; > >> u32 meta_format; > >> }; > >> > >> and > >> > >> +static const struct uvc_device_info uvc_forced_color_sgbrg8 = { > >> + .forced_color_format = V4L2_PIX_FMT_SGBRG8, > >> +}; > >> > >> and > >> > >> @@ -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 > >> }, > >> > >> ? > > > > With an additional > > > > static const struct uvc_device_info uvc_forced_color_y8 = { > > .forced_color_format = V4L2_PIX_FMT_GREY, > > }; > > > >> 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()? I'd rather do that, yes. We can store a pointer to uvc_device_info in the uvc_device structure to make that easier. I'll post a patch to do so in reply to this e-mail. > >> - 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. > > > >> 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. > >> > >> 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 ? > > > >> } > >> > >> Is this the way you want me to go?
Hi Laurent, Am Donnerstag, den 16.08.2018, 19:39 +0300 schrieb Laurent Pinchart: > Hi Christoph, > > (Philipp, there's a question for you at the end) > > On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote: [...] > > 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 ? 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. ----------8<---------- $ lsusb -d 2833:0211 -v Bus 003 Device 002: ID 2833:0211 Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 3.00 bDeviceClass 239 Miscellaneous Device bDeviceSubClass 2 ? bDeviceProtocol 1 Interface Association bMaxPacketSize0 9 idVendor 0x2833 idProduct 0x0211 bcdDevice 0.00 iManufacturer 1 Oculus VR iProduct 2 Rift Sensor iSerial 3 WMTD303N602SK6 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 342 bNumInterfaces 2 bConfigurationValue 1 iConfiguration 0 bmAttributes 0x80 (Bus Powered) MaxPower 200mA Interface Association: bLength 8 bDescriptorType 11 bFirstInterface 0 bInterfaceCount 2 bFunctionClass 255 Vendor Specific Class bFunctionSubClass 3 bFunctionProtocol 0 iFunction 4 CV1 External Camera Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 1 bInterfaceProtocol 0 iInterface 4 CV1 External Camera ** UNRECOGNIZED: 0d 24 01 00 01 4e 00 00 5a 62 02 01 01 ** UNRECOGNIZED: 12 24 02 01 01 02 00 00 00 00 00 00 00 00 03 0e 00 00 ** UNRECOGNIZED: 09 24 03 02 01 01 00 04 00 ** UNRECOGNIZED: 0b 24 05 03 01 00 00 02 7f 14 00 ** UNRECOGNIZED: 1b 24 06 04 ad cc b1 c2 f6 ab b8 48 8e 37 32 d4 f3 a3 fe ec 05 01 03 02 06 0e 00 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes 3 Transfer Type Interrupt Synch Type None Usage Type Data wMaxPacketSize 0x0020 1x 32 bytes bInterval 11 bMaxBurst 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 1 bAlternateSetting 0 bNumEndpoints 0 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 2 bInterfaceProtocol 0 iInterface 0 ** UNRECOGNIZED: 0e 24 01 01 a7 00 81 00 02 00 01 01 01 00 ** UNRECOGNIZED: 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 ** UNRECOGNIZED: 1e 24 05 01 00 80 02 d0 02 00 00 5e 1a 00 00 5e 1a 00 10 0e 00 3a c6 02 00 01 3a c6 02 00 ** UNRECOGNIZED: 1e 24 05 02 00 e0 01 40 02 00 00 bb 17 00 00 bb 17 00 70 08 00 07 b2 01 00 01 07 b2 01 00 ** UNRECOGNIZED: 1e 24 05 03 00 e0 01 c0 03 00 00 bb 17 00 00 bb 17 00 10 0e 00 61 d3 02 00 01 61 d3 02 00 ** UNRECOGNIZED: 1e 24 05 04 00 80 02 c0 03 00 00 4c 1d 00 00 4c 1d 00 c0 12 00 40 0d 03 00 01 40 0d 03 00 ** UNRECOGNIZED: 06 24 0d 01 01 04 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 1 bAlternateSetting 1 bNumEndpoints 1 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 2 bInterfaceProtocol 0 iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 5 Transfer Type Isochronous Synch Type Asynchronous Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 1 bMaxBurst 7 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 1 bAlternateSetting 2 bNumEndpoints 1 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass 2 bInterfaceProtocol 0 iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 5 Transfer Type Isochronous Synch Type Asynchronous Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 1 bMaxBurst 15 Binary Object Store Descriptor: bLength 5 bDescriptorType 15 wTotalLength 22 bNumDeviceCaps 2 USB 2.0 Extension Device Capability: bLength 7 bDescriptorType 16 bDevCapabilityType 2 bmAttributes 0x00000006 Link Power Management (LPM) Supported SuperSpeed USB Device Capability: bLength 10 bDescriptorType 16 bDevCapabilityType 3 bmAttributes 0x00 wSpeedsSupported 0x000e Device can operate at Full Speed (12Mbps) Device can operate at High Speed (480Mbps) Device can operate at SuperSpeed (5Gbps) bFunctionalitySupport 1 Lowest fully-functional device speed is Full Speed (12Mbps) bU1DevExitLat 4 micro seconds bU2DevExitLat 4 micro seconds Device Status: 0x000c (Bus Powered) U1 Enabled U2 Enabled ---------->8---------- regards Philipp
Hi Philipp, On Friday, 17 August 2018 20:46:33 EEST Philipp Zabel wrote: > Am Donnerstag, den 16.08.2018, 19:39 +0300 schrieb Laurent Pinchart: > > Hi Christoph, > > > > (Philipp, there's a question for you at the end) > > > On Thursday, 16 August 2018 15:48:15 EEST Christoph Fritz wrote: > [...] > > >> 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 ? > > 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. Thanks a lot, that's exactly the information I needed. We can thus compute the multiplier with something like if (dev->info->pixel_format) { fmtdesc = uvc_format_by_fourcc(dev->info->pixel_format); strlcpy(format->name, fmtdesc->name, sizeof(format->name)); format->fcc = fmtdesc->fcc; width_multiplier = format->bpp / fmtdesc->bpp; format->bpp = fmtdesc->bpp; } (possibly with a better name for the pixel_format field)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index cde43b6..8bfa40b 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -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 diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 7e4d3ee..ad51002 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -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 /* Format flags */ #define UVC_FMT_FLAG_COMPRESSED 0x00000001