Re: [PATCH v4 0/8] media: add virtio-media driver

[Brian Daniels]

On Tue, Aug 11, 2026 at 4:43 AM Dorinda Bassey <dbassey@xxxxxxxxxx> wrote:
>
> Hi Brian,
>
> I've been working on a vhost-user frontend for virtio-media
> in libkrun[1] and tested this series with vhost-device-media[2].
> A few things I noticed:
>
> 1. Wrong constant in vfl_dir detection in virtio_media_driver.c:
> >
> > + if (vd->device_caps & (V4L2_CAP_VIDEO_M2M | V4L2_CAP_VIDEO_M2M_MPLANE))
> > + vd->vfl_dir = VFL_DIR_M2M;
> > + else if (vd->device_caps &
> > + (V4L2_CAP_VIDEO_OUTPUT | V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> > + vd->vfl_dir = VFL_DIR_TX;
>
> V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE is a buffer
> type enum, not a capability flag. So it should be
> V4L2_CAP_VIDEO_OUTPUT_MPLANE.

Good catch! I've staged the fix in v7.

> 2. Kconfig architecture support
>
> > + depends on VIRTIO && VIDEO_DEV && 64BIT && (X86 || (ARM && CPU_LITTLE_ENDIAN))
>
> ARM64 is missing here, so the driver cannot be selected on
> aarch64. Also, CONFIG_ARM is 32-bit only, so 64BIT && ARM
> can never be true; that branch is dead code.

I was able to confirm that you can't build it for ARM64 as written in
v6, I've updated it for v7 as you suggested, thanks!

> Since the driver
> has no platform specific code, this could just be:
>
> + depends on VIRTIO && VIDEO_DEV && 64BIT
>
> as it already covers x86_64, ARM64, and any other 64 bit
> architecture without listing them individually.

While it covers the 64 bit requirement, it does not cover the little
endian requirement.

The virtio spec states that the v4l2 structs transferred should be in
little endian format (section 5.22.6.1.5) [1]. Currently, the driver
does not do any translation on big endian guests, so for now the
driver should be restricted to little endian guests.

As far as I can tell, ``CPU_LITTLE_ENDIAN`` isn't specified for all
architectures (importantly, it does't cover x86). Perhaps this could
be made less restrictive with:

+ depends on VIRTIO && VIDEO_DEV && 64BIT && (x86 || CPU_LITTLE_ENDIAN)

I'd lean on others' expertise here if they have suggestions.

I should note this driver has only been tested on x86 and ARM64
platforms, so I can't make any guarantees on compatibility :)

> 3. Error sign convention:
>
> > + resp_header = sg_virt(sgs[out_sgs]);
> > + if (resp_header->status)
> > + /* Host returns a positive error code. */
> > + return -resp_header->status;
>
> The comment in the driver and the protocol spec contradict
> each other. The protocol.h describes the status field as
> "one of the standard Linux error codes" without specifying
> a sign. It's probably worth making the protocol spec explicit
> that the host returns positive error codes to match the driver's
> assumption.

I'll update the comment in protocol.h to explicitly state the error
code is positive in v7.

> [1] https://github.com/libkrun/libkrun/pull/811
> [2] https://github.com/rust-vmm/vhost-device/tree/main/vhost-device-media
>
> BR,
> Dorinda.

Thanks again for testing and for sending fixes!

[1] https://docs.oasis-open.org/virtio/virtio/v1.4/csprd01/virtio-v1.4-csprd01-diff-from-v1.2-cs01.html#x1-8360005