[PATCH] platform: support non-pci platform devices
Rob Clark
robdclark at gmail.com
Mon Jun 16 08:09:25 PDT 2014
On Mon, Jun 16, 2014 at 7:50 AM, Hans de Goede <hdegoede at redhat.com> wrote:
> 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.
fyi, I just sent a pair of RFC patches, one for xserver and one for
xf86-video-modesetting to implement this workaround.
Although as soon as I sent, I realized I sent the patches against f20
versions of xserver/-modesetting rather than master. But I guess that
is enough for people to comment on the RFC.
BR,
-R
> 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
More information about the xorg-devel
mailing list