Re: [PATCH v9 4/4] media: virtio: Add ioctl operations and driver logic

[Nick Rogers <nick@getfieldwork.ai>]

Hi Brian, Alexandre,

We have been running v9 on 6.18 in lighter, a macOS VMM, against a
host-side stateful decoder and encoder backed by VideoToolbox, with
ffmpeg 5.1 and 7.1 and GStreamer 1.26 in the guest. It works well; we
found three problems along the way, all in this patch, and have been
carrying the fixes below.

1. queued_bufs can drift until poll stops reporting the queue writable

virtio_media_qbuf() increments queue->queued_bufs after the device has
replied, without queues_lock. A device that completes the buffer before
it replies (ours decodes within the command) sends the DQBUF event while
the QBUF caller is still waiting, and the event's decrement races the
unlocked increment. When the decrement is lost, queued_bufs creeps up
until it equals allocated_bufs and the OUTPUT queue is never reported
writable again. With our device this stalled ffmpeg about once a minute.
Counting the buffer before the device can see it, under the lock, and
undoing that if the command fails:

--- a/drivers/media/virtio/virtio_media_ioctls.c
+++ b/drivers/media/virtio/virtio_media_ioctls.c
@@ -899,15 +899,20 @@
 	old_flags = buffer->buffer.flags;
 	buffer->buffer.flags = V4L2_BUF_FLAG_QUEUED;

+	mutex_lock(&session->queues_lock);
+	queue->queued_bufs += 1;
+	mutex_unlock(&session->queues_lock);
+
 	ret = virtio_media_send_buffer_ioctl(vfh, VIDIOC_QBUF, b);
 	if (ret) {
 		/* Rollback the previous flags as the buffer is not queued. */
+		mutex_lock(&session->queues_lock);
+		queue->queued_bufs -= 1;
+		mutex_unlock(&session->queues_lock);
 		buffer->buffer.flags = old_flags;
 		return ret;
 	}

-	queue->queued_bufs += 1;
-
 	return 0;
 }

2. poll does not report the CAPTURE queue readable after the LAST buffer

Once the LAST buffer has been dequeued, DQBUF on the CAPTURE queue
returns -EPIPE, which is how clients such as ffmpeg's v4l2m2m wrapper
learn the stream has ended. vb2 reports the queue readable in that
state (vb2_core_poll() checks last_buffer_dequeued) so the client goes
on to call DQBUF; virtio_media_device_poll() does not, so a client that
polls once more after a drain waits forever. Debian's ffmpeg 5.1 does
exactly that at the end of a stream with no B-frames:

--- a/drivers/media/virtio/virtio_media_driver.c
+++ b/drivers/media/virtio/virtio_media_driver.c
@@ -633,7 +633,8 @@
 		    (capture_queue->queued_bufs == 0 &&
 		     list_empty(&capture_queue->pending_dqbufs)))
 			rc |= EPOLLERR;
-		else if (!list_empty(&capture_queue->pending_dqbufs))
+		else if (!list_empty(&capture_queue->pending_dqbufs) ||
+			 capture_queue->is_capture_last)
 			rc |= EPOLLIN | EPOLLRDNORM;
 	}
 	if (req_events & (EPOLLOUT | EPOLLWRNORM)) {

3. VIDIOC_G_CTRL and VIDIOC_S_CTRL fail with -EINVAL

The driver has no control handler, so the V4L2 core turns G_CTRL and
S_CTRL into a single extended control and calls the driver's
g/s_ext_ctrls. That control is built on the core's stack, and its size
field is never initialized; virtio_media_send_ext_controls_ioctl() takes
a nonzero size as a payload to copy from userspace, and the ioctl fails.
GStreamer's V4L2 encoders set their profile with S_CTRL and cannot
negotiate, and v4l2-ctl cannot read the MIN_BUFFERS controls.

The fault is really in the core, which should hand drivers a zeroed
structure, and I have sent a patch for that separately [1]. Until it
lands the driver sees garbage there, so you may also want to guard
against it; we have been clearing size when the controls array is on
the stack, which only the core's G/S_CTRL translation produces.

[1] https://lore.kernel.org/all/20260923160936.33445-1-nick@getfieldwork.ai/

Thanks for the driver,
Nick Rogers