[PATCH xserver 4/4] xfree86: promote one GPU screen if (NumScreens == 0 && NumGPUScreens > 0)

Mon Sep 5 12:39:40 UTC 2016

On 09/04/16 11:01, Hans de Goede wrote:
> HI,
> 
> On 04-09-16 03:11, Laszlo Ersek wrote:
>> Aarch64/KVM virtual machines cannot use emulated graphics cards with
>> linear framebuffers, due to architectural cache coherency issues. (For
>> this reason, "qemu-system-aarch64" doesn't even include the "virtio-vga"
>> device model.)
>>
>> Such guests can use the "virtio-gpu-pci" device well, through the
>> "modesetting" driver. However, given that "virtio-gpu-pci" is never
>> recognized as a primary graphics card (which is otherwise correct,
>> generally speaking),
> 
> Why is this otherwise correct, generally speaking ? AFAIK virtio-gpu-pci
> is needed for Virgil, so I would expect it to become the default graphical
> card in vms in the future, or is that what virtio-vga is for ?

Yes. Both virtio-vga and virtio-gpu-pci share the VirtIO GPU Device
part, but only the former has the VGA compat stuff (incl. the legacy
framebuffer), and only the former qualifies as "primary graphics card"
at the moment.

I didn't want to mess with the definition of "primary graphics card",
because that seems to have ties to platforms and graphics cards that I
have no clue about. Instead, picking an unspecified secondary graphics
card automatically, when there is no primary graphics card and the user
also doesn't select a secondary with Device.BusID, seems quite robust
for physical platforms too.

In fact Marcin just informed me about the same problem for physical
(aarch64) hosts, with Radeon (?) cards; that is, with cards that Xserver
currently considers "secondary / GPU":

https://bugzilla.redhat.com/show_bug.cgi?id=1256933

https://marcin.juszkiewicz.com.pl/2015/09/11/how-to-get-xserver-running-out-of-box-on-aarch64/

I think this series could solve that problem as well, without
customizing on what architecture what kind of card should be considered
primary / secondary.

> Either way I've a feeling that it would be better to just get
> virtio-gpu-pci recgonized as primary card rather then doing
> this moving a gpu from one list to the next trickery, which
> is likely to break.

Perfectly fine by me, if someone is willing to write and shepherd in
that patch :) My series may as well be considered an elaborate upstream
bug report; I don't mind that.

Note however the identical issue brought up by Marcin, for a different card:

* On x86_64 phys hw, you likely want to consider the Radeon GPU in
question secondary, and stick with the integrated graphics device as
primary. Similarly, in x86_64 KVM guests, you likely want to consider
"virtio-vga" (which includes the VGA compat stuff, e.g. the IO ports and
the linear framebuffer) the primary card, and consider any other
"virtio-gpu-pci" devices -- there might be several -- as secondaries.

* Whereas on aach64 phys hw, you won't have legacy IO ports, and if
there are three Readon GPUs in the system, you won't be able to pick any
of those as primary. So, pick just "one", unless the user says which
one. Similarly, in an aarch64/KVM guest, you might have three
"virtio-gpu-pci" devices (all secondaries), and certainly no
"virtio-vga". Again, just pick one.

I'm not against refining what "primary card" means on what architecture,
I'm just not up to do the design :)

> 
> Actually it is already broken, the PLATFORM_PROBE_GPU_SCREEN flag
> gets passed into the driver's probe method and at least the
> modesetting driver behaves differently depending on whether
> that flag as passed in or not. Now this only influences its
> prime functionality which is not likely to get used in your
> example, but it does show that simply moving the gpu from
> one list to another is doing things to late. The fix need
> to influence the Xserver's behavior before the drivers
> probe method gets called.
> 
> I would try to recognize there is no primary card before any
> probe methods are called and then specifically look for a
> virtio-gpu-pci device and promote that to primary.

Works for me, but (a) this kind of change is definitely out of my
xserver "skills", (b) see the Radeon + aarch64 example: this approach
might lead to an endless tweaking of the definition of "primary".

> Note I would not just take any non primary device, that
> may have unwanted consequences on actual hardware.

What consequences in particular?

> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> This also means that you can drop patch 3/4 from your patch-set.

Unfortunately, patch #4 is what I really care about. (Well, the
functionality that it adds.)

If a seasoned xserver developer is interested in fixing this problem,
I'd like to file an upstream bug tracker entry for it, and assign it to
him/her -- I'm way out of my depth here :)

Thanks
Laszlo

> 
> 
>  in these guests the X server currently exits with "No
>> devices detected", unless the user provides a config file with a Device
>> section that identifies the card by BusID.
>>
>> Although this behavior matches the documentation precisely, it is
>> inconvenient for virtual machines, where the X server is expected to
>> start
>> regardless of the (unpredictable) PCI B/D/F of the "virtio-gpu-pci"
>> device.
>>
>> Modify xf86BusConfig() so that if no screen has been found, but there is
>> at least one GPU screen, one of the GPU screens be promoted to the single
>> normal screen. (This change should be either unnoticeable on physical
>> platforms, or beneficial even.) For this, call the
>> xf86PromoteLastGPUScreenToLastScreen() helper function added in the last
>> patch.
>>
>> Update the manual page accordingly. The documentation remains
>> intentionally vague on what GPU screen gets promoted under the above
>> circumstances. The statements about multi-head setups, and about
>> single-head setups using the primary graphics card, are preserved intact.
>>
>> Cc: Adam Jackson <ajax at redhat.com>
>> Cc: Dave Airlie <airlied at redhat.com>
>> Cc: Keith Packard <keithp at keithp.com>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>>  hw/xfree86/common/xf86Bus.c  | 19 ++++++++++++++++---
>>  hw/xfree86/man/xorg.conf.man | 21 ++++++++++++++++-----
>>  2 files changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/xfree86/common/xf86Bus.c b/hw/xfree86/common/xf86Bus.c
>> index dc83e2dde099..572d91fd223b 100644
>> --- a/hw/xfree86/common/xf86Bus.c
>> +++ b/hw/xfree86/common/xf86Bus.c
>> @@ -127,10 +127,23 @@ xf86BusConfig(void)
>>          xf86CallDriverProbe(xf86DriverList[i], FALSE);
>>      }
>>
>> -    /* If nothing was detected, return now */
>>      if (xf86NumScreens == 0) {
>> -        xf86Msg(X_ERROR, "No devices detected.\n");
>> -        return FALSE;
>> +        if (xf86NumGPUScreens == 0) {
>> +            /* If nothing was detected, return now */
>> +            xf86Msg(X_ERROR, "No devices detected.\n");
>> +            return FALSE;
>> +        }
>> +
>> +        /*
>> +         * If no screen was detected, but there is at least one GPU
>> screen,
>> +         * promote one of the latter to be the single screen. Note
>> that a
>> +         * graphics card matched by a Device.BusID (i.e., from the
>> config file)
>> +         * is added as a screen, not as a GPU screen; so if that
>> happens, we
>> +         * don't reach this code.
>> +         */
>> +        xf86PromoteLastGPUScreenToLastScreen();
>> +        xf86Msg(X_WARNING, "Use Device.BusID to select a specific
>> secondary "
>> +                "graphics card.\n");
>>      }
>>
>>      xf86VGAarbiterInit();
>> diff --git a/hw/xfree86/man/xorg.conf.man b/hw/xfree86/man/xorg.conf.man
>> index 94b199e6bac4..36f04febf2f4 100644
>> --- a/hw/xfree86/man/xorg.conf.man
>> +++ b/hw/xfree86/man/xorg.conf.man
>> @@ -1353,11 +1353,22 @@ the
>>  string has the form
>>  .BI PCI: bus : device : function
>>  (e.g., \(lqPCI:1:0:0\(rq might be appropriate for an AGP card).
>> -This field is usually optional in single-head configurations when using
>> -the primary graphics card.
>> -In multi-head configurations, or when using a secondary graphics card
>> in a
>> -single-head configuration, this entry is mandatory.
>> -Its main purpose is to make an unambiguous connection between the device
>> +In multi-head configurations, this entry is mandatory.
>> +It is usually optional in single-head configurations when using the
>> primary
>> +graphics card.
>> +When using a secondary graphics card in a single-head configuration,
>> +.B BusID
>> +is recommended; if no graphics card can be identified as primary, nor
>> is any
>> +card selected by at least one
>> +.B Device
>> +section through the
>> +.B BusID
>> +field, then an unspecified secondary card (if any) is promoted to
>> primary
>> +status.
>> +The main purpose of
>> +.B BusID
>> +is to make an unambiguous connection between the
>> +.B Device
>>  section and the hardware it is representing.
>>  This information can usually be found by running the pciaccess tool
>>  scanpci.
>>