Re: [PATCH v2 5/5] usb: uvc: use v4l2_fill_fmtdesc instead of open coded format name

[Michael Grzeschik <mgr@pengutronix.de>]

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

Hi Laurent,

Thank you for the review.

On Wed, Dec 28, 2022 at 03:50:47AM +0200, Laurent Pinchart wrote:
>On Thu, Dec 15, 2022 at 11:45:14PM +0100, Michael Grzeschik wrote:
>> Since we have the helper function v4l2_fill_fmtdesc, we can use this to
>> get the corresponding descriptive string for the pixelformat and set the
>> compressed flag. This patch is removing the redundant name field in
>> uvc_format_desc and makes use of v4l2_fill_fmtdesc instead.
>
>I really like that.
>
>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> ---
>> v1 -> v2: - added reviewed and tested tags
>> ---
>>  drivers/media/common/uvc.c             | 37 --------------------------
>>  drivers/media/usb/uvc/uvc_driver.c     |  8 +++++-
>>  drivers/usb/gadget/function/uvc_v4l2.c |  6 +----
>>  include/linux/usb/uvc.h                |  1 -
>>  4 files changed, 8 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/media/common/uvc.c b/drivers/media/common/uvc.c
>> index a9d587490de8d5..ab2637b9b39b2a 100644
>> --- a/drivers/media/common/uvc.c
>> +++ b/drivers/media/common/uvc.c
>> @@ -13,187 +13,150 @@
>>
>>  static const struct uvc_format_desc uvc_fmts[] = {
>>  	{
>> -		.name		= "YUV 4:2:2 (YUYV)",
>>  		.guid		= UVC_GUID_FORMAT_YUY2,
>>  		.fcc		= V4L2_PIX_FMT_YUYV,
>>  	},
>>  	{
>> -		.name		= "YUV 4:2:2 (YUYV)",
>>  		.guid		= UVC_GUID_FORMAT_YUY2_ISIGHT,
>>  		.fcc		= V4L2_PIX_FMT_YUYV,
>>  	},
>>  	{
>> -		.name		= "YUV 4:2:0 (NV12)",
>>  		.guid		= UVC_GUID_FORMAT_NV12,
>>  		.fcc		= V4L2_PIX_FMT_NV12,
>>  	},
>>  	{
>> -		.name		= "MJPEG",
>>  		.guid		= UVC_GUID_FORMAT_MJPEG,
>>  		.fcc		= V4L2_PIX_FMT_MJPEG,
>>  	},
>>  	{
>> -		.name		= "YVU 4:2:0 (YV12)",
>>  		.guid		= UVC_GUID_FORMAT_YV12,
>>  		.fcc		= V4L2_PIX_FMT_YVU420,
>>  	},
>>  	{
>> -		.name		= "YUV 4:2:0 (I420)",
>>  		.guid		= UVC_GUID_FORMAT_I420,
>>  		.fcc		= V4L2_PIX_FMT_YUV420,
>>  	},
>>  	{
>> -		.name		= "YUV 4:2:0 (M420)",
>>  		.guid		= UVC_GUID_FORMAT_M420,
>>  		.fcc		= V4L2_PIX_FMT_M420,
>>  	},
>>  	{
>> -		.name		= "YUV 4:2:2 (UYVY)",
>>  		.guid		= UVC_GUID_FORMAT_UYVY,
>>  		.fcc		= V4L2_PIX_FMT_UYVY,
>>  	},
>>  	{
>> -		.name		= "Greyscale 8-bit (Y800)",
>>  		.guid		= UVC_GUID_FORMAT_Y800,
>>  		.fcc		= V4L2_PIX_FMT_GREY,
>>  	},
>>  	{
>> -		.name		= "Greyscale 8-bit (Y8  )",
>>  		.guid		= UVC_GUID_FORMAT_Y8,
>>  		.fcc		= V4L2_PIX_FMT_GREY,
>>  	},
>>  	{
>> -		.name		= "Greyscale 8-bit (D3DFMT_L8)",
>>  		.guid		= UVC_GUID_FORMAT_D3DFMT_L8,
>>  		.fcc		= V4L2_PIX_FMT_GREY,
>>  	},
>>  	{
>> -		.name		= "IR 8-bit (L8_IR)",
>>  		.guid		= UVC_GUID_FORMAT_KSMEDIA_L8_IR,
>>  		.fcc		= V4L2_PIX_FMT_GREY,
>>  	},
>>  	{
>> -		.name		= "Greyscale 10-bit (Y10 )",
>>  		.guid		= UVC_GUID_FORMAT_Y10,
>>  		.fcc		= V4L2_PIX_FMT_Y10,
>>  	},
>>  	{
>> -		.name		= "Greyscale 12-bit (Y12 )",
>>  		.guid		= UVC_GUID_FORMAT_Y12,
>>  		.fcc		= V4L2_PIX_FMT_Y12,
>>  	},
>>  	{
>> -		.name		= "Greyscale 16-bit (Y16 )",
>>  		.guid		= UVC_GUID_FORMAT_Y16,
>>  		.fcc		= V4L2_PIX_FMT_Y16,
>>  	},
>>  	{
>> -		.name		= "BGGR Bayer (BY8 )",
>>  		.guid		= UVC_GUID_FORMAT_BY8,
>>  		.fcc		= V4L2_PIX_FMT_SBGGR8,
>>  	},
>>  	{
>> -		.name		= "BGGR Bayer (BA81)",
>>  		.guid		= UVC_GUID_FORMAT_BA81,
>>  		.fcc		= V4L2_PIX_FMT_SBGGR8,
>>  	},
>>  	{
>> -		.name		= "GBRG Bayer (GBRG)",
>>  		.guid		= UVC_GUID_FORMAT_GBRG,
>>  		.fcc		= V4L2_PIX_FMT_SGBRG8,
>>  	},
>>  	{
>> -		.name		= "GRBG Bayer (GRBG)",
>>  		.guid		= UVC_GUID_FORMAT_GRBG,
>>  		.fcc		= V4L2_PIX_FMT_SGRBG8,
>>  	},
>>  	{
>> -		.name		= "RGGB Bayer (RGGB)",
>>  		.guid		= UVC_GUID_FORMAT_RGGB,
>>  		.fcc		= V4L2_PIX_FMT_SRGGB8,
>>  	},
>>  	{
>> -		.name		= "RGB565",
>>  		.guid		= UVC_GUID_FORMAT_RGBP,
>>  		.fcc		= V4L2_PIX_FMT_RGB565,
>>  	},
>>  	{
>> -		.name		= "BGR 8:8:8 (BGR3)",
>>  		.guid		= UVC_GUID_FORMAT_BGR3,
>>  		.fcc		= V4L2_PIX_FMT_BGR24,
>>  	},
>>  	{
>> -		.name		= "H.264",
>>  		.guid		= UVC_GUID_FORMAT_H264,
>>  		.fcc		= V4L2_PIX_FMT_H264,
>>  	},
>>  	{
>> -		.name		= "H.265",
>>  		.guid		= UVC_GUID_FORMAT_H265,
>>  		.fcc		= V4L2_PIX_FMT_HEVC,
>>  	},
>>  	{
>> -		.name		= "Greyscale 8 L/R (Y8I)",
>>  		.guid		= UVC_GUID_FORMAT_Y8I,
>>  		.fcc		= V4L2_PIX_FMT_Y8I,
>>  	},
>>  	{
>> -		.name		= "Greyscale 12 L/R (Y12I)",
>>  		.guid		= UVC_GUID_FORMAT_Y12I,
>>  		.fcc		= V4L2_PIX_FMT_Y12I,
>>  	},
>>  	{
>> -		.name		= "Depth data 16-bit (Z16)",
>>  		.guid		= UVC_GUID_FORMAT_Z16,
>>  		.fcc		= V4L2_PIX_FMT_Z16,
>>  	},
>>  	{
>> -		.name		= "Bayer 10-bit (SRGGB10P)",
>>  		.guid		= UVC_GUID_FORMAT_RW10,
>>  		.fcc		= V4L2_PIX_FMT_SRGGB10P,
>>  	},
>>  	{
>> -		.name		= "Bayer 16-bit (SBGGR16)",
>>  		.guid		= UVC_GUID_FORMAT_BG16,
>>  		.fcc		= V4L2_PIX_FMT_SBGGR16,
>>  	},
>>  	{
>> -		.name		= "Bayer 16-bit (SGBRG16)",
>>  		.guid		= UVC_GUID_FORMAT_GB16,
>>  		.fcc		= V4L2_PIX_FMT_SGBRG16,
>>  	},
>>  	{
>> -		.name		= "Bayer 16-bit (SRGGB16)",
>>  		.guid		= UVC_GUID_FORMAT_RG16,
>>  		.fcc		= V4L2_PIX_FMT_SRGGB16,
>>  	},
>>  	{
>> -		.name		= "Bayer 16-bit (SGRBG16)",
>>  		.guid		= UVC_GUID_FORMAT_GR16,
>>  		.fcc		= V4L2_PIX_FMT_SGRBG16,
>>  	},
>>  	{
>> -		.name		= "Depth data 16-bit (Z16)",
>>  		.guid		= UVC_GUID_FORMAT_INVZ,
>>  		.fcc		= V4L2_PIX_FMT_Z16,
>>  	},
>>  	{
>> -		.name		= "Greyscale 10-bit (Y10 )",
>>  		.guid		= UVC_GUID_FORMAT_INVI,
>>  		.fcc		= V4L2_PIX_FMT_Y10,
>>  	},
>>  	{
>> -		.name		= "IR:Depth 26-bit (INZI)",
>>  		.guid		= UVC_GUID_FORMAT_INZI,
>>  		.fcc		= V4L2_PIX_FMT_INZI,
>>  	},
>>  	{
>> -		.name		= "4-bit Depth Confidence (Packed)",
>>  		.guid		= UVC_GUID_FORMAT_CNF4,
>>  		.fcc		= V4L2_PIX_FMT_CNF4,
>>  	},
>>  	{
>> -		.name		= "HEVC",
>>  		.guid		= UVC_GUID_FORMAT_HEVC,
>>  		.fcc		= V4L2_PIX_FMT_HEVC,
>>  	},
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>> index 12b6ad0966d94a..af92e730bde7c7 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -251,7 +251,13 @@ static int uvc_parse_format(struct uvc_device *dev,
>>  		fmtdesc = uvc_format_by_guid(&buffer[5]);
>>
>>  		if (fmtdesc != NULL) {
>> -			strscpy(format->name, fmtdesc->name,
>> +			struct v4l2_fmtdesc fmt;
>> +
>> +			fmt.pixelformat = fmtdesc->fcc;
>> +
>> +			v4l2_fill_fmtdesc(&fmt);
>> +
>> +			strscpy(format->name, fmt.description,
>>  				sizeof(format->name));
>>  			format->fcc = fmtdesc->fcc;
>>  		} else {
>
>I've just sent "[PATCH v1] media: uvcvideo: Remove format descriptions"
>which drops usage of the name in the uvcvideo driver without having to
>call v4l2_fill_fmtdesc(). With that merged, changes to uvc_driver.c can
>be dropped in this patch.

Right.

>
>> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
>> index 21e573e628f4e7..6e46fa1695f212 100644
>> --- a/drivers/usb/gadget/function/uvc_v4l2.c
>> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
>> @@ -374,14 +374,10 @@ uvc_v4l2_enum_format(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>>  	if (!uformat)
>>  		return -EINVAL;
>>
>> -	if (uformat->type != UVCG_UNCOMPRESSED)
>> -		f->flags |= V4L2_FMT_FLAG_COMPRESSED;
>> -
>>  	fmtdesc = to_uvc_format(uformat);
>>  	f->pixelformat = fmtdesc->fcc;
>>
>> -	strscpy(f->description, fmtdesc->name, sizeof(f->description));
>> -	f->description[strlen(fmtdesc->name) - 1] = 0;
>> +	v4l2_fill_fmtdesc(f);
>
>
>v4l_fill_fmtdesc() is actually called by v4l_enum_fmt() after calling
>the driver's .vidioc_enum_fmt_vid_out() operation, so you don't have to
>call it manually here.
>
>By dropping the manual calls to v4l_fill_fmtdesc(), you can also drop
>patch 4/5 in the series.

This is a good point. I have dropped patch 4/5 in the stack.

>This creates a dependency between the patch I've just sent and this
>series. As I don't want to introduce any further delay, I'll create a
>stable branch based on v6.2-rc1 as soon as my patch gets reviewed, you
>you can then base the next version of this series on top of it. An
>alternative would be to merge this series through the media tree if Greg
>is OK with that.

I found your v3 patch. I sure can rebase my work on your patch if your
stable branch is available.

>>  	return 0;
>>  }
>> diff --git a/include/linux/usb/uvc.h b/include/linux/usb/uvc.h
>> index 227a03f252a5c0..e407a7b8a91c70 100644
>> --- a/include/linux/usb/uvc.h
>> +++ b/include/linux/usb/uvc.h
>> @@ -146,7 +146,6 @@
>>  	 0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71}
>>
>>  struct uvc_format_desc {
>> -	char *name;
>>  	u8 guid[16];
>>  	u32 fcc;
>>  };
>
>-- 
>Regards,
>
>Laurent Pinchart
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]