Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses

Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses

[Benjamin Herrenschmidt]

On Fri, 2005-04-08 at 01:58 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > Yes, that's very extreme, I suspect somebody is banging on set_par or
> > something like that.
> 
> fb_setcolreg is it.

Ok, what about that patch:

---

This patch adds to the fbdev interface a set_cmap callback that allow
the driver to "batch" palette changes. This is useful for drivers like
radeonfb which might require lenghtly workarounds on palette accesses,
thus allowing to factor out those workarounds efficiently.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/include/linux/fb.h
===================================================================
--- linux-work.orig/include/linux/fb.h	2005-04-01 09:04:19.000000000 +1000
+++ linux-work/include/linux/fb.h	2005-04-08 10:24:56.000000000 +1000
@@ -563,6 +563,9 @@
 	int (*fb_setcolreg)(unsigned regno, unsigned red, unsigned green,
 			    unsigned blue, unsigned transp, struct fb_info *info);
 
+	/* set color registers in batch */
+	int (*fb_setcmap)(struct fb_cmap *cmap, struct fb_info *info);
+
 	/* blank display */
 	int (*fb_blank)(int blank, struct fb_info *info);
 
Index: linux-work/drivers/video/aty/radeon_base.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_base.c	2005-04-01 09:04:18.000000000 +1000
+++ linux-work/drivers/video/aty/radeon_base.c	2005-04-08 11:15:13.000000000 +1000
@@ -1057,13 +1057,14 @@
 	return radeon_screen_blank(rinfo, blank, 0);
 }
 
-static int radeonfb_setcolreg (unsigned regno, unsigned red, unsigned green,
-                             unsigned blue, unsigned transp, struct fb_info *info)
+static int radeon_setcolreg (unsigned regno, unsigned red, unsigned green,
+                             unsigned blue, unsigned transp,
+			     struct radeonfb_info *rinfo)
 {
-        struct radeonfb_info *rinfo = info->par;
 	u32 pindex;
 	unsigned int i;
-	
+
+
 	if (regno > 255)
 		return 1;
 
@@ -1078,20 +1079,7 @@
         pindex = regno;
 
         if (!rinfo->asleep) {
-        	u32 dac_cntl2, vclk_cntl = 0;
-        	
 		radeon_fifo_wait(9);
-		if (rinfo->is_mobility) {
-			vclk_cntl = INPLL(VCLK_ECP_CNTL);
-			OUTPLL(VCLK_ECP_CNTL, vclk_cntl & ~PIXCLK_DAC_ALWAYS_ONb);
-		}
-
-		/* Make sure we are on first palette */
-		if (rinfo->has_CRTC2) {
-			dac_cntl2 = INREG(DAC_CNTL2);
-			dac_cntl2 &= ~DAC2_PALETTE_ACCESS_CNTL;
-			OUTREG(DAC_CNTL2, dac_cntl2);
-		}
 
 		if (rinfo->bpp == 16) {
 			pindex = regno * 8;
@@ -1101,24 +1089,27 @@
 			if (rinfo->depth == 15 && regno > 31)
 				return 1;
 
-			/* For 565, the green component is mixed one order below */
+			/* For 565, the green component is mixed one order
+			 * below
+			 */
 			if (rinfo->depth == 16) {
 		                OUTREG(PALETTE_INDEX, pindex>>1);
-	       	         	OUTREG(PALETTE_DATA, (rinfo->palette[regno>>1].red << 16) |
-	                        	(green << 8) | (rinfo->palette[regno>>1].blue));
+	       	         	OUTREG(PALETTE_DATA,
+				       (rinfo->palette[regno>>1].red << 16) |
+	                        	(green << 8) |
+				       (rinfo->palette[regno>>1].blue));
 	                	green = rinfo->palette[regno<<1].green;
 	        	}
 		}
 
 		if (rinfo->depth != 16 || regno < 32) {
 			OUTREG(PALETTE_INDEX, pindex);
-			OUTREG(PALETTE_DATA, (red << 16) | (green << 8) | blue);
+			OUTREG(PALETTE_DATA, (red << 16) |
+			       (green << 8) | blue);
 		}
-		if (rinfo->is_mobility)
-			OUTPLL(VCLK_ECP_CNTL, vclk_cntl);
 	}
  	if (regno < 16) {
-		u32 *pal = info->pseudo_palette;
+		u32 *pal = rinfo->info->pseudo_palette;
         	switch (rinfo->depth) {
 		case 15:
 			pal[regno] = (regno << 10) | (regno << 5) | regno;
@@ -1138,6 +1129,84 @@
 	return 0;
 }
 
+static int radeonfb_setcolreg (unsigned regno, unsigned red, unsigned green,
+			       unsigned blue, unsigned transp,
+			       struct fb_info *info)
+{
+        struct radeonfb_info *rinfo = info->par;
+	u32 dac_cntl2, vclk_cntl = 0;
+	int rc;
+
+        if (!rinfo->asleep) {
+		if (rinfo->is_mobility) {
+			vclk_cntl = INPLL(VCLK_ECP_CNTL);
+			OUTPLL(VCLK_ECP_CNTL,
+			       vclk_cntl & ~PIXCLK_DAC_ALWAYS_ONb);
+		}
+
+		/* Make sure we are on first palette */
+		if (rinfo->has_CRTC2) {
+			dac_cntl2 = INREG(DAC_CNTL2);
+			dac_cntl2 &= ~DAC2_PALETTE_ACCESS_CNTL;
+			OUTREG(DAC_CNTL2, dac_cntl2);
+		}
+	}
+
+	rc = radeon_setcolreg (regno, red, green, blue, transp, rinfo);
+	
+	if (!rinfo->asleep && rinfo->is_mobility)
+		OUTPLL(VCLK_ECP_CNTL, vclk_cntl);
+
+	return rc;
+}
+
+static int radeonfb_setcmap(struct fb_cmap *cmap, struct fb_info *info)
+{
+        struct radeonfb_info *rinfo = info->par;
+	u16 *red, *green, *blue, *transp;
+	u32 dac_cntl2, vclk_cntl = 0;
+	int i, start, rc = 0;
+
+        if (!rinfo->asleep) {
+		if (rinfo->is_mobility) {
+			vclk_cntl = INPLL(VCLK_ECP_CNTL);
+			OUTPLL(VCLK_ECP_CNTL,
+			       vclk_cntl & ~PIXCLK_DAC_ALWAYS_ONb);
+		}
+
+		/* Make sure we are on first palette */
+		if (rinfo->has_CRTC2) {
+			dac_cntl2 = INREG(DAC_CNTL2);
+			dac_cntl2 &= ~DAC2_PALETTE_ACCESS_CNTL;
+			OUTREG(DAC_CNTL2, dac_cntl2);
+		}
+	}
+
+	red = cmap->red;
+	green = cmap->green;
+	blue = cmap->blue;
+	transp = cmap->transp;
+	start = cmap->start;
+
+	for (i = 0; i < cmap->len; i++) {
+		u_int hred, hgreen, hblue, htransp = 0xffff;
+
+		hred = *red++;
+		hgreen = *green++;
+		hblue = *blue++;
+		if (transp)
+			htransp = *transp++;
+		rc = radeon_setcolreg (start++, hred, hgreen, hblue, htransp,
+				       rinfo);
+		if (rc)
+			break;
+	}
+	
+	if (!rinfo->asleep && rinfo->is_mobility)
+		OUTPLL(VCLK_ECP_CNTL, vclk_cntl);
+
+	return rc;
+}
 
 static void radeon_save_state (struct radeonfb_info *rinfo,
 			       struct radeon_regs *save)
@@ -1796,6 +1865,7 @@
 	.fb_check_var		= radeonfb_check_var,
 	.fb_set_par		= radeonfb_set_par,
 	.fb_setcolreg		= radeonfb_setcolreg,
+	.fb_setcmap		= radeonfb_setcmap,
 	.fb_pan_display 	= radeonfb_pan_display,
 	.fb_blank		= radeonfb_blank,
 	.fb_ioctl		= radeonfb_ioctl,
Index: linux-work/drivers/video/fbcmap.c
===================================================================
--- linux-work.orig/drivers/video/fbcmap.c	2005-03-15 11:58:33.000000000 +1100
+++ linux-work/drivers/video/fbcmap.c	2005-04-08 11:14:51.000000000 +1000
@@ -222,8 +222,11 @@
 	transp = cmap->transp;
 	start = cmap->start;
 
-	if (start < 0 || !info->fbops->fb_setcolreg)
+	if (start < 0 || (!info->fbops->fb_setcolreg &&
+			  !info->fbops->fb_setcmap))
 		return -EINVAL;
+	if (info->fbops->fb_setcmap)
+		return info->fbops->fb_setcmap(cmap, info);
 	for (i = 0; i < cmap->len; i++) {
 		hred = *red++;
 		hgreen = *green++;
@@ -243,15 +246,40 @@
 	int i, start;
 	u16 __user *red, *green, *blue, *transp;
 	u_int hred, hgreen, hblue, htransp = 0xffff;
-
+	
 	red = cmap->red;
 	green = cmap->green;
 	blue = cmap->blue;
 	transp = cmap->transp;
 	start = cmap->start;
 
-	if (start < 0 || !info->fbops->fb_setcolreg)
+	if (start < 0 || (!info->fbops->fb_setcolreg &&
+			  !info->fbops->fb_setcmap))
 		return -EINVAL;
+	
+	/* If we can batch, do it */
+	if (info->fbops->fb_setcmap && cmap->len > 1) {
+		struct fb_cmap umap;
+		int size = cmap->len * sizeof(u16);
+		int rc;
+
+		memset(&umap, 0, sizeof(struct fb_cmap));
+		rc = fb_alloc_cmap(&umap, cmap->len, transp != NULL);
+		if (rc)
+			return rc;
+		if (copy_from_user(umap.red, red, size) ||
+		    copy_from_user(umap.green, green, size) ||
+		    copy_from_user(umap.blue, blue, size) ||
+		    (transp && copy_from_user(umap.transp, transp, size))) {
+			rc = -EFAULT;
+		}
+		umap.start = start;
+		if (rc == 0)
+			rc = info->fbops->fb_setcmap(&umap, info);
+		fb_dealloc_cmap(&umap);
+		return rc;
+	}
+
 	for (i = 0; i < cmap->len; i++, red++, blue++, green++) {
 		if (get_user(hred, red) ||
 		    get_user(hgreen, green) ||




-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Fri, 2005-04-08 at 01:58 +0200, Andreas Schwab wrote:
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> 
>> > Yes, that's very extreme, I suspect somebody is banging on set_par or
>> > something like that.
>> 
>> fb_setcolreg is it.
>
> Ok, what about that patch:
>
> ---
>
> This patch adds to the fbdev interface a set_cmap callback that allow
> the driver to "batch" palette changes. This is useful for drivers like
> radeonfb which might require lenghtly workarounds on palette accesses,
> thus allowing to factor out those workarounds efficiently.

This makes it better. But there is still a delay of half a second, and
there is an annoying flicker when switching from X to the console.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses

[Benjamin Herrenschmidt]

> > This patch adds to the fbdev interface a set_cmap callback that allow
> > the driver to "batch" palette changes. This is useful for drivers like
> > radeonfb which might require lenghtly workarounds on palette accesses,
> > thus allowing to factor out those workarounds efficiently.
> 
> This makes it better. But there is still a delay of half a second, and
> there is an annoying flicker when switching from X to the console.

Can you redo the counting of the workarounds with the patch ?

Ben.




-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses

[Andreas Schwab]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> Can you redo the counting of the workarounds with the patch ?

Switching from X to console:

radeon_write_pll_regs: INPLL
radeon_write_pll_regs: INPLL
radeon_write_mode: OUTPLL
radeonfb_engine_reset: INPLL
radeonfb_engine_reset: OUTPLL
radeonfb_engine_reset: OUTPLL
radeonfb_setcmap: INPLL
radeonfb_setcmap: OUTPLL
radeonfb_setcmap: OUTPLL
radeon_write_pll_regs: INPLL
radeon_write_pll_regs: INPLL
radeon_write_mode: OUTPLL
radeonfb_engine_reset: INPLL
radeonfb_engine_reset: OUTPLL
radeonfb_engine_reset: OUTPLL
radeonfb_setcmap: INPLL
radeonfb_setcmap: OUTPLL
radeonfb_setcmap: OUTPLL
radeonfb_setcmap: INPLL
radeonfb_setcmap: OUTPLL
radeonfb_setcmap: OUTPLL
radeonfb_setcmap: INPLL
radeonfb_setcmap: OUTPLL
radeonfb_setcmap: OUTPLL
radeonfb_setcmap: INPLL
radeonfb_setcmap: OUTPLL

Switching from console to X:

radeonfb_setcmap: OUTPLL
radeon_write_pll_regs: INPLL
radeon_write_pll_regs: INPLL
radeon_write_mode: OUTPLL
radeonfb_engine_reset: INPLL
radeonfb_engine_reset: OUTPLL
radeonfb_engine_reset: OUTPLL
radeonfb_setcmap: INPLL
radeonfb_setcmap: OUTPLL
radeonfb_setcmap: OUTPLL
agpgart: Putting AGP V2 device at 0000:00:0b.0 into 1x mode
agpgart: Putting AGP V2 device at 0000:00:10.0 into 1x mode
radeonfb_setcolreg: INPLL
radeonfb_setcolreg: OUTPLL
radeonfb_setcolreg: OUTPLL
... last three lines repeated 63 times

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses

[Benjamin Herrenschmidt]

On Sat, 2005-04-09 at 18:24 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > Can you redo the counting of the workarounds with the patch ?
> 
> Switching from X to console:
> 
> radeon_write_pll_regs: INPLL
> radeon_write_pll_regs: INPLL
> radeon_write_mode: OUTPLL
> radeonfb_engine_reset: INPLL
> radeonfb_engine_reset: OUTPLL
> radeonfb_engine_reset: OUTPLL
> radeonfb_setcmap: INPLL
> radeonfb_setcmap: OUTPLL
> radeonfb_setcmap: OUTPLL
> radeon_write_pll_regs: INPLL
> radeon_write_pll_regs: INPLL
> radeon_write_mode: OUTPLL
> radeonfb_engine_reset: INPLL
> radeonfb_engine_reset: OUTPLL
> radeonfb_engine_reset: OUTPLL
> radeonfb_setcmap: INPLL
> radeonfb_setcmap: OUTPLL
> radeonfb_setcmap: OUTPLL
> radeonfb_setcmap: INPLL
> radeonfb_setcmap: OUTPLL
> radeonfb_setcmap: OUTPLL
> radeonfb_setcmap: INPLL
> radeonfb_setcmap: OUTPLL
> radeonfb_setcmap: OUTPLL
> radeonfb_setcmap: INPLL
> radeonfb_setcmap: OUTPLL

Ok, so the above is interesting, something is callign setcmap way too
much here I would say... Besides, it looks like set_par is called twice,
which is wrong too.

> Switching from console to X:
> 
> radeonfb_setcmap: OUTPLL
> radeon_write_pll_regs: INPLL
> radeon_write_pll_regs: INPLL
> radeon_write_mode: OUTPLL
> radeonfb_engine_reset: INPLL
> radeonfb_engine_reset: OUTPLL
> radeonfb_engine_reset: OUTPLL
> radeonfb_setcmap: INPLL
> radeonfb_setcmap: OUTPLL
> radeonfb_setcmap: OUTPLL
> agpgart: Putting AGP V2 device at 0000:00:0b.0 into 1x mode
> agpgart: Putting AGP V2 device at 0000:00:10.0 into 1x mode
> radeonfb_setcolreg: INPLL
> radeonfb_setcolreg: OUTPLL
> radeonfb_setcolreg: OUTPLL
> ... last three lines repeated 63 times

Hrm... the last (serie of 64 setcolreg) are probably X beeing extremely
dumb, and calling the ioctl 64 times to set each palette entry instead
of doing a single call for the whole palette...

Anyway. Except for maybe the double set-par on switch from X to console,
there isn't much more we can do here. We might be able to improve X but
there is a significant lag between a fix done to X.org HEAD appears in
any distro. The fact is, according to ATI, there is a HW bug on M6 taht
can cause lockups of the chip, and this 5ms workaround is necessary to
avoid it... 

Ben.




-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses

[Moritz Muehlenhoff]

Benjamin Herrenschmidt wrote:
>> radeonfb_setcolreg: INPLL
>> radeonfb_setcolreg: OUTPLL
>> radeonfb_setcolreg: OUTPLL
>> ... last three lines repeated 63 times
>
> Hrm... the last (serie of 64 setcolreg) are probably X beeing extremely
> dumb, and calling the ioctl 64 times to set each palette entry instead
> of doing a single call for the whole palette...
>
> Anyway. Except for maybe the double set-par on switch from X to console,
> there isn't much more we can do here. We might be able to improve X but
> there is a significant lag between a fix done to X.org HEAD appears in
> any distro. The fact is, according to ATI, there is a HW bug on M6 taht
> can cause lockups of the chip, and this 5ms workaround is necessary to
> avoid it... 

But it's not specific to X11; I've applied the patch you posted and the
same symptoms occur for pure tty switching as well, the delay has decreased
a bit (it's hard to measure, but around a second), but it's still rather
annoying to work with.

Is it distinguishable which M6 models are buggy? I'm using my X31 for about
a year now and have probably made some tens of thousands of switches without
lockups, so presumably not all models cause lockups.

Cheers,
        Moritz

Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses

[Benjamin Herrenschmidt]

> But it's not specific to X11; I've applied the patch you posted and the
> same symptoms occur for pure tty switching as well, the delay has decreased
> a bit (it's hard to measure, but around a second), but it's still rather
> annoying to work with.
> 
> Is it distinguishable which M6 models are buggy? I'm using my X31 for about
> a year now and have probably made some tens of thousands of switches without
> lockups, so presumably not all models cause lockups.

ATI hasn't been very precise about that unfortunately...

Ben.