[PATCH] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[PATCH] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Sergey Zakharchenko]

This device does not function correctly in raw mode in kernel
versions validating buffer sizes in bulk mode. It erroneously
announces 16 bits per pixel instead of 12 for NV12 format, so it
needs this quirk to fix computed frame size and avoid legitimate
frames getting discarded.

Signed-off-by: Sergey Zakharchenko <szakharchenko@digital-loggers.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 28 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 66ee168ddc7e..ffb3bc0992cc 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -446,10 +446,12 @@ static int uvc_parse_format(struct uvc_device *dev,
 	struct usb_host_interface *alts = intf->cur_altsetting;
 	struct uvc_format_desc *fmtdesc;
 	struct uvc_frame *frame;
+	const struct v4l2_format_info *info;
 	const unsigned char *start = buffer;
 	unsigned int width_multiplier = 1;
 	unsigned int interval;
 	unsigned int i, n;
+	unsigned int div;
 	u8 ftype;
 
 	format->type = buffer[2];
@@ -497,6 +499,18 @@ static int uvc_parse_format(struct uvc_device *dev,
 			}
 		}
 
+		/* Some devices report bpp that doesn't match the format. */
+		if (dev->quirks & UVC_QUIRK_FORCE_BPP) {
+			info = v4l2_format_info(format->fcc);
+			if (info) {
+				div = info->hdiv * info->vdiv;
+				n = info->bpp[i] * div;
+				for (i = 1; i < info->comp_planes; i++)
+					n += info->bpp[i];
+				format->bpp = DIV_ROUND_UP(8 * n, div);
+			}
+		}
+
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
 			ftype = UVC_VS_FRAME_UNCOMPRESSED;
 		} else {
@@ -2384,6 +2398,10 @@ static const struct uvc_device_info uvc_quirk_force_y8 = {
 	.quirks = UVC_QUIRK_FORCE_Y8,
 };
 
+static const struct uvc_device_info uvc_quirk_force_bpp = {
+	.quirks = UVC_QUIRK_FORCE_BPP,
+};
+
 #define UVC_INFO_QUIRK(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks = q}
 #define UVC_INFO_META(m) (kernel_ulong_t)&(struct uvc_device_info) \
 	{.meta_format = m}
@@ -2869,6 +2887,15 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
+	/* GEO Semiconductor GC6500 */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x29fe,
+	  .idProduct		= 0x4d53,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_bpp },
 	/* Generic USB Video Class */
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index c7c1baa90dea..24e3d8c647e7 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -198,6 +198,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_BPP		0x00001000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
-- 
2.23.0

Re: [PATCH] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 11650 bytes --]

Hi Sergey,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[cannot apply to v5.4-rc1 next-20191002]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Sergey-Zakharchenko/media-uvcvideo-Add-a-quirk-to-force-GEO-GC6500-Camera-bits-per-pixel-value/20191002-185359
base:   git://linuxtv.org/media_tree.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sparc64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/media/usb/uvc/uvc_driver.c: In function 'uvc_parse_format.isra.6':
>> drivers/media/usb/uvc/uvc_driver.c:507:18: warning: 'i' may be used uninitialized in this function [-Wmaybe-uninitialized]
        n = info->bpp[i] * div;
            ~~~~~~~~~^~~

vim +/i +507 drivers/media/usb/uvc/uvc_driver.c

   436	
   437	/* ------------------------------------------------------------------------
   438	 * Descriptors parsing
   439	 */
   440	
   441	static int uvc_parse_format(struct uvc_device *dev,
   442		struct uvc_streaming *streaming, struct uvc_format *format,
   443		u32 **intervals, unsigned char *buffer, int buflen)
   444	{
   445		struct usb_interface *intf = streaming->intf;
   446		struct usb_host_interface *alts = intf->cur_altsetting;
   447		struct uvc_format_desc *fmtdesc;
   448		struct uvc_frame *frame;
   449		const struct v4l2_format_info *info;
   450		const unsigned char *start = buffer;
   451		unsigned int width_multiplier = 1;
   452		unsigned int interval;
   453		unsigned int i, n;
   454		unsigned int div;
   455		u8 ftype;
   456	
   457		format->type = buffer[2];
   458		format->index = buffer[3];
   459	
   460		switch (buffer[2]) {
   461		case UVC_VS_FORMAT_UNCOMPRESSED:
   462		case UVC_VS_FORMAT_FRAME_BASED:
   463			n = buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED ? 27 : 28;
   464			if (buflen < n) {
   465				uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming "
   466				       "interface %d FORMAT error\n",
   467				       dev->udev->devnum,
   468				       alts->desc.bInterfaceNumber);
   469				return -EINVAL;
   470			}
   471	
   472			/* Find the format descriptor from its GUID. */
   473			fmtdesc = uvc_format_by_guid(&buffer[5]);
   474	
   475			if (fmtdesc != NULL) {
   476				strscpy(format->name, fmtdesc->name,
   477					sizeof(format->name));
   478				format->fcc = fmtdesc->fcc;
   479			} else {
   480				uvc_printk(KERN_INFO, "Unknown video format %pUl\n",
   481					&buffer[5]);
   482				snprintf(format->name, sizeof(format->name), "%pUl\n",
   483					&buffer[5]);
   484				format->fcc = 0;
   485			}
   486	
   487			format->bpp = buffer[21];
   488	
   489			/* Some devices report a format that doesn't match what they
   490			 * really send.
   491			 */
   492			if (dev->quirks & UVC_QUIRK_FORCE_Y8) {
   493				if (format->fcc == V4L2_PIX_FMT_YUYV) {
   494					strscpy(format->name, "Greyscale 8-bit (Y8  )",
   495						sizeof(format->name));
   496					format->fcc = V4L2_PIX_FMT_GREY;
   497					format->bpp = 8;
   498					width_multiplier = 2;
   499				}
   500			}
   501	
   502			/* Some devices report bpp that doesn't match the format. */
   503			if (dev->quirks & UVC_QUIRK_FORCE_BPP) {
   504				info = v4l2_format_info(format->fcc);
   505				if (info) {
   506					div = info->hdiv * info->vdiv;
 > 507					n = info->bpp[i] * div;
   508					for (i = 1; i < info->comp_planes; i++)
   509						n += info->bpp[i];
   510					format->bpp = DIV_ROUND_UP(8 * n, div);
   511				}
   512			}
   513	
   514			if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
   515				ftype = UVC_VS_FRAME_UNCOMPRESSED;
   516			} else {
   517				ftype = UVC_VS_FRAME_FRAME_BASED;
   518				if (buffer[27])
   519					format->flags = UVC_FMT_FLAG_COMPRESSED;
   520			}
   521			break;
   522	
   523		case UVC_VS_FORMAT_MJPEG:
   524			if (buflen < 11) {
   525				uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming "
   526				       "interface %d FORMAT error\n",
   527				       dev->udev->devnum,
   528				       alts->desc.bInterfaceNumber);
   529				return -EINVAL;
   530			}
   531	
   532			strscpy(format->name, "MJPEG", sizeof(format->name));
   533			format->fcc = V4L2_PIX_FMT_MJPEG;
   534			format->flags = UVC_FMT_FLAG_COMPRESSED;
   535			format->bpp = 0;
   536			ftype = UVC_VS_FRAME_MJPEG;
   537			break;
   538	
   539		case UVC_VS_FORMAT_DV:
   540			if (buflen < 9) {
   541				uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming "
   542				       "interface %d FORMAT error\n",
   543				       dev->udev->devnum,
   544				       alts->desc.bInterfaceNumber);
   545				return -EINVAL;
   546			}
   547	
   548			switch (buffer[8] & 0x7f) {
   549			case 0:
   550				strscpy(format->name, "SD-DV", sizeof(format->name));
   551				break;
   552			case 1:
   553				strscpy(format->name, "SDL-DV", sizeof(format->name));
   554				break;
   555			case 2:
   556				strscpy(format->name, "HD-DV", sizeof(format->name));
   557				break;
   558			default:
   559				uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming "
   560				       "interface %d: unknown DV format %u\n",
   561				       dev->udev->devnum,
   562				       alts->desc.bInterfaceNumber, buffer[8]);
   563				return -EINVAL;
   564			}
   565	
   566			strlcat(format->name, buffer[8] & (1 << 7) ? " 60Hz" : " 50Hz",
   567				sizeof(format->name));
   568	
   569			format->fcc = V4L2_PIX_FMT_DV;
   570			format->flags = UVC_FMT_FLAG_COMPRESSED | UVC_FMT_FLAG_STREAM;
   571			format->bpp = 0;
   572			ftype = 0;
   573	
   574			/* Create a dummy frame descriptor. */
   575			frame = &format->frame[0];
   576			memset(&format->frame[0], 0, sizeof(format->frame[0]));
   577			frame->bFrameIntervalType = 1;
   578			frame->dwDefaultFrameInterval = 1;
   579			frame->dwFrameInterval = *intervals;
   580			*(*intervals)++ = 1;
   581			format->nframes = 1;
   582			break;
   583	
   584		case UVC_VS_FORMAT_MPEG2TS:
   585		case UVC_VS_FORMAT_STREAM_BASED:
   586			/* Not supported yet. */
   587		default:
   588			uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming "
   589			       "interface %d unsupported format %u\n",
   590			       dev->udev->devnum, alts->desc.bInterfaceNumber,
   591			       buffer[2]);
   592			return -EINVAL;
   593		}
   594	
   595		uvc_trace(UVC_TRACE_DESCR, "Found format %s.\n", format->name);
   596	
   597		buflen -= buffer[0];
   598		buffer += buffer[0];
   599	
   600		/* Parse the frame descriptors. Only uncompressed, MJPEG and frame
   601		 * based formats have frame descriptors.
   602		 */
   603		while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
   604		       buffer[2] == ftype) {
   605			frame = &format->frame[format->nframes];
   606			if (ftype != UVC_VS_FRAME_FRAME_BASED)
   607				n = buflen > 25 ? buffer[25] : 0;
   608			else
   609				n = buflen > 21 ? buffer[21] : 0;
   610	
   611			n = n ? n : 3;
   612	
   613			if (buflen < 26 + 4*n) {
   614				uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming "
   615				       "interface %d FRAME error\n", dev->udev->devnum,
   616				       alts->desc.bInterfaceNumber);
   617				return -EINVAL;
   618			}
   619	
   620			frame->bFrameIndex = buffer[3];
   621			frame->bmCapabilities = buffer[4];
   622			frame->wWidth = get_unaligned_le16(&buffer[5])
   623				      * width_multiplier;
   624			frame->wHeight = get_unaligned_le16(&buffer[7]);
   625			frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
   626			frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
   627			if (ftype != UVC_VS_FRAME_FRAME_BASED) {
   628				frame->dwMaxVideoFrameBufferSize =
   629					get_unaligned_le32(&buffer[17]);
   630				frame->dwDefaultFrameInterval =
   631					get_unaligned_le32(&buffer[21]);
   632				frame->bFrameIntervalType = buffer[25];
   633			} else {
   634				frame->dwMaxVideoFrameBufferSize = 0;
   635				frame->dwDefaultFrameInterval =
   636					get_unaligned_le32(&buffer[17]);
   637				frame->bFrameIntervalType = buffer[21];
   638			}
   639			frame->dwFrameInterval = *intervals;
   640	
   641			/* Several UVC chipsets screw up dwMaxVideoFrameBufferSize
   642			 * completely. Observed behaviours range from setting the
   643			 * value to 1.1x the actual frame size to hardwiring the
   644			 * 16 low bits to 0. This results in a higher than necessary
   645			 * memory usage as well as a wrong image size information. For
   646			 * uncompressed formats this can be fixed by computing the
   647			 * value from the frame size.
   648			 */
   649			if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
   650				frame->dwMaxVideoFrameBufferSize = format->bpp
   651					* frame->wWidth * frame->wHeight / 8;
   652	
   653			/* Some bogus devices report dwMinFrameInterval equal to
   654			 * dwMaxFrameInterval and have dwFrameIntervalStep set to
   655			 * zero. Setting all null intervals to 1 fixes the problem and
   656			 * some other divisions by zero that could happen.
   657			 */
   658			for (i = 0; i < n; ++i) {
   659				interval = get_unaligned_le32(&buffer[26+4*i]);
   660				*(*intervals)++ = interval ? interval : 1;
   661			}
   662	
   663			/* Make sure that the default frame interval stays between
   664			 * the boundaries.
   665			 */
   666			n -= frame->bFrameIntervalType ? 1 : 2;
   667			frame->dwDefaultFrameInterval =
   668				min(frame->dwFrameInterval[n],
   669				    max(frame->dwFrameInterval[0],
   670					frame->dwDefaultFrameInterval));
   671	
   672			if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
   673				frame->bFrameIntervalType = 1;
   674				frame->dwFrameInterval[0] =
   675					frame->dwDefaultFrameInterval;
   676			}
   677	
   678			uvc_trace(UVC_TRACE_DESCR, "- %ux%u (%u.%u fps)\n",
   679				frame->wWidth, frame->wHeight,
   680				10000000/frame->dwDefaultFrameInterval,
   681				(100000000/frame->dwDefaultFrameInterval)%10);
   682	
   683			format->nframes++;
   684			buflen -= buffer[0];
   685			buffer += buffer[0];
   686		}
   687	
   688		if (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
   689		    buffer[2] == UVC_VS_STILL_IMAGE_FRAME) {
   690			buflen -= buffer[0];
   691			buffer += buffer[0];
   692		}
   693	
   694		if (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
   695		    buffer[2] == UVC_VS_COLORFORMAT) {
   696			if (buflen < 6) {
   697				uvc_trace(UVC_TRACE_DESCR, "device %d videostreaming "
   698				       "interface %d COLORFORMAT error\n",
   699				       dev->udev->devnum,
   700				       alts->desc.bInterfaceNumber);
   701				return -EINVAL;
   702			}
   703	
   704			format->colorspace = uvc_colorspace(buffer[3]);
   705	
   706			buflen -= buffer[0];
   707			buffer += buffer[0];
   708		}
   709	
   710		return buffer - start;
   711	}
   712	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59095 bytes --]

[PATCH v2] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Sergey Zakharchenko]

This device does not function correctly in raw mode in kernel
versions validating buffer sizes in bulk mode. It erroneously
announces 16 bits per pixel instead of 12 for NV12 format, so it
needs this quirk to fix computed frame size and avoid legitimate
frames getting discarded.

Signed-off-by: Sergey Zakharchenko <szakharchenko@digital-loggers.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 28 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 66ee168ddc7e..23f62456946d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -446,10 +446,12 @@ static int uvc_parse_format(struct uvc_device *dev,
 	struct usb_host_interface *alts = intf->cur_altsetting;
 	struct uvc_format_desc *fmtdesc;
 	struct uvc_frame *frame;
+	const struct v4l2_format_info *info;
 	const unsigned char *start = buffer;
 	unsigned int width_multiplier = 1;
 	unsigned int interval;
 	unsigned int i, n;
+	unsigned int div;
 	u8 ftype;
 
 	format->type = buffer[2];
@@ -497,6 +499,18 @@ static int uvc_parse_format(struct uvc_device *dev,
 			}
 		}
 
+		/* Some devices report bpp that doesn't match the format. */
+		if (dev->quirks & UVC_QUIRK_FORCE_BPP) {
+			info = v4l2_format_info(format->fcc);
+			if (info) {
+				div = info->hdiv * info->vdiv;
+				n = info->bpp[0] * div;
+				for (i = 1; i < info->comp_planes; i++)
+					n += info->bpp[i];
+				format->bpp = DIV_ROUND_UP(8 * n, div);
+			}
+		}
+
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
 			ftype = UVC_VS_FRAME_UNCOMPRESSED;
 		} else {
@@ -2384,6 +2398,10 @@ static const struct uvc_device_info uvc_quirk_force_y8 = {
 	.quirks = UVC_QUIRK_FORCE_Y8,
 };
 
+static const struct uvc_device_info uvc_quirk_force_bpp = {
+	.quirks = UVC_QUIRK_FORCE_BPP,
+};
+
 #define UVC_INFO_QUIRK(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks = q}
 #define UVC_INFO_META(m) (kernel_ulong_t)&(struct uvc_device_info) \
 	{.meta_format = m}
@@ -2869,6 +2887,15 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
+	/* GEO Semiconductor GC6500 */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x29fe,
+	  .idProduct		= 0x4d53,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_bpp },
 	/* Generic USB Video Class */
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
 	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index c7c1baa90dea..24e3d8c647e7 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -198,6 +198,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_BPP		0x00001000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001

base-commit: 20a438d53fd9d12a894161bc56cbeab7a9993c39
prerequisite-patch-id: 521eb9602d395ea667eecc75cd2273b59cd3ed76
-- 
2.23.0

Re: [PATCH v2] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Laurent Pinchart]

Hi Sergey,

Thank you for the patch.

On Wed, Oct 02, 2019 at 01:01:02PM +0000, Sergey Zakharchenko wrote:
> This device does not function correctly in raw mode in kernel
> versions validating buffer sizes in bulk mode. It erroneously
> announces 16 bits per pixel instead of 12 for NV12 format, so it
> needs this quirk to fix computed frame size and avoid legitimate
> frames getting discarded.
> 
> Signed-off-by: Sergey Zakharchenko <szakharchenko@digital-loggers.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |  1 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 66ee168ddc7e..23f62456946d 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -446,10 +446,12 @@ static int uvc_parse_format(struct uvc_device *dev,
>  	struct usb_host_interface *alts = intf->cur_altsetting;
>  	struct uvc_format_desc *fmtdesc;
>  	struct uvc_frame *frame;
> +	const struct v4l2_format_info *info;
>  	const unsigned char *start = buffer;
>  	unsigned int width_multiplier = 1;
>  	unsigned int interval;
>  	unsigned int i, n;
> +	unsigned int div;
>  	u8 ftype;
>  
>  	format->type = buffer[2];
> @@ -497,6 +499,18 @@ static int uvc_parse_format(struct uvc_device *dev,
>  			}
>  		}
>  
> +		/* Some devices report bpp that doesn't match the format. */
> +		if (dev->quirks & UVC_QUIRK_FORCE_BPP) {
> +			info = v4l2_format_info(format->fcc);
> +			if (info) {
> +				div = info->hdiv * info->vdiv;
> +				n = info->bpp[0] * div;
> +				for (i = 1; i < info->comp_planes; i++)
> +					n += info->bpp[i];
> +				format->bpp = DIV_ROUND_UP(8 * n, div);
> +			}
> +		}

Do you think it would make sense to do this by default, without
requiring a quirk ? Or are there cases where this calculation would lead
to incorrect results while the bpp reported by the camera would be right
?

> +
>  		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
>  			ftype = UVC_VS_FRAME_UNCOMPRESSED;
>  		} else {
> @@ -2384,6 +2398,10 @@ static const struct uvc_device_info uvc_quirk_force_y8 = {
>  	.quirks = UVC_QUIRK_FORCE_Y8,
>  };
>  
> +static const struct uvc_device_info uvc_quirk_force_bpp = {
> +	.quirks = UVC_QUIRK_FORCE_BPP,
> +};
> +
>  #define UVC_INFO_QUIRK(q) (kernel_ulong_t)&(struct uvc_device_info){.quirks = q}
>  #define UVC_INFO_META(m) (kernel_ulong_t)&(struct uvc_device_info) \
>  	{.meta_format = m}
> @@ -2869,6 +2887,15 @@ static const struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= UVC_INFO_META(V4L2_META_FMT_D4XX) },
> +	/* GEO Semiconductor GC6500 */
> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x29fe,
> +	  .idProduct		= 0x4d53,

Could you please keep the entries sorted by idVendor/idProduct ?

> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_bpp },

As this is the only device using this quirk, you can drop
uvc_quirk_force_bpp and use

	.driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FORCE_BPP) },

>  	/* Generic USB Video Class */
>  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) },
>  	{ USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) },
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index c7c1baa90dea..24e3d8c647e7 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -198,6 +198,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_BPP		0x00001000
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
> 
> base-commit: 20a438d53fd9d12a894161bc56cbeab7a9993c39
> prerequisite-patch-id: 521eb9602d395ea667eecc75cd2273b59cd3ed76

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Sergey Zakharchenko]

Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Do you think it would make sense to do this by default, without
> requiring a quirk ? Or are there cases where this calculation would lead
> to incorrect results while the bpp reported by the camera would be right
> ?

I don't nearly have the experience with a broad range of webcams
required to answer this question. At the very least that would tax
well-behaved cameras with a tiny but unnecessary bit of initial
computations.

The loop is a simplified version of the v4l2_fill_pixfmt() loop. The
calculation might need some checking, and might be invalid, in case
block_w/block_h format fields are significant (not 0 and not 1),
because then effective bits-per-pixel would seemingly be fractional,
and depend on the image dimensions if they weren't aligned; however I
see no formats using the block_w/block_h fields defined so far.

AFAICT the division should need no rounding for the raw formats
currently listed; just being cautious there.

> Could you please keep the entries sorted by idVendor/idProduct ?
> As this is the only device using this quirk, you can drop
> uvc_quirk_force_bpp and use
>
>         .driver_info            = UVC_INFO_QUIRK(UVC_QUIRK_FORCE_BPP) },

All makes sense and will be adjusted, thanks for the review!

Best regards,

--
Sergey Zakharchenko
Digital Loggers, Inc.

Re: [PATCH v2] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Sergey Zakharchenko]

Sergey Zakharchenko <doublef.mobile@gmail.com>:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> > Do you think it would make sense to do this by default, without
> > requiring a quirk ? Or are there cases where this calculation would lead
> > to incorrect results while the bpp reported by the camera would be right
> > ?
>
> The loop is a simplified version of the v4l2_fill_pixfmt() loop. The
> calculation might need some checking, and might be invalid, in case
> block_w/block_h format fields are significant (not 0 and not 1),
> because then effective bits-per-pixel would seemingly be fractional,
> and depend on the image dimensions if they weren't aligned; however I
> see no formats using the block_w/block_h fields defined so far.

It's likely possible to directly replace the bpp-using computation in
https://github.com/torvalds/linux/blob/2874c5fd284268364ece81a7bd936f3c8168e567/drivers/media/usb/uvc/uvc_driver.c#L636
with a call to v4l2_fill_pixfmt() and the sizeimage it returns.
However, bpp is used elsewhere, and it's hard to tell what it should
be taken to be to in the hypothetical exotic cases I'm considering, so
I'm reluctant to go that route.

I'm going to send v3 in an hour unless there are other suggestions.

Best regards,

-- 
Sergey Zakharchenko
Digital Loggers, Inc.

Re: [PATCH v2] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Laurent Pinchart]

Hi Sergey,

On Wed, Oct 02, 2019 at 09:15:45PM +0400, Sergey Zakharchenko wrote:
> Sergey Zakharchenko <doublef.mobile@gmail.com>:
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> > > Do you think it would make sense to do this by default, without
> > > requiring a quirk ? Or are there cases where this calculation would lead
> > > to incorrect results while the bpp reported by the camera would be right
> > > ?
> >
> > The loop is a simplified version of the v4l2_fill_pixfmt() loop. The
> > calculation might need some checking, and might be invalid, in case
> > block_w/block_h format fields are significant (not 0 and not 1),
> > because then effective bits-per-pixel would seemingly be fractional,
> > and depend on the image dimensions if they weren't aligned; however I
> > see no formats using the block_w/block_h fields defined so far.
> 
> It's likely possible to directly replace the bpp-using computation in
> https://github.com/torvalds/linux/blob/2874c5fd284268364ece81a7bd936f3c8168e567/drivers/media/usb/uvc/uvc_driver.c#L636
> with a call to v4l2_fill_pixfmt() and the sizeimage it returns.
> However, bpp is used elsewhere, and it's hard to tell what it should
> be taken to be to in the hypothetical exotic cases I'm considering, so
> I'm reluctant to go that route.

Would it make sense to split the calculation from v4l2_fill_pixfmt() to
a helper function that the UVC driver could call ?

> I'm going to send v3 in an hour unless there are other suggestions.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Sergey Zakharchenko]

Laurent,

> Would it make sense to split the calculation from v4l2_fill_pixfmt() to
> a helper function that the UVC driver could call ?

It would of course be possible, and would benefit v4l2-common.c as it
would be common between v4l2_fill_pixfmt() and v4l2_fill_pixfmt_mp(),
but as I noted before that approach would leave the bpp struct member,
used elsewhere, possibly incorrect. I'm also not sure where
multi-planar formats fit in (probably not related to the problem at
hand).

> > I'm going to send v3 in an hour unless there are other suggestions.

Till tomorrow, then...

Best regards,

--
Sergey Zakharchenko
Digital Loggers, Inc.

[PATCH v3] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Sergey Zakharchenko]

This device does not function correctly in raw mode in kernel
versions validating buffer sizes in bulk mode. It erroneously
announces 16 bits per pixel instead of 12 for NV12 format, so it
needs this quirk to fix computed frame size and avoid legitimate
frames getting discarded.

Signed-off-by: Sergey Zakharchenko <szakharchenko@digital-loggers.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++++++
 drivers/media/usb/uvc/uvcvideo.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 66ee168ddc7e..d63db65ef8b2 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -446,10 +446,12 @@ static int uvc_parse_format(struct uvc_device *dev,
 	struct usb_host_interface *alts = intf->cur_altsetting;
 	struct uvc_format_desc *fmtdesc;
 	struct uvc_frame *frame;
+	const struct v4l2_format_info *info;
 	const unsigned char *start = buffer;
 	unsigned int width_multiplier = 1;
 	unsigned int interval;
 	unsigned int i, n;
+	unsigned int div;
 	u8 ftype;
 
 	format->type = buffer[2];
@@ -497,6 +499,18 @@ static int uvc_parse_format(struct uvc_device *dev,
 			}
 		}
 
+		/* Some devices report bpp that doesn't match the format. */
+		if (dev->quirks & UVC_QUIRK_FORCE_BPP) {
+			info = v4l2_format_info(format->fcc);
+			if (info) {
+				div = info->hdiv * info->vdiv;
+				n = info->bpp[0] * div;
+				for (i = 1; i < info->comp_planes; i++)
+					n += info->bpp[i];
+				format->bpp = DIV_ROUND_UP(8 * n, div);
+			}
+		}
+
 		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
 			ftype = UVC_VS_FRAME_UNCOMPRESSED;
 		} else {
@@ -2860,6 +2874,15 @@ static const struct usb_device_id uvc_ids[] = {
 	  .bInterfaceSubClass	= 1,
 	  .bInterfaceProtocol	= 0,
 	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
+	/* GEO Semiconductor GC6500 */
+	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
+				| USB_DEVICE_ID_MATCH_INT_INFO,
+	  .idVendor		= 0x29fe,
+	  .idProduct		= 0x4d53,
+	  .bInterfaceClass	= USB_CLASS_VIDEO,
+	  .bInterfaceSubClass	= 1,
+	  .bInterfaceProtocol	= 0,
+	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FORCE_BPP) },
 	/* Intel RealSense D4M */
 	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
 				| USB_DEVICE_ID_MATCH_INT_INFO,
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index c7c1baa90dea..24e3d8c647e7 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -198,6 +198,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_BPP		0x00001000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001

base-commit: 20a438d53fd9d12a894161bc56cbeab7a9993c39
prerequisite-patch-id: 521eb9602d395ea667eecc75cd2273b59cd3ed76
-- 
2.23.0

Re: [PATCH v3] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Laurent Pinchart]

Hi Sergey,

I'm sorry for the late reply, this patch fell through the cracks :-S

On Thu, Oct 03, 2019 at 09:31:23AM +0000, Sergey Zakharchenko wrote:
> This device does not function correctly in raw mode in kernel
> versions validating buffer sizes in bulk mode. It erroneously
> announces 16 bits per pixel instead of 12 for NV12 format, so it
> needs this quirk to fix computed frame size and avoid legitimate
> frames getting discarded.
> 
> Signed-off-by: Sergey Zakharchenko <szakharchenko@digital-loggers.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++++++
>  drivers/media/usb/uvc/uvcvideo.h   |  1 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 66ee168ddc7e..d63db65ef8b2 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -446,10 +446,12 @@ static int uvc_parse_format(struct uvc_device *dev,
>  	struct usb_host_interface *alts = intf->cur_altsetting;
>  	struct uvc_format_desc *fmtdesc;
>  	struct uvc_frame *frame;
> +	const struct v4l2_format_info *info;
>  	const unsigned char *start = buffer;
>  	unsigned int width_multiplier = 1;
>  	unsigned int interval;
>  	unsigned int i, n;
> +	unsigned int div;
>  	u8 ftype;
>  
>  	format->type = buffer[2];
> @@ -497,6 +499,18 @@ static int uvc_parse_format(struct uvc_device *dev,
>  			}
>  		}
>  
> +		/* Some devices report bpp that doesn't match the format. */
> +		if (dev->quirks & UVC_QUIRK_FORCE_BPP) {

Both info and div can be declared here. The patch otherwise looks good
to me, so I'll make this modification when applying it, unless you would
prefer resending it.

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

I however had a closer look at how bpp is used, and I think it would
make sense to actually drop usage of this field. There's a usage in
uvc_video.c that is easy to replace (I will send a patch and CC you),
and I think the other two usages in uvc_driver.c and uvc_v4l2.c to
compute the image size and line size respectively should eventually be
replaced by calculation based on the format info.

> +			info = v4l2_format_info(format->fcc);
> +			if (info) {
> +				div = info->hdiv * info->vdiv;
> +				n = info->bpp[0] * div;
> +				for (i = 1; i < info->comp_planes; i++)
> +					n += info->bpp[i];
> +				format->bpp = DIV_ROUND_UP(8 * n, div);
> +			}
> +		}
> +
>  		if (buffer[2] == UVC_VS_FORMAT_UNCOMPRESSED) {
>  			ftype = UVC_VS_FRAME_UNCOMPRESSED;
>  		} else {
> @@ -2860,6 +2874,15 @@ static const struct usb_device_id uvc_ids[] = {
>  	  .bInterfaceSubClass	= 1,
>  	  .bInterfaceProtocol	= 0,
>  	  .driver_info		= (kernel_ulong_t)&uvc_quirk_force_y8 },
> +	/* GEO Semiconductor GC6500 */
> +	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
> +				| USB_DEVICE_ID_MATCH_INT_INFO,
> +	  .idVendor		= 0x29fe,
> +	  .idProduct		= 0x4d53,
> +	  .bInterfaceClass	= USB_CLASS_VIDEO,
> +	  .bInterfaceSubClass	= 1,
> +	  .bInterfaceProtocol	= 0,
> +	  .driver_info		= UVC_INFO_QUIRK(UVC_QUIRK_FORCE_BPP) },
>  	/* Intel RealSense D4M */
>  	{ .match_flags		= USB_DEVICE_ID_MATCH_DEVICE
>  				| USB_DEVICE_ID_MATCH_INT_INFO,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index c7c1baa90dea..24e3d8c647e7 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -198,6 +198,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_BPP		0x00001000
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
> 
> base-commit: 20a438d53fd9d12a894161bc56cbeab7a9993c39
> prerequisite-patch-id: 521eb9602d395ea667eecc75cd2273b59cd3ed76

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3] media: uvcvideo: Add a quirk to force GEO GC6500 Camera bits-per-pixel value

[Sergey Zakharchenko]

Hello Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> I'm sorry for the late reply, this patch fell through the cracks :-S

It's OK, thanks for getting back to it!

> > +             /* Some devices report bpp that doesn't match the format. */
> > +             if (dev->quirks & UVC_QUIRK_FORCE_BPP) {
>
> Both info and div can be declared here.

Right; however I was just following what seemed to be the existing
practice of declaring all variables at the start of a function (which
I'm not fond of myself; e.g. fmtdesc is only used in a single switch
branch). Now after looking at the code more, there are precedents for
declaring closer to point of use in e.g. uvc_parse_control and
elsewhere, so this is probably OK.

> The patch otherwise looks good
> to me, so I'll make this modification when applying it, unless you would
> prefer resending it.

Sure, go ahead, and thanks!

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I however had a closer look at how bpp is used, and I think it would
> make sense to actually drop usage of this field.

This is most probably possible (the only cases I could think of in
which it couldn't be calculated, it would have to be fractional or no
meaningful value could be assigned to it) but out of scope of this
patch.

Best regards,

-- 
Sergey Zakharchenko
Digital Loggers, Inc.