| Message ID | 4b1bc0d5-808b-816d-d7de-5baa8851e74f@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: usb: uvc: fill in description for unknown pixelformats | expand |
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. Is there an app that uses this unknown format code ? Or the only complaint is that WARN() is too loud for the user? Regards! On Wed, 29 Mar 2023 at 14:39, Hans Verkuil <hverkuil@xs4all.nl> 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? 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]; > + > unsigned int nframes; > struct uvc_frame *frame; > }; > -- Ricardo Ribalda
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. > > Regards! > > On Wed, 29 Mar 2023 at 14:39, Hans Verkuil <hverkuil@xs4all.nl> 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]; >> + >> unsigned int nframes; >> struct uvc_frame *frame; >> }; >> > > > -- > Ricardo Ribalda
Hi, On 3/29/23 14:28, 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") Thanks, patch looks good to me: FWIW: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > 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]); > + > 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]; > + > unsigned int nframes; > struct uvc_frame *frame; > }; >
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; > >> };
On 29.03.23 14:28, 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") Thx for working on this. Would be good to have Reported-by and Link tags for any reports about the issue; I'm aware of the following two, maybe there were more: https://bugzilla.kernel.org/show_bug.cgi?id=217252 https://bugzilla.redhat.com/show_bug.cgi?id=2180107 And a Cc: <stable@vger.kernel.org> # 5.15.x would likely be good as well, as the culprit was backported. But I write for a different reason: how urgent is this fix? Is this "just" fixing a kernel warning, or do users notice this as some apps crash? The bugzilla.redhat.com ticket's subject indicates it's the latter; and I think I saw someone else mentioning that this leads to crashes, but maybe I'm mixing things up. Because if this fixes a crash, it afaics would be good to get this fixed rather sooner than later in mainline, so that it can be fixed in stable as well. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. > --- > 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]); > + > 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]; > + > unsigned int nframes; > struct uvc_frame *frame; > }; > > >
Hi Thorsten, On Tue, Apr 11, 2023 at 01:49:03PM +0200, Thorsten Leemhuis wrote: > On 29.03.23 14:28, 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") > > Thx for working on this. > > Would be good to have Reported-by and Link tags for any reports about > the issue; I'm aware of the following two, maybe there were more: > > https://bugzilla.kernel.org/show_bug.cgi?id=217252 > https://bugzilla.redhat.com/show_bug.cgi?id=2180107 > > And a Cc: <stable@vger.kernel.org> # 5.15.x would likely be good as > well, as the culprit was backported. > > But I write for a different reason: how urgent is this fix? Is this > "just" fixing a kernel warning, or do users notice this as some apps > crash? The bugzilla.redhat.com ticket's subject indicates it's the > latter; and I think I saw someone else mentioning that this leads to > crashes, but maybe I'm mixing things up. > > Because if this fixes a crash, it afaics would be good to get this fixed > rather sooner than later in mainline, so that it can be fixed in stable > as well. As far as I understand, it fixes a WARN_ON(). That's technically not a crash, but it looks like a big scary one to anyone watching the kernel log. > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > -- > Everything you wanna know about Linux kernel regression tracking: > https://linux-regtracking.leemhuis.info/about/#tldr > If I did something stupid, please tell me, as explained on that page. > > > --- > > 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]); > > + > > 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]; > > + > > unsigned int nframes; > > struct uvc_frame *frame; > > };
Hi Thorsten On Tue, 11 Apr 2023 at 13:51, Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > On 29.03.23 14:28, 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") > > Thx for working on this. > > Would be good to have Reported-by and Link tags for any reports about > the issue; I'm aware of the following two, maybe there were more: > > https://bugzilla.kernel.org/show_bug.cgi?id=217252 > https://bugzilla.redhat.com/show_bug.cgi?id=2180107 > > And a Cc: <stable@vger.kernel.org> # 5.15.x would likely be good as > well, as the culprit was backported. > > But I write for a different reason: how urgent is this fix? Is this > "just" fixing a kernel warning, or do users notice this as some apps > crash? The bugzilla.redhat.com ticket's subject indicates it's the > latter; and I think I saw someone else mentioning that this leads to > crashes, but maybe I'm mixing things up. I think it only a crash if panic_on_warn is enabled. > > Because if this fixes a crash, it afaics would be good to get this fixed > rather sooner than later in mainline, so that it can be fixed in stable > as well. > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > -- > Everything you wanna know about Linux kernel regression tracking: > https://linux-regtracking.leemhuis.info/about/#tldr > If I did something stupid, please tell me, as explained on that page. > > > > --- > > 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]); > > + > > 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]; > > + > > unsigned int nframes; > > struct uvc_frame *frame; > > }; > > > > > >
On 11.04.23 14:06, Laurent Pinchart wrote: > Hi Thorsten, > > On Tue, Apr 11, 2023 at 01:49:03PM +0200, Thorsten Leemhuis wrote: >> On 29.03.23 14:28, 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") >> >> Thx for working on this. >> >> Would be good to have Reported-by and Link tags for any reports about >> the issue; I'm aware of the following two, maybe there were more: >> >> https://bugzilla.kernel.org/show_bug.cgi?id=217252 >> https://bugzilla.redhat.com/show_bug.cgi?id=2180107 >> >> And a Cc: <stable@vger.kernel.org> # 5.15.x would likely be good as >> well, as the culprit was backported. >> >> But I write for a different reason: how urgent is this fix? Is this >> "just" fixing a kernel warning, or do users notice this as some apps >> crash? The bugzilla.redhat.com ticket's subject indicates it's the >> latter; and I think I saw someone else mentioning that this leads to >> crashes, but maybe I'm mixing things up. >> >> Because if this fixes a crash, it afaics would be good to get this fixed >> rather sooner than later in mainline, so that it can be fixed in stable >> as well. > > As far as I understand, it fixes a WARN_ON(). That's technically not a > crash, but it looks like a big scary one to anyone watching the kernel > log. Thx for clarifying. I know what WARN_ON() does, but for some reason was under the strong impression that one of the reports mentioned the video app was crashing in parallel to the kernel warning. Well, guess I mixed this up with some other regression. Sorry for the noise! Ciao, Thorsten >>> --- >>> 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]); >>> + >>> 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]; >>> + >>> unsigned int nframes; >>> struct uvc_frame *frame; >>> }; >
Hi Hans, On Wed, Apr 05, 2023 at 09:40:20AM +0300, Laurent Pinchart wrote: > 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. Any opinion on this ? I'm increasingly tempted by not reporting unsupported formats to userspace. > > >> unsigned int nframes; > > >> struct uvc_frame *frame; > > >> };
On 19/04/2023 08:23, Laurent Pinchart wrote: > Hi Hans, > > On Wed, Apr 05, 2023 at 09:40:20AM +0300, Laurent Pinchart wrote: >> 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. > > Any opinion on this ? I'm increasingly tempted by not reporting > unsupported formats to userspace. Since they cannot be selected, I think that is the best approach. Perhaps only show them in the kernel log if debugging is enabled. Regards, Hans > >>>>> unsigned int nframes; >>>>> struct uvc_frame *frame; >>>>> }; >
On 29.03.23 14:28, 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. What happened to this? It seems this fall through the cracks. BTW: > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Afaics it might be good to have these in here: Link: https://bugzilla.kernel.org/show_bug.cgi?id=217252 Link: https://bugzilla.redhat.com/show_bug.cgi?id=2180107 A comment in the former is what brought me here. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page. #regzbot ^backmonitor: https://lore.kernel.org/lkml/dc8e5276-ef88-648f-9f0d-10151ea62c90@leemhuis.info/ #regzbot poke > 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]); > + > 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]; > + > unsigned int nframes; > struct uvc_frame *frame; > }; >
Hi Thorsten, On Wed, May 31, 2023 at 01:48:51PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote: > On 29.03.23 14:28, 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. > > What happened to this? It seems this fall through the cracks. I've posted a better fix, see https://lore.kernel.org/linux-media/20230506065809.24645-1-laurent.pinchart@ideasonboard.com/. Of course, fate had it that it got reviewed exactly on the day when I started by holidays :-S I'm now back, and will send a pull request. > BTW: > > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Afaics it might be good to have these in here: > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217252 > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2180107 I'll add those two links. > A comment in the former is what brought me here. > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > -- > Everything you wanna know about Linux kernel regression tracking: > https://linux-regtracking.leemhuis.info/about/#tldr > If I did something stupid, please tell me, as explained on that page. > > #regzbot ^backmonitor: > https://lore.kernel.org/lkml/dc8e5276-ef88-648f-9f0d-10151ea62c90@leemhuis.info/ > #regzbot poke > > > 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]); > > + > > 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]; > > + > > unsigned int nframes; > > struct uvc_frame *frame; > > };
On 31.05.23 14:44, Laurent Pinchart wrote: > Hi Thorsten, > > On Wed, May 31, 2023 at 01:48:51PM +0200, Linux regression tracking (Thorsten Leemhuis) wrote: >> On 29.03.23 14:28, 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. >> >> What happened to this? It seems this fall through the cracks. > > I've posted a better fix, You mean an even better fix, as this one already was a better fix for a earlier fix ;) > see > https://lore.kernel.org/linux-media/20230506065809.24645-1-laurent.pinchart@ideasonboard.com/. > Of course, fate had it that it got reviewed exactly on the day when I started by holidays :-S :-D > I'm now back, and will send a pull request. In that case: welcome back. >> BTW: >> >>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> Afaics it might be good to have these in here: >> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217252 >> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2180107 > > I'll add those two links. Great, many thx! Ciao, Thorsten >> A comment in the former is what brought me here. >> >> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) >> -- >> Everything you wanna know about Linux kernel regression tracking: >> https://linux-regtracking.leemhuis.info/about/#tldr >> If I did something stupid, please tell me, as explained on that page. >> >> #regzbot ^backmonitor: >> https://lore.kernel.org/lkml/dc8e5276-ef88-648f-9f0d-10151ea62c90@leemhuis.info/ >> #regzbot poke >> >>> 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]); >>> + >>> 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]; >>> + >>> 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 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]); + 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]; + unsigned int nframes; struct uvc_frame *frame; };
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") ---