On Mon, Jul 20, 2026 at 6:09 AM Michael S. Tsirkin <[email protected]> wrote:
>
> On Mon, Jul 20, 2026 at 05:35:19AM -0700, Alistair Delva wrote:
> > On Sat, Jul 18, 2026 at 10:29 AM Michael S. Tsirkin <[email protected]> wrote:
> > >
> > > On Sun, Jul 12, 2026 at 08:57:26AM +0200, Mauro Carvalho Chehab wrote:
> > > > On Thu, 25 Jun 2026 16:18:48 -0400
> > > > Brian Daniels <[email protected]> wrote:
> > > >
> > > > > > > From: Alexandre Courbot <[email protected]>
> > > > > > >
> > > > > > > virtio_media_driver.c provides the expected driver hooks, and
> > > > > > > support
> > > > > > > for mmapping and polling.
> > > > > > >
> > > > > > > Signed-off-by: Alexandre Courbot <[email protected]>
> > > > > > > Co-developed-by: Brian Daniels <[email protected]>
> > > > > > > Signed-off-by: Brian Daniels <[email protected]>
> > > > > > > ---
> > > > > > > drivers/media/virtio/virtio_media_driver.c | 959
> > > > > > > +++++++++++++++++++++
> > > > > > > 1 file changed, 959 insertions(+)
> > > > > > > create mode 100644 drivers/media/virtio/virtio_media_driver.c
> > > > > > >
> > > > > > > diff --git a/drivers/media/virtio/virtio_media_driver.c
> > > > > > > b/drivers/media/virtio/virtio_media_driver.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000..d6363c673
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/media/virtio/virtio_media_driver.c
> > > > > > > @@ -0,0 +1,959 @@
> > > > > > > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0+
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Virtio-media driver.
> > > > > > > + *
> > > > > > > + * Copyright (c) 2024-2025 Google LLC.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/delay.h>
> > > > > > > +#include <linux/device.h>
> > > > > > > +#include <linux/dev_printk.h>
> > > > > > > +#include <linux/mm.h>
> > > > > > > +#include <linux/mutex.h>
> > > > > > > +#include <linux/scatterlist.h>
> > > > > > > +#include <linux/types.h>
> > > > > > > +#include <linux/videodev2.h>
> > > > > > > +#include <linux/vmalloc.h>
> > > > > > > +#include <linux/wait.h>
> > > > > > > +#include <linux/workqueue.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/moduleparam.h>
> > > > > > > +#include <linux/virtio.h>
> > > > > > > +#include <linux/virtio_config.h>
> > > > > > > +#include <linux/virtio_ids.h>
> > > > > > > +
> > > > > > > +#include <media/frame_vector.h>
> > > > > > > +#include <media/v4l2-dev.h>
> > > > > > > +#include <media/v4l2-event.h>
> > > > > > > +#include <media/videobuf2-memops.h>
> > > > > > > +#include <media/v4l2-device.h>
> > > > > > > +#include <media/v4l2-ioctl.h>
> > > > > > > +
> > > > > > > +#include "protocol.h"
> > > > > > > +#include "session.h"
> > > > > > > +#include "virtio_media.h"
> > > > > > > +
> > > > > > > +#define VIRTIO_MEDIA_NUM_EVENT_BUFS 16
> > > > > > > +
> > > > > > > +/* ID of the SHM region into which MMAP buffer will be mapped. */
> > > > > > > +#define VIRTIO_MEDIA_SHM_MMAP 0
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Name of the driver to expose to user-space.
> > > > > > > + *
> > > > > > > + * This is configurable because v4l2-compliance has workarounds
> > > > > > > specific to
> > > > > > > + * some drivers. When proxying these directly from the host,
> > > > > > > this allows it to
> > > > > > > + * apply them as needed.
> > > > > > > + */
> > > > > > > +char *virtio_media_driver_name;
> > > > > > > +module_param_named(driver_name, virtio_media_driver_name, charp,
> > > > > > > 0660);
> > > > > >
> > > > > >
> > > > > > Um. What? Not how it should be handled.
> > > > >
> > > > > I can remove this module param. I didn't end up using this when
> > > > > compliance testing.
> > > > > Instead, I patched v4l-utils:
> > > > > https://lore.kernel.org/all/[email protected]/
> > > > >
> > > > > Let me know if you think the v4l-utils patch is a good approach,
> > > > > otherwise let
> > > > > me know how you'd prefer to address the v4l2-compliance
> > > > > driver-specific workounds
> > > > > when they're being proxied with virtio-media.
> > > >
> > > > This kind of discussion should happen on a separate PR for
> > > > v4l2-compliance,
> > > > c/c to the proper developers and maintainers of it.
> > >
> > >
> > > I wonder how migration can work when guest is tied to host driver model
> > > like
> > > this.
> >
> > We haven't given migration much thought yet, but I think it wouldn't
> > be so different to GPU, where to do snapshot/migration we have to
> > record all initialization / context setup state and replay it against
> > the GPU driver on restore (see https://github.com/google/gfxstream).
> > Some migrations will be possible, some will not, and the host should
> > be able to decide.
> >
> > Also, Brian mentioned the host device pass-through use case, but we
> > also have device implementations on the host that work with camera
> > emulators or some other data source like video/webrtc, which will
> > support snapshot/migration more easily. This use case will probably
> > will see more real-world use and migration will be more relevant.
>
> This is par for the course.
>
>
> But I'm afraid I wasn't clear enough. This thread mentions
> supplying the host driver name to guest userspace
> in order to implement "driver specific work arounds".
Pardon my ignorance, but do a lot of v4l2 apps actually do things
differently for different drivers?
IIUC, this data is so far only used as a command line flag specified
to v4l2-compliance in the guest to pass some tests. It seems like this
information could be provided out of band for that use case.
> But given such, for migration to work userspace needs
> to be notified about driver change? And what to do about
> the things it started after migration but before the
> notification?
If this was used in a local development environment or lab, I think an
equipment change is unlikely or could be avoided by the setup. IMO,
the driver actually changing during a migration is an unlikely use
case and the host could just fail to restore the guest if a driver
change is detected.
That's why I brought up GPU - we might not support e.g. migrating from
nvidia to amd, but that's probably fine and migration is still useful
most of the time.
> > > > >
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Whether USERPTR buffers are allowed.
> > > > > > > + *
> > > > > > > + * This is disabled by default as USERPTR buffers are dangerous,
> > > > > > > but the option
> > > > > > > + * is left to enable them if desired.
> > > > > > > + */
> > > > > > > +bool virtio_media_allow_userptr;
> > > > > > > +module_param_named(allow_userptr, virtio_media_allow_userptr,
> > > > > > > bool, 0660);
> > > > > >
> > > > > >
> > > > > > is this kind of thing common?
> > > >
> > > > There is one old media device that has it (saa7134).
> > > >
> > > > >
> > > > > To be honest, I don't really know. I'm also not that familiar with
> > > > > the USERPTR
> > > > > issues. I see a few references online about their use being
> > > > > discouraged due to
> > > > > possible race conditions, perhaps that was the original motivation
> > > > > for this
> > > > > parameter (I'm not the original author of this driver).
> > > > >
> > > > > I'm open to alternatives, feel free to let me know if you have a
> > > > > preference.
> > > >
> > > > We tend to not implement USERPTR on newer drivers. I suggest you
> > > > to place the logic with regards to V4L2_MEMORY_USERPTR on a separate
> > > > patch for further discussions.
> > > >
> > > > >
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * virtio_media_session_alloc - Allocate a new session.
> > > > > > > + * @vv: virtio-media device the session belongs to.
> > > > > > > + * @id: ID of the session.
> > > > > > > + * @nonblocking_dequeue: whether dequeuing of buffers should be
> > > > > > > blocking or
> > > > > > > + * not.
> > > > > > > + *
> > > > > > > + * The ``id`` and ``list`` fields must still be set by the
> > > > > > > caller.
> > > > > >
> > > > > > still in what sense?
> > > > >
> > > > > Based on the code below, I'm not so sure that the caller is
> > > > > responsible for
> > > > > setting these values. They seem to be initialized in the function.
> > > > >
> > > > > Perhaps Alexandre Courbot (the original author) would know more.
> > > > > Unless he
> > > > > says otherwise though I'm inclined to remove this comment.
> > > > >
> > > > > > > + */
> > > > > > > +static struct virtio_media_session *
> > > > > > > +virtio_media_session_alloc(struct virtio_media *vv, u32 id,
> > > > > > > + struct file *file)
> > > > > > > +{
> > > > > > > + struct virtio_media_session *session;
> > > > > > > + int i;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + session = kzalloc_obj(*session, GFP_KERNEL);
> > > > > > > + if (!session)
> > > > > > > + goto err_session;
> > > > > > > +
> > > > > > > + session->shadow_buf = kzalloc(VIRTIO_SHADOW_BUF_SIZE,
> > > > > > > GFP_KERNEL);
> > > > > > > + if (!session->shadow_buf)
> > > > > > > + goto err_shadow_buf;
> > > > > > > +
> > > > > > > + ret = sg_alloc_table(&session->command_sgs,
> > > > > > > DESC_CHAIN_MAX_LEN,
> > > > > > > + GFP_KERNEL);
> > > > > > > + if (ret)
> > > > > > > + goto err_payload_sgs;
> > > > > > > +
> > > > > > > + session->id = id;
> > > > > > > + session->nonblocking_dequeue = file->f_flags & O_NONBLOCK;
> > > > > > > +
> > > > > > > + INIT_LIST_HEAD(&session->list);
> > > > > > > + v4l2_fh_init(&session->fh, &vv->video_dev);
> > > > > > > + virtio_media_session_fh_add(session, file);
> > > > > > > +
> > > > > > > + for (i = 0; i <= VIRTIO_MEDIA_LAST_QUEUE; i++)
> > > > > > > +
> > > > > > > INIT_LIST_HEAD(&session->queues[i].pending_dqbufs);
> > > > > > > + mutex_init(&session->queues_lock);
> > > > > > > +
> > > > > > > + init_waitqueue_head(&session->dqbuf_wait);
> > > > > > > +
> > > > > > > + mutex_lock(&vv->sessions_lock);
> > > > > > > + list_add_tail(&session->list, &vv->sessions);
> > > > > > > + mutex_unlock(&vv->sessions_lock);
> > > > > > > +
> > > > > > > + return session;
> > > > > > > +
> > > > > > > +err_payload_sgs:
> > > > > > > + kfree(session->shadow_buf);
> > > > > > > +err_shadow_buf:
> > > > > > > + kfree(session);
> > > > > > > +err_session:
> > > > > > > + return ERR_PTR(-ENOMEM);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * virtio_media_session_free - Free all resources of a session.
> > > > > > > + * @vv: virtio-media device the session belongs to.
> > > > > > > + * @session: session to destroy.
> > > > > > > + *
> > > > > > > + * All the resources of @sesssion, as well as the backing memory
> > > > > > > of @session
> > > > > > > + * itself, are freed.
> > > > > >
> > > > > > why @ here and `` above? And typo in the name.
> > > > >
> > > > > The `@` here was an attempt to follow the guide here for referencing
> > > > > function
> > > > > parameters:
> > > > > https://docs.kernel.org/doc-guide/kernel-doc.html#highlights-and-cross-references
> > > >
> > > > Yes. This is part of Linux Kernel kernel-doc markup: when referring to
> > > > struct fields, you should use @field (or ``field`` if one wants to
> > > > place an
> > > > asterisk on it, like ``*field``).
> > > >
> > > > >
> > > > > That being said, I don't believe this file is 100% consistent with
> > > > > that. I will
> > > > > spend some time cleaning up the comments throughout this patch set to
> > > > > get them
> > > > > consistent for v5. Thanks!
> > > >
> > > > Please use it on a consistent way along the driver.
> > > >
> > > >
> > > > Thanks,
> > > > Mauro
> > >
>
