Bug 1156

Summary: radeon driver fails to build on PPC due to redefinition of RADEONMMIO
Product: xorg Reporter: Adam Jackson <ajax>
Component: Driver/RadeonAssignee: Michel Dänzer <michel>
Status: RESOLVED FIXED QA Contact:
Severity: critical    
Priority: high CC: eric, michel
Version: git   
Hardware: PowerPC   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 351    
Attachments:
Description Flags
Proposed fix none

Description Adam Jackson 2004-08-21 22:49:26 UTC
http://bugs.gentoo.org/show_bug.cgi?id=60295

patch at http://bugs.gentoo.org/attachment.cgi?id=37757 but i'm not convinced i
like it.

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...

the commit in question:
http://freedesktop.org/cgi-bin/viewcvs.cgi/xorg/xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_render.c?r1=1.3&r2=1.4
Comment 1 Michel Dänzer 2004-08-22 10:34:00 UTC
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?
Comment 2 Eric Anholt 2004-08-23 20:21:05 UTC
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
Comment 3 Michel Dänzer 2004-08-24 21:56:16 UTC
(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?
Comment 4 René Rebe 2004-08-25 17:09:52 UTC
> > 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 ;-) 
Comment 5 Eric Anholt 2004-08-26 17:53:51 UTC
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.
Comment 6 Michel Dänzer 2004-08-26 19:43:22 UTC
(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.
Comment 7 Eric Anholt 2004-08-26 20:37:42 UTC
Committed, thanks!

We decided that my synchronization concerns were pointless, because the register
isn't FIFOed.
Comment 8 Michel Dänzer 2004-08-29 21:13:26 UTC
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.

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.