Page MenuHomeFreeBSD

Add a virtio-gpu 2D driver
ClosedPublic

Authored by andrew on May 14 2023, 8:23 PM.
Tags
None
Referenced Files
F102917435: D40094.id125966.diff
Mon, Nov 18, 5:45 PM
Unknown Object (File)
Sat, Nov 16, 6:50 AM
Unknown Object (File)
Sat, Nov 16, 6:49 AM
Unknown Object (File)
Fri, Nov 8, 1:31 PM
Unknown Object (File)
Sat, Oct 26, 5:17 AM
Unknown Object (File)
Thu, Oct 24, 11:16 PM
Unknown Object (File)
Thu, Oct 24, 11:15 PM
Unknown Object (File)
Thu, Oct 24, 11:09 PM

Details

Summary

Add a driver to connect vt to the VirtIO GPU device in 2D mode. This
provides a output on the display when a qemu virtio gpu device is
added, e.g. with -device virtio-gpu-pci.

Tested on qemu using UTM, and a Hetzner arm64 VM instance.

Sponsored by: Arm Ltd

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 53083
Build 49974: arc lint + arc unit

Event Timeline

dchagin added inline comments.
sys/dev/virtio/gpu/virtio_gpu.c
3

By 4d846d26 this clause is changed

jrtc27 added inline comments.
sys/dev/virtio/gpu/virtio_gpu.c
71

?

155

?

224

Why here?

228

Why not?

295

!= 0

296

Print error

303

!= 0

304

Print error

312

!= 0

313

Print error

326

!= 0

327

Print error

332

!= 0

333

Print error

339

!= 0

340

Print error

351

!= 0

365

Various attach error paths end up here with one or both not allocated yet

391

!= 0

397

Why here

431

?

452

Print error

458

Print error

463

Print error

478

If this is to avoid implicit padding you should probably be documenting that, maybe also with a CTASSERT

518

Ditto

553

[1]? Huh?

sys/dev/virtio/gpu/virtio_gpu.c
535

Uhh

573

Ditto

610

Ditto

646

Ditto

682

Ditto

sys/dev/virtio/gpu/virtio_gpu.c
136–139

What should happen for these functions when there's an error?..

345–348

These surely need to check for errors though?

sys/dev/virtio/gpu/virtio_gpu.c
36

Don't need that any more?

Thanks so much for this work! 1-2 years back I think there was an effort to port the full Linux DRM virito_gpu driver over, but I believe that work has stalled. This is great to have.

sys/dev/virtio/gpu/virtio_gpu.c
229

Yah while here, might as well add the currently defined feature bits: https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-3680003, so the probe/attach can print the feature names instead of hex values.

300

It looks like gpucfg isn't later used - I'm not sure if there is anything to gain though with the current code by using gpucfg.num_scanouts in vtgpu_get_display_info() instead of always MAX_SCANOUTS.

432

I think a strict reading of the spec requires this queue (so the guest presents 2 queues); actual devices may be lenient though. I believe this queue is guest -> host only so should be able to include it here but otherwise not actually use it; or did qemu have some error with this?

528

The RHS should be wrapped in the appropriate htoleXX() to ensure they're LE. Similarly, fields read from the responses will need leXXtoh(). The Virtio spec will denote these fields as le{16,32,64}. I don't believe the GPU device was ever a "legacy" (pre V1 spec) device, so there isn't a need to handle the pre-V1 guest endian stuff the other drivers do.

This revision is now accepted and ready to land.May 26 2023, 3:19 AM

reminder: this driver needs a man page, and an entry in virtio(4)

This revision now requires review to proceed.Aug 13 2023, 9:01 PM
sys/dev/virtio/gpu/virtio_gpu.c
136–139

There's not much we can do given these functions don't return any data.

296

Why? We already print the return value when an device_attach implementation returns non-zero.

478

This is for the sglist and is a common pattern in virtio drivers.

  • Set the offset in vtgpu_transfer_to_host_2d
  • Reduce the rectangle transfered in vtgpu_fb_bitblt_text
share/man/man4/virtio_gpu.4
2 ↗(On Diff #125966)

@bryanv Can I remove this line? The man page is based on another you wrote so I kept your copyright in, but All rights reserved is being removed as it's now meaningless.

please add an entry to virtio(4) itself

share/man/man4/virtio_gpu.4
51 ↗(On Diff #125966)

.Fx instead of FreeBSD

Mention virtio_gpu(4) in virtio(4)

Other than these 2 nits and what others mentioned, manual page English LGTM.

share/man/man4/virtio_gpu.4
3 ↗(On Diff #126029)

Should have a SDPX license ID as a new file, per https://docs.freebsd.org/en/articles/license-guide/#pref-license

31 ↗(On Diff #126029)

Taste: GPU in all caps here as it's an acronym (for consistency with "IO")

This revision was not accepted when it landed; it landed in state Needs Review.Aug 17 2023, 11:27 AM
This revision was automatically updated to reflect the committed changes.