jca@ has recently committed a change to video(4) to allow the same process to do multiple opens on the same video device to satisfy certain applications, and start to go in to the V4L2 "1.1.4 Multiple Opens" specification direction as described here:
https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/open.html#f1 As well he recently sent me some locking code to prevent concurrent access to certain video(4) functions. On that I think it makes more sense to introduce the locking code together with the next step, which is to allow different processes to open the same video device. Therefore I have added on top of jca@s locking code the code to define a device owner, and based on that distinguish between which process is allowed to call certain video(4) functions. Basically the process starting with the buffer memory allocation or/and starting the video stream becomes the device owner. Other processes can do things like calling VIDIOC_G_CTRL or VIDIOC_S_CTRL ioctls. In this diff certainly more ioctls can be moved up to the "shared" part, but I only started with the video controls for now. Also video(1) requires a small change to make the read(2) method signal that the stream needs to be stopped on close. I checked that other applications do implement this behavior as well. Otherwise if you have multiple file handles open on a video device, and the read(2) process exists before another process, we won't notice that the stream needs to be stopped, since only the last file handle close will call the videoclose() function. I would appreciate some regression testing and feedback to make a start on implementing this specification. Index: sys/dev/video.c =================================================================== RCS file: /cvs/src/sys/dev/video.c,v retrieving revision 1.48 diff -u -p -u -p -r1.48 video.c --- sys/dev/video.c 31 Jan 2021 19:32:01 -0000 1.48 +++ sys/dev/video.c 7 Feb 2021 15:33:52 -0000 @@ -48,6 +48,8 @@ struct video_softc { struct video_hw_if *hw_if; /* hardware interface */ char sc_dying; /* device detached */ struct process *sc_owner; /* owner process */ + struct rwlock sc_lock; /* device lock */ + uint8_t sc_open; /* device opened */ int sc_fsize; uint8_t *sc_fbuffer; @@ -122,6 +124,7 @@ videoopen(dev_t dev, int flags, int fmt, { int unit; struct video_softc *sc; + int r = 0; unit = VIDEOUNIT(dev); if (unit >= video_cd.cd_ndevs || @@ -129,22 +132,27 @@ videoopen(dev_t dev, int flags, int fmt, sc->hw_if == NULL) return (ENXIO); - if (sc->sc_owner != NULL) { - if (sc->sc_owner == p->p_p) - return (0); - else - return (EBUSY); - } else - sc->sc_owner = p->p_p; + rw_enter_write(&sc->sc_lock); + + if (sc->sc_open) { + DPRINTF(("%s: device already open\n", __func__)); + rw_exit_write(&sc->sc_lock); + return (r); + } else { + sc->sc_open = 1; + DPRINTF(("%s: set device to open\n", __func__)); + } sc->sc_vidmode = VIDMODE_NONE; sc->sc_frames_ready = 0; if (sc->hw_if->open != NULL) - return (sc->hw_if->open(sc->hw_hdl, flags, &sc->sc_fsize, - sc->sc_fbuffer, video_intr, sc)); - else - return (0); + r = sc->hw_if->open(sc->hw_hdl, flags, &sc->sc_fsize, + sc->sc_fbuffer, video_intr, sc); + + rw_exit_write(&sc->sc_lock); + + return (r); } int @@ -155,11 +163,23 @@ videoclose(dev_t dev, int flags, int fmt sc = video_cd.cd_devs[VIDEOUNIT(dev)]; + rw_enter_write(&sc->sc_lock); + if (sc->hw_if->close != NULL) r = sc->hw_if->close(sc->hw_hdl); + if (p != NULL) { + sc->sc_open = 0; + DPRINTF(("%s: last close\n", __func__)); + } + DPRINTF(("%s: stream close\n", __func__)); + + sc->sc_vidmode = VIDMODE_NONE; + sc->sc_frames_ready = 0; sc->sc_owner = NULL; + rw_exit_write(&sc->sc_lock); + return (r); } @@ -175,11 +195,28 @@ videoread(dev_t dev, struct uio *uio, in (sc = video_cd.cd_devs[unit]) == NULL) return (ENXIO); - if (sc->sc_dying) + rw_enter_write(&sc->sc_lock); + + if (sc->sc_dying) { + rw_exit_write(&sc->sc_lock); return (EIO); + } - if (sc->sc_vidmode == VIDMODE_MMAP) + if (sc->sc_vidmode == VIDMODE_MMAP) { + rw_exit_write(&sc->sc_lock); return (EBUSY); + } + + if (sc->sc_owner != NULL && sc->sc_owner != uio->uio_procp->p_p) { + DPRINTF(("%s: already owned=%p\n", __func__, sc->sc_owner)); + return (EBUSY); + } + if (sc->sc_owner == NULL) { + sc->sc_owner = uio->uio_procp->p_p; + DPRINTF(("%s: new owner=%p\n", __func__, sc->sc_owner)); + } + + rw_exit_write(&sc->sc_lock); /* start the stream if not already started */ if (sc->sc_vidmode == VIDMODE_NONE && sc->hw_if->start_read) { @@ -205,7 +242,7 @@ videoread(dev_t dev, struct uio *uio, in if (!video_record_enable) bzero(sc->sc_fbuffer, size); error = uiomove(sc->sc_fbuffer, size, uio); - sc->sc_frames_ready--; + atomic_dec_int_nv(&sc->sc_frames_ready); if (error) return (error); @@ -229,6 +266,44 @@ videoioctl(dev_t dev, u_long cmd, caddr_ DPRINTF(("video_ioctl(%zu, '%c', %zu)\n", IOCPARM_LEN(cmd), (int) IOCGROUP(cmd), cmd & 0xff)); + rw_enter_write(&sc->sc_lock); + + error = EOPNOTSUPP; + switch (cmd) { + case VIDIOC_G_CTRL: + if (sc->hw_if->g_ctrl) + error = (sc->hw_if->g_ctrl)(sc->hw_hdl, + (struct v4l2_control *)data); + break; + case VIDIOC_S_CTRL: + if (sc->hw_if->s_ctrl) + error = (sc->hw_if->s_ctrl)(sc->hw_hdl, + (struct v4l2_control *)data); + break; + default: + error = (ENOTTY); + } + if (error != ENOTTY) { + rw_exit_write(&sc->sc_lock); + return (error); + } + + if (sc->sc_owner != NULL && sc->sc_owner != p->p_p) { + DPRINTF(("%s: already owned=%p\n", __func__, sc->sc_owner)); + rw_exit_write(&sc->sc_lock); + return (EBUSY); + } + if (sc->sc_owner == NULL) { + sc->sc_owner = p->p_p; + DPRINTF(("%s: new owner=%p\n", __func__, sc->sc_owner)); + } + + rw_exit_write(&sc->sc_lock); + + /* + * The following IOCTLs can only be called by the device owner. + * For further shared IOCTLs please move it up. + */ error = EOPNOTSUPP; switch (cmd) { case VIDIOC_QUERYCAP: @@ -326,6 +401,9 @@ videoioctl(dev_t dev, u_long cmd, caddr_ if (sc->hw_if->streamoff) error = (sc->hw_if->streamoff)(sc->hw_hdl, (int)*data); + if (!error) + /* Release device ownership and streaming buffers. */ + videoclose(dev, 0, 0, NULL); break; case VIDIOC_TRY_FMT: if (sc->hw_if->try_fmt) @@ -337,16 +415,6 @@ videoioctl(dev_t dev, u_long cmd, caddr_ error = (sc->hw_if->queryctrl)(sc->hw_hdl, (struct v4l2_queryctrl *)data); break; - case VIDIOC_G_CTRL: - if (sc->hw_if->g_ctrl) - error = (sc->hw_if->g_ctrl)(sc->hw_hdl, - (struct v4l2_control *)data); - break; - case VIDIOC_S_CTRL: - if (sc->hw_if->s_ctrl) - error = (sc->hw_if->s_ctrl)(sc->hw_hdl, - (struct v4l2_control *)data); - break; default: error = (ENOTTY); } @@ -365,8 +433,24 @@ videopoll(dev_t dev, int events, struct (sc = video_cd.cd_devs[unit]) == NULL) return (POLLERR); - if (sc->sc_dying) + rw_enter_write(&sc->sc_lock); + + if (sc->sc_dying) { + rw_exit_write(&sc->sc_lock); return (POLLERR); + } + + if (sc->sc_owner != NULL && sc->sc_owner != p->p_p) { + DPRINTF(("%s: already owned=%p\n", __func__, sc->sc_owner)); + rw_exit_write(&sc->sc_lock); + return (EBUSY); + } + if (sc->sc_owner == NULL) { + sc->sc_owner = p->p_p; + DPRINTF(("%s: new owner=%p\n", __func__, sc->sc_owner)); + } + + rw_exit_write(&sc->sc_lock); DPRINTF(("%s: events=0x%x\n", __func__, events)); @@ -412,15 +496,23 @@ videommap(dev_t dev, off_t off, int prot (sc = video_cd.cd_devs[unit]) == NULL) return (-1); - if (sc->sc_dying) + rw_enter_write(&sc->sc_lock); + + if (sc->sc_dying) { + rw_exit_write(&sc->sc_lock); return (-1); + } - if (sc->hw_if->mappage == NULL) + if (sc->hw_if->mappage == NULL) { + rw_exit_write(&sc->sc_lock); return (-1); + } p = sc->hw_if->mappage(sc->hw_hdl, off, prot); - if (p == NULL) + if (p == NULL) { + rw_exit_write(&sc->sc_lock); return (-1); + } if (pmap_extract(pmap_kernel(), (vaddr_t)p, &pa) == FALSE) panic("videommap: invalid page"); sc->sc_vidmode = VIDMODE_MMAP; @@ -429,6 +521,8 @@ videommap(dev_t dev, off_t off, int prot if (off == 0) sc->sc_fbuffer_mmap = p; + rw_exit_write(&sc->sc_lock); + return (pa); } @@ -472,8 +566,12 @@ videokqfilter(dev_t dev, struct knote *k (sc = video_cd.cd_devs[unit]) == NULL) return (ENXIO); - if (sc->sc_dying) + rw_enter_write(&sc->sc_lock); + + if (sc->sc_dying) { + rw_exit_write(&sc->sc_lock); return (ENXIO); + } switch (kn->kn_filter) { case EVFILT_READ: @@ -481,9 +579,22 @@ videokqfilter(dev_t dev, struct knote *k kn->kn_hook = sc; break; default: + rw_exit_write(&sc->sc_lock); return (EINVAL); } + if (sc->sc_owner != NULL && sc->sc_owner != kn->kn_ptr.p_process) { + DPRINTF(("%s: already owned=%p\n", __func__, sc->sc_owner)); + rw_exit_write(&sc->sc_lock); + return (EBUSY); + } + if (sc->sc_owner == NULL) { + sc->sc_owner = kn->kn_ptr.p_process; + DPRINTF(("%s: new owner=%p\n", __func__, sc->sc_owner)); + } + + rw_exit_write(&sc->sc_lock); + /* * Start the stream in read() mode if not already started. If * the user wanted mmap() mode, he should have called mmap() @@ -531,7 +642,7 @@ video_intr(void *addr) DPRINTF(("video_intr sc=%p\n", sc)); if (sc->sc_vidmode != VIDMODE_NONE) - sc->sc_frames_ready++; + atomic_inc_int_nv(&sc->sc_frames_ready); else printf("%s: interrupt but no streams!\n", __func__); if (sc->sc_vidmode == VIDMODE_READ) Index: app/video/video.c =================================================================== RCS file: /cvs/xenocara/app/video/video.c,v retrieving revision 1.39 diff -u -p -u -p -r1.39 video.c --- app/video/video.c 7 Sep 2020 10:35:22 -0000 1.39 +++ app/video/video.c 7 Feb 2021 12:05:36 -0000 @@ -2056,6 +2056,8 @@ cleanup(struct video *vid, int excode) if (vid->dev.fd >= 0) { if (vid->mmap_on) mmap_stop(vid); + else + (void)ioctl(vid->dev.fd, VIDIOC_STREAMOFF, &type); close(vid->dev.fd); }
