Bug 66663

Summary: broken on big-endian systems
Product: xorg Reporter: Mark Kettenis <kettenis>
Component: Driver/RadeonAssignee: xf86-video-ati maintainers <xorg-driver-ati>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium    
Version: git   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
patch
none
patch that fixes the accelerated case none

Description Mark Kettenis 2013-07-07 12:12:42 UTC 
Created attachment 82134 [details]
patch

The current xf86-video-ati is broken on big-endian systems.  The problem is that
RADEON_TILING_SURFACE isn't set on the front buffer since commits ef9bfb262db7004bef3704e5d914687e50d3fca4 and e5bd99faa3b6629a55168386d5dfa936ee4e97ae.

As a result the RADEON_TILING_SWAP_32BIT and RADEON_TILING_SWAP_16BIT flags don't have any effect, and any direct access to the front buffer will not do the necessary byte swapping.  The effect is blatantly obvious if you disable acceleration (option NoAccel "on"), but also happens with (EXA) acceleration enabled.  With acceleration disabled, the colors are all wrong.  With acceleration enabled, the background of my xterms is yellow when it should be white.

The attached patch fixes this, but might reintroduce problems with page flipping as mentioned in https://bugs.freedesktop.org/show_bug.cgi?id=33738 (the bug report that seems to have triggered the removal of the RADEON_TILING_SURFACE flag for the front buffer).  But only on big-endian platforms.
Comment 1 Michel Dänzer 2013-07-08 12:01:50 UTC 
Have you tried completely disallowing direct CPU access to the scanout buffer instead? Seems like that should fix this problem without reintroducing the other one.
Comment 2 Mark Kettenis 2013-07-08 18:27:22 UTC 
Created attachment 82197 [details] [review]
patch that fixes the accelerated case
Comment 3 Mark Kettenis 2013-07-08 18:41:02 UTC 
Looks like I jumped to conclusions here.  For the (EXA) accelerated case, the
problem is that UTS/DFS get bypassed for the "screen pixmap".  That was the right
thing to do when the front buffer had a surface reg allocated to it that made sure
the required byte swapping was done in hardware.  But now that this isn't the case anymore, all pixmaps over 8bpp need byte swapping.  See the attached patch.

Of course this will leave the unaccelerated case broken.  In that case the RADEON_TILING_SURFACE flag really has to be set.  But there should be no interference with page flipping in that case.
Comment 4 Michel Dänzer 2013-07-10 09:06:45 UTC 
Patch pushed, thanks.

(In reply to comment #3)
> Of course this will leave the unaccelerated case broken.  In that case the
> RADEON_TILING_SURFACE flag really has to be set.  But there should be no
> interference with page flipping in that case.

Right. Or, the shadow update hook could swap the bytes. Do you want to look into fixing the unaccelerated case as well?
Comment 5 Alex Deucher 2013-07-10 13:03:21 UTC 
We only need swapping for r1xx-r5xx asics.  For r6xx+, the display hardware already handles the swapping.
Comment 6 Mark Kettenis 2013-07-10 15:56:30 UTC 
Michel, thanks.  Yes I will look at fixing the shadowfb case as well.

Alex, when you say that for r6xx the display hardware already handles the swapping, are you referring to the unaccelerated case?  Or are you saying my change to RADEONPrepareAccess_CS() is wrong?
Comment 7 Alex Deucher 2013-07-10 16:04:21 UTC 
(In reply to comment #6)
> Alex, when you say that for r6xx the display hardware already handles the
> swapping, are you referring to the unaccelerated case?  Or are you saying my
> change to RADEONPrepareAccess_CS() is wrong?

For unaccelerated.  RADEONPrepareAccess_CS() only applies to r1xx-r5xx GPUs.  R6xx and newer asics have their own PrepareAccess functions.
Comment 8 Mark Kettenis 2013-09-10 19:35:40 UTC 
shadowfb has been fixed now as well

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.