[PATCH] platform: support non-pci platform devices

[Rob Clark]

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.

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

BR,
-R

> With this hunk removed, this patch is:
>
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
>
> Regards,
>
> Hans