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 Passed

Unit

No Test Coverage

Build Status

Buildable 51488
Build 48379: arc lint + arc unit

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
2	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
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?

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

sys/dev/virtio/gpu/virtio_gpu.c
534	Uhh
572	Ditto
609	Ditto
645	Ditto
681	Ditto

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

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?

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

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)

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

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

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

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 121940

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 121940

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