Page MenuHomeFreeBSD

virtio_gpu: add support for xf86-video-scfb
Needs ReviewPublic

Authored by tiga on Sat, Jan 31, 3:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Feb 22, 9:07 AM
Unknown Object (File)
Thu, Feb 19, 9:20 AM
Unknown Object (File)
Sat, Feb 14, 6:23 PM
Unknown Object (File)
Sat, Feb 14, 3:24 PM
Unknown Object (File)
Fri, Feb 13, 7:26 AM
Unknown Object (File)
Wed, Feb 11, 12:04 PM
Unknown Object (File)
Tue, Feb 10, 1:02 AM
Unknown Object (File)
Mon, Feb 9, 10:54 AM
Subscribers

Details

Summary

virtio_gpu: add support for xf86-video-scfb

Added virtio_gpu support for UTM/MacOS configured with virtio-gpu-pci.
Tunable parameters:

  • hw.virtio_gpu.width
  • hw.virtio_gpu.height
  • dev.vtgpu.X.refresh_rate

Tested with FreeBSD 15 (arm64) running on MacOS 26.2 and UTM (version 4.7.5 / qemu 10.0.2).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

tiga requested review of this revision.Sat, Jan 31, 3:10 PM

So I don't understand the underlying code well enough to have a good take on this... It seems like this is always active, but maybe shouldn't be.
It would be helpful, though, if you could upload the next set of diffs created with -U9999 so that we get enough context in phabricator. It made it hard to check on stuff

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

why is this. unconditional?

405

this seems backwards.

423

Do we always need this periodic flush, or just when this feature is active?

473

when would this get called before initialized is true?

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

I am guessing that VirtIO GPU always allocates the framebuffer in system RAM rather than mapping actual GPU VRAM, so I decided to use VM_MEMATTR_DEFAULT. I am not sure 100% about this, though, but it seemed right...

405

yes, i agree looks very strange. when running my tests, it turns out that after registering with fbd_register, the device /dev/fb0 appears, and the result if EEXIST. not really sure why...
I propose to change the code to the following (tested and seems working):

} else {
    device_printf(dev, "/dev/fb0 created\n");
    sc->vtgpu_fb_registered = true;
}
423

You are right - this periodic flush should only be done when sc->vtgpu_fb_registered is true
I will change the code to reflect this, and test it again

473

i tried to do some defensive programming here, you are right, when this function is called, the vtgpu_initialized should always be true. this check can be reduced to

if (!sc->vtgpu_detaching)

i will change accordingly and test

Updated code according to review comments:

  • removed vtgpu_initialized flag
  • flush task only started when /dev/fd0 is available
  • improved logic for device_printf when attached
  • removed unnecessary if conditions

Sorry, new here... still learning
This patch has -U9999, as requested

What is the overhead form all the extra wakeups this adds? I'd prefer we add a way for userspace to tell the kernel to perform an update.

What is the overhead form all the extra wakeups this adds? I'd prefer we add a way for userspace to tell the kernel to perform an update.

Hi. In my computer, the overhead is not really noticeable - and if this is a problem, it can be controlled with the "refresh_rate" setting...
This is for sure not a "good" solution, but it is kind of working...
I see this as temporary until we can have a solution that e.g. properly implements 3D / hardware acceleration - unfortunately I do not know how to do this.
In any case, this is my first patch - I am open to suggestions, in case someone knows a better way to implement this :-)