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);
> }
>
>
