| Message ID | 20230104111944.962-3-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvcvideo: Drop format descriptions | expand |
Hi Laurent For what it's worth, I am pro squash :) On Wed, 4 Jan 2023 at 12:19, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 ;) > + if (format->fcc) > + uvc_dbg(dev, DESCR, "Found format %p4cc", &format->fcc); > > buflen -= buffer[0]; > buffer += buffer[0]; > -- > Regards, > > Laurent Pinchart >
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];
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); - } + if (format->fcc) + uvc_dbg(dev, DESCR, "Found format %p4cc", &format->fcc); buflen -= buffer[0]; buffer += buffer[0];
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> --- drivers/media/usb/uvc/uvc_driver.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)