Re: [PATCH] media: usb: uvc: fill in description for unknown pixelformats

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Wed, Mar 29, 2023 at 07:20:59PM +0200, Hans Verkuil wrote:
> On 29/03/2023 18:05, Ricardo Ribalda wrote:
> > Hi Hans
> > 
> > Thanks for the patch.
> > 
> > I believe the user can fetch this info from lsusb, so this is kind of
> > duplicated info, and this is why it was removed.
> 
> You got to set some description, so using the GUID this seems best.
> 
> > Is there an app that uses this unknown format code ? Or the only
> > complaint is that WARN() is too loud for the user?
> 
> Normally drivers do not pass on unknown formats, but if a driver does,
> then I want a WARN. If a driver does this legitimately (and I understand
> that's the case for UVC), then the driver should fill in the description
> to avoid this WARN.

In hindsight we shouldn't have added a text description to formats :-)

> > On Wed, 29 Mar 2023 at 14:39, Hans Verkuil wrote:
> >>
> >> If the fcc is 0 (indicating an unknown GUID format), then fill in the
> >> description field in ENUM_FMT. Otherwise the V4L2 core will WARN.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> Fixes: 50459f103edf ("media: uvcvideo: Remove format descriptions")
> >> ---
> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> >> index 7aefa76a42b3..2f1ced1212cd 100644
> >> --- a/drivers/media/usb/uvc/uvc_driver.c
> >> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >> @@ -256,6 +256,9 @@ static int uvc_parse_format(struct uvc_device *dev,
> >>                 } else {
> >>                         dev_info(&streaming->intf->dev,
> >>                                  "Unknown video format %pUl\n", &buffer[5]);
> >> +                       snprintf(format->name, sizeof(format->name), "%pUl\n",
> >> +                                &buffer[5]);
> > Don't we need at least 38 chars for this?
> 
> Yes. But all we have is 31 chars, so we take what we can :-)
> 
> This is what uvc did before this was removed.
> 
> Regards,
> 
> 	Hans
> 
> > https://docs.kernel.org/core-api/printk-formats.html#uuid-guid-addresses
> > 
> >> +
> >>                         format->fcc = 0;
> >>                 }
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> >> index 35453f81c1d9..fc6f9e7d8506 100644
> >> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> >> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >> @@ -713,6 +713,10 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream,
> >>         if (format->flags & UVC_FMT_FLAG_COMPRESSED)
> >>                 fmt->flags |= V4L2_FMT_FLAG_COMPRESSED;
> >>         fmt->pixelformat = format->fcc;
> >> +       if (format->name[0])
> >> +               strscpy(fmt->description, format->name,
> >> +                       sizeof(fmt->description));
> >> +
> >>         return 0;
> >>  }
> >>
> >> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> >> index 9a596c8d894a..22656755a801 100644
> >> --- a/drivers/media/usb/uvc/uvcvideo.h
> >> +++ b/drivers/media/usb/uvc/uvcvideo.h
> >> @@ -264,6 +264,8 @@ struct uvc_format {
> >>         u32 fcc;
> >>         u32 flags;
> >>
> >> +       char name[32];
> >> +

I'd not really nice to have to store the name for every format, when we
know it will very rarely be used.

One alternative option would be to store the GUID, which would halve the
amount of memory. Another option would be to stop reporting those
formats to userspace in uvc_ioctl_enum_fmt(). They can't be selected
anyway, they have no unique 4CC.

> >>         unsigned int nframes;
> >>         struct uvc_frame *frame;
> >>  };

-- 
Regards,

Laurent Pinchart