Re: [PATCH] fbdev: Fix IO access in rivafb

You probably broke ppc versions here. The driver enables "big endian"
register access, but readw/writew/l functions do byteswap, which will
lead to incorrect results.

The fix would be to probably just remove the code that puts the chip
registers into big endian mode, this isn't necessary nor a very good
idea actually.

I don't have an nVidia card on ppc to test any more unfortunately.

Ben.

On Monday 08 November 2004 13:57, Benjamin Herrenschmidt wrote:

quoted

diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h

--- a/drivers/video/riva/riva_hw.h	2004-11-07 21:21:47 -08:00
+++ b/drivers/video/riva/riva_hw.h	2004-11-07 21:21:47 -08:00

@@ -78,13 +78,13 @@
 #define NV_WR08(p,i,d)	out_8(p+i, d)
 #define NV_RD08(p,i)	in_8(p+i)
 #else
-#define NV_WR08(p,i,d)  (((U008 *)(p))[i]=(d))
-#define NV_RD08(p,i)    (((U008 *)(p))[i])
+#define NV_WR08(p,i,d)  (writeb((d), (u8 __iomem *)(p) + (i)))
+#define NV_RD08(p,i)    (readb((u8 __iomem *)(p) + (i)))
 #endif
-#define NV_WR16(p,i,d)  (((U016 *)(p))[(i)/2]=(d))
-#define NV_RD16(p,i)    (((U016 *)(p))[(i)/2])
-#define NV_WR32(p,i,d)  (((U032 *)(p))[(i)/4]=(d))
-#define NV_RD32(p,i)    (((U032 *)(p))[(i)/4])
+#define NV_WR16(p,i,d)  (writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i)    (readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d)  (writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i)    (readl((u32 __iomem *)(p) + (i)/4))
 #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
 #define VGA_RD08(p,i)   NV_RD08(p,i)

You probably broke ppc versions here. The driver enables "big endian"
register access, but readw/writew/l functions do byteswap, which will
lead to incorrect results.

The fix would be to probably just remove the code that puts the chip
registers into big endian mode, this isn't necessary nor a very good
idea actually.

I don't have an nVidia card on ppc to test any more unfortunately.

Hmm, I'll ask Guido Guenther if he can test the changes. I think a different
set of access macros for PPC might be a more preferrable solution. 

Tony

On Mon, 2004-11-08 at 16:33 +0800, Antonino A. Daplas wrote:

Hmm, I'll ask Guido Guenther if he can test the changes. I think a different
set of access macros for PPC might be a more preferrable solution. 

Well, I'd rather leave the registers Little Endian, but then, it will
clash with X which does put them into Big Endian mode, so that would
have to be restored all the time.

Ben.

On Tuesday 09 November 2004 05:50, Benjamin Herrenschmidt wrote:

On Mon, 2004-11-08 at 16:33 +0800, Antonino A. Daplas wrote:

quoted

Hmm, I'll ask Guido Guenther if he can test the changes. I think a
different set of access macros for PPC might be a more preferrable
solution.

Well, I'd rather leave the registers Little Endian, but then, it will
clash with X which does put them into Big Endian mode, so that would
have to be restored all the time.

I also asked Guido if he can test the removal of the code that puts the
hardware in big endian, although I told him that I prefer a new set of
access macros.  But I'll leave this decision to you people. 

Tony

On Monday 08 November 2004 13:57, Benjamin Herrenschmidt wrote:

quoted

diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h

--- a/drivers/video/riva/riva_hw.h	2004-11-07 21:21:47 -08:00
+++ b/drivers/video/riva/riva_hw.h	2004-11-07 21:21:47 -08:00

@@ -78,13 +78,13 @@
 #define NV_WR08(p,i,d)	out_8(p+i, d)
 #define NV_RD08(p,i)	in_8(p+i)
 #else
-#define NV_WR08(p,i,d)  (((U008 *)(p))[i]=(d))
-#define NV_RD08(p,i)    (((U008 *)(p))[i])
+#define NV_WR08(p,i,d)  (writeb((d), (u8 __iomem *)(p) + (i)))
+#define NV_RD08(p,i)    (readb((u8 __iomem *)(p) + (i)))
 #endif
-#define NV_WR16(p,i,d)  (((U016 *)(p))[(i)/2]=(d))
-#define NV_RD16(p,i)    (((U016 *)(p))[(i)/2])
-#define NV_WR32(p,i,d)  (((U032 *)(p))[(i)/4]=(d))
-#define NV_RD32(p,i)    (((U032 *)(p))[(i)/4])
+#define NV_WR16(p,i,d)  (writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i)    (readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d)  (writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i)    (readl((u32 __iomem *)(p) + (i)/4))
 #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
 #define VGA_RD08(p,i)   NV_RD08(p,i)

You probably broke ppc versions here. The driver enables "big endian"
register access, but readw/writew/l functions do byteswap, which will
lead to incorrect results.

The fix would be to probably just remove the code that puts the chip
registers into big endian mode, this isn't necessary nor a very good
idea actually.

How about this patch?  This is almost the original macro in riva_hw.h,
with the __force annotation.

Signed-off-by: Antonino Daplas <redacted>
---

diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h

--- a/drivers/video/riva/riva_hw.h	2004-11-06 12:42:19 +08:00
+++ b/drivers/video/riva/riva_hw.h	2004-11-08 16:59:34 +08:00

@@ -77,14 +77,18 @@
 #include <asm/io.h>
 #define NV_WR08(p,i,d)	out_8(p+i, d)
 #define NV_RD08(p,i)	in_8(p+i)
+#define NV_WR16(p,i,d)  (((volatile u16 __force *)(p))[(i)/2]=(d))
+#define NV_RD16(p,i)    (((volatile u16 __force *)(p))[(i)/2])
+#define NV_WR32(p,i,d)  (((volatile u32 __force *)(p))[(i)/4]=(d))
+#define NV_RD32(p,i)    (((volatile u32 __force *)(p))[(i)/4])
 #else
 #define NV_WR08(p,i,d)  (writeb((d), (u8 __iomem *)(p) + (i)))
 #define NV_RD08(p,i)    (readb((u8 __iomem *)(p) + (i)))
-#endif
 #define NV_WR16(p,i,d)  (writew((d), (u16 __iomem *)(p) + (i)/2))
 #define NV_RD16(p,i)    (readw((u16 __iomem *)(p) + (i)/2))
 #define NV_WR32(p,i,d)  (writel((d), (u32 __iomem *)(p) + (i)/4))
 #define NV_RD32(p,i)    (readl((u32 __iomem *)(p) + (i)/4))
+#endif
 #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
 #define VGA_RD08(p,i)   NV_RD08(p,i)
 




-------------------------------------------------------

This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

On Mon, 8 Nov 2004, Antonino A. Daplas wrote:

PPC also has nice {out,in}_be{16,32}() operations.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

On Mon, 8 Nov 2004, Antonino A. Daplas wrote:

How about this patch?  This is almost the original macro in riva_hw.h,
with the __force annotation.

Why not just use __raw_readl/__raw_writel?

That's what they exist for, and they still do any IO accesses correctly, 
which a direct store does not do (it would seriously break on older 
alphas, for example).

Of course, clearly the thing has never worked on things like alphas 
anyway, but the point is that accessing IO memory with direct loads and 
stores really _is_ fundamentally wrong, even if it happens to work on many 
architectures. The keyword is "happens". 

		Linus


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

On Monday 08 November 2004 23:21, Linus Torvalds wrote:

On Mon, 8 Nov 2004, Antonino A. Daplas wrote:

quoted

How about this patch?  This is almost the original macro in riva_hw.h,
with the __force annotation.

Why not just use __raw_readl/__raw_writel?

Ok.

Tony

In big endian machines, the read*/write* accessors do a byteswap for an
inherently little endian PCI bus.  However, rivafb puts the hardwire in big
endian register access, thus the byteswap is not needed. So, instead of
read*/write*, use __raw_read*/__raw_write*.
 
Signed-off-by: Antonino Daplas <redacted>
---

diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h

--- a/drivers/video/riva/riva_hw.h	2004-11-09 03:45:55 +08:00
+++ b/drivers/video/riva/riva_hw.h	2004-11-09 03:50:32 +08:00

@@ -77,14 +77,18 @@
 #include <asm/io.h>
 #define NV_WR08(p,i,d)	out_8(p+i, d)
 #define NV_RD08(p,i)	in_8(p+i)
+#define NV_WR16(p,i,d)  (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i)    (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d)  (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i)    (__raw_readl((u32 __iomem *)(p) + (i)/4))
 #else
 #define NV_WR08(p,i,d)  (writeb((d), (u8 __iomem *)(p) + (i)))
 #define NV_RD08(p,i)    (readb((u8 __iomem *)(p) + (i)))
-#endif
 #define NV_WR16(p,i,d)  (writew((d), (u16 __iomem *)(p) + (i)/2))
 #define NV_RD16(p,i)    (readw((u16 __iomem *)(p) + (i)/2))
 #define NV_WR32(p,i,d)  (writel((d), (u32 __iomem *)(p) + (i)/4))
 #define NV_RD32(p,i)    (readl((u32 __iomem *)(p) + (i)/4))
+#endif
 #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
 #define VGA_RD08(p,i)   NV_RD08(p,i)
 

On Tue, 9 Nov 2004, Antonino A. Daplas wrote:

In big endian machines, the read*/write* accessors do a byteswap for an
inherently little endian PCI bus.  However, rivafb puts the hardwire in big
endian register access, thus the byteswap is not needed. So, instead of
read*/write*, use __raw_read*/__raw_write*.

This fix should make the #ifdef CONFIG_PCC entirely superfluous afaik. 

The thing is, once riva does its HW accesses right, the special cases just 
go away. There's a reason we have abstractions.. 

Does anybody have the hardware to test with?

		Linus

On Tuesday 09 November 2004 04:13, Linus Torvalds wrote:

On Tue, 9 Nov 2004, Antonino A. Daplas wrote:

quoted

In big endian machines, the read*/write* accessors do a byteswap for an
inherently little endian PCI bus.  However, rivafb puts the hardwire in
big endian register access, thus the byteswap is not needed. So, instead
of read*/write*, use __raw_read*/__raw_write*.

This fix should make the #ifdef CONFIG_PCC entirely superfluous afaik.

Ah, of course.

The thing is, once riva does its HW accesses right, the special cases just
go away. There's a reason we have abstractions..

Does anybody have the hardware to test with?

I've asked Guido Guenther to test, might take a couple of days.  I also
asked him if he can use the in/out_be* which is probably safer for ppc.
And the mb()'s scattered all over the driver may be removed. 

Tony


In big endian machines, the read*/write* accessors do a byteswap for an
inherently little endian PCI bus.  However, rivafb puts the hardwire in big
endian register access, thus the byteswap is not needed. So for 16- and
32-bit access, instead of read*/write*, use __raw_read*/__raw_write* for all
archs.
 
Signed-off-by: Antonino Daplas <redacted>
---

diff -Nru a/drivers/video/riva/riva_hw.h b/drivers/video/riva/riva_hw.h

--- a/drivers/video/riva/riva_hw.h	2004-11-09 05:39:48 +08:00
+++ b/drivers/video/riva/riva_hw.h	2004-11-09 06:00:09 +08:00

@@ -73,18 +73,13 @@
 /*
  * HW access macros.
  */
-#if defined(__powerpc__)
 #include <asm/io.h>
-#define NV_WR08(p,i,d)	out_8(p+i, d)
-#define NV_RD08(p,i)	in_8(p+i)
-#else
 #define NV_WR08(p,i,d)  (writeb((d), (u8 __iomem *)(p) + (i)))
 #define NV_RD08(p,i)    (readb((u8 __iomem *)(p) + (i)))
-#endif
-#define NV_WR16(p,i,d)  (writew((d), (u16 __iomem *)(p) + (i)/2))
-#define NV_RD16(p,i)    (readw((u16 __iomem *)(p) + (i)/2))
-#define NV_WR32(p,i,d)  (writel((d), (u32 __iomem *)(p) + (i)/4))
-#define NV_RD32(p,i)    (readl((u32 __iomem *)(p) + (i)/4))
+#define NV_WR16(p,i,d)  (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i)    (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d)  (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i)    (__raw_readl((u32 __iomem *)(p) + (i)/4))
 #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
 #define VGA_RD08(p,i)   NV_RD08(p,i)
 

On Tue, 9 Nov 2004, Antonino A. Daplas wrote:

In big endian machines, the read*/write* accessors do a byteswap for an
inherently little endian PCI bus.  However, rivafb puts the hardwire in big
endian register access, thus the byteswap is not needed. So for 16- and
32-bit access, instead of read*/write*, use __raw_read*/__raw_write* for all
archs.

Ok, applied with some further simplifications (use "void __iomem *" and 
suddenly those /2 and /4 just go away - also use __raw_xxxx for the 
single-byte versions to be consistent).

		Linus

On Mon, Nov 08, 2004 at 02:25:04PM -0800, Linus Torvalds wrote:

On Tue, 9 Nov 2004, Antonino A. Daplas wrote:

quoted

In big endian machines, the read*/write* accessors do a byteswap for an
inherently little endian PCI bus.  However, rivafb puts the hardwire in big
endian register access, thus the byteswap is not needed. So for 16- and
32-bit access, instead of read*/write*, use __raw_read*/__raw_write* for all
archs.

Ok, applied with some further simplifications (use "void __iomem *" and 
suddenly those /2 and /4 just go away - also use __raw_xxxx for the 
single-byte versions to be consistent).

Doesn't work for me. This one works:

diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h

--- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h	2004-11-12 13:42:54.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h	2004-11-12 13:47:29.400807920 +0100

@@ -75,12 +75,12 @@
  */
 #include <asm/io.h>
 
-#define NV_WR08(p,i,d)  (__raw_writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i)    (__raw_readb((void __iomem *)(p) + (i)))
-#define NV_WR16(p,i,d)  (__raw_writew((d), (void __iomem *)(p) + (i)))
-#define NV_RD16(p,i)    (__raw_readw((void __iomem *)(p) + (i)))
-#define NV_WR32(p,i,d)  (__raw_writel((d), (void __iomem *)(p) + (i)))
-#define NV_RD32(p,i)    (__raw_readl((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d)  (writeb((d), (u8 __iomem *)(p) + (i)))
+#define NV_RD08(p,i)    (readb((u8 __iomem *)(p) + (i)))
+#define NV_WR16(p,i,d)  (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i)    (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d)  (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i)    (__raw_readl((u32 __iomem *)(p) + (i)/4))
 
 #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
 #define VGA_RD08(p,i)   NV_RD08(p,i)

Interesting enough this one doesn't (only differenc in NV_{WR,RW}08:

diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h

--- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h	2004-11-12 13:42:54.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h	2004-11-12 13:47:29.400807920 +0100

@@ -75,12 +75,12 @@
  */
 #include <asm/io.h>
 
-#define NV_WR08(p,i,d)  (__raw_writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i)    (__raw_readb((void __iomem *)(p) + (i)))
-#define NV_WR16(p,i,d)  (__raw_writew((d), (void __iomem *)(p) + (i)))
-#define NV_RD16(p,i)    (__raw_readw((void __iomem *)(p) + (i)))
-#define NV_WR32(p,i,d)  (__raw_writel((d), (void __iomem *)(p) + (i)))
-#define NV_RD32(p,i)    (__raw_readl((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d)  (writeb((d), (u8 __iomem *)(p) + (i)))
+#define NV_RD08(p,i)    (readb((u8 __iomem *)(p) + (i)))
+#define NV_WR16(p,i,d)  (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i)    (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d)  (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i)    (__raw_readl((u32 __iomem *)(p) + (i)/4))
 
 #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
 #define VGA_RD08(p,i)   NV_RD08(p,i)

Cheers,
 -- Guido


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

On Fri, 12 Nov 2004, Guido Guenther wrote:

Can you please try without the broken "u16" and "(i/2)" things (and u32
and i/4)? They really should not be needed, unless something is
_seriously_ broken. The only thing they do is the automatically align the
reference - and quite frankly, it looks like anybody passing an unaligned
register offset is _quite_ buggy.

IOW, it would seem that the real difference is the "__raw_writeb" vs
"writeb". Can you test just that change?

I don't see the difference to your other patch. Did you put in the wrong
patch?

			Linus

On Fri, Nov 12, 2004 at 08:01:00AM -0800, Linus Torvalds wrote:

On Fri, 12 Nov 2004, Guido Guenther wrote:

quoted

Doesn't work for me. This one works:

diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h

--- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h	2004-11-12 13:42:54.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h	2004-11-12 13:47:29.400807920 +0100

@@ -75,12 +75,12 @@
  */
 #include <asm/io.h>
 
-#define NV_WR08(p,i,d)  (__raw_writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i)    (__raw_readb((void __iomem *)(p) + (i)))
-#define NV_WR16(p,i,d)  (__raw_writew((d), (void __iomem *)(p) + (i)))
-#define NV_RD16(p,i)    (__raw_readw((void __iomem *)(p) + (i)))
-#define NV_WR32(p,i,d)  (__raw_writel((d), (void __iomem *)(p) + (i)))
-#define NV_RD32(p,i)    (__raw_readl((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d)  (writeb((d), (u8 __iomem *)(p) + (i)))
+#define NV_RD08(p,i)    (readb((u8 __iomem *)(p) + (i)))
+#define NV_WR16(p,i,d)  (__raw_writew((d), (u16 __iomem *)(p) + (i)/2))
+#define NV_RD16(p,i)    (__raw_readw((u16 __iomem *)(p) + (i)/2))
+#define NV_WR32(p,i,d)  (__raw_writel((d), (u32 __iomem *)(p) + (i)/4))
+#define NV_RD32(p,i)    (__raw_readl((u32 __iomem *)(p) + (i)/4))

Can you please try without the broken "u16" and "(i/2)" things (and u32
and i/4)? They really should not be needed, unless something is
_seriously_ broken. The only thing they do is the automatically align the
reference - and quite frankly, it looks like anybody passing an unaligned
register offset is _quite_ buggy.

O.k., it was the __raw_{write,read}b which broke things, not the
"alignment". This one works:

diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h

--- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h	2004-11-12 13:42:54.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h	2004-11-12 17:39:22.000000000 +0100

@@ -75,8 +75,8 @@
  */
 #include <asm/io.h>
 
-#define NV_WR08(p,i,d)  (__raw_writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i)    (__raw_readb((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d)  (writeb((d), (void __iomem *)(p) + (i)))
+#define NV_RD08(p,i)    (readb((void __iomem *)(p) + (i)))
 #define NV_WR16(p,i,d)  (__raw_writew((d), (void __iomem *)(p) + (i)))
 #define NV_RD16(p,i)    (__raw_readw((void __iomem *)(p) + (i)))
 #define NV_WR32(p,i,d)  (__raw_writel((d), (void __iomem *)(p) + (i)))

Signed-off-by: Guido Guenther <agx@sigxcpu.org>

IOW, it would seem that the real difference is the "__raw_writeb" vs
"writeb". Can you test just that change?

quoted

 #define VGA_WR08(p,i,d) NV_WR08(p,i,d)
 #define VGA_RD08(p,i)   NV_RD08(p,i)

Interesting enough this one doesn't (only differenc in NV_{WR,RW}08:

I don't see the difference to your other patch. Did you put in the wrong
patch?

There aren't any, I actually attached the wrong patch. The non-working
version has __raw_{read,write}b8 instead of {read,write}b8. In 2.6.9
riva_hw.h used {in,out}_8 for NV_{RD,WR}08 so using the "non-raw"
writeb/readb now looks correct since these map to {in,out}_8 now.
Cheers,
 -- Guido


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

On Fri, 12 Nov 2004, Guido Guenther wrote:

O.k., it was the __raw_{write,read}b which broke things, not the
"alignment". This one works:

All right, that's as expected. However, it does seem to point out that the 
riva driver depends on _different_ memory ordering guarantees for the 
8-bit accesses as opposed to the other ones. Whee. Can you say "UGGLEE"?

Anyway, patch applied. Thanks,

		Linus

On Fri, Nov 12, 2004 at 11:32:07AM -0800, Linus Torvalds wrote:

On Fri, 12 Nov 2004, Guido Guenther wrote:

quoted

O.k., it was the __raw_{write,read}b which broke things, not the
"alignment". This one works:

All right, that's as expected. However, it does seem to point out that the 
riva driver depends on _different_ memory ordering guarantees for the 
8-bit accesses as opposed to the other ones. Whee. Can you say "UGGLEE"?

Aglie. This scared me too, so I had another look. It seems P{V,C}IO
areas are only accessed using VGA_{RD,WR}8 macros. NV_{RW,WR}08 are
never actually used directly. So this patch makes at least usage
consistent. VGA_{RD,WR}8 to access "I/O areas" in an ordered way. NV_*
for the rest. Please apply.
Cheers,
 -- Guido

--- linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.orig.2	2004-11-13 12:24:48.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h	2004-11-13 12:24:56.000000000 +0100

@@ -75,15 +75,15 @@
  */
 #include <asm/io.h>
 
-#define NV_WR08(p,i,d)  (writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i)    (readb((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d)  (__raw_writeb((d), (void __iomem *)(p) + (i)))
+#define NV_RD08(p,i)    (__raw_readb((void __iomem *)(p) + (i)))
 #define NV_WR16(p,i,d)  (__raw_writew((d), (void __iomem *)(p) + (i)))
 #define NV_RD16(p,i)    (__raw_readw((void __iomem *)(p) + (i)))
 #define NV_WR32(p,i,d)  (__raw_writel((d), (void __iomem *)(p) + (i)))
 #define NV_RD32(p,i)    (__raw_readl((void __iomem *)(p) + (i)))
 
-#define VGA_WR08(p,i,d) NV_WR08(p,i,d)
-#define VGA_RD08(p,i)   NV_RD08(p,i)
+#define VGA_WR08(p,i,d) (writeb((d), (void __iomem *)(p) + (i)))
+#define VGA_RD08(p,i)   (readb((void __iomem *)(p) + (i)))
 
 /*
  * Define different architectures.

Signed-Off-By: Guido Guenther <agx@sigxcpu.org>


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:

Interesting. The only difference here should be barriers. I hate the
lack of barriers in that driver ... I'm not sure the driver may not have
other bugs related to the lack of them in the 16 and 32 bits accessors.
It does use non-barrier version on purpose in some accel ops though,
when filling the fifo with pixels, but that's pretty much the only case
where it makes sense.

There aren't any, I actually attached the wrong patch. The non-working
version has __raw_{read,write}b8 instead of {read,write}b8. In 2.6.9
riva_hw.h used {in,out}_8 for NV_{RD,WR}08 so using the "non-raw"
writeb/readb now looks correct since these map to {in,out}_8 now.

{in,out}_8 are ppc-specific things that are identical to readb/writeb
indeed, with barriers.

Ben.




-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

On Sat, Nov 13, 2004 at 12:39:32PM +1100, Benjamin Herrenschmidt wrote:

On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:

quoted

O.k., it was the __raw_{write,read}b which broke things, not the
"alignment". This one works:

diff -u -u linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h

--- linux-2.6.10-rc1-mm5.orig/drivers/video/riva/riva_hw.h	2004-11-12 13:42:54.000000000 +0100
+++ linux-2.6.10-rc1-mm5/drivers/video/riva/riva_hw.h	2004-11-12 17:39:22.000000000 +0100

@@ -75,8 +75,8 @@
  */
 #include <asm/io.h>
 
-#define NV_WR08(p,i,d)  (__raw_writeb((d), (void __iomem *)(p) + (i)))
-#define NV_RD08(p,i)    (__raw_readb((void __iomem *)(p) + (i)))
+#define NV_WR08(p,i,d)  (writeb((d), (void __iomem *)(p) + (i)))
+#define NV_RD08(p,i)    (readb((void __iomem *)(p) + (i)))

Interesting. The only difference here should be barriers. I hate the
lack of barriers in that driver ... I'm not sure the driver may not have

Yes, it's a real pain. Missing barriers in RIVA_FIFO_FREE caused lots of
lockups, until I found them in your 2.4 tree.

other bugs related to the lack of them in the 16 and 32 bits accessors.
It does use non-barrier version on purpose in some accel ops though,
when filling the fifo with pixels, but that's pretty much the only case
where it makes sense.

quoted

There aren't any, I actually attached the wrong patch. The non-working
version has __raw_{read,write}b8 instead of {read,write}b8. In 2.6.9
riva_hw.h used {in,out}_8 for NV_{RD,WR}08 so using the "non-raw"
writeb/readb now looks correct since these map to {in,out}_8 now.

{in,out}_8 are ppc-specific things that are identical to readb/writeb
indeed, with barriers.

In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
__raw_{read,write}b is different. So you mean __raw_{read,write}b in the
above? (no nitpicking, just want to be sure I understand this
correctly).
Cheers,
 -- Guido


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

On Saturday 13 November 2004 19:22, Guido Guenther wrote:

On Sat, Nov 13, 2004 at 12:39:32PM +1100, Benjamin Herrenschmidt wrote:

quoted

On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:

In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
__raw_{read,write}b is different. So you mean __raw_{read,write}b in the
above? (no nitpicking, just want to be sure I understand this
correctly).

Why not use in_be* and out_be* for __raw_read and raw_write?  If I
understand correctly, they also have barriers.  Or would that hurt
performance?

Tony




-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

On Sat, Nov 13, 2004 at 08:00:30PM +0800, Antonino A. Daplas wrote:

On Saturday 13 November 2004 19:22, Guido Guenther wrote:

quoted

On Sat, Nov 13, 2004 at 12:39:32PM +1100, Benjamin Herrenschmidt wrote:

quoted

On Fri, 2004-11-12 at 20:18 +0100, Guido Guenther wrote:

In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
__raw_{read,write}b is different. So you mean __raw_{read,write}b in the
above? (no nitpicking, just want to be sure I understand this
correctly).

Why not use in_be* and out_be* for __raw_read and raw_write?  If I
understand correctly, they also have barriers.  Or would that hurt
performance?

I think it would. XFree86 comes along without these barriers nicely (and
this this driver was written with documentation ;), so rivafb should be
o.k. too.
 -- Guido


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

On Sat, 13 Nov 2004, Antonino A. Daplas wrote:

Why not use in_be* and out_be* for __raw_read and raw_write? 

Please don't start using some stupid magic ppc-specific macros for a 
driver that has no reason to be PPC-specific. It then only causes bugs 
that show on one platform and not another.

		Linus

On Sunday 14 November 2004 02:00, Linus Torvalds wrote:

On Sat, 13 Nov 2004, Antonino A. Daplas wrote:

quoted

Why not use in_be* and out_be* for __raw_read and raw_write?

Please don't start using some stupid magic ppc-specific macros for a
driver that has no reason to be PPC-specific. It then only causes bugs
that show on one platform and not another.

I'm not. I'm just wondering that if the other approach was taken (keep the
hardware in little endian mode), then the write/read* macros, which are just
wrappers for in_le*/out_le*, would have been used. Would it help fix (or
cover up) bugs that are in PPC but not x86? 

Tony 





-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

On Sun, Nov 14, 2004 at 05:29:46AM +0800, Antonino A. Daplas wrote:

On Sunday 14 November 2004 02:00, Linus Torvalds wrote:

quoted

On Sat, 13 Nov 2004, Antonino A. Daplas wrote:

quoted

Why not use in_be* and out_be* for __raw_read and raw_write?

Please don't start using some stupid magic ppc-specific macros for a
driver that has no reason to be PPC-specific. It then only causes bugs
that show on one platform and not another.

I'm not. I'm just wondering that if the other approach was taken (keep the
hardware in little endian mode), then the write/read* macros, which are just
wrappers for in_le*/out_le*, would have been used. Would it help fix (or
cover up) bugs that are in PPC but not x86? 

XFree86 switches the card to big endian mode anyway. Running rivafb in
little endian might cause us great deals of pain for little gain.
Cheers,
 -- Guido


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

On Sun, 2004-11-14 at 05:29 +0800, Antonino A. Daplas wrote:

On Sunday 14 November 2004 02:00, Linus Torvalds wrote:

quoted

On Sat, 13 Nov 2004, Antonino A. Daplas wrote:

quoted

Why not use in_be* and out_be* for __raw_read and raw_write?

Please don't start using some stupid magic ppc-specific macros for a
driver that has no reason to be PPC-specific. It then only causes bugs
that show on one platform and not another.

I'm not. I'm just wondering that if the other approach was taken (keep the
hardware in little endian mode), then the write/read* macros, which are just
wrappers for in_le*/out_le*, would have been used. Would it help fix (or
cover up) bugs that are in PPC but not x86? 

If you switch the HW to LE, I'm afraid you'll lockup when VT switching
back & forth with X ...

Ben.




-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

On Sat, 2004-11-13 at 12:22 +0100, Guido Guenther wrote:

quoted

{in,out}_8 are ppc-specific things that are identical to readb/writeb
indeed, with barriers.

In 2.6.10-rc1-mm5 {in,out}_8 and read/writeb are exactly identical, only
__raw_{read,write}b is different. So you mean __raw_{read,write}b in the
above? (no nitpicking, just want to be sure I understand this
correctly).

I just meant they are identical and they have both barriers, sorry.

Ben.




-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8

Hi Linus,

At Mon, 8 Nov 2004 07:21:53 -0800 (PST),
Linus Torvalds wrote:

On Mon, 8 Nov 2004, Antonino A. Daplas wrote:

quoted

How about this patch?  This is almost the original macro in riva_hw.h,
with the __force annotation.

Why not just use __raw_readl/__raw_writel?

That's what they exist for, and they still do any IO accesses correctly, 
which a direct store does not do (it would seriously break on older 
alphas, for example).

sorry for a dumb question but should readl/writel on big endian system
swap like ppc does?

hmm... checking source code, it seems like most arch indeed does that,
except m68knommu, h8300 and microblaze. is it nommu thing?
--
             yashi

On Mon, 2004-11-08 at 17:06 +0800, Antonino A. Daplas wrote:

How about this patch?  This is almost the original macro in riva_hw.h,
with the __force annotation.

I don't like it neither. It lacks barriers. the rivafb driver
notoriously lacks barriers, except in a few places where it was so bad
that it actually broke all the time, where we added some. This
originates from the X "nv" driver written by Mark Vojkovich who didn't
want to hear about barriers for perfs reasons I think.

Ben.

Benjamin Herrenschmidt wrote:

On Mon, 2004-11-08 at 17:06 +0800, Antonino A. Daplas wrote:

quoted

How about this patch?  This is almost the original macro in riva_hw.h,
with the __force annotation.

I don't like it neither. It lacks barriers. the rivafb driver
notoriously lacks barriers, except in a few places where it was so bad
that it actually broke all the time, where we added some. This
originates from the X "nv" driver written by Mark Vojkovich who didn't
want to hear about barriers for perfs reasons I think.

Could this be the reason why in 2.6.7 I get solid lockups when 
switching to nv-driven X and rivafb-driven console? Is there 
something I could test to see if this is the reason?

-- 
Giuseppe "Oblomov" Bilotta

Can't you see
It all makes perfect sense
Expressed in dollar and cents
Pounds shillings and pence
                  (Roger Waters)

On Tue, Nov 09, 2004 at 12:07:58AM +0100, Giuseppe Bilotta wrote:

Benjamin Herrenschmidt wrote:

quoted

On Mon, 2004-11-08 at 17:06 +0800, Antonino A. Daplas wrote:

quoted

How about this patch?  This is almost the original macro in riva_hw.h,
with the __force annotation.

I don't like it neither. It lacks barriers. the rivafb driver
notoriously lacks barriers, except in a few places where it was so bad
that it actually broke all the time, where we added some. This
originates from the X "nv" driver written by Mark Vojkovich who didn't
want to hear about barriers for perfs reasons I think.

Could this be the reason why in 2.6.7 I get solid lockups when 
switching to nv-driven X and rivafb-driven console? Is there 
something I could test to see if this is the reason?

Please try 2.6.10-rc1-mm5. It should work there.
 -- Guido


-------------------------------------------------------
This SF.Net email is sponsored by: InterSystems CACHE
FREE OODBMS DOWNLOAD - A multidimensional database that combines
robust object and relational technologies, making it a perfect match
for Java, C++,COM, XML, ODBC and JDBC. www.intersystems.com/match8