Page MenuHomeFreeBSD

Add a virtio-gpu 2D driver
ClosedPublic

Authored by andrew on May 14 2023, 8:23 PM.
Tags
None
Referenced Files
F88934117: D40094.diff
Sun, Jul 21, 1:19 AM
Unknown Object (File)
Tue, Jul 9, 6:49 PM
Unknown Object (File)
Sat, Jul 6, 11:39 PM
Unknown Object (File)
Thu, Jul 4, 5:54 AM
Unknown Object (File)
Thu, Jul 4, 4:51 AM
Unknown Object (File)
Mon, Jul 1, 11:01 PM
Unknown Object (File)
Mon, Jul 1, 1:07 AM
Unknown Object (File)
Sat, Jun 22, 10:56 AM

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 Passed
Unit
No Test Coverage
Build Status
Buildable 51488
Build 48379: arc lint + arc unit

Event Timeline

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

By 4d846d26 this clause is changed

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

?

154

?

223

Why here?

227

Why not?

294

!= 0

295

Print error

302

!= 0

303

Print error

311

!= 0

312

Print error

325

!= 0

326

Print error

331

!= 0

332

Print error

338

!= 0

339

Print error

350

!= 0

364

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

390

!= 0

396

Why here

430

?

451

Print error

457

Print error

462

Print error

477

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

517

Ditto

552

[1]? Huh?

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

Uhh

572

Ditto

609

Ditto

645

Ditto

681

Ditto

sys/dev/virtio/gpu/virtio_gpu.c
135–138

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

344–347

These surely need to check for errors though?

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

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
228

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.

299

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.

431

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?

527

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
135–138

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

295

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

477

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.