Re: [PATCH v2 2/2] media: uvcvideo: Drop custom format names for DV formats

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

Hi Ricardo,

On Wed, Jan 04, 2023 at 12:51:11PM +0100, Ricardo Ribalda wrote:
> Hi Laurent
> 
> For what it's worth, I am pro squash :)

Works for me. I'll give a few more days for people to comment and then
I'll send a squashed v3.

> On Wed, 4 Jan 2023 at 12:19, Laurent Pinchart wrote:
> >
> > Unlike V4L2, UVC makes a distinction between the SD-DV, SDL-DV and HD-DV
> > formats. It also indicates whether the DV format uses 50Hz or 60Hz. This
> > information is parsed by the driver to construct a format name string
> > that is printed in a debug message, but serves no other purpose as V4L2
> > has a single V4L2_PIX_FMT_DV pixel format that covers all those cases.
> >
> > As the information is available in the UVC descriptors, and thus
> > accessible to users with lsusb if they really care, don't log it in a
> > debug message and drop the format name string to simplify the code.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> With or without my nits
> 
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 18 +++---------------
> >  1 file changed, 3 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 9852d6f63ae8..ba41f13a2491 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -228,7 +228,6 @@ static int uvc_parse_format(struct uvc_device *dev,
> >         struct uvc_format_desc *fmtdesc;
> >         struct uvc_frame *frame;
> >         const unsigned char *start = buffer;
> > -       char fmtname[12] = { 0, };
> >         unsigned int width_multiplier = 1;
> >         unsigned int interval;
> >         unsigned int i, n;
> > @@ -325,14 +324,10 @@ static int uvc_parse_format(struct uvc_device *dev,
> >
> >                 switch (buffer[8] & 0x7f) {
> >                 case 0:
> > -                       strscpy(fmtname, "SD-DV", sizeof(fmtname));
> > -                       break;
> >                 case 1:
> > -                       strscpy(fmtname, "SDL-DV", sizeof(fmtname));
> > -                       break;
> >                 case 2:
> > -                       strscpy(fmtname, "HD-DV", sizeof(fmtname));
> >                         break;
> > +
> >                 default:
> >                         uvc_dbg(dev, DESCR,
> >                                 "device %d videostreaming interface %d: unknown DV format %u\n",
> > @@ -341,9 +336,6 @@ static int uvc_parse_format(struct uvc_device *dev,
> >                         return -EINVAL;
> >                 }
> >
> > -               strlcat(fmtname, buffer[8] & (1 << 7) ? " 60Hz" : " 50Hz",
> > -                       sizeof(fmtname));
> > -
> >                 format->fcc = V4L2_PIX_FMT_DV;
> >                 format->flags = UVC_FMT_FLAG_COMPRESSED | UVC_FMT_FLAG_STREAM;
> >                 format->bpp = 0;
> > @@ -370,12 +362,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> >                 return -EINVAL;
> >         }
> >
> > -       if (format->fcc) {
> > -               if (fmtname[0])
> > -                       uvc_dbg(dev, DESCR, "Found format %s\n", fmtname);
> > -               else
> > -                       uvc_dbg(dev, DESCR, "Found format %p4cc", &format->fcc);
> > -       }
> 
> Maybe it is the way that I debug the issues. I run an OK execution
> with a FAIl one and then I compare results. I tend to prefer that the
> extra lines are errors and there is no missing lines.... but I if your
> prefer it this way, I am ok with it ;)

Given that formats with a 0 fourcc are useless from a userspace point of
view, I think I'd rather drop them actually. That's a candidate for a
further patch.

> > +       if (format->fcc)
> > +               uvc_dbg(dev, DESCR, "Found format %p4cc", &format->fcc);
> >
> >         buflen -= buffer[0];
> >         buffer += buffer[0];

-- 
Regards,

Laurent Pinchart