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
F111463091: D40094.diff
Tue, Mar 4, 1:58 AM
Unknown Object (File)
Thu, Feb 27, 7:10 AM
Unknown Object (File)
Wed, Feb 26, 10:14 PM
Unknown Object (File)
Wed, Feb 26, 10:06 AM
Unknown Object (File)
Wed, Feb 26, 9:41 AM
Unknown Object (File)
Wed, Feb 26, 4:32 AM
Unknown Object (File)
Wed, Feb 26, 1:54 AM
Unknown Object (File)
Tue, Feb 25, 10:35 PM
View All Files
Subscribers
afedorov
dchagin
emaste
freebsd_igalic.co
imp
jrtc27
pauamma_gundo.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 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?

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

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 ↗(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.

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 ↗(On Diff #125966)

.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_gundo.com added a subscriber: pauamma_gundo.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
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.

Revision Contents
Changeset List

PathSize
sys/
arm64/
conf/
1 line
conf/
1 line
dev/
virtio/
gpu/
454 lines
691 lines

Diff 121940

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