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

[Laszlo Ersek]

On 09/06/16 14:46, Hans de Goede wrote:
> 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.

It is true that patch #3 does not take care of this universally (i.e.,
in a future-proof way).

It does take care of all of the *current* effects of
PLATFORM_PROBE_GPU_SCREEN, which is that the modesetting driver sets
XF86_ALLOCATE_GPU_SCREEN for xf86AllocateScreen().

That in turn makes a difference for the "scrnIndex" and "is_gpu" fields
of the newly allocated ScrnInfoRec object.

And, patch #3 does handle those as part of the promotion.

If the modesetting driver (or any other driver) used
PLATFORM_PROBE_GPU_SCREEN for anything more than passing
XF86_ALLOCATE_GPU_SCREEN to xf86AllocateScreen(), then this approach
wouldn't work.

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

Sure, that does happen (I've been aware of it, but I haven't realized it
as a problem); in this part of the function:

    /* If we haven't found a primary device try a different heuristic */
    if (primaryBus.type == BUS_NONE && num) {
        for (i = 0; i < num; i++) {
            uint16_t command;

            info = xf86PciVideoInfo[i];
            pci_device_cfg_read_u16(info, &command, 4);

            if ((command & PCI_CMD_MEM_ENABLE)
                && ((num == 1) || IS_VGA(info->device_class))) {
                if (primaryBus.type == BUS_NONE) {
                    primaryBus.type = BUS_PCI;
                    primaryBus.id.pci = info;

IS_VGA does not apply to virtio-gpu-pci, but num==1 does.

Also, virtio-gpu-pci has MMIO decoding enabled in the command register,
because virtio-gpu-pci is virtio-1.0 only, and it uses an MMIO BAR for
virtio register access and signaling.

The corresponding log snippet is

[   190.155] (II) xfree86: Adding drm device (/dev/dri/card0)
[   389.067] (--) PCI:*(0:0:1:0) 1af4:1050:1af4:1100 rev 1, Mem @
0x10040000/4096, 0x8000000000/8388608, BIOS @ 0x????????/65536

Note the "*" in "PCI:*" -- that comes from

    /* Print a summary of the video devices found */
    for (k = 0; k < num; k++) {
        const char *prim = " ";
        Bool memdone = FALSE, iodone = FALSE;

        info = xf86PciVideoInfo[k];

        if (!PCIALWAYSPRINTCLASSES(info->device_class))
            continue;

        if (xf86IsPrimaryPci(info))
            prim = "*";

        xf86Msg(X_PROBED, "PCI:%s(%u:%u:%u:%u) %04x:%04x:%04x:%04x ", prim,
                info->domain, info->bus, info->dev, info->func,
                info->vendor_id, info->device_id,
                info->subvendor_id, info->subdevice_id);

> So I think we need to figure out why the existing fallback
> path is not working.

It's because the num==1 part makes xf86PciProbe() assign BUS_PCI to
primaryBus.type.

Here's a more complete call tree:

  InitOutput()

    xf86BusProbe()
      xf86platformProbe()
      xf86PciProbe()
        primaryBus.type := BUS_PCI /* due to num == 1 */
      xf86platformPrimary()
        /* does nothing */

    xf86BusConfig()
      xf86CallDriverProbe()
        foundScreen := xf86platformProbeDev()
          /* xf86PlatformDeviceCheckBusID() is never called because
           * the user never specified Device.BusID */

          /* xf86IsPrimaryPlatform() does not match
           * because primaryBus.type == BUS_PCI */

          /* autoAddGPU is TRUE, so we call: */
          probeSingleDevice(... PLATFORM_PROBE_GPU_SCREEN ...)
          /* and because that succeeds, we set: */
          foundScreen := TRUE

      /* the libpciaccess branch is not reached
       * because foundScreen is TRUE from xf86platformProbeDev() */

I looked into why xf86IsPrimaryPlatform() wouldn't match this device in
xf86platformProbeDev(). Here's my notes from earlier, standing in
xf86IsPrimaryPlatform():

      - primaryBus.type == BUS_PCI

      - primaryBus.id.pci == 0x671600 (pointer to "struct pci_device")

      - xf86_platform_devices[0].attribs->busid == "pci:0000:00:01.0"
        (good, it matches the location of the virtio-gpu-pci device)

      - xf86_platform_devices[0].pdev == 0x670d80 (pointer to a
        different "struct pci_device" object)

      - however, the *contents* of "*primaryBus.id.pci" and
        "*xf86_platform_devices[0].pdev" are identical! They both
        describe the "virtio-gpu-pci" device.

      This means that xf86IsPrimaryPlatform() returns false despite
      "primaryBus" describing the exact same PCI device as
      "xf86_platform_devices[0]".

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

Thanks
Laszlo