[PATCH] Fix colors in mach64 accelerator

[PATCH] Fix colors in mach64 accelerator

[Mikulas Patocka]

Use the same color-calculating algorithm as in atyfb_imageblit in this driver
or in generic cfb_fillrect.

This patch fixes bad colors when using an accelerator in 15-bit and 16-bit
modes.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com

---
 drivers/video/aty/mach64_accel.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Index: linux-2.6.30-fast/drivers/video/aty/mach64_accel.c
===================================================================
--- linux-2.6.30-fast.orig/drivers/video/aty/mach64_accel.c	2009-06-15 21:42:10.000000000 +0200
+++ linux-2.6.30-fast/drivers/video/aty/mach64_accel.c	2009-06-17 08:49:01.000000000 +0200
@@ -239,7 +239,7 @@ void atyfb_copyarea(struct fb_info *info
 void atyfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
 {
 	struct atyfb_par *par = (struct atyfb_par *) info->par;
-	u32 color = rect->color, dx = rect->dx, width = rect->width, rotation = 0;
+	u32 color, dx = rect->dx, width = rect->width, rotation = 0;
 
 	if (par->asleep)
 		return;
@@ -250,8 +250,11 @@ void atyfb_fillrect(struct fb_info *info
 		return;
 	}
 
-	color |= (rect->color << 8);
-	color |= (rect->color << 16);
+	if (info->fix.visual == FB_VISUAL_TRUECOLOR ||
+	    info->fix.visual == FB_VISUAL_DIRECTCOLOR)
+		color = ((u32 *)(info->pseudo_palette))[rect->color];
+	else
+		color = rect->color;
 
 	if (info->var.bits_per_pixel == 24) {
 		/* In 24 bpp, the engine is in 8 bpp - this requires that all */

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing 
server and web deployment.
http://p.sf.net/sfu/businessobjects

[PATCH] Disable mach64 accelerator if stride is not multiple of 64

[Mikulas Patocka]

On my card, the accelerator corrupts display if its line length is not
multiple of 64 bytes.

Misaligned lines in the drawn recrangle are shifted left, as if the accelerator
thought that every line begins on 64-byte boundary. For example, in 800x600x8
(scanlines are aligned on 32 bytes) every odd scanline is drawn shifted by
32 bytes to the left.

The card is ATI Technologies Inc 3D Rage Pro 215GP (rev 5c) onboard on
Sparc64 Ultra 5.

This patch disables accelerator if scanline is not multiple of 64 bytes.

I don't have any other cards to test. If someone tests that his card doesn't
have this bug, he can whitelist it.

(note: if you find a better place where to disable the accelerator on 
non-conforming modes, do it there)

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com

---
 drivers/video/aty/mach64_accel.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Index: linux-2.6.30-fast/drivers/video/aty/mach64_accel.c
===================================================================
--- linux-2.6.30-fast.orig/drivers/video/aty/mach64_accel.c	2009-06-17 08:49:01.000000000 +0200
+++ linux-2.6.30-fast/drivers/video/aty/mach64_accel.c	2009-06-17 10:46:03.000000000 +0200
@@ -35,6 +35,14 @@ static u32 rotation24bpp(u32 dx, u32 dir
 	return ((rotation << 8) | DST_24_ROTATION_ENABLE);
 }
 
+static inline int no_accel(struct fb_info *info)
+{
+	/*
+	 * The lines must be aligned on 64-byte boundary.
+	 */
+	return (info->var.xres_virtual * ((info->var.bits_per_pixel + 7) >> 3)) & 63;
+}
+
 void aty_reset_engine(const struct atyfb_par *par)
 {
 	/* reset engine */
@@ -199,7 +207,7 @@ void atyfb_copyarea(struct fb_info *info
 		return;
 	if (!area->width || !area->height)
 		return;
-	if (!par->accel_flags) {
+	if (!par->accel_flags || no_accel(info)) {
 		cfb_copyarea(info, area);
 		return;
 	}
@@ -245,7 +253,7 @@ void atyfb_fillrect(struct fb_info *info
 		return;
 	if (!rect->width || !rect->height)
 		return;
-	if (!par->accel_flags) {
+	if (!par->accel_flags || no_accel(info)) {
 		cfb_fillrect(info, rect);
 		return;
 	}
@@ -285,7 +293,7 @@ void atyfb_imageblit(struct fb_info *inf
 		return;
 	if (!image->width || !image->height)
 		return;
-	if (!par->accel_flags ||
+	if (!par->accel_flags || no_accel(info) ||
 	    (image->depth != 1 && info->var.bits_per_pixel != image->depth)) {
 		cfb_imageblit(info, image);
 		return;


------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing 
server and web deployment.
http://p.sf.net/sfu/businessobjects

Re: [PATCH] Disable mach64 accelerator if stride is not multiple of 64

[Ville Syrjälä]

On Wed, Jun 17, 2009 at 06:02:30AM -0400, Mikulas Patocka wrote:
> On my card, the accelerator corrupts display if its line length is not
> multiple of 64 bytes.
> 
> Misaligned lines in the drawn recrangle are shifted left, as if the accelerator
> thought that every line begins on 64-byte boundary. For example, in 800x600x8
> (scanlines are aligned on 32 bytes) every odd scanline is drawn shifted by
> 32 bytes to the left.
> 
> The card is ATI Technologies Inc 3D Rage Pro 215GP (rev 5c) onboard on
> Sparc64 Ultra 5.

I think normally the offset should be just aligned to 8 bytes and pitch
to 8 bytes and 8 pixels.

I was under the impression that 64 byte aligned offset and pitch are only
required if block writes are used. But perhaps it is always required
with SGRAM/WRAM, or perhaps you have auto block writes enabled. Can you
check the memory type and the the value of the HW_DEBUG register?

> This patch disables accelerator if scanline is not multiple of 64 bytes.

A better option would be to round the pitch up in check_var(). Also it
would be more proper to use fix.line_length as the pitch value instead
of var.xres_virtual. That way check_var() could leave var.xres_virtual
alone and just round fix.line_length to whatever is required.

Actually now that I think about it I would prefer to round to 64 bytes
always when SGRAM/WRAM is found. The reason is that the mach64 DirectFB
driver always enables auto block writes on SGRAM boards and if atyfb
uses less alignment things will not work correctly. Perhaps atyfb should
enable auto block writes too.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing 
server and web deployment.
http://p.sf.net/sfu/businessobjects

Re: [PATCH] Disable mach64 accelerator if stride is not multiple of 64

[Mikulas Patocka]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2562 bytes --]



On Wed, 17 Jun 2009, Ville Syrjälä wrote:

> On Wed, Jun 17, 2009 at 06:02:30AM -0400, Mikulas Patocka wrote:
> > On my card, the accelerator corrupts display if its line length is not
> > multiple of 64 bytes.
> > 
> > Misaligned lines in the drawn recrangle are shifted left, as if the accelerator
> > thought that every line begins on 64-byte boundary. For example, in 800x600x8
> > (scanlines are aligned on 32 bytes) every odd scanline is drawn shifted by
> > 32 bytes to the left.
> > 
> > The card is ATI Technologies Inc 3D Rage Pro 215GP (rev 5c) onboard on
> > Sparc64 Ultra 5.
> 
> I think normally the offset should be just aligned to 8 bytes and pitch
> to 8 bytes and 8 pixels.
> 
> I was under the impression that 64 byte aligned offset and pitch are only
> required if block writes are used. But perhaps it is always required
> with SGRAM/WRAM, or perhaps you have auto block writes enabled. Can you
> check the memory type and the the value of the HW_DEBUG register?

Memory is SGRAM M32L1632512A-8Q. See this: 
http://www.digchip.com/datasheets/parts/datasheet/581/M32L1632512A.php

The status line is:
atyfb: 3D RAGE PRO (Mach64 GP, PQFP, PCI) [0x4750 rev 0x7c]
atyfb: 4M SGRAM (1:1), 14.31818 MHz XTAL, 230 MHz PLL, 100 Mhz MCLK, 100 
MHz XCLK

The debug register is zero (printed with printk("ATI INIT ENGINE, DEBUG 
%08X", aty_ld_le32(HW_DEBUG, par)); at the end of aty_init_engine).

> > This patch disables accelerator if scanline is not multiple of 64 bytes.
> 
> A better option would be to round the pitch up in check_var(). Also it
> would be more proper to use fix.line_length as the pitch value instead
> of var.xres_virtual. That way check_var() could leave var.xres_virtual
> alone and just round fix.line_length to whatever is required.
> 
> Actually now that I think about it I would prefer to round to 64 bytes
> always when SGRAM/WRAM is found. The reason is that the mach64 DirectFB
> driver always enables auto block writes on SGRAM boards and if atyfb
> uses less alignment things will not work correctly. Perhaps atyfb should
> enable auto block writes too.

So patch it to align line length (I don't know fb well to pick the right 
place) and send me the patch and I'll test it.

BTW. why doesn't it use accelerator for screen scrolling? It seems to 
redraw all non-empty characters, which is fast if most of the screen is 
empty but gets slower if the screen is full of text.

Mikulas

> -- 
> Ville Syrjälä
> syrjala@sci.fi
> http://www.sci.fi/~syrjala/
> 

[-- Attachment #2: Type: text/plain, Size: 332 bytes --]

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing 
server and web deployment.
http://p.sf.net/sfu/businessobjects

[-- Attachment #3: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

Re: [PATCH] Disable mach64 accelerator if stride is not multiple of 64

[Ville Syrjälä]

On Wed, Jun 17, 2009 at 05:05:31PM -0400, Mikulas Patocka wrote:
> On Wed, 17 Jun 2009, Ville Syrjälä wrote:
> 
> > I was under the impression that 64 byte aligned offset and pitch are only
> > required if block writes are used. But perhaps it is always required
> > with SGRAM/WRAM, or perhaps you have auto block writes enabled. Can you
> > check the memory type and the the value of the HW_DEBUG register?
> 
> Memory is SGRAM M32L1632512A-8Q. See this: 
> http://www.digchip.com/datasheets/parts/datasheet/581/M32L1632512A.php
> 
> The status line is:
> atyfb: 3D RAGE PRO (Mach64 GP, PQFP, PCI) [0x4750 rev 0x7c]
> atyfb: 4M SGRAM (1:1), 14.31818 MHz XTAL, 230 MHz PLL, 100 Mhz MCLK, 100 
> MHz XCLK
> 
> The debug register is zero (printed with printk("ATI INIT ENGINE, DEBUG 
> %08X", aty_ld_le32(HW_DEBUG, par)); at the end of aty_init_engine).

OK then it has auto block writes enabled.

> > > This patch disables accelerator if scanline is not multiple of 64 bytes.
> > 
> > A better option would be to round the pitch up in check_var(). Also it
> > would be more proper to use fix.line_length as the pitch value instead
> > of var.xres_virtual. That way check_var() could leave var.xres_virtual
> > alone and just round fix.line_length to whatever is required.
> > 
> > Actually now that I think about it I would prefer to round to 64 bytes
> > always when SGRAM/WRAM is found. The reason is that the mach64 DirectFB
> > driver always enables auto block writes on SGRAM boards and if atyfb
> > uses less alignment things will not work correctly. Perhaps atyfb should
> > enable auto block writes too.
> 
> So patch it to align line length (I don't know fb well to pick the right 
> place) and send me the patch and I'll test it.

I cooked up a patch and briefly tested it on my Dell Inspiron 7000
which has an LT Pro /w SGRAM. I will send it as part of a series now.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Are you an open source citizen? Join us for the Open Source Bridge conference!
Portland, OR, June 17-19. Two days of sessions, one day of unconference: $250.
Need another reason to go? 24-hour hacker lounge. Register today!
http://ad.doubleclick.net/clk;215844324;13503038;v?http://opensourcebridge.org

Re: [PATCH] Fix colors in mach64 accelerator

[Ville Syrjälä]

On Wed, Jun 17, 2009 at 06:00:53AM -0400, Mikulas Patocka wrote:
> Use the same color-calculating algorithm as in atyfb_imageblit in this driver
> or in generic cfb_fillrect.
> 
> This patch fixes bad colors when using an accelerator in 15-bit and 16-bit
> modes.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com

Acked-by: Ville Syrjala <syrjala@sci.fi>

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing 
server and web deployment.
http://p.sf.net/sfu/businessobjects

Re: [PATCH] Fix colors in mach64 accelerator

[Krzysztof Helt]

On Wed, 17 Jun 2009 06:00:53 -0400 (EDT)
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Use the same color-calculating algorithm as in atyfb_imageblit in this driver
> or in generic cfb_fillrect.
> 
> This patch fixes bad colors when using an accelerator in 15-bit and 16-bit
> modes.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com
> 

Acked-by: Krzysztof Helt <krzysztof.h1@wp.pl>

----------------------------------------------------------------------
Masz juz dosc wysokich rachunkow? Wygraj nowa mozliwosc komunikacji!
Sprawdz >>> http://link.interia.pl/f21d2


------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing 
server and web deployment.
http://p.sf.net/sfu/businessobjects