Re: [PATCH v3] media: add virtio-media driver

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

On Fri, 29 May 2026 12:03:04 -0400
Brian Daniels <briandaniels@google.com> wrote:

> Hi there! My name is Brian Daniels and I'll be taking over upstreaming this
> driver from Alexandre Courbot.
> 
> I've consulted with Alexandre and my plan is to upload a v4 set of patches
> shortly based on the feedback from this revision.

As you promised a v5 to Michael, I'll wait for it to review the new series.

For now, let me address the replies from you on the top of v3.

> Before doing so, I'd like to address some of your comments:
> 
> > Hi Alex,
> > 
> > I didn't see on a first glance anything that would cause locking
> > issues here, but, as I pointed on my last e-mail, testing with
> > qv4l2 at the max res of my C920 camera, it ended keeping 24 CPUs
> > busy without showing any results with qv4l2 (via ssh at the same
> > machine). So, I suspect that there are issues somewhere, but I didn't
> > debug any further.
> > 
> > Please do some tests with a high-res camera, using either ssh or
> > GPU emulation to see how this behaves with real apps.  
> 
> I did some testing with a 1080p USB webcam over ssh using ffmpeg and I was able
> to stream video to disk without any issue. Let me know if you'd prefer I test
> with a specific setup.

You need to test it displaying on a X11 or Wayland window inside the VM, not 
just via an app like ffmeg.

> 
> > > +config MEDIA_VIRTIO
> > > +	tristate "Virtio-media Driver"
> > > +	depends on VIRTIO && VIDEO_DEV && 64BIT && (X86 || (ARM && CPU_LITTLE_ENDIAN))  
> >
> > Why are you limiting it to x86_64 and arm64 little endian?  
> 
> The little endian requirement comes for the section 5.22.6.1.5 of the virtio
> v1.4 specification [1]. The limitation to x86_64 and arm64 is for two reasons:
> 
> 1. The specification requires all v4l2 structures to use the 64-bit layout
> 2. This driver has only been tested on x86_64 and arm64 so far

I don't like non-portable interfaces. If the spec is not good, it needs to
be fixed; if the problem is at the API interface, you may need compat32 or
similar to ensure that the interface will work independently on host/guest
CPU differences.

> 
> 
> > > +/**
> > > + * enum virtio_media_memory - Memory types supported by virtio-media.
> > > + * @VIRTIO_MEDIA_MMAP: memory allocated and managed by device. Can be mapped
> > > + * into the guest using VIRTIO_MEDIA_CMD_MMAP.
> > > + * @VIRTIO_MEDIA_SHARED_PAGES: memory allocated by the driver. Passed to the
> > > + * device using virtio_media_sg_entry.
> > > + * @VIRTIO_MEDIA_OBJECT: memory backed by a virtio object.
> > > + */
> > > +enum virtio_media_memory {
> > > +	VIRTIO_MEDIA_MMAP = V4L2_MEMORY_MMAP,
> > > +	VIRTIO_MEDIA_SHARED_PAGES = V4L2_MEMORY_USERPTR,
> > > +	VIRTIO_MEDIA_OBJECT = V4L2_MEMORY_DMABUF,
> > > +};  
> > 
> > I'm not a big fan of renaming USERPTR to SHARED_PAGES and
> > DMABUF to OBJECT, as it makes harder for reviewers and contributors
> > to remember about this mapping. Also, everybody knows exactly what
> > DMABUF means, so, it sounds to me that this obfuscates a little bit 
> > the driver.
> > 
> > Also, why are you encapsulating V4L2 names into VIRTIO_* namespace?
> > This just adds extra complexity for reviewers without any real
> > benefit.
> > 
> > Besides that, doing a grep on the patch, it sounds that this ma
> > is not used anywhere.
> > 
> > So, please drop this mapping.  
> 
> Agreed I don't see it being used anywhere, I think this was included originally
> since it's part of the virtio spec. I will remove it.
> 
> 
> > > +#define VIRTIO_MEDIA_EVT_ERROR 0
> > > +#define VIRTIO_MEDIA_EVT_DQBUF 1
> > > +#define VIRTIO_MEDIA_EVT_EVENT 2  
> > 
> > OK, here, media events are different than virtio events, so having
> > a virtio-specific events make sense. Yet, better to add a comment
> > about that. 
> > 
> > Also, V4L events are defined as:
> > 
> > 	#define V4L2_EVENT_ALL                          0
> > 	#define V4L2_EVENT_VSYNC                        1
> > 	#define V4L2_EVENT_EOS                          2
> > 	#define V4L2_EVENT_CTRL                         3
> > 	#define V4L2_EVENT_FRAME_SYNC                   4
> > 	#define V4L2_EVENT_SOURCE_CHANGE                5
> > 	#define V4L2_EVENT_MOTION_DET                   6
> > 	#define V4L2_EVENT_PRIVATE_START                0x08000000
> > 
> > As one may end wanting to map them on some future, I would change
> > the definitions above to:
> > 
> > 	#define VIRTIO_MEDIA_EVT_ERROR V4L2_EVENT_PRIVATE_START
> > 	#define VIRTIO_MEDIA_EVT_DQBUF (V4L2_EVENT_PRIVATE_START + 1)
> > 	#define VIRTIO_MEDIA_EVT_EVENT (V4L2_EVENT_PRIVATE_START + 2)  
> 
> These event values (VIRTIO_MEDIA_EVT_*) are set by section 5.22.6.2.1 of the
> virtio v1.4 specification [2]. I don't believe we have the ability to change
> these values as suggested without changing the specification. That would most
> likely be a lengthy process, so I'd prefer to keep it as written if that's not
> an issue.

The specs you pointed seem to be specific to V4L2. I can't see any reason
why not adjusting it to provide an interface that doesn't conflict with
V4L2 existing events.

> 
> > > +#define VIRTIO_MEDIA_MAX_PLANES VIDEO_MAX_PLANES  
> > 
> > Here: why renaming it to VIRTIO_* namespace?  
> 
> This matches the name in section 5.22.6.2.3 of the virtio v1.4 specification
> [3]. And Alexandre stated the reason why it was renamed from VIDEO_* to
> VIRTIO_MEDIA_* was from an early piece of feedback to avoid V4L2-specific names.
> Even though virtio-media reuses the v4l2 structures and API, its possible to use
> virtio-media with a different media implementation other than v4l2.

In thesis, it would be possible, but it sounds unlikely that this would
ever happen, as other OSes may have completely different media APIs.

So, in practice, if one wants, let's say, a Windows host to run a media 
virio, it would either need to implement V4L2 internally or will need
different virtio drivers using a different API.

> > > +/**
> > > + * struct virtio_media_session - A session on a virtio_media device.
> > > + * @fh: file handler for the session.
> > > + * @id: session ID used to communicate with the device.
> > > + * @nonblocking_dequeue: whether dequeue should block or not (nonblocking if
> > > + * file opened with O_NONBLOCK).
> > > + * @uses_mplane: whether the queues for this session use the MPLANE API or not.
> > > + * @cmd: union of session-related commands. A session can have one command currently running.
> > > + * @resp: union of session-related responses. A session can wait on one command only.
> > > + * @shadow_buf: shadow buffer where data to be added to the descriptor chain can
> > > + * be staged before being sent to the device.
> > > + * @command_sgs: SG table gathering descriptors for a given command and its response.
> > > + * @queues: state of all the queues for this session.
> > > + * @queues_lock: protects all members fo the queues for this session.
> > > + * virtio_media_queue_state`.
> > > + * @dqbuf_wait: waitqueue for dequeued buffers, if ``VIDIOC_DQBUF`` needs to
> > > + * block or when polling.
> > > + * @list: link into the list of sessions for the device.
> > > + */
> > > +struct virtio_media_session {
> > > +	struct v4l2_fh fh;
> > > +	u32 id;
> > > +	bool nonblocking_dequeue;
> > > +	bool uses_mplane;
> > > +
> > > +	union {
> > > +		struct virtio_media_cmd_close close;
> > > +		struct virtio_media_cmd_ioctl ioctl;
> > > +		struct virtio_media_cmd_mmap mmap;
> > > +	} cmd;
> > > +
> > > +	union {
> > > +		struct virtio_media_resp_ioctl ioctl;
> > > +		struct virtio_media_resp_mmap mmap;
> > > +	} resp;
> > > +  
> > 
> > Heh, the above is tricky, as to parse the struct, one needs first
> > to check cmd.cmd to identify what values to pick from enums.
> > Also, the command is stored as: cmd.[close|ioctl|imap].cmd.
> > 
> > IMO, better to place the headers explicitly there, e.g.
> > 
> > 	union {
> > 		struct virtio_media_cmd_header hdr;
> > 		struct virtio_media_cmd_close close;
> > 		struct virtio_media_cmd_ioctl ioctl;
> > 		struct virtio_media_cmd_mmap mmap;
> > 	} send;
> > 	union {
> > 		struct virtio_media_resp_header hdr;
> > 		struct virtio_media_resp_ioctl ioctl;
> > 		struct virtio_media_resp_mmap mmap;
> > 	} resp;
> > 
> > Also, currently, there are 5 defined commands:
> > 
> > 	#define VIRTIO_MEDIA_CMD_OPEN 1
> > 	#define VIRTIO_MEDIA_CMD_CLOSE 2
> > 	#define VIRTIO_MEDIA_CMD_IOCTL 3
> > 	#define VIRTIO_MEDIA_CMD_MMAP 4
> > 	#define VIRTIO_MEDIA_CMD_MUNMAP 5
> > 
> > If the data struct is limited only for close/ioctl/mmap, please
> > document it and point what structure(s) other commands use.  
> 
> Tricky indeed!
> 
> However, I don't believe its necessary to add an explict header member to the
> union.

My main concern is that the field used to check what is the content
of the union is inside the union itself, e.g. virtio_media_cmd_header.cmd.

So, if one does:

	struct virtio_media_session session;

it can't check what's the command with session.cmd or session.hdr.cmd.
Instead, it needs to guess one type, like session.close.cmd to check
if the command encoded there is close, mmap or ioctl.

> Whenever the driver parses the union, it already has the necessary
> context to determine which union member to use:
> 
> - The struct virtio_media_cmd_* instances are always created by the driver and
>   sent to the device, so there are no unknowns there
> - The struct virtio_media_resp_* instances are always parsed in the same
>   function that sends the corresponding command, so again there's no
>   uncertainty about which union member to access.

If it is always the same function that creates and uses it, you don't
need an union. You can just use for instance

	struct virtio_media_cmd_ioctl

inside the instances that deal with ioctl resp (or ioctl creation).

> 
> For these reasons, I would hesistate to add the `struct virtio_media_cmd_header
> hdr` as previously suggested. Please let me know if you disagree or if I've
> misunderstood your concern.

> 
> That all being said, I have added comments about which structures use which
> commands in the upcoming v4 of the patches.
> 
> 
> > > +/**
> > > + * struct virtio_media - Virtio-media device.
> > > + * @v4l2_dev: v4l2_device for the media device.
> > > + * @video_dev: video_device for the media device.
> > > + * @virtio_dev: virtio device for the media device.
> > > + * @commandq: virtio command queue.
> > > + * @eventq: virtio event queue.
> > > + * @eventq_work: work to run when events are received on @eventq.
> > > + * @mmap_region: region into which MMAP buffers are mapped by the host.
> > > + * @event_buffer: buffer for event descriptors.
> > > + * @sessions: list of active sessions on the device.
> > > + * @sessions_lock: protects @sessions and ``virtio_media_session::list``.
> > > + * @events_lock: prevents concurrent processing of events.
> > > + * @cmd: union of device-related commands.
> > > + * @resp: union of device-related responses.
> > > + * @vlock: serializes access to the command queue.
> > > + * @wq: waitqueue for host responses on the command queue.
> > > + */
> > > +struct virtio_media {
> > > +	struct v4l2_device v4l2_dev;
> > > +	struct video_device video_dev;
> > > +
> > > +	struct virtio_device *virtio_dev;
> > > +	struct virtqueue *commandq;
> > > +	struct virtqueue *eventq;
> > > +	struct work_struct eventq_work;
> > > +
> > > +	struct virtio_shm_region mmap_region;
> > > +
> > > +	void *event_buffer;
> > > +
> > > +	struct list_head sessions;
> > > +	struct mutex sessions_lock;
> > > +
> > > +	struct mutex events_lock;
> > > +

> > > +	union {
> > > +		struct virtio_media_cmd_open open;
> > > +		struct virtio_media_cmd_munmap munmap;
> > > +	} cmd;
> > > +
> > > +	union {
> > > +		struct virtio_media_resp_open open;
> > > +		struct virtio_media_resp_munmap munmap;
> > > +	} resp;  

btw the same comment about the past union also applies here:
if the code needs somehow to identify if the command is open or
munmap, the field used to determine what command is there should
be independent of open/munmap.

> > 
> > Based on struct virtio_media_session, I'm assuming here that
> > this struct is used only for two commands, right? Please document
> > it at kernel-doc markup and add a point to the other structure used 
> > for the other commands.
> > 
> > The same comment about headers apply to the union here: place
> > the header explicitly at the union.  
> 
> The same thing I said above applies here as well. I will add the comments as
> requested in v4.
> 
> [1] https://docs.oasis-open.org/virtio/virtio/v1.4/csprd01/virtio-v1.4-csprd01-diff-from-v1.2-cs01.html#x1-8360005
> [2] https://docs.oasis-open.org/virtio/virtio/v1.4/csprd01/virtio-v1.4-csprd01-diff-from-v1.2-cs01.html#x1-8560001
> [3] https://docs.oasis-open.org/virtio/virtio/v1.4/csprd01/virtio-v1.4-csprd01-diff-from-v1.2-cs01.html#x1-8600003



Thanks,
Mauro