On 09/02/21(Tue) 20:35, Marcus Glocker wrote: > 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.
Which fields is the new lock protecting? Why isn't the KERNEL_LOCK() enough? Is it because of sleeping points? Could you annotate them? > 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); > } > >