[PATCH 0/1] media: uvcvideo: Use a count variable for meta_formats

[PATCH 0/1] media: uvcvideo: Use a count variable for meta_formats

[Hans de Goede]

Hi,

I still was not entirely happy with how the meta_formats handling code
looked, especially not with uvc_meta_v4l2_enum_formats().

So here is a small patch / attempt to clean it up a bit.

Ricardo, assuming you're ok with the suggested changes can you give
this a test please ?

My main UVC test cam only supports standard UVC metadata which is
not helpful for testing (1), I did give this patch a spin with that
camera but that does not say much.

Regards,

Hans

1) It is a microsoft HD camera (but quite old) so I wonder if maybe
it does have the metadata but not the extended control for enabling it.
How would I test this ? 


Hans de Goede (1):
  media: uvcvideo: Use a count variable for meta_formats instead of 0
    terminating

 drivers/media/usb/uvc/uvc_metadata.c | 21 +++++++++------------
 drivers/media/usb/uvc/uvcvideo.h     |  4 ++--
 2 files changed, 11 insertions(+), 14 deletions(-)

-- 
2.49.0

[PATCH] media: uvcvideo: Use a count variable for meta_formats instead of 0 terminating

[Hans de Goede]

The code dealing with the 0 terminated meta_formats array is a bit klunky
especially for the uvc_meta_v4l2_enum_formats() case.

Instead of 0 terminating add an unsigned int nmeta_formats member to struct
uvc_device and use that. This leads to slightly cleaner code.

Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/media/usb/uvc/uvc_metadata.c | 21 +++++++++------------
 drivers/media/usb/uvc/uvcvideo.h     |  4 ++--
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
index 12972527ab8d..229e08ff323e 100644
--- a/drivers/media/usb/uvc/uvc_metadata.c
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -64,17 +64,16 @@ static int uvc_meta_v4l2_try_format(struct file *file, void *fh,
 	struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
 	struct uvc_device *dev = stream->dev;
 	struct v4l2_meta_format *fmt = &format->fmt.meta;
-	u32 fmeta = fmt->dataformat;
-	u32 i;
+	u32 fmeta = V4L2_META_FMT_UVC;
 
 	if (format->type != vfh->vdev->queue->type)
 		return -EINVAL;
 
-	for (i = 0; (fmeta != dev->meta_formats[i]) && dev->meta_formats[i];
-	     i++)
-		;
-	if (!dev->meta_formats[i])
-		fmeta = V4L2_META_FMT_UVC;
+	for (unsigned int i = 0; i < dev->nmeta_formats; i++)
+		if (dev->meta_formats[i] == fmt->dataformat) {
+			fmeta = fmt->dataformat;
+			break;
+		}
 
 	memset(fmt, 0, sizeof(*fmt));
 
@@ -119,14 +118,12 @@ static int uvc_meta_v4l2_enum_formats(struct file *file, void *fh,
 	struct v4l2_fh *vfh = file->private_data;
 	struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
 	struct uvc_device *dev = stream->dev;
-	u32 i;
+	u32 i = fdesc->index;
 
 	if (fdesc->type != vfh->vdev->queue->type)
 		return -EINVAL;
 
-	for (i = 0; (i < fdesc->index) && dev->meta_formats[i]; i++)
-		;
-	if (!dev->meta_formats[i])
+	if (i >= dev->nmeta_formats)
 		return -EINVAL;
 
 	memset(fdesc, 0, sizeof(*fdesc));
@@ -265,7 +262,7 @@ int uvc_meta_init(struct uvc_device *dev)
 		dev->meta_formats[i++] = V4L2_META_FMT_UVC_MSXU_1_5;
 
 	 /* IMPORTANT: for new meta-formats update UVC_MAX_META_DATA_FORMATS. */
-	dev->meta_formats[i++] = 0;
+	dev->nmeta_formats = i;
 
 	return 0;
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b34c1914ff39..757254fc4fe9 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -588,8 +588,8 @@ struct uvc_device {
 
 	const struct uvc_device_info *info;
 
-	/* Zero-ended list of meta formats */
-	u32 meta_formats[UVC_MAX_META_DATA_FORMATS + 1];
+	u32 meta_formats[UVC_MAX_META_DATA_FORMATS];
+	unsigned int nmeta_formats;
 
 	atomic_t nmappings;
 
-- 
2.49.0

Re: [PATCH] media: uvcvideo: Use a count variable for meta_formats instead of 0 terminating

[Ricardo Ribalda]

Hi Hans

Indeed it looks nicer, thanks :)

On Tue, 8 Jul 2025 at 12:46, Hans de Goede <hansg@kernel.org> wrote:
>
> The code dealing with the 0 terminated meta_formats array is a bit klunky
> especially for the uvc_meta_v4l2_enum_formats() case.
>
> Instead of 0 terminating add an unsigned int nmeta_formats member to struct
> uvc_device and use that. This leads to slightly cleaner code.
>
Reviewed-by: Ricardo Ribalda <ribalda@chrium.org>
Tested-by: Ricardo Ribalda <ribalda@chromium.org> # Camera with
MSXU_CONTROL_METADATA
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/usb/uvc/uvc_metadata.c | 21 +++++++++------------
>  drivers/media/usb/uvc/uvcvideo.h     |  4 ++--
>  2 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> index 12972527ab8d..229e08ff323e 100644
> --- a/drivers/media/usb/uvc/uvc_metadata.c
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -64,17 +64,16 @@ static int uvc_meta_v4l2_try_format(struct file *file, void *fh,
>         struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
>         struct uvc_device *dev = stream->dev;
>         struct v4l2_meta_format *fmt = &format->fmt.meta;
> -       u32 fmeta = fmt->dataformat;
> -       u32 i;
> +       u32 fmeta = V4L2_META_FMT_UVC;
>
>         if (format->type != vfh->vdev->queue->type)
>                 return -EINVAL;
>
> -       for (i = 0; (fmeta != dev->meta_formats[i]) && dev->meta_formats[i];
> -            i++)
> -               ;
> -       if (!dev->meta_formats[i])
> -               fmeta = V4L2_META_FMT_UVC;
Super nitpick:
I'd have placed this here: and not at the beginning of the function,
to make clear my intentions.

 fmeta = V4L2_META_FMT_UVC;
> +       for (unsigned int i = 0; i < dev->nmeta_formats; i++)
> +               if (dev->meta_formats[i] == fmt->dataformat) {
> +                       fmeta = fmt->dataformat;
> +                       break;
> +               }
>
>         memset(fmt, 0, sizeof(*fmt));
>
> @@ -119,14 +118,12 @@ static int uvc_meta_v4l2_enum_formats(struct file *file, void *fh,
>         struct v4l2_fh *vfh = file->private_data;
>         struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
>         struct uvc_device *dev = stream->dev;
> -       u32 i;
> +       u32 i = fdesc->index;
>
>         if (fdesc->type != vfh->vdev->queue->type)
>                 return -EINVAL;
>
> -       for (i = 0; (i < fdesc->index) && dev->meta_formats[i]; i++)
> -               ;
> -       if (!dev->meta_formats[i])
> +       if (i >= dev->nmeta_formats)
>                 return -EINVAL;
>
>         memset(fdesc, 0, sizeof(*fdesc));
> @@ -265,7 +262,7 @@ int uvc_meta_init(struct uvc_device *dev)
>                 dev->meta_formats[i++] = V4L2_META_FMT_UVC_MSXU_1_5;
>
>          /* IMPORTANT: for new meta-formats update UVC_MAX_META_DATA_FORMATS. */
> -       dev->meta_formats[i++] = 0;
> +       dev->nmeta_formats = i;
>
>         return 0;
>  }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b34c1914ff39..757254fc4fe9 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -588,8 +588,8 @@ struct uvc_device {
>
>         const struct uvc_device_info *info;
>
> -       /* Zero-ended list of meta formats */
> -       u32 meta_formats[UVC_MAX_META_DATA_FORMATS + 1];
> +       u32 meta_formats[UVC_MAX_META_DATA_FORMATS];
> +       unsigned int nmeta_formats;
>
>         atomic_t nmappings;
>
> --
> 2.49.0
>


-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: Use a count variable for meta_formats instead of 0 terminating

[Hans de Goede]

Hi Ricardo,

On 8-Jul-25 1:42 PM, Ricardo Ribalda wrote:
> Hi Hans
> 
> Indeed it looks nicer, thanks :)
> 
> On Tue, 8 Jul 2025 at 12:46, Hans de Goede <hansg@kernel.org> wrote:
>>
>> The code dealing with the 0 terminated meta_formats array is a bit klunky
>> especially for the uvc_meta_v4l2_enum_formats() case.
>>
>> Instead of 0 terminating add an unsigned int nmeta_formats member to struct
>> uvc_device and use that. This leads to slightly cleaner code.
>>
> Reviewed-by: Ricardo Ribalda <ribalda@chrium.org>
> Tested-by: Ricardo Ribalda <ribalda@chromium.org> # Camera with MSXU_CONTROL_METADATA

Thank you. I've pushed this to uvc/for-next now.

Regards,

Hans

Re: [PATCH] media: uvcvideo: Use a count variable for meta_formats instead of 0 terminating

[Laurent Pinchart]

Hi Hans,

A few comments here. I see you've included this in your pull request
today, so I'll send patch on top. No need for any action on your side
(except perhaps reviewing the patches :-)).

On Tue, Jul 08, 2025 at 12:46:22PM +0200, Hans de Goede wrote:
> The code dealing with the 0 terminated meta_formats array is a bit klunky
> especially for the uvc_meta_v4l2_enum_formats() case.
> 
> Instead of 0 terminating add an unsigned int nmeta_formats member to struct
> uvc_device and use that. This leads to slightly cleaner code.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
>  drivers/media/usb/uvc/uvc_metadata.c | 21 +++++++++------------
>  drivers/media/usb/uvc/uvcvideo.h     |  4 ++--
>  2 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> index 12972527ab8d..229e08ff323e 100644
> --- a/drivers/media/usb/uvc/uvc_metadata.c
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -64,17 +64,16 @@ static int uvc_meta_v4l2_try_format(struct file *file, void *fh,
>  	struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
>  	struct uvc_device *dev = stream->dev;
>  	struct v4l2_meta_format *fmt = &format->fmt.meta;
> -	u32 fmeta = fmt->dataformat;
> -	u32 i;
> +	u32 fmeta = V4L2_META_FMT_UVC;
>  
>  	if (format->type != vfh->vdev->queue->type)
>  		return -EINVAL;
>  
> -	for (i = 0; (fmeta != dev->meta_formats[i]) && dev->meta_formats[i];
> -	     i++)
> -		;
> -	if (!dev->meta_formats[i])
> -		fmeta = V4L2_META_FMT_UVC;
> +	for (unsigned int i = 0; i < dev->nmeta_formats; i++)
> +		if (dev->meta_formats[i] == fmt->dataformat) {
> +			fmeta = fmt->dataformat;
> +			break;
> +		}

Missing curcly braces for the for loop.

>  
>  	memset(fmt, 0, sizeof(*fmt));

We can actually drop the memset, as the V4L2 ioctl core already zeroes
fmt->fmt.

>  
> @@ -119,14 +118,12 @@ static int uvc_meta_v4l2_enum_formats(struct file *file, void *fh,
>  	struct v4l2_fh *vfh = file->private_data;
>  	struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
>  	struct uvc_device *dev = stream->dev;
> -	u32 i;
> +	u32 i = fdesc->index;
>  
>  	if (fdesc->type != vfh->vdev->queue->type)
>  		return -EINVAL;
>  
> -	for (i = 0; (i < fdesc->index) && dev->meta_formats[i]; i++)
> -		;
> -	if (!dev->meta_formats[i])
> +	if (i >= dev->nmeta_formats)
>  		return -EINVAL;
>  
>  	memset(fdesc, 0, sizeof(*fdesc));

Same here, we can drop the memset, which will lead to simplifications in
the function.

> @@ -265,7 +262,7 @@ int uvc_meta_init(struct uvc_device *dev)
>  		dev->meta_formats[i++] = V4L2_META_FMT_UVC_MSXU_1_5;
>  
>  	 /* IMPORTANT: for new meta-formats update UVC_MAX_META_DATA_FORMATS. */
> -	dev->meta_formats[i++] = 0;
> +	dev->nmeta_formats = i;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b34c1914ff39..757254fc4fe9 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -588,8 +588,8 @@ struct uvc_device {
>  
>  	const struct uvc_device_info *info;
>  
> -	/* Zero-ended list of meta formats */
> -	u32 meta_formats[UVC_MAX_META_DATA_FORMATS + 1];
> +	u32 meta_formats[UVC_MAX_META_DATA_FORMATS];
> +	unsigned int nmeta_formats;
>  
>  	atomic_t nmappings;
>  

-- 
Regards,

Laurent Pinchart