Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Michel Dänzer 
On Mon, 2003-02-03 at 18:09, Benjamin Herrenschmidt wrote:
 On Mon, 2003-02-03 at 17:05, Michel Dänzer wrote:
  On Mon, 2003-02-03 at 17:34, Alan Cox wrote:
   On Mon, 2003-02-03 at 15:02, Keith Whitwell wrote:
  
 -#define COMMIT_RING() do {   \
 - RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
 +#define COMMIT_RING() do {   \
 + /* read from PCI bus to ensure correct posting */   \
 + RADEON_READ( RADEON_CP_RB_WPTR );   \
 + RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
 + RADEON_READ( RADEON_CP_RB_WPTR );   \
  } while (0)

Ouch.  Put a conditional around that at least, so that not everybody suffers...
   
   PCI posting applies to all platforms. However I'm trying to understand what this
   is trying to do. The final read has an effect in that it ensures that the WPTR is
   written not left posted for an undefined time. What does the previous one 
achieve.
   Is there some kind of synchronization requirement against the GART/main memory ?
  
  That's my understanding, we need to make sure the chip reads from the
  ring what we wrote to it.
 
 Well... You are asking for trouble ;)
 
 The problem is that the behaviour will be pretty much HW implementation
 dependant. 
 
 In the AGP case, the ring is mapped uncacheable. So your card and the
 ring are typically on the same memory type from the CPU, that helps.
 Though I would still make sure the correct bus path is flushed by doing
 that first read from the ring and not from the card.
 
 In the PCI case, the ring is mapped cacheable in normal memory and you
 rely on the PCI cache coherency (snooping). That means that you have a
 new problem which is to synchronize writes to cacheable memory (the
 ring) with write to non cacheable MMIO space (the card). At least on
 PPC, I don't think anything but a full sync instruction will acheive
 that, so you'd rather add an mb(). And do the read from memory (actually
 cache), not the card.

After various tests, it looks like all of this is indeed necessary even
with AGP. As an example, the Cube used to crash after a couple x11perf
tests at 1x, now it's passed several complete x11perf runs at 4x with
fast writes.

And there's even more: newer compilers seem to optimize away some of the
reads with strict aliasing. I thought I'd steal some code from the
kernel to detect if the compiler supports -fno-strict-aliasing, but it
looks like it just uses that unconditionally. We probably want to do the
same for the DRM at least? AFAIR it's been supported since early 2.95.


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Michel Dänzer 
On Die, 2003-02-04 at 19:06, Michel Dänzer wrote:

 After various tests, it looks like all of this is indeed necessary even
 with AGP.

Also, I think at least the r128 driver could use the same treatment, but
we probably want to split COMMIT_RING() off ADVANCE_RING() as well for
that, and I'm not sure rushing that in for 4.3.0 is a good idea.
Opinions, also for the other drivers?


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Keith Whitwell 
Michel Dänzer wrote:

On Die, 2003-02-04 at 19:06, Michel Dänzer wrote:



After various tests, it looks like all of this is indeed necessary even
with AGP.



Also, I think at least the r128 driver could use the same treatment, but
we probably want to split COMMIT_RING() off ADVANCE_RING() as well for
that, and I'm not sure rushing that in for 4.3.0 is a good idea.
Opinions, also for the other drivers?


The r128 probably still has the mb() in place at least.

Keith



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Michel Dänzer 
On Die, 2003-02-04 at 19:36, Keith Whitwell wrote:
 Michel Dänzer wrote:
  On Die, 2003-02-04 at 19:06, Michel Dänzer wrote:
  
  
 After various tests, it looks like all of this is indeed necessary even
 with AGP.
  
  
  Also, I think at least the r128 driver could use the same treatment, but
  we probably want to split COMMIT_RING() off ADVANCE_RING() as well for
  that, and I'm not sure rushing that in for 4.3.0 is a good idea.
  Opinions, also for the other drivers?
 
 The r128 probably still has the mb() in place at least.

Yeah, maybe that avoids the worst problems, but it definitely wasn't
enough for Chris.


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Keith Whitwell 
Michel Dänzer wrote:

On Die, 2003-02-04 at 19:36, Keith Whitwell wrote:


Michel Dänzer wrote:


On Die, 2003-02-04 at 19:06, Michel Dänzer wrote:




After various tests, it looks like all of this is indeed necessary even
with AGP.



Also, I think at least the r128 driver could use the same treatment, but
we probably want to split COMMIT_RING() off ADVANCE_RING() as well for
that, and I'm not sure rushing that in for 4.3.0 is a good idea.
Opinions, also for the other drivers?


The r128 probably still has the mb() in place at least.



Yeah, maybe that avoids the worst problems, but it definitely wasn't
enough for Chris.


Well, I'm all for avoiding crashes.  This probably affects every card in some 
way or another.

Keith



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Leif Delgass 
On 4 Feb 2003, Michel Dänzer wrote:

 On Die, 2003-02-04 at 19:36, Keith Whitwell wrote:
  Michel Dänzer wrote:
   On Die, 2003-02-04 at 19:06, Michel Dänzer wrote:
   
   
  After various tests, it looks like all of this is indeed necessary even
  with AGP.
   
   
   Also, I think at least the r128 driver could use the same treatment, but
   we probably want to split COMMIT_RING() off ADVANCE_RING() as well for
   that, and I'm not sure rushing that in for 4.3.0 is a good idea.
   Opinions, also for the other drivers?
  
  The r128 probably still has the mb() in place at least.
 
 Yeah, maybe that avoids the worst problems, but it definitely wasn't
 enough for Chris.

At least with r128 PCI, I have seen lockups happen after a period of days.  
When that happens, I see a sort of greenish bar of corrupted framebuffer
at the top of the screen.  Usually killing the server with
Ctl-Alt-Backspace works and I can restart X without a problem.  I've also 
seen that with same behavior with AGP Radeon 7500 from time to time.

-- 
Leif Delgass 
http://www.retinalburn.net



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Michel Dänzer 
On Die, 2003-02-04 at 20:09, Leif Delgass wrote:
 On 4 Feb 2003, Michel Dänzer wrote:
 
  On Die, 2003-02-04 at 19:36, Keith Whitwell wrote:
   Michel Dänzer wrote:
On Die, 2003-02-04 at 19:06, Michel Dänzer wrote:


   After various tests, it looks like all of this is indeed necessary even
   with AGP.


Also, I think at least the r128 driver could use the same treatment, but
we probably want to split COMMIT_RING() off ADVANCE_RING() as well for
that, and I'm not sure rushing that in for 4.3.0 is a good idea.
Opinions, also for the other drivers?
   
   The r128 probably still has the mb() in place at least.
  
  Yeah, maybe that avoids the worst problems, but it definitely wasn't
  enough for Chris.
 
 At least with r128 PCI, I have seen lockups happen after a period of days.  
 When that happens, I see a sort of greenish bar of corrupted framebuffer
 at the top of the screen.  Usually killing the server with
 Ctl-Alt-Backspace works and I can restart X without a problem.  I've also 
 seen that with same behavior with AGP Radeon 7500 from time to time.

It will be very interesting to see if this change helps for those.


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Pawel Salek 

On 2003.02.04 19:06 Michel Dänzer wrote:


And there's even more: newer compilers seem to optimize away some of
the
reads with strict aliasing. I thought I'd steal some code from the
kernel to detect if the compiler supports -fno-strict-aliasing, but it
looks like it just uses that unconditionally. We probably want to do
the
same for the DRM at least? AFAIR it's been supported since early 2.95.


Isn't the volatile keyword the right way to disable such optimizations?

-pawel


---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-04 Thread Benjamin Herrenschmidt 
On Tue, 2003-02-04 at 21:25, Pawel Salek wrote:
 On 2003.02.04 19:06 Michel Dänzer wrote:
  
  And there's even more: newer compilers seem to optimize away some of
  the
  reads with strict aliasing. I thought I'd steal some code from the
  kernel to detect if the compiler supports -fno-strict-aliasing, but it
  looks like it just uses that unconditionally. We probably want to do
  the
  same for the DRM at least? AFAIR it's been supported since early 2.95.
 
 Isn't the volatile keyword the right way to disable such optimizations?

The problem with strict aliasing is slightly different, though I don't
pretend to understand it fully, so far, what I've figured out is that
it's the definitive enemy of anybody doing typecasts, like we do
regulary in drivers.

Ben.



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Keith Whitwell 


 
-#define COMMIT_RING() do {	\
-	RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail );		\
+#define COMMIT_RING() do {	\
+	/* read from PCI bus to ensure correct posting */   \
+	RADEON_READ( RADEON_CP_RB_WPTR );   \
+	RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail );		\
+	RADEON_READ( RADEON_CP_RB_WPTR );   \
 } while (0)

Ouch.  Put a conditional around that at least, so that not everybody suffers...

Keith



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Michel Dänzer 
On Mon, 2003-02-03 at 16:02, Keith Whitwell wrote:
   
  -#define COMMIT_RING() do { \
  -   RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
  +#define COMMIT_RING() do { \
  +   /* read from PCI bus to ensure correct posting */   \
  +   RADEON_READ( RADEON_CP_RB_WPTR );   \
  +   RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
  +   RADEON_READ( RADEON_CP_RB_WPTR );   \
   } while (0)
 
 Ouch.  Put a conditional around that at least, so that not everybody suffers...

If it makes a difference on AGP here, it's for the good. It may well be
necessary as well.


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Alan Cox 
On Mon, 2003-02-03 at 15:02, Keith Whitwell wrote:
   
  -#define COMMIT_RING() do { \
  -   RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
  +#define COMMIT_RING() do { \
  +   /* read from PCI bus to ensure correct posting */   \
  +   RADEON_READ( RADEON_CP_RB_WPTR );   \
  +   RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
  +   RADEON_READ( RADEON_CP_RB_WPTR );   \
   } while (0)
 
 Ouch.  Put a conditional around that at least, so that not everybody suffers...

PCI posting applies to all platforms. However I'm trying to understand what this
is trying to do. The final read has an effect in that it ensures that the WPTR is
written not left posted for an undefined time. What does the previous one achieve.
Is there some kind of synchronization requirement against the GART/main memory ?




---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Michel Dänzer 
On Mon, 2003-02-03 at 17:34, Alan Cox wrote:
 On Mon, 2003-02-03 at 15:02, Keith Whitwell wrote:

   -#define COMMIT_RING() do {   \
   - RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
   +#define COMMIT_RING() do {   \
   + /* read from PCI bus to ensure correct posting */   \
   + RADEON_READ( RADEON_CP_RB_WPTR );   \
   + RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
   + RADEON_READ( RADEON_CP_RB_WPTR );   \
} while (0)
  
  Ouch.  Put a conditional around that at least, so that not everybody suffers...
 
 PCI posting applies to all platforms. However I'm trying to understand what this
 is trying to do. The final read has an effect in that it ensures that the WPTR is
 written not left posted for an undefined time. What does the previous one achieve.
 Is there some kind of synchronization requirement against the GART/main memory ?

That's my understanding, we need to make sure the chip reads from the
ring what we wrote to it.


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Michel Dänzer 
On Mon, 2003-02-03 at 15:53, Chris Ison wrote:
 please find attached a complete patch that allows pci Radeon cards to
 work with DRI. It was created against the DRI CVS xc branch/trunk.

Thanks, I'll commit this unless someone else comes up with a better
solution.


 Note: also contains the pcigart patch many often refer too.

This is already on the XFree86 CVS trunk; if people feel this absolutely
belongs on the DRI trunk before the post 4.3.0 merge, I'll move it over
from there to try and avoid future conflicts.


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Keith Whitwell 
Michel Dänzer wrote:

On Mon, 2003-02-03 at 15:53, Chris Ison wrote:


please find attached a complete patch that allows pci Radeon cards to
work with DRI. It was created against the DRI CVS xc branch/trunk.



Thanks, I'll commit this unless someone else comes up with a better
solution.


Well, I'd like to at least understand why *two* reads are necessary.

Keith



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Michel Dänzer 
On Mon, 2003-02-03 at 17:47, Keith Whitwell wrote:
 Michel Dänzer wrote:
  On Mon, 2003-02-03 at 15:53, Chris Ison wrote:
  
 please find attached a complete patch that allows pci Radeon cards to
 work with DRI. It was created against the DRI CVS xc branch/trunk.
  
  
  Thanks, I'll commit this unless someone else comes up with a better
  solution.
 
 Well, I'd like to at least understand why *two* reads are necessary.

Because it doesn't work with only one. ;)

Seriously though, here's an excerpt from IRC (with a few useless
comments of mine stripped):

mharris mharris #define COMMIT_RING() do { 
 \
mharris mharris RADEON_WRITE( RADEON_CP_RB_WPTR,
dev_priv-ring.tail ); \
mharris mharris } while (0)
mharris arjan yup that has a pci posting bug :)
mharris arjan well
mharris arjan technically doing a read just before that IS the right
fix
mharris arjan eg it requires all previous writes to be flushed
mharris arjan also just after this write you want to ALSO do a read
mharris arjan technically it's cleaner to have a special
PCI_FLUSH_RING() that's called where needed
mharris arjan but it'll be all the places that then immediatly call
COMMIT_RING
mharris arjan so including it into COMMIT_RING isn't all THAT ugly
mharris mharris So, AGP and PCI should be special cased?
mharris arjan I'd assume AGP has the same posting rules for posting?
mharris mharris The problem doesn't seem to occur for AGP hardware
mharris arjan maybe CURRENT agp bridges don't do posting
MrCooper mharris: not sure about that
MrCooper mharris: there are random lockups on AGP after a couple hours
mharris arjan the read costs you a bit
mharris arjan but it's needed for correctness
mharris arjan leaving it out is invalid code
mharris mharris Coincidence?  Or possibly rarer bug in AGP case?
mharris arjan I'd have to read the agp spec for that
mharris arjan but I'd not be surprised if AGP has the same posting
rules


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Benjamin Herrenschmidt 
On Mon, 2003-02-03 at 17:05, Michel Dänzer wrote:
 On Mon, 2003-02-03 at 17:34, Alan Cox wrote:
  On Mon, 2003-02-03 at 15:02, Keith Whitwell wrote:
 
-#define COMMIT_RING() do { \
-   RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
+#define COMMIT_RING() do { \
+   /* read from PCI bus to ensure correct posting */   \
+   RADEON_READ( RADEON_CP_RB_WPTR );   \
+   RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
+   RADEON_READ( RADEON_CP_RB_WPTR );   \
 } while (0)
   
   Ouch.  Put a conditional around that at least, so that not everybody suffers...
  
  PCI posting applies to all platforms. However I'm trying to understand what this
  is trying to do. The final read has an effect in that it ensures that the WPTR is
  written not left posted for an undefined time. What does the previous one achieve.
  Is there some kind of synchronization requirement against the GART/main memory ?
 
 That's my understanding, we need to make sure the chip reads from the
 ring what we wrote to it.

Well... You are asking for trouble ;)

The problem is that the behaviour will be pretty much HW implementation
dependant. 

In the AGP case, the ring is mapped uncacheable. So your card and the
ring are typically on the same memory type from the CPU, that helps.
Though I would still make sure the correct bus path is flushed by doing
that first read from the ring and not from the card.

In the PCI case, the ring is mapped cacheable in normal memory and you
rely on the PCI cache coherency (snooping). That means that you have a
new problem which is to synchronize writes to cacheable memory (the
ring) with write to non cacheable MMIO space (the card). At least on
PPC, I don't think anything but a full sync instruction will acheive
that, so you'd rather add an mb(). And do the read from memory (actually
cache), not the card.

Ben


---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Michel Dänzer 
On Mon, 2003-02-03 at 18:09, Benjamin Herrenschmidt wrote:
 On Mon, 2003-02-03 at 17:05, Michel Dänzer wrote:
  On Mon, 2003-02-03 at 17:34, Alan Cox wrote:
   On Mon, 2003-02-03 at 15:02, Keith Whitwell wrote:
  
 -#define COMMIT_RING() do {   \
 - RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
 +#define COMMIT_RING() do {   \
 + /* read from PCI bus to ensure correct posting */   \
 + RADEON_READ( RADEON_CP_RB_WPTR );   \
 + RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail ); \
 + RADEON_READ( RADEON_CP_RB_WPTR );   \
  } while (0)

Ouch.  Put a conditional around that at least, so that not everybody suffers...
   
   PCI posting applies to all platforms. However I'm trying to understand what this
   is trying to do. The final read has an effect in that it ensures that the WPTR is
   written not left posted for an undefined time. What does the previous one 
achieve.
   Is there some kind of synchronization requirement against the GART/main memory ?
  
  That's my understanding, we need to make sure the chip reads from the
  ring what we wrote to it.
 
 Well... You are asking for trouble ;)
 
 The problem is that the behaviour will be pretty much HW implementation
 dependant. 
 
 In the AGP case, the ring is mapped uncacheable. So your card and the
 ring are typically on the same memory type from the CPU, that helps.
 Though I would still make sure the correct bus path is flushed by doing
 that first read from the ring and not from the card.
 
 In the PCI case, the ring is mapped cacheable in normal memory and you
 rely on the PCI cache coherency (snooping). That means that you have a
 new problem which is to synchronize writes to cacheable memory (the
 ring) with write to non cacheable MMIO space (the card). At least on
 PPC, I don't think anything but a full sync instruction will acheive
 that, so you'd rather add an mb(). And do the read from memory (actually
 cache), not the card.

You mean like this? Chris, does that work for you?


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast

Index: radeon_drv.h
===
RCS file: /cvsroot/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/shared/drm/kernel/radeon_drv.h,v
retrieving revision 1.12
diff -p -u -r1.12 radeon_drv.h
--- radeon_drv.h	2 Jan 2003 18:38:07 -	1.12
+++ radeon_drv.h	3 Feb 2003 18:25:00 -
@@ -864,8 +857,13 @@ do {	\
 		dev_priv-ring.tail = write;\
 } while (0)
 
-#define COMMIT_RING() do {	\
-	RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail );		\
+#define COMMIT_RING() do {		\
+	/* Flush writes to ring */	\
+	DRM_READMEMORYBARRIER();	\
+	GET_RING_HEAD( dev_priv-ring );\
+	RADEON_WRITE( RADEON_CP_RB_WPTR, dev_priv-ring.tail );		\
+	/* read from PCI bus to ensure correct posting */		\
+	RADEON_READ( RADEON_CP_RB_RPTR );\
 } while (0)
 
 #define OUT_RING( x ) do {		\

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Chris Ison 
Yeah, it works, just a little weary of it cause remembering without the
read it won't work, but it still appears to work with the read at the
end.

 
 You mean like this? Chris, does that work for you?
 
 --
 Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
 XFree86 and DRI project member   /  CS student, Free Software enthusiast
 
   
   Name: radeon-commit_ring.diff
radeon-commit_ring.diffType: Plain Text (text/plain)
   Encoding: 7bit


---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Michel Dänzer 
On Mon, 2003-02-03 at 22:31, Chris Ison wrote:
 Yeah, it works, 

Great!

 just a little weary of it cause remembering without the read it won't work, 
 but it still appears to work with the read at the end.

Well, I think Ben has given the best explanation of what's going on so
far. Keith, are you happy with this?


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Keith Whitwell 
Michel Dänzer wrote:

On Mon, 2003-02-03 at 22:31, Chris Ison wrote:


Yeah, it works, 


Great!



just a little weary of it cause remembering without the read it won't work, 
but it still appears to work with the read at the end.


Well, I think Ben has given the best explanation of what's going on so
far. Keith, are you happy with this?


Relatively, though a little uncertain why we hadn't seen problems with this 
before.

Keith



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Michel Dänzer 
On Mon, 2003-02-03 at 22:59, Keith Whitwell wrote:
 Michel Dänzer wrote:
  On Mon, 2003-02-03 at 22:31, Chris Ison wrote:
  
 Yeah, it works, 
  
  
  Great!
  
  
 just a little weary of it cause remembering without the read it won't work, 
 but it still appears to work with the read at the end.
  
  
  Well, I think Ben has given the best explanation of what's going on so
  far. Keith, are you happy with this?
 
 Relatively, though a little uncertain why we hadn't seen problems with this 
 before.

Well, I think we have. I've seen lots of reports of random crashes after
hours to days with AGP cards, with no activity when the crash occurs.
Two examples from my own experience are the AGP Radeon 7500 I used to
have in an Athlon box at work, which would usually die after a couple
days, and an AGP Radeon 7200 in a Cube here which used to crash after an
hour maximum, but now it's been running for several hours with Chris'
patch (going to test the one based on Ben's suggestions afterwards).

Also, we used to at least have a radeon_flush_write_combining() call
before the ring write pointer update when it was still done in
ADVANCE_RING(). That probably got lost when COMMIT_RING() was split out?


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel

Re: [Dri-devel] Radeon PCI Fix

2003-02-03 Thread Chris Ison 
This has been seen before, and fixes a known issue with PCI cards
locking up, the same fix possably fixes an issue occassionaly seen with
AGP card locking up.

It wasn't easy to track down and the fix was only found by accident, it
could well of remained alot longer if I hadn't gotten impatient and
tried to find the problem myself.

anyways, yes mike, your new patch does work, now to check out the texmem
branch/trunk to see if I still have that texture problem.

Keith Whitwell wrote:
 
 Relatively, though a little uncertain why we hadn't seen problems with this
 before.
 
 Keith



---
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel