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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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