Hi Ricardo,
On Mon, May 01, 2023 at 01:44:11PM +0200, Ricardo Ribalda wrote:
> Hi Laurent
>
> I agree with the intent of the patch but am not sure that this will work.
>
> Regards!
>
> On Thu, 20 Apr 2023 at 12:07, Laurent Pinchart wrote:
> >
> > When the uvcvideo driver encounters a format descriptor with an unknown
> > format GUID, it creates a corresponding struct uvc_format instance with
> > the fcc field set to 0. Since commit 50459f103edf ("media: uvcvideo:
> > Remove format descriptions"), the driver relies on the V4L2 core to
> > provide the format description string, which the V4L2 core can't do
> > without a valid 4CC. This triggers a WARN_ON.
> >
> > As a format with a zero 4CC can't be selected, it is unusable for
> > applications. Ignore the format completely without creating a uvc_format
> > instance, which fixes the warning.
> >
> > Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index de64c9d789fd..25b8199f4e82 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -251,14 +251,17 @@ static int uvc_parse_format(struct uvc_device *dev,
> > /* Find the format descriptor from its GUID. */
> > fmtdesc = uvc_format_by_guid(&buffer[5]);
> >
> > - if (fmtdesc != NULL) {
> > - format->fcc = fmtdesc->fcc;
> > - } else {
> > + if (!fmtdesc) {
> > + /*
> > + * Unknown video formats are not fatal errors, the
> > + * caller will skip this descriptor.
> > + */
> > dev_info(&streaming->intf->dev,
> > "Unknown video format %pUl\n", &buffer[5]);
> > - format->fcc = 0;
> > + return 0;
> > }
> >
> > + format->fcc = fmtdesc->fcc;
> > format->bpp = buffer[21];
> >
> > /*
> > @@ -689,6 +692,8 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> > &interval, buffer, buflen);
>
> For devices with unknown formats streaming->nformats will not be
> valid, it will be higher than the actual formats on
> streaming->formats.
Indeed. I think this could be handled quite simply:
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index d966d003632b..cc6608eb3ab9 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -679,7 +679,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
interval = (u32 *)&frame[nframes];
streaming->formats = format;
- streaming->nformats = nformats;
+ streaming->nformats = 0;
/* Parse the format descriptors. */
while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE) {
@@ -695,6 +695,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
if (!ret)
break;
+ streaming->nformats++;
frame += format->nframes;
format++;
What do you think ? I can submit a v2 with this change.
> > if (ret < 0)
> > goto error;
> > + if (!ret)
> > + break;
> >
> > frame += format->nframes;
> > format++;
--
Regards,
Laurent Pinchart
