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

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.

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. 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.

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.
[1] https://github.com/libkrun/libkrun/pull/811
[2] https://github.com/rust-vmm/vhost-device/tree/main/vhost-device-media

BR,
Dorinda.