On Mon, Jun 16, 2014 at 7:50 AM, Hans de Goede <[email protected]> wrote: > Hi, > > On 06/16/2014 01:32 PM, Rob Clark wrote: >> On Mon, Jun 16, 2014 at 2:49 AM, Hans de Goede <[email protected]> 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 <[email protected]> >>> >>> 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 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
