[RFC PATCH 0/1] Set uvc_no_drop parameter for MJPEG_NO_EOF quirk

[RFC PATCH 0/1] Set uvc_no_drop parameter for MJPEG_NO_EOF quirk

[Isaac Scott]

This patch refers to the UVC_QUIRK_MJPEG_NO_EOF quirk. The series can be
viewed at: 

https://lore.kernel.org/all/20241128145144.61475-1-isaac.scott@ideasonboard.com/

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, if this happens, we should avoid
losing a whole frame of data by enabling the uvc_no_drop parameter. Only
the buffer containing the EOF is lost, so it does not make sense to drop
the entirety of an otherwise correct buffer just because of this issue.
The frame should be marked as erroneous, but the otherwise correct frame
should be passed to user space.

To do this, you would have to enable the uvc_no_drop parameter. To avoid
the extra steps needed to configure the kernel in such a way, enable the
no_drop parameter by default to comply with this use case.


Isaac Scott (1):
  media: uvcvideo: Add no drop parameter by default for MJPEG_NO_EOF
    quirk

 drivers/media/usb/uvc/uvc_driver.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.43.0

[RFC PATCH 1/1] media: uvcvideo: Add no drop parameter for MJPEG_NO_EOF quirk

[Isaac Scott]

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;
+
 	/* Initialize the video buffers queue. */
 	ret = uvc_queue_init(queue, type, !uvc_no_drop_param);
 	if (ret)
-- 
2.43.0

Re: [RFC PATCH 1/1] media: uvcvideo: Add no drop parameter for MJPEG_NO_EOF quirk

[Ricardo Ribalda]

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

Re: [RFC PATCH 1/1] media: uvcvideo: Add no drop parameter for MJPEG_NO_EOF quirk

[Laurent Pinchart]

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)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/1] media: uvcvideo: Add no drop parameter for MJPEG_NO_EOF quirk

[Ricardo Ribalda]

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

Re: [RFC PATCH 1/1] media: uvcvideo: Add no drop parameter for MJPEG_NO_EOF quirk

[Isaac Scott]

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

Re: [RFC PATCH 1/1] media: uvcvideo: Add no drop parameter for MJPEG_NO_EOF quirk

[Ricardo Ribalda]

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



-- 
Ricardo Ribalda

Re: [RFC PATCH 1/1] media: uvcvideo: Add no drop parameter for MJPEG_NO_EOF quirk

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/1] media: uvcvideo: Add no drop parameter for MJPEG_NO_EOF quirk

[Ricardo Ribalda]

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



-- 
Ricardo Ribalda