On Tue, Jan 05, 2021 at 11:54:31PM +0100, Jeremie Courreges-Anglas wrote:
>
> I hit a weird failure with firefox and BigBlueButton
> (https://bigbluebutton.org/) where firefox can't use my webcam.
> video(1) works, same for other webrtc sites in firefox, eg meet.jit.si.
> ktrace shows that a single firefox process tries to open /dev/video0
> more than once, and that fails with EBUSY. The code lies in the
> libwebrtc library
>
>
> https://webrtc.googlesource.com/src/+/refs/heads/master/modules/video_capture/linux/
>
> In my tests the multiple open() calls only happen at initialization
> time, video streaming only uses one fd.
>
> Back to our kernel, the current restrictive behavior was introduced with
>
> revision 1.18
> date: 2008/07/23 22:10:21; author: mglocker; state: Exp; lines: +11 -4;
> If /dev/video* is already used by an application, return EBUSY to other
> applications. Fixes a kernel panic.
>
> Reported by ian@
>
> Meanwhile, the V4L2 API specifies that a device "can be opened more than
> once" from multiple processes, with access to certain methods blocked
> when an application starts reading from the device.
>
>
> https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/open.html#multiple-opens
>
> So the API says we're being too restrictive, but we don't want to go
> back to uncontrolled access either. I guess the ideal solution would be
> to implement multiple process access with fine-grained checks, but...
> I don't have time for that!
Oh, I guess that makes video(1) an "old and obscure" driver according
to the API [2] footnote ;-)
Finally we should implement the controls for multiple video device
openings based on what has been exactly requested. Since the basic
idea mentioned in the API is that other processes, like a panel
application, can be started to e.g. change controls. I will have a
look at that when I find some time.
> An easy improvement for my use case would be to allow the same process
> to open a device multiple times. It fixes firefox + BigBlueButton for
> me and doesn't make concurrent accesses worse (multiple threads from the
> same process can already do concurrent accesses, which is something
> we might want to look at).
Interesting. I was not aware that threads of the same process can do
that. I need to have a closer look at what is happening here.
> Here's the diff. IIUC the use of atomic operations isn't strictly
> needed here since open(2) runs with the kernel lock, but the result
> is easy to understand IMO.
I don't agree with that, see my comments bellow in point a) and b).
> Thoughts? ok?
I think it doesn't harm to allow the same process to do multiple opens
on video(1) as a first improvement. But I'm not happy using
atomic_cas_ptr(9) and atomic_swap_ptr(9) for this because:
a) As you already have mentioned, we don't require atomic
operations here. I checked that those functions are very
rarely used in the kernel, and only there were atomic
operation is required. I'm afraid that when we use those in
video(1), and somebody looks at the code later on to
eventually replace it, there will be confusion if there was
a specific reason why to use atomic functions.
b) IMO it doesn't simplify the code. I first had to read the
manual pages to understand how those functions work.
I would prefer to do something simpler, like in this adapted diff.
If it works for you, I'm OK if you commit this instead.
> Index: dev/video.c
> ===================================================================
> RCS file: /d/cvs/src/sys/dev/video.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 video.c
> --- dev/video.c 28 Dec 2020 18:28:11 -0000 1.46
> +++ dev/video.c 5 Jan 2021 22:34:04 -0000
> @@ -28,6 +28,8 @@
> #include <sys/kernel.h>
> #include <sys/malloc.h>
> #include <sys/conf.h>
> +#include <sys/atomic.h>
> +#include <sys/proc.h>
> #include <sys/videoio.h>
>
> #include <dev/video_if.h>
> @@ -46,8 +48,7 @@ struct video_softc {
> struct device *sc_dev; /* hardware device struct */
> struct video_hw_if *hw_if; /* hardware interface */
> char sc_dying; /* device detached */
> -#define VIDEO_OPEN 0x01
> - char sc_open;
> + struct process *sc_owner; /* owner process */
>
> int sc_fsize;
> uint8_t *sc_fbuffer;
> @@ -101,6 +102,7 @@ videoattach(struct device *parent, struc
> sc->hw_hdl = sa->hdl;
> sc->sc_dev = parent;
> sc->sc_fbufferlen = 0;
> + sc->sc_owner = NULL;
>
> if (sc->hw_if->get_bufsize)
> sc->sc_fbufferlen = (sc->hw_if->get_bufsize)(sc->hw_hdl);
> @@ -121,6 +123,7 @@ videoopen(dev_t dev, int flags, int fmt,
> {
> int unit;
> struct video_softc *sc;
> + struct process *owner;
>
> unit = VIDEOUNIT(dev);
> if (unit >= video_cd.cd_ndevs ||
> @@ -128,9 +131,13 @@ videoopen(dev_t dev, int flags, int fmt,
> sc->hw_if == NULL)
> return (ENXIO);
>
> - if (sc->sc_open & VIDEO_OPEN)
> - return (EBUSY);
> - sc->sc_open |= VIDEO_OPEN;
> + owner = atomic_cas_ptr(&sc->sc_owner, NULL, p->p_p);
> + if (owner != NULL) {
> + if (owner == p->p_p)
> + return (0);
> + else
> + return (EBUSY);
> + }
>
> sc->sc_vidmode = VIDMODE_NONE;
> sc->sc_frames_ready = 0;
> @@ -153,7 +160,7 @@ videoclose(dev_t dev, int flags, int fmt
> if (sc->hw_if->close != NULL)
> r = sc->hw_if->close(sc->hw_hdl);
>
> - sc->sc_open &= ~VIDEO_OPEN;
> + atomic_swap_ptr(&sc->sc_owner, NULL);
>
> return (r);
> }
Index: dev/video.c
===================================================================
RCS file: /cvs/src/sys/dev/video.c,v
retrieving revision 1.46
diff -u -p -u -p -r1.46 video.c
--- dev/video.c 28 Dec 2020 18:28:11 -0000 1.46
+++ dev/video.c 6 Jan 2021 13:19:38 -0000
@@ -28,6 +28,7 @@
#include <sys/kernel.h>
#include <sys/malloc.h>
#include <sys/conf.h>
+#include <sys/proc.h>
#include <sys/videoio.h>
#include <dev/video_if.h>
@@ -46,8 +47,7 @@ struct video_softc {
struct device *sc_dev; /* hardware device struct */
struct video_hw_if *hw_if; /* hardware interface */
char sc_dying; /* device detached */
-#define VIDEO_OPEN 0x01
- char sc_open;
+ struct process *sc_owner; /* owner process */
int sc_fsize;
uint8_t *sc_fbuffer;
@@ -101,6 +101,7 @@ videoattach(struct device *parent, struc
sc->hw_hdl = sa->hdl;
sc->sc_dev = parent;
sc->sc_fbufferlen = 0;
+ sc->sc_owner = NULL;
if (sc->hw_if->get_bufsize)
sc->sc_fbufferlen = (sc->hw_if->get_bufsize)(sc->hw_hdl);
@@ -128,9 +129,13 @@ videoopen(dev_t dev, int flags, int fmt,
sc->hw_if == NULL)
return (ENXIO);
- if (sc->sc_open & VIDEO_OPEN)
- return (EBUSY);
- sc->sc_open |= VIDEO_OPEN;
+ if (sc->sc_owner != NULL) {
+ if (sc->sc_owner == p->p_p)
+ return (0);
+ else
+ return (EBUSY);
+ } else
+ sc->sc_owner = p->p_p;
sc->sc_vidmode = VIDMODE_NONE;
sc->sc_frames_ready = 0;
@@ -153,7 +158,7 @@ videoclose(dev_t dev, int flags, int fmt
if (sc->hw_if->close != NULL)
r = sc->hw_if->close(sc->hw_hdl);
- sc->sc_open &= ~VIDEO_OPEN;
+ sc->sc_owner = NULL;
return (r);
}
