| Message ID | 20241217112138.1891708-2-isaac.scott@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Set uvc_no_drop parameter for MJPEG_NO_EOF quirk | expand |
Hi Issac On Tue, 17 Dec 2024 at 12:22, Isaac Scott <isaac.scott@ideasonboard.com> wrote: > > In use cases where a camera needs to use the UVC_QUIRK_MJPEG_NO_EOF, > erroneous frames that do not conform to MJPEG standards are correctly > being marked as erroneous. However, when using the camera in a product, > manufacturers usually want to enable the quirk in order to pass the > buffers into user space. To do this, they have to enable the uvc_no_drop > parameter. To avoid the extra steps to configure the kernel in such a > way, enable the no_drop parameter by default to comply with this use > case. I am not sure what you want to do with this patch. Why can't people simply set the quirk with modprobe uvcvideo quirks=0x20000
On Tue, Dec 17, 2024 at 11:21:38AM +0000, Isaac Scott wrote: > In use cases where a camera needs to use the UVC_QUIRK_MJPEG_NO_EOF, > erroneous frames that do not conform to MJPEG standards are correctly > being marked as erroneous. However, when using the camera in a product, > manufacturers usually want to enable the quirk in order to pass the > buffers into user space. To do this, they have to enable the uvc_no_drop > parameter. To avoid the extra steps to configure the kernel in such a > way, enable the no_drop parameter by default to comply with this use > case. > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_driver.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 947c4bf6bfeb..45028b45906a 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1948,6 +1948,13 @@ int uvc_register_video_device(struct uvc_device *dev, > { > int ret; > > + /* > + * If the MJPEG stream occasionally loses the EOF marker, we set the > + * no_drop parameter by default to avoid dropping frames erroneously. > + */ > + if (dev->quirks & UVC_QUIRK_MJPEG_NO_EOF) > + uvc_no_drop_param = 1; One issue with this is that it becomes impossible for someone to unset the no_drop parameter. This being said, I think we should have enabled no_drop by default. I wonder if that's a change we could do. > + > /* Initialize the video buffers queue. */ > ret = uvc_queue_init(queue, type, !uvc_no_drop_param); > if (ret)
On Tue, 17 Dec 2024 at 12:58, Ricardo Ribalda <ribalda@chromium.org> wrote: > > Hi Issac > > > On Tue, 17 Dec 2024 at 12:22, Isaac Scott <isaac.scott@ideasonboard.com> wrote: > > > > In use cases where a camera needs to use the UVC_QUIRK_MJPEG_NO_EOF, > > erroneous frames that do not conform to MJPEG standards are correctly > > being marked as erroneous. However, when using the camera in a product, > > manufacturers usually want to enable the quirk in order to pass the > > buffers into user space. To do this, they have to enable the uvc_no_drop > > parameter. To avoid the extra steps to configure the kernel in such a > > way, enable the no_drop parameter by default to comply with this use > > case. > > I am not sure what you want to do with this patch. > > Why can't people simply set the quirk with > > modprobe uvcvideo quirks=0x20000 Sorry, I meant modprobe uvcvideo nodrop=1 or echo 1 > /sys/module/uvcvideo/parameters/nodrop Regards! -- Ricardo Ribalda
Hi Ricardo, On Tue, 2024-12-17 at 13:08 +0100, Ricardo Ribalda wrote: > On Tue, 17 Dec 2024 at 12:58, Ricardo Ribalda <ribalda@chromium.org> > wrote: > > > > Hi Issac > > > > > > On Tue, 17 Dec 2024 at 12:22, Isaac Scott > > <isaac.scott@ideasonboard.com> wrote: > > > > > > In use cases where a camera needs to use the > > > UVC_QUIRK_MJPEG_NO_EOF, > > > erroneous frames that do not conform to MJPEG standards are > > > correctly > > > being marked as erroneous. However, when using the camera in a > > > product, > > > manufacturers usually want to enable the quirk in order to pass > > > the > > > buffers into user space. To do this, they have to enable the > > > uvc_no_drop > > > parameter. To avoid the extra steps to configure the kernel in > > > such a > > > way, enable the no_drop parameter by default to comply with this > > > use > > > case. > > > > I am not sure what you want to do with this patch. > > > > Why can't people simply set the quirk with > > > > modprobe uvcvideo quirks=0x20000 > > Sorry, I meant > > modprobe uvcvideo nodrop=1 > > or > > echo 1 > /sys/module/uvcvideo/parameters/nodrop > That would also work, absolutely! If a user has these cameras, they should always add the no_drop to avoid losing frames that would otherwise be discarded as they are erroneous. This quirk will always trigger, and it is likely they would want to have all the frames sent by the camera, while still marking them as errors they can handle later in user space if they want to. > > > -- > Ricardo Ribalda Best wishes, Isaac
On Tue, 17 Dec 2024 at 16:43, Isaac Scott <isaac.scott@ideasonboard.com> wrote: > > Hi Ricardo, > > On Tue, 2024-12-17 at 13:08 +0100, Ricardo Ribalda wrote: > > On Tue, 17 Dec 2024 at 12:58, Ricardo Ribalda <ribalda@chromium.org> > > wrote: > > > > > > Hi Issac > > > > > > > > > On Tue, 17 Dec 2024 at 12:22, Isaac Scott > > > <isaac.scott@ideasonboard.com> wrote: > > > > > > > > In use cases where a camera needs to use the > > > > UVC_QUIRK_MJPEG_NO_EOF, > > > > erroneous frames that do not conform to MJPEG standards are > > > > correctly > > > > being marked as erroneous. However, when using the camera in a > > > > product, > > > > manufacturers usually want to enable the quirk in order to pass > > > > the > > > > buffers into user space. To do this, they have to enable the > > > > uvc_no_drop > > > > parameter. To avoid the extra steps to configure the kernel in > > > > such a > > > > way, enable the no_drop parameter by default to comply with this > > > > use > > > > case. > > > > > > I am not sure what you want to do with this patch. > > > > > > Why can't people simply set the quirk with > > > > > > modprobe uvcvideo quirks=0x20000 > > > > Sorry, I meant > > > > modprobe uvcvideo nodrop=1 > > > > or > > > > echo 1 > /sys/module/uvcvideo/parameters/nodrop > > > > That would also work, absolutely! > > If a user has these cameras, they should always add the no_drop to > avoid losing frames that would otherwise be discarded as they are > erroneous. > > This quirk will always trigger, and it is likely they would want to > have all the frames sent by the camera, while still marking them as > errors they can handle later in user space if they want to. Besides what Laurent is saying: ``` One issue with this is that it becomes impossible for someone to unset the no_drop parameter. ``` There is also the issue that with your current implementation you are changing the module parameter for ALL the cameras probed after this one: I believe that you have to do: - ret = uvc_queue_init(queue, type, !uvc_no_drop_param); + ret = uvc_queue_init(queue, type, !uvc_no_drop_param && + !(dev->quirks & UVC_QUIRK_MJPEG_NO_EOF)) But maybe before that we need to have the discussion about: shall we remove uvc_no_drop_param altogether?. We could start by swapping the default value and see what happens.... > > > > > > > -- > > Ricardo Ribalda > > Best wishes, > > Isaac
On Tue, Dec 17, 2024 at 05:02:43PM +0100, Ricardo Ribalda wrote: > On Tue, 17 Dec 2024 at 16:43, Isaac Scott wrote: > > On Tue, 2024-12-17 at 13:08 +0100, Ricardo Ribalda wrote: > > > On Tue, 17 Dec 2024 at 12:58, Ricardo Ribalda wrote: > > > > On Tue, 17 Dec 2024 at 12:22, Isaac Scott wrote: > > > > > > > > > > In use cases where a camera needs to use the UVC_QUIRK_MJPEG_NO_EOF, > > > > > erroneous frames that do not conform to MJPEG standards are correctly > > > > > being marked as erroneous. However, when using the camera in a product, > > > > > manufacturers usually want to enable the quirk in order to pass the > > > > > buffers into user space. To do this, they have to enable the uvc_no_drop > > > > > parameter. To avoid the extra steps to configure the kernel in such a > > > > > way, enable the no_drop parameter by default to comply with this use > > > > > case. > > > > > > > > I am not sure what you want to do with this patch. > > > > > > > > Why can't people simply set the quirk with > > > > > > > > modprobe uvcvideo quirks=0x20000 > > > > > > Sorry, I meant > > > > > > modprobe uvcvideo nodrop=1 > > > > > > or > > > > > > echo 1 > /sys/module/uvcvideo/parameters/nodrop > > > > > > > That would also work, absolutely! > > > > If a user has these cameras, they should always add the no_drop to > > avoid losing frames that would otherwise be discarded as they are > > erroneous. > > > > This quirk will always trigger, and it is likely they would want to > > have all the frames sent by the camera, while still marking them as > > errors they can handle later in user space if they want to. > > Besides what Laurent is saying: > ``` > One issue with this is that it becomes impossible for someone to unset > the no_drop parameter. > ``` > > There is also the issue that with your current implementation you are > changing the module parameter for ALL the cameras probed after this > one: > > I believe that you have to do: > - ret = uvc_queue_init(queue, type, !uvc_no_drop_param); > + ret = uvc_queue_init(queue, type, !uvc_no_drop_param && > + !(dev->quirks & > UVC_QUIRK_MJPEG_NO_EOF)) > > > But maybe before that we need to have the discussion about: shall we > remove uvc_no_drop_param altogether?. We could start by swapping the > default value and see what happens.... Unless someone objects, I think swapping the default value and waiting a bit is a good idea. If the universe doesn't collapse immediately, we could later drop the parameter.
On Tue, 17 Dec 2024 at 18:02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Dec 17, 2024 at 05:02:43PM +0100, Ricardo Ribalda wrote: > > On Tue, 17 Dec 2024 at 16:43, Isaac Scott wrote: > > > On Tue, 2024-12-17 at 13:08 +0100, Ricardo Ribalda wrote: > > > > On Tue, 17 Dec 2024 at 12:58, Ricardo Ribalda wrote: > > > > > On Tue, 17 Dec 2024 at 12:22, Isaac Scott wrote: > > > > > > > > > > > > In use cases where a camera needs to use the UVC_QUIRK_MJPEG_NO_EOF, > > > > > > erroneous frames that do not conform to MJPEG standards are correctly > > > > > > being marked as erroneous. However, when using the camera in a product, > > > > > > manufacturers usually want to enable the quirk in order to pass the > > > > > > buffers into user space. To do this, they have to enable the uvc_no_drop > > > > > > parameter. To avoid the extra steps to configure the kernel in such a > > > > > > way, enable the no_drop parameter by default to comply with this use > > > > > > case. > > > > > > > > > > I am not sure what you want to do with this patch. > > > > > > > > > > Why can't people simply set the quirk with > > > > > > > > > > modprobe uvcvideo quirks=0x20000 > > > > > > > > Sorry, I meant > > > > > > > > modprobe uvcvideo nodrop=1 > > > > > > > > or > > > > > > > > echo 1 > /sys/module/uvcvideo/parameters/nodrop > > > > > > > > > > That would also work, absolutely! > > > > > > If a user has these cameras, they should always add the no_drop to > > > avoid losing frames that would otherwise be discarded as they are > > > erroneous. > > > > > > This quirk will always trigger, and it is likely they would want to > > > have all the frames sent by the camera, while still marking them as > > > errors they can handle later in user space if they want to. > > > > Besides what Laurent is saying: > > ``` > > One issue with this is that it becomes impossible for someone to unset > > the no_drop parameter. > > ``` > > > > There is also the issue that with your current implementation you are > > changing the module parameter for ALL the cameras probed after this > > one: > > > > I believe that you have to do: > > - ret = uvc_queue_init(queue, type, !uvc_no_drop_param); > > + ret = uvc_queue_init(queue, type, !uvc_no_drop_param && > > + !(dev->quirks & > > UVC_QUIRK_MJPEG_NO_EOF)) > > > > > > But maybe before that we need to have the discussion about: shall we > > remove uvc_no_drop_param altogether?. We could start by swapping the > > default value and see what happens.... > > Unless someone objects, I think swapping the default value and waiting a > bit is a good idea. If the universe doesn't collapse immediately, we > could later drop the parameter. I have prepared a series with this: https://patchwork.linuxtv.org/project/linux-media/list/?series=14229 > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 947c4bf6bfeb..45028b45906a 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1948,6 +1948,13 @@ int uvc_register_video_device(struct uvc_device *dev, { int ret; + /* + * If the MJPEG stream occasionally loses the EOF marker, we set the + * no_drop parameter by default to avoid dropping frames erroneously. + */ + if (dev->quirks & UVC_QUIRK_MJPEG_NO_EOF) + uvc_no_drop_param = 1; + /* Initialize the video buffers queue. */ ret = uvc_queue_init(queue, type, !uvc_no_drop_param); if (ret)
In use cases where a camera needs to use the UVC_QUIRK_MJPEG_NO_EOF, erroneous frames that do not conform to MJPEG standards are correctly being marked as erroneous. However, when using the camera in a product, manufacturers usually want to enable the quirk in order to pass the buffers into user space. To do this, they have to enable the uvc_no_drop parameter. To avoid the extra steps to configure the kernel in such a way, enable the no_drop parameter by default to comply with this use case. Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> --- drivers/media/usb/uvc/uvc_driver.c | 7 +++++++ 1 file changed, 7 insertions(+)