Details

Reviewers

bryanv
dch
manu

Group Reviewers

manpages

Commits

rG02f2706606e1: Add a virtio-gpu 2D driver

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

andrew created this revision.May 14 2023, 8:23 PM

Herald added a reviewer: manu. · View Herald TranscriptMay 14 2023, 8:23 PM

Herald added subscribers: emaste, imp. · View Herald Transcript

andrew requested review of this revision.May 14 2023, 8:23 PM

Harbormaster completed remote builds in B51488: Diff 121940.May 14 2023, 8:23 PM

dchagin added a subscriber: dchagin.May 14 2023, 8:45 PM

dchagin added inline comments.

sys/dev/virtio/gpu/virtio_gpu.c
3	By 4d846d26 this clause is changed

afedorov added a subscriber: afedorov.May 14 2023, 8:47 PM

freebsd_igalic.co added a subscriber: freebsd_igalic.co.May 14 2023, 9:25 PM

jrtc27 added a subscriber: jrtc27.May 14 2023, 10:05 PM

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?

jrtc27 added inline comments.May 14 2023, 10:10 PM

sys/dev/virtio/gpu/virtio_gpu.c
535	Uhh
573	Ditto
610	Ditto
646	Ditto
682	Ditto

jrtc27 added inline comments.May 14 2023, 10:15 PM

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?

jrtc27 added inline comments.May 14 2023, 10:16 PM

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)

Updates based on feedback

This revision now requires review to proceed.Aug 13 2023, 9:01 PM

Harbormaster completed remote builds in B53083: Diff 125950.Aug 13 2023, 9:01 PM

andrew added inline comments.Aug 14 2023, 11:40 AM

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

Harbormaster completed remote builds in B53090: Diff 125964.Aug 14 2023, 1:16 PM

Add a man page

Harbormaster completed remote builds in B53091: Diff 125966.Aug 14 2023, 2:25 PM

andrew added a reviewer: manpages.Aug 14 2023, 2:26 PM

andrew added inline comments.Aug 14 2023, 2:28 PM

share/man/man4/virtio_gpu.4
3	@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
52	.Fx instead of FreeBSD

Mention virtio_gpu(4) in virtio(4)

Harbormaster completed remote builds in B53113: Diff 126029.Aug 15 2023, 10:32 AM

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

share/man/man4/virtio_gpu.4
4	Should have a SDPX license ID as a new file, per https://docs.freebsd.org/en/articles/license-guide/#pref-license
32	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

Closed by commit rG02f2706606e1: Add a virtio-gpu 2D driver (authored by andrew). · Explain Why

This revision was automatically updated to reflect the committed changes.

andrew added a commit: rG02f2706606e1: Add a virtio-gpu 2D driver.

Add a virtio-gpu 2D driver
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 126115

share/man/man4/Makefile

share/man/man4/virtio.4

share/man/man4/virtio_gpu.4

sys/arm64/conf/std.virt

sys/conf/files

sys/dev/virtio/gpu/virtio_gpu.h

sys/dev/virtio/gpu/virtio_gpu.c

Add a virtio-gpu 2D driverClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 126115

share/man/man4/Makefile

share/man/man4/virtio.4

share/man/man4/virtio_gpu.4

sys/arm64/conf/std.virt

sys/conf/files

sys/dev/virtio/gpu/virtio_gpu.h

sys/dev/virtio/gpu/virtio_gpu.c

Add a virtio-gpu 2D driver
ClosedPublic
Actions

Revision Contents
Changeset List