| Message ID | 20221228014200.20462-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] media: uvcvideo: Remove format descriptions | expand |
Hi Laurent On Wed, 28 Dec 2022 at 02:42, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > The V4L2 core fills format description on its own in v4l_fill_fmtdesc(), Maybe make more explicit the rewrite, always fills > there's no need to manually set the descriptions in the driver. This > prepares for removal of the format descriptions from the uvc_fmts table. > With or without the nits: Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > Michael, I'd like to merge this before the "[PATCH v2 5/5] usb: uvc: use > v4l2_fill_fmtdesc instead of open coded format name" patch you have > submitted, if that's OK with you. It removes the need to call > v4l2_fill_fmtdesc() in that patch, which will also allow dropping > "[PATCH v2 4/5] media: v4l2: move v4l_fill_fmtdesc to common > v4l2_fill_fmtdesc function" in that series. I think we can schedule all > of that for v6.3. > > --- > drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++-------------- > drivers/media/usb/uvc/uvc_v4l2.c | 2 -- > drivers/media/usb/uvc/uvcvideo.h | 2 -- > 3 files changed, 13 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 8132a5c3bc33..6de3d55daf36 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -227,6 +227,7 @@ 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, }; What about char fmtname[] = "\0SDL-DV 60Hz"; /* Longest possible string. */ > unsigned int width_multiplier = 1; > unsigned int interval; > unsigned int i, n; > @@ -251,14 +252,10 @@ static int uvc_parse_format(struct uvc_device *dev, > fmtdesc = uvc_format_by_guid(&buffer[5]); > > if (fmtdesc != NULL) { > - strscpy(format->name, fmtdesc->name, > - sizeof(format->name)); > format->fcc = fmtdesc->fcc; > } else { > dev_info(&streaming->intf->dev, > - "Unknown video format %pUl\n", &buffer[5]); > - snprintf(format->name, sizeof(format->name), "%pUl\n", > - &buffer[5]); > + "Unknown video format %p4cc\n", &buffer[5]); > format->fcc = 0; > } > > @@ -270,8 +267,6 @@ static int uvc_parse_format(struct uvc_device *dev, > */ > if (dev->quirks & UVC_QUIRK_FORCE_Y8) { > if (format->fcc == V4L2_PIX_FMT_YUYV) { > - strscpy(format->name, "Greyscale 8-bit (Y8 )", > - sizeof(format->name)); > format->fcc = V4L2_PIX_FMT_GREY; > format->bpp = 8; > width_multiplier = 2; > @@ -312,7 +307,6 @@ static int uvc_parse_format(struct uvc_device *dev, > return -EINVAL; > } > > - strscpy(format->name, "MJPEG", sizeof(format->name)); > format->fcc = V4L2_PIX_FMT_MJPEG; > format->flags = UVC_FMT_FLAG_COMPRESSED; > format->bpp = 0; > @@ -330,13 +324,13 @@ static int uvc_parse_format(struct uvc_device *dev, > > switch (buffer[8] & 0x7f) { > case 0: > - strscpy(format->name, "SD-DV", sizeof(format->name)); > + strscpy(fmtname, "SD-DV", sizeof(fmtname)); > break; > case 1: > - strscpy(format->name, "SDL-DV", sizeof(format->name)); > + strscpy(fmtname, "SDL-DV", sizeof(fmtname)); > break; > case 2: > - strscpy(format->name, "HD-DV", sizeof(format->name)); > + strscpy(fmtname, "HD-DV", sizeof(fmtname)); > break; > default: > uvc_dbg(dev, DESCR, > @@ -346,8 +340,8 @@ static int uvc_parse_format(struct uvc_device *dev, > return -EINVAL; > } > > - strlcat(format->name, buffer[8] & (1 << 7) ? " 60Hz" : " 50Hz", > - sizeof(format->name)); > + 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; > @@ -375,7 +369,12 @@ static int uvc_parse_format(struct uvc_device *dev, > return -EINVAL; > } > > - uvc_dbg(dev, DESCR, "Found format %s\n", format->name); What about simply: uvc_dbg(dev, DESCR, "Found format %p4cc %s\n", &format->fcc, fmtname); Even when format->fcc is zero, specially in uvc_dbg is nice to have that info. Thanks! > + 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); > + } > > buflen -= buffer[0]; > buffer += buffer[0]; > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index f4d4c33b6dfb..dcd178d249b6 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -660,8 +660,6 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream, > fmt->flags = 0; > if (format->flags & UVC_FMT_FLAG_COMPRESSED) > fmt->flags |= V4L2_FMT_FLAG_COMPRESSED; > - strscpy(fmt->description, format->name, sizeof(fmt->description)); > - fmt->description[sizeof(fmt->description) - 1] = 0; > fmt->pixelformat = format->fcc; > return 0; > } > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 84326991ec36..cd5861cae3b0 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -262,8 +262,6 @@ struct uvc_format { > u32 fcc; > u32 flags; > > - char name[32]; > - > unsigned int nframes; > struct uvc_frame *frame; > }; > -- > Regards, > > Laurent Pinchart >
Hi Ricardo, On Wed, Jan 04, 2023 at 11:37:33AM +0100, Ricardo Ribalda wrote: > On Wed, 28 Dec 2022 at 02:42, Laurent Pinchart wrote: > > > > The V4L2 core fills format description on its own in v4l_fill_fmtdesc(), > > Maybe make more explicit the rewrite, always fills Sure. > > there's no need to manually set the descriptions in the driver. This > > prepares for removal of the format descriptions from the uvc_fmts table. > > With or without the nits: > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > Michael, I'd like to merge this before the "[PATCH v2 5/5] usb: uvc: use > > v4l2_fill_fmtdesc instead of open coded format name" patch you have > > submitted, if that's OK with you. It removes the need to call > > v4l2_fill_fmtdesc() in that patch, which will also allow dropping > > "[PATCH v2 4/5] media: v4l2: move v4l_fill_fmtdesc to common > > v4l2_fill_fmtdesc function" in that series. I think we can schedule all > > of that for v6.3. > > > > --- > > drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++-------------- > > drivers/media/usb/uvc/uvc_v4l2.c | 2 -- > > drivers/media/usb/uvc/uvcvideo.h | 2 -- > > 3 files changed, 13 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > index 8132a5c3bc33..6de3d55daf36 100644 > > --- a/drivers/media/usb/uvc/uvc_driver.c > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > @@ -227,6 +227,7 @@ 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, }; > > What about > char fmtname[] = "\0SDL-DV 60Hz"; /* Longest possible string. */ I'm a bit tempted, but I think it will just increase the kernel size a little bit for no real gain. We will still need to keep this in sync manually with any change we may make below in the future. > > unsigned int width_multiplier = 1; > > unsigned int interval; > > unsigned int i, n; > > @@ -251,14 +252,10 @@ static int uvc_parse_format(struct uvc_device *dev, > > fmtdesc = uvc_format_by_guid(&buffer[5]); > > > > if (fmtdesc != NULL) { > > - strscpy(format->name, fmtdesc->name, > > - sizeof(format->name)); > > format->fcc = fmtdesc->fcc; > > } else { > > dev_info(&streaming->intf->dev, > > - "Unknown video format %pUl\n", &buffer[5]); > > - snprintf(format->name, sizeof(format->name), "%pUl\n", > > - &buffer[5]); > > + "Unknown video format %p4cc\n", &buffer[5]); > > format->fcc = 0; > > } > > > > @@ -270,8 +267,6 @@ static int uvc_parse_format(struct uvc_device *dev, > > */ > > if (dev->quirks & UVC_QUIRK_FORCE_Y8) { > > if (format->fcc == V4L2_PIX_FMT_YUYV) { > > - strscpy(format->name, "Greyscale 8-bit (Y8 )", > > - sizeof(format->name)); > > format->fcc = V4L2_PIX_FMT_GREY; > > format->bpp = 8; > > width_multiplier = 2; > > @@ -312,7 +307,6 @@ static int uvc_parse_format(struct uvc_device *dev, > > return -EINVAL; > > } > > > > - strscpy(format->name, "MJPEG", sizeof(format->name)); > > format->fcc = V4L2_PIX_FMT_MJPEG; > > format->flags = UVC_FMT_FLAG_COMPRESSED; > > format->bpp = 0; > > @@ -330,13 +324,13 @@ static int uvc_parse_format(struct uvc_device *dev, > > > > switch (buffer[8] & 0x7f) { > > case 0: > > - strscpy(format->name, "SD-DV", sizeof(format->name)); > > + strscpy(fmtname, "SD-DV", sizeof(fmtname)); > > break; > > case 1: > > - strscpy(format->name, "SDL-DV", sizeof(format->name)); > > + strscpy(fmtname, "SDL-DV", sizeof(fmtname)); > > break; > > case 2: > > - strscpy(format->name, "HD-DV", sizeof(format->name)); > > + strscpy(fmtname, "HD-DV", sizeof(fmtname)); > > break; > > default: > > uvc_dbg(dev, DESCR, > > @@ -346,8 +340,8 @@ static int uvc_parse_format(struct uvc_device *dev, > > return -EINVAL; > > } > > > > - strlcat(format->name, buffer[8] & (1 << 7) ? " 60Hz" : " 50Hz", > > - sizeof(format->name)); > > + 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; > > @@ -375,7 +369,12 @@ static int uvc_parse_format(struct uvc_device *dev, > > return -EINVAL; > > } > > > > - uvc_dbg(dev, DESCR, "Found format %s\n", format->name); > > What about simply: > > uvc_dbg(dev, DESCR, "Found format %p4cc %s\n", &format->fcc, fmtname); > > Even when format->fcc is zero, specially in uvc_dbg is nice to have that info. Note that, when format->fcc is zero, there's already a louder dev_info(&streaming->intf->dev, "Unknown video format %p4cc\n", &buffer[5]); above, and fmtname will not be set. Your proposal would then print "Found format .... (0x00000000) \n" which I don't think would add much value. Another option would be to simply drop the format name for DV formats. It adds little value, and would simplify the code. I think I'll do so in v2. This also makes me realize I shouldn't have changed the dev_info() to %p4cc. I'll fix that in a v2. > > + 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); > > + } > > > > buflen -= buffer[0]; > > buffer += buffer[0]; > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index f4d4c33b6dfb..dcd178d249b6 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -660,8 +660,6 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream, > > fmt->flags = 0; > > if (format->flags & UVC_FMT_FLAG_COMPRESSED) > > fmt->flags |= V4L2_FMT_FLAG_COMPRESSED; > > - strscpy(fmt->description, format->name, sizeof(fmt->description)); > > - fmt->description[sizeof(fmt->description) - 1] = 0; > > fmt->pixelformat = format->fcc; > > return 0; > > } > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 84326991ec36..cd5861cae3b0 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -262,8 +262,6 @@ struct uvc_format { > > u32 fcc; > > u32 flags; > > > > - char name[32]; > > - > > unsigned int nframes; > > struct uvc_frame *frame; > > };
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 8132a5c3bc33..6de3d55daf36 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -227,6 +227,7 @@ 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; @@ -251,14 +252,10 @@ static int uvc_parse_format(struct uvc_device *dev, fmtdesc = uvc_format_by_guid(&buffer[5]); if (fmtdesc != NULL) { - strscpy(format->name, fmtdesc->name, - sizeof(format->name)); format->fcc = fmtdesc->fcc; } else { dev_info(&streaming->intf->dev, - "Unknown video format %pUl\n", &buffer[5]); - snprintf(format->name, sizeof(format->name), "%pUl\n", - &buffer[5]); + "Unknown video format %p4cc\n", &buffer[5]); format->fcc = 0; } @@ -270,8 +267,6 @@ static int uvc_parse_format(struct uvc_device *dev, */ if (dev->quirks & UVC_QUIRK_FORCE_Y8) { if (format->fcc == V4L2_PIX_FMT_YUYV) { - strscpy(format->name, "Greyscale 8-bit (Y8 )", - sizeof(format->name)); format->fcc = V4L2_PIX_FMT_GREY; format->bpp = 8; width_multiplier = 2; @@ -312,7 +307,6 @@ static int uvc_parse_format(struct uvc_device *dev, return -EINVAL; } - strscpy(format->name, "MJPEG", sizeof(format->name)); format->fcc = V4L2_PIX_FMT_MJPEG; format->flags = UVC_FMT_FLAG_COMPRESSED; format->bpp = 0; @@ -330,13 +324,13 @@ static int uvc_parse_format(struct uvc_device *dev, switch (buffer[8] & 0x7f) { case 0: - strscpy(format->name, "SD-DV", sizeof(format->name)); + strscpy(fmtname, "SD-DV", sizeof(fmtname)); break; case 1: - strscpy(format->name, "SDL-DV", sizeof(format->name)); + strscpy(fmtname, "SDL-DV", sizeof(fmtname)); break; case 2: - strscpy(format->name, "HD-DV", sizeof(format->name)); + strscpy(fmtname, "HD-DV", sizeof(fmtname)); break; default: uvc_dbg(dev, DESCR, @@ -346,8 +340,8 @@ static int uvc_parse_format(struct uvc_device *dev, return -EINVAL; } - strlcat(format->name, buffer[8] & (1 << 7) ? " 60Hz" : " 50Hz", - sizeof(format->name)); + 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; @@ -375,7 +369,12 @@ static int uvc_parse_format(struct uvc_device *dev, return -EINVAL; } - uvc_dbg(dev, DESCR, "Found format %s\n", format->name); + 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); + } buflen -= buffer[0]; buffer += buffer[0]; diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index f4d4c33b6dfb..dcd178d249b6 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -660,8 +660,6 @@ static int uvc_ioctl_enum_fmt(struct uvc_streaming *stream, fmt->flags = 0; if (format->flags & UVC_FMT_FLAG_COMPRESSED) fmt->flags |= V4L2_FMT_FLAG_COMPRESSED; - strscpy(fmt->description, format->name, sizeof(fmt->description)); - fmt->description[sizeof(fmt->description) - 1] = 0; fmt->pixelformat = format->fcc; return 0; } diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 84326991ec36..cd5861cae3b0 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -262,8 +262,6 @@ struct uvc_format { u32 fcc; u32 flags; - char name[32]; - unsigned int nframes; struct uvc_frame *frame; };
The V4L2 core fills format description on its own in v4l_fill_fmtdesc(), there's no need to manually set the descriptions in the driver. This prepares for removal of the format descriptions from the uvc_fmts table. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Michael, I'd like to merge this before the "[PATCH v2 5/5] usb: uvc: use v4l2_fill_fmtdesc instead of open coded format name" patch you have submitted, if that's OK with you. It removes the need to call v4l2_fill_fmtdesc() in that patch, which will also allow dropping "[PATCH v2 4/5] media: v4l2: move v4l_fill_fmtdesc to common v4l2_fill_fmtdesc function" in that series. I think we can schedule all of that for v6.3. --- drivers/media/usb/uvc/uvc_driver.c | 27 +++++++++++++-------------- drivers/media/usb/uvc/uvc_v4l2.c | 2 -- drivers/media/usb/uvc/uvcvideo.h | 2 -- 3 files changed, 13 insertions(+), 18 deletions(-)