Created attachment 702 [details] [review]
Proposed fix

(In reply to comment #0)
> 
> michel, you were the one who committed this piece.  why do you need to
declare
> RADEONMMIO explicitly even when ACCEL_CP ?  i don't see it used anywhere in
> those two functions...

It's used by OUTREG(). Does this patch help, which splits the framebuffer
aperture byte order handling off into helper functions?

Shouldn't these registers be set using BEGIN_ACCEL/OUT_ACCEL_REG/FINISH_ACCEL
instead?  I do like it being split into a separate function, to avoid duplication.
Also, I'm confused, shouldn't the swap cases per bytepp be:
1: 0
2: RADEON_NONSURF_AP0_SWP_16BPP
4: RADEON_NONSURF_AP0_SWP_32BPP

(In reply to comment #2)
> Shouldn't these registers be set using BEGIN_ACCEL/OUT_ACCEL_REG/FINISH_ACCEL
> instead?  

That doesn't work 100% correctly, which hints at WaitForIdle() still not being
100% correct, with DRI enabled. It'll have to be changed once the data gets
uploaded via the CP though.

> Also, I'm confused, shouldn't the swap cases per bytepp be:
> 1: 0
> 2: RADEON_NONSURF_AP0_SWP_16BPP
> 4: RADEON_NONSURF_AP0_SWP_32BPP

You mean the 4 case is broken in my patch? You're probably right, do you have a
test case for that?

> > Also, I'm confused, shouldn't the swap cases per bytepp be: 
> > 1: 0 
> > 2: RADEON_NONSURF_AP0_SWP_16BPP 
> > 4: RADEON_NONSURF_AP0_SWP_32BPP 
>  
> You mean the 4 case is broken in my patch? You're probably right, do you 
have a 
> test case for that? 
 
Just running the server, e.g. with Composite extension is enought to see 
byteswapped image data. 
 
I just verified that fixing the 4: case to just set SWP_32BPP does fix this 
here (ROCK LInux fork on PowerPC iBook2 G3). I would sign-off a commit ;-) 

Yeah, that case 4 looks wrong.  Rendercheck should show it, or another, more
visual test would be http://freedesktop.org/~anholt/test-render-3.tar.gz.

Ahh, the NONSURF affects CPU writes to framebuffer?  I would probably use the
texture registers' swapping ability instead, which you then wouldn't have to
worry about synchronization with, and save some register writes at the same
time.  If you'd like, I could prepare a patch in the next day I think (my
internet access is very scarce between now and next Thursday), or we could just
go with this one plus removing the 16-bit swap from the 4 case.

(In reply to comment #5)
> Yeah, that case 4 looks wrong.

Indeed, now I know why xcompmgr never worked right before. :\

> Ahh, the NONSURF affects CPU writes to framebuffer?  I would probably use the
> texture registers' swapping ability instead, which you then wouldn't have to
> worry about synchronization with, and save some register writes at the same
> time.  If you'd like, I could prepare a patch in the next day I think (my
> internet access is very scarce between now and next Thursday) [...]

I wouldn't bother:

* The synchronization isn't an issue because it needs to happen before writing
to the framebuffer anyway. Once we upload the data via the CP, the byte order
can be handled there.
* You'd still have to take the depth-dependent byte swapping of the framebuffer
into account, so the code might well end up being more complex.
* Last but not least, the texture byte swapping bits didn't seem to have any
effect when I played with them, though admittedly that was a long time ago, on
an M6 IIRC.

Committed, thanks!

We decided that my synchronization concerns were pointless, because the register
isn't FIFOed.

Thanks Eric, but you forgot to clear the RADEON_NONSURF_AP0_SWP_16BPP bit, which
will break at depth 16. I'll fix this shortly.