[PATCH] radeonfb: copyarea() "memmove()" problem

[PATCH] radeonfb: copyarea() "memmove()" problem

[David Eger]

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


Now that my local kernel actually calls copyarea(), I see that my code was
buggy... That is, the hardware needs to be told which way to do its memory
copies if areas overlap.  Think of memmove() versus memcpy().  
Attached is the fix.

Unfortunately, I think the fbcon layer is also buggy... 
(try opening a file with vim, and typing "5ddP"  the copyarea()'s that 
get sent to the fb driver seem totally bogus...)

-dte

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1054 bytes --]

--- bk-linuxppc-2.6-benh/drivers/video/aty/radeon_accel.c	2004-03-29 16:37:41.000000000 +0200
+++ linux/drivers/video/aty/radeon_accel.c	2004-03-29 17:06:43.000000000 +0200
@@ -53,14 +53,27 @@
 static void radeonfb_prim_copyarea(struct radeonfb_info *rinfo, 
 				   const struct fb_copyarea *area)
 {
+	int xdir, ydir;
+	u32 sx, sy, dx, dy, w, h;
+
+	w = area->width; h = area->height;
+	dx = area->dx; dy = area->dy;
+	sx = area->sx; sy = area->sy;
+	xdir = sx - dx;
+	ydir = sy - dy;
+
+	if ( xdir < 0 ) { sx += w-1; dx += w-1; }
+	if ( ydir < 0 ) { sy += h-1; dy += h-1; }
+
 	radeon_fifo_wait(3);
 	OUTREG(DP_GUI_MASTER_CNTL,
 		rinfo->dp_gui_master_cntl /* i.e. GMC_DST_32BPP */
 		| GMC_SRC_DSTCOLOR
 		| ROP3_S 
 		| DP_SRC_RECT );
-	OUTREG(DP_WRITE_MSK, 0xffffffff);
-	OUTREG(DP_CNTL, (DST_X_LEFT_TO_RIGHT | DST_Y_TOP_TO_BOTTOM));
+	OUTREG(DP_CNTL, 
+	        (xdir>=0 ? DST_X_LEFT_TO_RIGHT : 0)
+			| (ydir>=0 ? DST_Y_TOP_TO_BOTTOM : 0));
 
 	radeon_fifo_wait(3);
 	OUTREG(SRC_Y_X, (area->sy << 16) | area->sx);

Re: [PATCH] radeonfb: copyarea() "memmove()" problem

[Geert Uytterhoeven]

	Hi David,

On Mon, 29 Mar 2004, David Eger wrote:
> Now that my local kernel actually calls copyarea(), I see that my code was
> buggy... That is, the hardware needs to be told which way to do its memory
> copies if areas overlap.  Think of memmove() versus memcpy().
> Attached is the fix.

Do you know if there's a noticable performance difference between forward and
backward copies? If yes, you may want to check whether the areas really overlap
instead of testing their relative positions.

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


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: [PATCH] radeonfb: copyarea() "memmove()" problem

[Benjamin Herrenschmidt]

On Tue, 2004-03-30 at 01:13, David Eger wrote:
> Now that my local kernel actually calls copyarea(), I see that my code was
> buggy... That is, the hardware needs to be told which way to do its memory
> copies if areas overlap.  Think of memmove() versus memcpy().  
> Attached is the fix.
> 
> Unfortunately, I think the fbcon layer is also buggy... 
> (try opening a file with vim, and typing "5ddP"  the copyarea()'s that 
> get sent to the fb driver seem totally bogus...)

Excellent. James, any clue about the copyarea issue ?

Ben.




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: [PATCH] radeonfb: copyarea() "memmove()" problem

[James Simmons]

> > Now that my local kernel actually calls copyarea(), I see that my code was
> > buggy... That is, the hardware needs to be told which way to do its memory
> > copies if areas overlap.  Think of memmove() versus memcpy().  
> > Attached is the fix.
> > 
> > Unfortunately, I think the fbcon layer is also buggy... 
> > (try opening a file with vim, and typing "5ddP"  the copyarea()'s that 
> > get sent to the fb driver seem totally bogus...)
> 
> Excellent. James, any clue about the copyarea issue ?

I have seen this before. The issue comes you when we have over lapping 
areas. The direction you draw in is important. I have a patch that I 
tested. Give this a shot.


diff -urN -X /home/jsimmons/dontdiff fbdev-2.6/drivers/video/aty/radeon_accel.c fbdev-2.5/drivers/video/aty/radeon_accel.c
--- fbdev-2.6/drivers/video/aty/radeon_accel.c	2004-04-02 15:32:04.566209515 -0800
+++ fbdev-2.5/drivers/video/aty/radeon_accel.c	2004-04-02 15:29:06.337306396 -0800
@@ -53,6 +53,9 @@
 static void radeonfb_prim_copyarea(struct radeonfb_info *rinfo, 
 				   const struct fb_copyarea *area)
 {
+	u32 sx = area->sx, sy = area->sy, dx = area->dx, dy = area->dy;
+	u32 flag;
+
 	radeon_fifo_wait(3);
 	OUTREG(DP_GUI_MASTER_CNTL,
 		rinfo->dp_gui_master_cntl /* i.e. GMC_DST_32BPP */
@@ -60,7 +63,24 @@
 		| ROP3_S 
 		| DP_SRC_RECT );
 	OUTREG(DP_WRITE_MSK, 0xffffffff);
-	OUTREG(DP_CNTL, (DST_X_LEFT_TO_RIGHT | DST_Y_TOP_TO_BOTTOM));
+
+	if (sy < dy) {
+		sy += (area->height - 1);
+		dy += (area->height - 1);
+
+		flag |= DST_Y_BOTTOM_TO_TOP;
+	} else
+		flag |= DST_Y_TOP_TO_BOTTOM;
+
+	if (area->sx < area->dx) {
+		sx += (area->width - 1);
+		dx += (area->width - 1);
+
+		flag |= DST_X_LEFT_TO_RIGHT;
+	} else
+		flag |= DST_X_RIGHT_TO_LEFT;
+
+	OUTREG(DP_CNTL, flag);
 
 	radeon_fifo_wait(3);
 	OUTREG(SRC_Y_X, (area->sy << 16) | area->sx);
diff -urN -X /home/jsimmons/dontdiff fbdev-2.6/drivers/video/aty/radeonfb.h fbdev-2.5/drivers/video/aty/radeonfb.h
--- fbdev-2.6/drivers/video/aty/radeonfb.h	2004-04-02 15:38:57.115407055 -0800
+++ fbdev-2.5/drivers/video/aty/radeonfb.h	2004-04-02 15:35:53.237300302 -0800
@@ -295,8 +295,6 @@
 	struct panel_info	panel_info;
 	int			mon1_type;
 	u8			*mon1_EDID;
-	struct fb_videomode	*mon1_modedb;
-	int			mon1_dbsize;
 	int			mon2_type;
 	u8		        *mon2_EDID;
 
diff -urN -X /home/jsimmons/dontdiff fbdev-2.6/drivers/video/aty/radeon_monitor.c fbdev-2.5/drivers/video/aty/radeon_monitor.c
--- fbdev-2.6/drivers/video/aty/radeon_monitor.c	2004-04-02 15:31:42.854142518 -0800
+++ fbdev-2.5/drivers/video/aty/radeon_monitor.c	2004-04-02 12:09:07.000000000 -0800
@@ -804,12 +804,8 @@
 	/*
 	 * Now build modedb from EDID
 	 */
-	if (rinfo->mon1_EDID) {
-		rinfo->mon1_modedb = fb_create_modedb(rinfo->mon1_EDID,
-						      &rinfo->mon1_dbsize);
+	if (rinfo->mon1_EDID)
 		fb_get_monitor_limits(rinfo->mon1_EDID, &rinfo->info->monspecs);
-	}
-
 	
 	/*
 	 * Finally, if we don't have panel infos we need to figure some (or
@@ -835,8 +831,8 @@
 		}
 		printk(KERN_WARNING "radeonfb: Assuming panel size %dx%d\n",
 		       rinfo->panel_info.xres, rinfo->panel_info.yres);
-		modedb = rinfo->mon1_modedb;
-		dbsize = rinfo->mon1_dbsize;
+		modedb = rinfo->info->monspecs.modedb;
+		dbsize = rinfo->info->monspecs.modedb_len;
 		snprintf(modename, 31, "%dx%d", rinfo->panel_info.xres, rinfo->panel_info.yres);
 		if (fb_find_mode(&rinfo->info->var, rinfo->info, modename,
 				 modedb, dbsize, NULL, 8) == 0) {
@@ -859,7 +855,7 @@
 		else
 			radeon_var_to_videomode(&default_mode, &radeonfb_default_var);
 		if (fb_find_mode(&rinfo->info->var, rinfo->info, mode_option,
-				 rinfo->mon1_modedb, rinfo->mon1_dbsize, &default_mode, 8) == 0)
+				 rinfo->info->monspecs.modedb, rinfo->info->monspecs.modedb_len, &default_mode, 8) == 0)
 			rinfo->info->var = radeonfb_default_var;
 	}
 
@@ -915,9 +911,9 @@
 	memcpy(dest, src, sizeof(struct fb_var_screeninfo));
 
 	/* Check if we have a modedb built from EDID */
-	if (rinfo->mon1_modedb) {
-		db = rinfo->mon1_modedb;
-		dbsize = rinfo->mon1_dbsize;
+	if (rinfo->info->monspecs.modedb) {
+		db = rinfo->info->monspecs.modedb;
+		dbsize = rinfo->info->monspecs.modedb_len;
 		native_db = 1;
 	}
 
@@ -935,7 +931,7 @@
 		 * 640x480-60, but I assume userland knows what it's doing here
 		 * (though I may be proven wrong...)
 		 */
-		if (has_rmx == 0 && rinfo->mon1_modedb)
+		if (has_rmx == 0 && rinfo->info->monspecs.modedb)
 			if (fb_validate_mode((struct fb_var_screeninfo *)src, rinfo->info))
 				return -EINVAL;
 		return 0;




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: [PATCH] radeonfb: copyarea() "memmove()" problem

[David Eger]

James says:
> try my patch, I think it will fix things!

Sorry for the delay James -- I have a paper due this wednesday that I've 
been working on all week ;-)

I tried your patch against BenH's tree.  

radeon_accel.c patched fine (the relevant file) but the other two patches
failed; I assume BenH had just merged those patches already...

Then I set fbcon to always enable use of the accel functions (as noted in 
a previous email).  The results weren't nice :-/

I'll try to figure out what's wrong when I have more time Thursday.  
Till then, my previous patch is about as close to good as we have :-/

-David

---
getting bounce messages from me? let me know and you'll go on my whitelist


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click

Re: [PATCH] radeonfb: copyarea() "memmove()" problem

[James Simmons]

> I tried your patch against BenH's tree.  
> 
> radeon_accel.c patched fine (the relevant file) but the other two patches
> failed; I assume BenH had just merged those patches already...

My patch had a bug in it which I can't figure out way yet.
 
> Then I set fbcon to always enable use of the accel functions (as noted in 
> a previous email).  The results weren't nice :-/

Do you still have the patch?
 
> I'll try to figure out what's wrong when I have more time Thursday.  
> Till then, my previous patch is about as close to good as we have :-/

I don't have teh specs for the radeon so I went by my programming 
experience on other chips.




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click