Page MenuHomeFreeBSD
Differential D40094

Add a virtio-gpu 2D driver
ClosedPublic
Actions

Authored by andrew on May 14 2023, 8:23 PM.
Tags
None
Referenced Files
F162849144: D40094.id125964.diff
Fri, Jul 17, 2:26 PM
F162848690: D40094.id125964.diff
Fri, Jul 17, 2:18 PM
F162832398: D40094.id121940.diff
Fri, Jul 17, 9:26 AM
F162831752: D40094.id121940.diff
Fri, Jul 17, 9:19 AM
F162821056: D40094.id126029.diff
Fri, Jul 17, 6:03 AM
F162820885: D40094.id126029.diff
Fri, Jul 17, 6:00 AM
F162797369: D40094.id126115.diff
Fri, Jul 17, 12:25 AM
F162796875: D40094.id126115.diff
Fri, Jul 17, 12:18 AM
View All Files
Subscribers
afedorov
dchagin
emaste
freebsd_igalic.co
imp
jrtc27
pauamma_chezdork.com

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.

Sponsored by: Arm Ltd

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 53091
Build 49982: 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
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?

bryanv accepted this revision.May 26 2023, 3:19 AM

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
freebsd_igalic.co added a comment.May 26 2023, 9:14 AM

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

andrew updated this revision to Diff 125950.Aug 13 2023, 9:01 PM

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.

andrew updated this revision to Diff 125964.Aug 14 2023, 1:16 PM
  • 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
andrew updated this revision to Diff 125966.Aug 14 2023, 2:25 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

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

freebsd_igalic.co added a comment.Aug 15 2023, 8:59 AM

please add an entry to virtio(4) itself

share/man/man4/virtio_gpu.4
51

.Fx instead of FreeBSD

andrew updated this revision to Diff 126029.Aug 15 2023, 10:32 AM

Mention virtio_gpu(4) in virtio(4)

Harbormaster completed remote builds in B53113: Diff 126029.Aug 15 2023, 10:32 AM
pauamma_chezdork.com added a subscriber: pauamma_chezdork.com.Aug 15 2023, 1:45 PM

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.

Revision Contents
Changeset List

PathSize
share/
man/
man4/
1 line
51 lines
sys/
arm64/
conf/
1 line
conf/
1 line
dev/
virtio/
gpu/
454 lines
697 lines

Diff 125966

View Options

share/man/man4/Makefile

Loading...
View Options

share/man/man4/virtio_gpu.4

Loading...
View Options

sys/arm64/conf/std.virt

Loading...
View Options

sys/conf/files

Loading...
View Options

sys/dev/virtio/gpu/virtio_gpu.h

Loading...
View Options

sys/dev/virtio/gpu/virtio_gpu.c

Loading...