[PATCH] drm_virtgpu: Add functional coverage for core VirtIO-GPU ioctls

[Kamil Konieczny]

Hi Dorinda,
On 2025-05-26 at 16:06:07 +0200, Dorinda Bassey wrote:
> Hi Kamil,
> 
> Thank you very much for the reviews!
> 
> please use 'tests/' as a prefix in subject, also change test name,
> > so instead of
> > [PATCH] drm_virtgpu: Add functional coverage for core VirtIO-GPU ioctls
> >
> > better:
> > [PATCH] tests/virtgpu: Add functional coverage for core VirtIO-GPU ioctls
> >
> Ack, but I would rather prefer to use tests/drm_virtgpu instead considering
> that I'm testing DRM ioctls for the virtio-GPU driver and this matches
> other test patterns

Up to you, it could be also tests/drm_virtgpu:
and a test file drm_virtgpu.c

> 
> I would name them without 'drm-virtgpu-' prefix. Up to you.
> >
> I think the drm-virtgpu prefix looks better, I also figured it fits with
> the igt naming convention, like the drm_mm follows this format.
> 
> Could someone from your team help with review?
> 
> Sure @Albert Esteve <aesteve at redhat.com> @Javier Martinez Canillas
> <javierm at redhat.com> could you help?
> 
> Please rename it into virtgpu.c:
> >
> 
> > s/drm_virtgpu.c/virtgpu.c/
> 
> As per the first reply, I believe drm_virtgpu is better

ok

Regards,
Kamil

> 
> 
> > Please move igt headers after system ones.
> 
> Ack
> 
> Remove space from " Testing", so
> 
> Yes, thanks for the catch!
> 
> At least try to open all cardN until you find virtio, there is
> > sometimes simpleDRM driver loaded and then cards could start from 1,
> > /dev/dri/card1
> >
> > What about checking with getversion? For idea see core_getversion test.
> 
>  Right! This makes sense - Thank you for the reference.
> 
> Please do not make initialization with asserts here, I would suggest
> > to move this into a function, for example:
> > int create_resource(int fd, ... other params...)
> >
> > and then calling this before each test. For example:
> >
> > bool create_res(struct drm_virtgpu_resource_create *res)
> > {
> >         if(res->array_size == 1) /* already created */
> >                 return true;
> >
> >         ... rest of creation ...
> > }
> >
> >         igt_subtest("virtgpu-map") {
> >                 ...vars here...
> >
> >                 igt_assert(create_res(&args));
> >
> >
> > Or you could just create it in this fixture without asserts,
> > place asserts in a function check_res() and call it in each
> > subtests which needs them. For example:
> >
> >         igt_subtest("virtgpu-map") {
> >                 ...vars here...
> >
> >                 check_res(&args);
> >
> > This also looks like a good candidate for a separate subtest?
> >
> Yes, Initially I thought about making it a seperate subtest but considering
> that many subtests needed it, I put it in the igt_fixture().
> I will put it in a separate function and create a
> `drm-virtgpu-resource-create` subtest to run it independently, this will
> also enable it to be used by other subtests.
> 
> 
> > Please use C-style comments:
> >                 /* Request mmap offset */
> >
> > Same applies for following code.
> 
> Noted with thanks!
> 
> BR,
> Dorinda Bassey.
> 
> 
> On Thu, May 22, 2025 at 6:20 PM Kamil Konieczny <
> kamil.konieczny at linux.intel.com> wrote:
> 
> > Hi Dorinda,
> > On 2025-05-14 at 12:28:00 +0200, Dorinda Bassey wrote:
> >
> > please use 'tests/' as a prefix in subject, also change test name,
> > so instead of
> > [PATCH] drm_virtgpu: Add functional coverage for core VirtIO-GPU ioctls
> >
> > better:
> > [PATCH] tests/virtgpu: Add functional coverage for core VirtIO-GPU ioctls
> >
> > > This test suite adds coverage for multiple DRM ioctls specific
> > > to the VirtIO-GPU driver, verifying functionality such as
> > > resource creation, memory mapping, 3D transfers, context
> > > initialization, and parameter querying.
> > > Each test validates a key ioctl to ensure correct behavior from
> > > user space and backend implementations. Each subtest is
> > > self-contained and can be executed independently.
> > >
> > > Included subtests:
> > >   - drm-virtgpu-map
> > >   - drm-virtgpu-execbuffer
> > >   - drm-virtgpu-resource-info
> > >   - drm-virtgpu-3d-transfer-to-host
> > >   - drm-virtgpu-3d-transfer-from-host
> > >   - drm-virtgpu-3d-wait
> > >   - drm-virtgpu-resource-create-blob
> > >   - drm-virtgpu-get-caps
> > >   - drm-virtgpu-context-init
> > >   - drm-virtgpu-getparam
> >
> > I would name them without 'drm-virtgpu-' prefix. Up to you.
> >
> > >
> > > How to Test with QEMU virtio-vga-gl or rustvmm vhost-device-gpu
> > >
> > > 1. Launch a QEMU guest with virtio-vga-gl
> > > ./build/qemu-system-x86_64 \
> > >   -enable-kvm \
> > >   -cpu host \
> > >   -m 4096 \
> > >   -machine q35 \
> > >   -display gtk,gl=on \
> > >   -vga none \
> > >   -device virtio-vga-gl \
> > >   -drive file=image.qcow2,format=qcow2 \
> > >   -netdev user,id=n0,hostfwd=tcp::2222-:22 \
> > >   -device virtio-net-pci,netdev=n0
> > >
> > > ssh into the guest and run the tests.
> > >
> > > 2. Start the vhost-device-gpu backend and Launch a QEMU
> > > guest with vhost-user-gpu-pci or vhost-user-vga, see guide on:
> > > https://crates.io/crates/vhost-device-gpu
> >
> > Could someone from your team help with review?
> >
> > >
> > > Signed-off-by: Dorinda Bassey <dbassey at redhat.com>
> > > ---
> > >  tests/drm_virtgpu.c | 388 ++++++++++++++++++++++++++++++++++++++++++++
> >

[...cut...]

> > > diff --git a/tests/meson.build b/tests/meson.build
> > > index 20ddddb89..ac85cac30 100644
> > > --- a/tests/meson.build
> > > +++ b/tests/meson.build
> > > @@ -13,6 +13,7 @@ test_progs = [
> > >       'drm_buddy',
> > >       'drm_mm',
> > >       'drm_read',
> > > +     'drm_virtgpu',
> > >       'fbdev',
> > >       'kms_3d',
> > >       'kms_addfb_basic',
> > > --
> > > 2.48.1
> > >
> >
> >