Bug 100276

Summary: xf86-video-amdgpu-1.3.0 segfaults with a rotated screen (regression caused by commit ad53635af1)
Product: xorg Reporter: timon <timon37>
Component: Driver/AMDgpuAssignee: xf86-video-ati maintainers <xorg-driver-ati>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: major    
Priority: high    
Version: unspecified   
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
xorg.log
none
xorg.conf that resulted in the crash
none
Bail from drmmode_set_mode_major if it's called too early none

Description timon 2017-03-19 11:23:09 UTC
Created attachment 130307 [details]
xorg.log

There's a regression between xf86-video-amdgpu-1.2.0 and 1.3.0, that causes a segfault (xorg.log attached).
If I take 1.3.0 and revert commit ad53635af1, everything's fine.

It only happens when a monitor is rotated (I have it configured inside xorg.conf).
Comment 1 timon 2017-03-19 11:27:19 UTC
Forgot to mention:
The gpu's AMD Radeon R7 260X (Bonaire XTX) PCI ID: 1002:6658
Comment 2 Michel Dänzer 2017-03-20 09:41:26 UTC
Please attach the /etc/X11/xorg.conf file.
Comment 3 timon 2017-03-20 18:16:54 UTC
Created attachment 130327 [details]
xorg.conf that resulted in the crash

Huh, it also seems to work after deleting the: Option "DRI" "3"
Comment 4 Michel Dänzer 2017-03-22 09:51:12 UTC
Created attachment 130373 [details] [review]
Bail from drmmode_set_mode_major if it's called too early

(In reply to timon37 from comment #3)
> Huh, it also seems to work after deleting the: Option "DRI" "3"

That must be luck, DRI3 should be enabled by default anyway.

I can't seem to reproduce the problem. It looks like maybe crtc->enabled isn't always initialized to FALSE in drmmode_xf86crtc_resize at this point (valgrind doesn't complain about any access to uninitialized memory though).

Does this patch avoid the problem for you?
Comment 5 timon 2017-03-22 22:00:10 UTC
Patch didn't help. Actually it initially didn't compile for me (AMDGPUWindowExposures_oneshot was undefined) but I added a prototype for it.

Indeed DRI3 didn't seem to affect it on further tries (not sure what happened originally).

But when I comment out the "Virtual 4096 4096" line it consistently works, and if then uncomment it it crashes.

To clarify the crash is because pScreen->root is 0. It doesn't crash without rotation because that's not queried in the else case (I verified that it was also 0 without screen rotation).

And I'm guessing it's not 0 when the Virtual setting isn't used.
Alternatively the code never reaches that point, I can try to verify that or anything else you want. Though hopefully you'll be able to repro it now?
Comment 6 Michel Dänzer 2017-03-23 09:58:14 UTC
Comment on attachment 130373 [details] [review]
Bail from drmmode_set_mode_major if it's called too early

(In reply to timon37 from comment #5)
> Patch didn't help. Actually it initially didn't compile for me
> (AMDGPUWindowExposures_oneshot was undefined)

Oops, sorry, I thought I'd at least compile-tested the patch, but I obviously hadn't.

> but I added a prototype for it.

Did you also remove the static keyword from the function declaration src/amdgpu_kms.c? If not, that might explain why the patch didn't work.


> But when I comment out the "Virtual 4096 4096" line it consistently works,
> and if then uncomment it it crashes.

Thanks, this allowed me to reproduce the problem. https://patchwork.freedesktop.org/patch/145922/ fixes it for me.

BTW, does the Virtual line serve any purpose? Xorg seems to resize the screen to fit the rotated output anyway for me.
Comment 7 timon 2017-03-23 23:25:54 UTC
(In reply to Michel Dänzer from comment #6)
> Did you also remove the static keyword from the function declaration
> src/amdgpu_kms.c? If not, that might explain why the patch didn't work.

Hmm, I thought I did. But I guess it probably would only fail on load saying it can't find the symbol, and I probably stupidly didn't properly verify what the output was...

Really sorry about the sloppiness on my side.

> BTW, does the Virtual line serve any purpose? Xorg seems to resize the
> screen to fit the rotated output anyway for me.

Probably was need at some point a long time ago, and was simply leftover.
Right now doesn't seem to be necessary.
Comment 8 Michel Dänzer 2017-04-17 09:41:27 UTC
Thanks for the report, fixed in Git master:

https://cgit.freedesktop.org/xorg/driver/xf86-video-amdgpu/commit/?id=981bac185cfd74ae50dffc28f57cf34623a9595f

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.