On Tue, 17 Dec 2024 at 18:02, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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
