| Message ID | 1350468407-27681-1-git-send-email-thierry.reding@avionic-design.de |
|---|---|
| State | Accepted |
| Commit | 76d9c62eb2be2010a19bf36285012d086cdd180b |
| Headers | show |
diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c index 0525e39..599d84a 100644 --- a/hw/xfree86/common/xf86platformBus.c +++ b/hw/xfree86/common/xf86platformBus.c @@ -377,6 +377,14 @@ xf86platformProbeDev(DriverPtr drvp) continue; } + /* + * If all of the above fails, which can happen if X was started without + * configuration or if BusID wasn't set for non-PCI devices, use the first + * device by default. + */ + if (!foundScreen && xf86_num_platform_devices > 0 && numDevs > 0) + foundScreen = probeSingleDevice(&xf86_platform_devices[0], drvp, devList[0], 0); + /* if autoaddgpu devices is enabled then go find a few more and add them as GPU screens */ if (xf86Info.autoAddGPU && numDevs) { for (j = 0; j < xf86_num_platform_devices; j++) {
On Thu, Nov 08, 2012 at 09:09:01AM +0100, Thierry Reding wrote: > On Wed, Oct 17, 2012 at 12:06:47PM +0200, Thierry Reding wrote: > > For non-PCI video devices, such as those found on many ARM embedded > > systems, the X server currently requires the BusID option to specify the > > full path to the DRM device's sysfs node in order to properly match it > > against the probed platform devices. > > > > In order to allow X to start up properly if either the BusID option was > > omitted or no configuration is present at all, the first video device is > > used by default. > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > --- > > Changes in v2: > > - Add additional checks for safety (I don't think numDevs will ever be 0 > > since a default will be generated if no configuration is present, but > > it doesn't hurt to check anyway). Without these checks there is a > > possibility of the X server crashing if no platform devices have been > > found. > > > > hw/xfree86/common/xf86platformBus.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c > > index 0525e39..599d84a 100644 > > --- a/hw/xfree86/common/xf86platformBus.c > > +++ b/hw/xfree86/common/xf86platformBus.c > > @@ -377,6 +377,14 @@ xf86platformProbeDev(DriverPtr drvp) > > continue; > > } > > > > + /* > > + * If all of the above fails, which can happen if X was started without > > + * configuration or if BusID wasn't set for non-PCI devices, use the first > > + * device by default. > > + */ > > + if (!foundScreen && xf86_num_platform_devices > 0 && numDevs > 0) > > + foundScreen = probeSingleDevice(&xf86_platform_devices[0], drvp, devList[0], 0); > > + > > /* if autoaddgpu devices is enabled then go find a few more and add them as GPU screens */ > > if (xf86Info.autoAddGPU && numDevs) { > > for (j = 0; j < xf86_num_platform_devices; j++) { > > Can anyone review this patch? I think it is also a candidate for 1.13, > since it fixes a regression from the 1.12 series. Can somebody merge this? The first version of this patch[0] was reviewed by Adam Jackson and the fix is required to make xf86-video-modesetting work on 1.13 and later X servers. Thierry [0]: http://lists.x.org/archives/xorg-devel/2012-October/033974.html
On Sat, Nov 24, 2012 at 6:28 AM, Thierry Reding <thierry.reding@avionic-design.de> wrote: > On Thu, Nov 08, 2012 at 09:09:01AM +0100, Thierry Reding wrote: >> On Wed, Oct 17, 2012 at 12:06:47PM +0200, Thierry Reding wrote: >> > For non-PCI video devices, such as those found on many ARM embedded >> > systems, the X server currently requires the BusID option to specify the >> > full path to the DRM device's sysfs node in order to properly match it >> > against the probed platform devices. >> > >> > In order to allow X to start up properly if either the BusID option was >> > omitted or no configuration is present at all, the first video device is >> > used by default. >> > >> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> >> > --- >> > Changes in v2: >> > - Add additional checks for safety (I don't think numDevs will ever be 0 >> > since a default will be generated if no configuration is present, but >> > it doesn't hurt to check anyway). Without these checks there is a >> > possibility of the X server crashing if no platform devices have been >> > found. >> > >> > hw/xfree86/common/xf86platformBus.c | 8 ++++++++ >> > 1 file changed, 8 insertions(+) >> > >> > diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c >> > index 0525e39..599d84a 100644 >> > --- a/hw/xfree86/common/xf86platformBus.c >> > +++ b/hw/xfree86/common/xf86platformBus.c >> > @@ -377,6 +377,14 @@ xf86platformProbeDev(DriverPtr drvp) >> > continue; >> > } >> > >> > + /* >> > + * If all of the above fails, which can happen if X was started without >> > + * configuration or if BusID wasn't set for non-PCI devices, use the first >> > + * device by default. >> > + */ >> > + if (!foundScreen && xf86_num_platform_devices > 0 && numDevs > 0) >> > + foundScreen = probeSingleDevice(&xf86_platform_devices[0], drvp, devList[0], 0); >> > + >> > /* if autoaddgpu devices is enabled then go find a few more and add them as GPU screens */ >> > if (xf86Info.autoAddGPU && numDevs) { >> > for (j = 0; j < xf86_num_platform_devices; j++) { >> >> Can anyone review this patch? I think it is also a candidate for 1.13, >> since it fixes a regression from the 1.12 series. > > Can somebody merge this? The first version of this patch[0] was reviewed > by Adam Jackson and the fix is required to make xf86-video-modesetting > work on 1.13 and later X servers. You need to send it to Keith with the reviewed by tag in the patch. Dave.
On Sat, Nov 24, 2012 at 10:55:13AM +1000, Dave Airlie wrote: > On Sat, Nov 24, 2012 at 6:28 AM, Thierry Reding > <thierry.reding@avionic-design.de> wrote: > > On Thu, Nov 08, 2012 at 09:09:01AM +0100, Thierry Reding wrote: > >> On Wed, Oct 17, 2012 at 12:06:47PM +0200, Thierry Reding wrote: > >> > For non-PCI video devices, such as those found on many ARM embedded > >> > systems, the X server currently requires the BusID option to specify the > >> > full path to the DRM device's sysfs node in order to properly match it > >> > against the probed platform devices. > >> > > >> > In order to allow X to start up properly if either the BusID option was > >> > omitted or no configuration is present at all, the first video device is > >> > used by default. > >> > > >> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > >> > --- > >> > Changes in v2: > >> > - Add additional checks for safety (I don't think numDevs will ever be 0 > >> > since a default will be generated if no configuration is present, but > >> > it doesn't hurt to check anyway). Without these checks there is a > >> > possibility of the X server crashing if no platform devices have been > >> > found. > >> > > >> > hw/xfree86/common/xf86platformBus.c | 8 ++++++++ > >> > 1 file changed, 8 insertions(+) > >> > > >> > diff --git a/hw/xfree86/common/xf86platformBus.c b/hw/xfree86/common/xf86platformBus.c > >> > index 0525e39..599d84a 100644 > >> > --- a/hw/xfree86/common/xf86platformBus.c > >> > +++ b/hw/xfree86/common/xf86platformBus.c > >> > @@ -377,6 +377,14 @@ xf86platformProbeDev(DriverPtr drvp) > >> > continue; > >> > } > >> > > >> > + /* > >> > + * If all of the above fails, which can happen if X was started without > >> > + * configuration or if BusID wasn't set for non-PCI devices, use the first > >> > + * device by default. > >> > + */ > >> > + if (!foundScreen && xf86_num_platform_devices > 0 && numDevs > 0) > >> > + foundScreen = probeSingleDevice(&xf86_platform_devices[0], drvp, devList[0], 0); > >> > + > >> > /* if autoaddgpu devices is enabled then go find a few more and add them as GPU screens */ > >> > if (xf86Info.autoAddGPU && numDevs) { > >> > for (j = 0; j < xf86_num_platform_devices; j++) { > >> > >> Can anyone review this patch? I think it is also a candidate for 1.13, > >> since it fixes a regression from the 1.12 series. > > > > Can somebody merge this? The first version of this patch[0] was reviewed > > by Adam Jackson and the fix is required to make xf86-video-modesetting > > work on 1.13 and later X servers. > > You need to send it to Keith with the reviewed by tag in the patch. Okay, I can do that. Adam, is you're Reviewed-by still valid for this version? All I added were some additional checks for safety, but I don't feel comfortable adding your Reviewed-by without permission. Thanks, Thierry
Thierry Reding <thierry.reding@avionic-design.de> writes: > For non-PCI video devices, such as those found on many ARM embedded > systems, the X server currently requires the BusID option to specify the > full path to the DRM device's sysfs node in order to properly match it > against the probed platform devices. Merged. d50a945..76d9c62 master -> master
So this got merged into Fedora, and nobody reads bug reports after merging stuff to know when it breaks lots of things. > } > > + /* > + * If all of the above fails, which can happen if X was started without > + * configuration or if BusID wasn't set for non-PCI devices, use the first > + * device by default. > + */ > + if (!foundScreen && xf86_num_platform_devices > 0 && numDevs > 0) > + foundScreen = probeSingleDevice(&xf86_platform_devices[0], drvp, devList[0], 0); > + So this patch doesn't get foundScreen right at all, I've got a fix I'm just testing now, though it just shows that the code originally was broken, but we didn't notice! Dave.
On Wed, Dec 12, 2012 at 11:49 AM, Dave Airlie <airlied@gmail.com> wrote: > So this got merged into Fedora, and nobody reads bug reports after > merging stuff to know when it breaks lots of things. >> } >> >> + /* >> + * If all of the above fails, which can happen if X was started without >> + * configuration or if BusID wasn't set for non-PCI devices, use the first >> + * device by default. >> + */ >> + if (!foundScreen && xf86_num_platform_devices > 0 && numDevs > 0) >> + foundScreen = probeSingleDevice(&xf86_platform_devices[0], drvp, devList[0], 0); >> + > > So this patch doesn't get foundScreen right at all, I've got a fix I'm > just testing now, though it just shows that the code originally was > broken, but we didn't notice! Okay my initial fix was even more bogus than this patch was, but anyways, lets get it out of master, and Thierry you can go try again! I'm getting other distros merged this as well, so hopefully they know to remove it. But here on a dual-gpu machine, with that patch, I end up running in two X desktop mode, instead of landing in master/slave GPU mode, this means XINERAMA doesn't get enabled, and lots of stuff falls apart. Dave.
For non-PCI video devices, such as those found on many ARM embedded systems, the X server currently requires the BusID option to specify the full path to the DRM device's sysfs node in order to properly match it against the probed platform devices. In order to allow X to start up properly if either the BusID option was omitted or no configuration is present at all, the first video device is used by default. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- Changes in v2: - Add additional checks for safety (I don't think numDevs will ever be 0 since a default will be generated if no configuration is present, but it doesn't hurt to check anyway). Without these checks there is a possibility of the X server crashing if no platform devices have been found. hw/xfree86/common/xf86platformBus.c | 8 ++++++++ 1 file changed, 8 insertions(+)