Bug 97124 - XF86HandleColormaps,AddScreen,InitOutput segfault in intel_drv on startx
Summary: XF86HandleColormaps,AddScreen,InitOutput segfault in intel_drv on startx
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/DDX/Xorg (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: highest blocker
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
: 97191 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-07-29 03:28 UTC by Tod Jackson
Modified: 2016-08-16 14:53 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Xorg.0.log (8.70 KB, text/plain)
2016-07-29 03:28 UTC, Tod Jackson
no flags Details
systemd-coredump (692 bytes, text/plain)
2016-07-29 03:29 UTC, Tod Jackson
no flags Details
dmesg (64.01 KB, text/plain)
2016-07-29 04:33 UTC, Tod Jackson
no flags Details
coredumpctl gdb bt full bt (10.05 KB, text/plain)
2016-07-29 04:34 UTC, Tod Jackson
no flags Details

Description Tod Jackson 2016-07-29 03:28:42 UTC
Created attachment 125391 [details]
Xorg.0.log

I can no longer start X after commit 0b2f30834b1a9f4a03542e25c5f54ae800df57e2 in xorg/xserver git. AccelMethod or level of DRI doesn't matter, it still segfaults.

I'm not sure how to capture this with gdb.

I attached the Xorg.0.log and the stack trace from systemd-coredump.
Comment 1 Tod Jackson 2016-07-29 03:29:12 UTC
Created attachment 125392 [details]
systemd-coredump
Comment 2 Tod Jackson 2016-07-29 04:33:58 UTC
Created attachment 125393 [details]
dmesg
Comment 3 Tod Jackson 2016-07-29 04:34:31 UTC
Created attachment 125394 [details]
coredumpctl gdb bt full bt
Comment 4 Chris Wilson 2016-07-29 07:36:25 UTC
The call to xf86HandleColormaps() happens in ScreenInit (for all drivers).

commit b4e46c0444bb09f4af59d9d13acc939a0fbbc6d6
Author: Michel Dänzer <michel.daenzer@amd.com>
Date:   Sat Nov 28 16:50:47 2015 +0900

    xfree86: Hook up colormaps and RandR 1.2 gamma code v6

installs a scrn->LoadPalette callback inside xf86HandleColormaps() which is immediately used by CMapInstallColormap(). But xf86RandrR12LoadPalette expects pScrn->pScreen to already be valid but that is only set after ScreenInit completes. Given the assertions, it would be fine to preset pScrn->pScreen ala

diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
index 7a267f8..dd2e520 100644
--- a/hw/xfree86/common/xf86Init.c
+++ b/hw/xfree86/common/xf86Init.c
@@ -790,13 +790,12 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv)
         pScrn->LoadPalette = NULL;
         pScrn->SetOverscan = NULL;
         pScrn->DriverFunc = NULL;
-        pScrn->pScreen = NULL;
+       pScrn->pScreen = screenInfo.gpuscreens[i];
         scr_index = AddGPUScreen(pScrn->ScreenInit, argc, argv);
         xf86VGAarbiterUnlock(pScrn);
         if (scr_index == i) {
             dixSetPrivate(&screenInfo.gpuscreens[scr_index]->devPrivates,
                           xf86ScreenKey, xf86GPUScreens[i]);
-            pScrn->pScreen = screenInfo.gpuscreens[scr_index];
             /* The driver should set this, but make sure it is set anyway */
             pScrn->vtSema = TRUE;
         } else {
@@ -818,7 +817,7 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv)
         xf86Screens[i]->LoadPalette = NULL;
         xf86Screens[i]->SetOverscan = NULL;
         xf86Screens[i]->DriverFunc = NULL;
-        xf86Screens[i]->pScreen = NULL;
+       xf86Screens[i]->pScreen = screenInfo.screens[i];
         scr_index = AddScreen(xf86Screens[i]->ScreenInit, argc, argv);
         xf86VGAarbiterUnlock(xf86Screens[i]);
         if (scr_index == i) {
@@ -828,7 +827,6 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv)
              */
             dixSetPrivate(&screenInfo.screens[scr_index]->devPrivates,
                           xf86ScreenKey, xf86Screens[i]);
-            xf86Screens[i]->pScreen = screenInfo.screens[scr_index];
             /* The driver should set this, but make sure it is set anyway */
             xf86Screens[i]->vtSema = TRUE;
         }
Comment 5 Chris Wilson 2016-07-29 07:49:04 UTC
(In reply to Chris Wilson from comment #4)
> diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
> index 7a267f8..dd2e520 100644
> --- a/hw/xfree86/common/xf86Init.c
> +++ b/hw/xfree86/common/xf86Init.c
> @@ -790,13 +790,12 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char
> **argv)
>          pScrn->LoadPalette = NULL;
>          pScrn->SetOverscan = NULL;
>          pScrn->DriverFunc = NULL;
> -        pScrn->pScreen = NULL;
> +       pScrn->pScreen = screenInfo.gpuscreens[i];
>          scr_index = AddGPUScreen(pScrn->ScreenInit, argc, argv);

Which doesn't work because the pScreen isn't even allocated at that point.

The simplest solution would be just to let drivers go back to being able to inherit the gamma ramp.
Comment 6 Michel Dänzer 2016-07-29 08:10:49 UTC
https://patchwork.freedesktop.org/patch/101625/ fixes this, sorry about that.
Comment 7 Michel Dänzer 2016-08-15 00:32:42 UTC
commit 69b782aa75bc06f11b8f9b532d5213f252c4c6c4
Author: Keith Packard <keithp@keithp.com>
Date:   Fri Jul 29 17:45:45 2016 -0700

    xfree86: Set pScrn->pScreen before driver ScreenInit is called
Comment 8 Andreas Reis 2016-08-16 14:53:49 UTC
*** Bug 97191 has been marked as a duplicate of this bug. ***


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.