[PATCH] platform: support non-pci platform devices

[Hans de Goede]

Hi,

On 06/16/2014 01:32 PM, Rob Clark wrote:
> On Mon, Jun 16, 2014 at 2:49 AM, Hans de Goede <hdegoede at redhat.com> wrote:
>> Hi,
>>
>> On 06/14/2014 09:58 PM, Rob Clark wrote:
>>> This makes things not completely fail if DDX implements platformProbe()
>>> but the device is not actually a PCI device.  Also, the platform device
>>> name does not always match the DDX name, so deal with that.  I'm sure
>>> there are more cases that find_non_pci_driver() needs to handle for
>>> that, but this is a start.
>>>
>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>
>> Looks good, one small remark below.
>>
>>> ---
>>> NOTE: I still need to figure out a sane way to workaround the existing
>>> bug with non-pci platform devices.  Currently, if DDX implements the
>>> platformProbe() hook, then in xf86platformProbeDev() it will get claimed
>>> in the autoAddGPU loop, resulting that the old-school Probe() fallback
>>> (if you have .conf file) fails too because the device is already claimed.
>>> We kind of need a way that the DDX can detect that the xserver does not
>>> have this fix, so that it can work around the bug by failing
>>> platformProbe().
>>>
>>>  hw/xfree86/common/xf86platformBus.c | 65 ++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 61 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c
>>> index dd118a2..eca0f8c 100644
>>> --- a/hw/xfree86/common/xf86platformBus.c
>>> +++ b/hw/xfree86/common/xf86platformBus.c
>>> @@ -199,6 +199,41 @@ xf86_check_platform_slot(const struct xf86_platform_device *pd)
>>>      return TRUE;
>>>  }
>>>
>>> +static int
>>> +find_non_pci_driver(const char *busid, char *returnList[], int returnListMax)
>>> +{
>>> +    /* Add more entries here if we ever return more than 4 drivers for
>>> +       any device */
>>> +    const char *driverList[5] = { NULL, NULL, NULL, NULL, NULL };
>>> +    int i = 0;
>>> +    char *p, *s;
>>> +
>>> +    s = xstrdup(busid);
>>> +    p = strtok(s, ":");
>>> +
>>> +    if (strcmp(p, "platform"))
>>> +        goto out;
>>> +
>>> +    /* extract device name: */
>>> +    p = strtok(NULL, ":");
>>> +
>>> +    /* check for special cases where DDX driver name does not match busid: */
>>> +    if (!strcmp(p, "mdp")) {
>>> +        driverList[i++] = "freedreno";
>>> +    }
>>> +
>>> +    /* add name derived from busid last: */
>>> +    driverList[i++] = p;
>>> +
>>> +    for (i = 0; (i < returnListMax) && (driverList[i] != NULL); i++) {
>>> +        returnList[i] = xnfstrdup(driverList[i]);
>>> +    }
>>> +
>>> +out:
>>> +    free(s);
>>> +    return i;                   /* Number of entries added */
>>> +}
>>> +
>>>  /**
>>>   *  @return The numbers of found devices that match with the current system
>>>   *  drivers.
>>> @@ -230,6 +265,9 @@ xf86PlatformMatchDriver(char *matches[], int nmatches)
>>>
>>>              if ((info != NULL) && (j < nmatches)) {
>>>                  j += xf86VideoPtrToDriverList(info, &(matches[j]), nmatches - j);
>>> +            } else if (j < nmatches) {
>>> +                char *busid = xf86_get_platform_attrib(i, ODEV_ATTRIB_BUSID);
>>> +                j += find_non_pci_driver(busid, &(matches[j]), nmatches - j);
>>>              }
>>>          }
>>>      }
>>> @@ -248,6 +286,9 @@ xf86platformProbe(void)
>>>          pci = FALSE;
>>>      }
>>>
>>> +    /* First pass, look for PCI devices.  If we find a suitable
>>> +     * PCI device that takes priority.
>>> +     */
>>>      for (i = 0; i < xf86_num_platform_devices; i++) {
>>>          char *busid = xf86_get_platform_attrib(i, ODEV_ATTRIB_BUSID);
>>>
>>> @@ -255,6 +296,24 @@ xf86platformProbe(void)
>>>              platform_find_pci_info(&xf86_platform_devices[i], busid);
>>>          }
>>>      }
>>> +
>>> +    /* if we found something, we are done: */
>>> +    if (primaryBus.type != BUS_NONE)
>>> +        return 0;
>>> +
>>> +    /* Second pass, look for real platform devices (ie. in the linux-
>>> +     * kernel sense of platform device.. something that is not pci)
>>> +     */
>>> +    for (i = 0; i < xf86_num_platform_devices; i++) {
>>> +        char *busid = xf86_get_platform_attrib(i, ODEV_ATTRIB_BUSID);
>>> +
>>> +        if (strncmp(busid, "platform:", 9) == 0) {
>>> +            primaryBus.type = BUS_PLATFORM;
>>> +            primaryBus.id.plat = &xf86_platform_devices[i];
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>>      return 0;
>>>  }
>>>
>>> @@ -401,10 +460,8 @@ xf86platformProbeDev(DriverPtr drvp)
>>>                  /* for non-seat0 servers assume first device is the master */
>>>                  if (ServerIsNotSeat0())
>>>                      break;
>>> -                if (xf86_platform_devices[j].pdev) {
>>> -                    if (xf86IsPrimaryPlatform(&xf86_platform_devices[j]))
>>> -                        break;
>>> -                }
>>> +                if (xf86IsPrimaryPlatform(&xf86_platform_devices[j]))
>>> +                    break;
>>>              }
>>>          }
>>
>> This seems like an unrelated cleanup which should be in its own patch,
>> with a commit message explaining why it is safe to remove the
>> "if (xf86_platform_devices[j].pdev)" check.
> 
> Well, not completely unrelated.. it is necessary to make non-pci
> devices work.  But I can split it into two patches if you prefer, but
> both patches would be required to fix the issue.

Ah right, pdev stands for pci_dev (we really should rename it to make this
clear one of these days), so removing the check does make sense.

Have you audited the xf86IsPrimaryPlatform function to check that it
does not try to deref pdev unconditionally somewhere ?

> That said, does anyone have suggestions on workarounds from DDX side,
> or do we just bump ABI version?  The problem is, if I add
> platformProbe() support to DDX, and xserver does not have this fix,
> then xserver won't find a screen, even if the user has a .conf file.
> Which isn't very nice.  If we have an ABI bump, or if there is some
> other way from DDX that I can tell xserver does not have this fix,
> then I can make my platformProbe() not claim the device, and then
> things will at least continue to work the old way.
>
> (That said, I'd prefer if there was a way to tell at runtime, without
> ABI bump, so we could get this fix into stable branch too rather than
> having to live with broken xserver until next major release.)

You could do something with the driverFunc callback, add a new
SERVER_SUPPORTS_PLATFORM_DEVS (*) xorgDriverFuncOp and have the patched version
of the server call driverFunc with this new Op. This seems the most
natural fit, although normally driverFunc is used for the server to query
driver capabilities, it can be used the otherway around too I guess.

Another solution would be to use a bit in the struct xf86_platform_device
flags member to indicate the server supports platform devices being probed
this way, but it is a bit weird to make this a per device flag as it is
something global really.

Regards,

Hans


*) Could use a better name