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

[Hans de Goede]

Hi,

On 06-09-16 13:47, Laszlo Ersek wrote:
> Adding Matt and Ard
>
> On 09/06/16 00:32, Dave Airlie wrote:

<snip>

> Picking different primaries on different boots sounds perfectly
> acceptable to me as long as:
> - no explicit DeviceId.BusID is specified by the user,
> - no primary emerges otherwise,
> - there are several secondaries.
>
> This behavior is clearly not ideal in the long term (like at the third,
> fourth, fifth etc boots of the same machine), but the user can trivially
> fix it *after* the installation finishes (by setting Device.BusID).
> Plus, the suggested documentation (man page) and the server log are
> clear about it.
>
> Importantly, the suggested behavior (even if non-deterministic) is
> clearly superior to the X Server not starting at all. "Output somewhere"
> is better than no output at all. (And, in 99% of the cases, there's only
> one secondary, which gives exactly the expected result.)
>
> ... If it made the series acceptable, I'd be happy to tweak patch #4 so
> that the code get active only for
>
>   (NumScreens == 0 && NumGPUScreens == 1)

I agree that this is a reasonable thing to do, if there is only
one GPUScreen we don't get the problem of which GPUScreen to pick
and then falling back to the single available GPUScreen seems
like the sensible thing to do.

That still leaves the problem though, that as I wrote on your original
submission, doing this after the probing is done is too late, because
the flags argumet passed into the probeSingleDevice call in
hw/xfree86/common/xf86platformBus.c: xf86platformProbeDev()
(and passed into the driver from there)  must match if the screen is
going to be a normal screen or a GPU-screen.

Hmm, looking at this closer, the easy fix I had in mind is not
possible, at least not in the place where I wanted to add it.

Looking even further, we should already do a fallback to the first
non-primary dev for seat0 servers and at the right time (before calling
probe), take a look at: hw/xfree86/common/xf86Bus.c: xf86BusProbe()

void
xf86BusProbe(void)
{
#ifdef XSERVER_PLATFORM_BUS
     xf86platformProbe();
     if (ServerIsNotSeat0() && xf86_num_platform_devices > 0)
         return;
#endif
#ifdef XSERVER_LIBPCIACCESS
     xf86PciProbe();
#endif
#if (defined(__sparc__) || defined(__sparc)) && !defined(__OpenBSD__)
     xf86SbusProbe();
#endif
#ifdef XSERVER_PLATFORM_BUS
     xf86platformPrimary();
#endif
}

A normal server is seat0, so ServerIsNotSeat0() returns False
and we end up calling xf86platformPrimary() at the end of the
function which falls back to the first non-primary dev.

The only reason this would not work is if xf86PciProbe()
has set primaryBus.type to a value other then BUS_NONE.

So I think we need to figure out why the existing fallback
path is not working. I suggest adding a bunch of

xf86Msg(X_INFO, "mesage text here %d\n", anintnumber);

Calls to xf86BusProbe() and xf86platformPrimary() to
figure out why this is not working.

Regards,

Hans



>
>> So I'd really prefer we try and solve this kernel side first, but I realise that
>> might not be practical.
>
> Yeah, not practical.
>
> I named kernel commit 38cb5ef4473c6 above as one tweak for the "first
> GOP" logic. Its predecessor was commit 291f36325f9f2 ("x86, efi: EFI
> boot stub support"), which had tried to remedy the non-uniqueness of the
> GOP by intersecting GOP presence with PciIo presence on the handle. It
> had proved insufficient, hence commit 38cb5ef4473c6 got written, but as
> I've showed above, that one is also insufficient for uniqueness (to
> present day).
>
> Ultimately my goal is to dump the non-determinism on the user's head,
> because at that price they'll have output "somewhere", at first boot,
> without configuring anything. I think convincing the kernel to take such
> a policy (assuming I knew where to start with the code, which I don't)
> would be harder than getting the same policy into xserver.
>
> Hans, do you think we can carry this at least in Fedora? Perhaps
> restricted to aarch64?
>
> Thanks
> Laszlo
>