[PATCH] radeonfb: Implement proper workarounds for PLL accesses

[PATCH] radeonfb: Implement proper workarounds for PLL accesses

[Benjamin Herrenschmidt]

Hi !

After discussion with ATIs, it seems that the workarounds they initially
gave me were not completely correct.

This patch implements the proper ones, which includes sleeping in PLL
accesses, and thus requires the previous patch to make sure we do not
call unblank at interrupt time (unless oops_in_progress is set, in which
case I use an mdelay).

It also removes obsolete code that used to disable some power management
features in the accel init code.

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

Index: linux-work/drivers/video/aty/radeonfb.h
===================================================================
--- linux-work.orig/drivers/video/aty/radeonfb.h	2005-03-11 15:05:34.000000000 +1100
+++ linux-work/drivers/video/aty/radeonfb.h	2005-03-11 15:42:19.000000000 +1100
@@ -77,6 +77,15 @@
 	CHIP_HAS_CRTC2		= 0x00040000UL,	
 };
 
+/*
+ * Errata workarounds
+ */
+enum radeon_errata {
+	CHIP_ERRATA_R300_CG		= 0x00000001,
+	CHIP_ERRATA_PLL_DUMMYREADS	= 0x00000002,
+	CHIP_ERRATA_PLL_DELAY		= 0x00000004,
+};
+
 
 /*
  * Monitor types
@@ -295,6 +304,7 @@
 	int			chipset;
 	u8			family;
 	u8			rev;
+	unsigned int		errata;
 	unsigned long		video_ram;
 	unsigned long		mapped_vram;
 	int			vram_width;
@@ -305,7 +315,6 @@
 	int			has_CRTC2;
 	int			is_mobility;
 	int			is_IGP;
-	int			R300_cg_workaround;
 	int			reversed_DAC;
 	int			reversed_TMDS;
 	struct panel_info	panel_info;
@@ -369,6 +378,21 @@
  * IO macros
  */
 
+/* Note about this function: we have some rare cases where we must not schedule,
+ * this typically happen with our special "wake up early" hook which allows us to
+ * wake up the graphic chip (and thus get the console back) before everything else
+ * on some machines that support that mecanism. At this point, interrupts are off
+ * and scheduling is not permitted
+ */
+static inline void _radeon_msleep(struct radeonfb_info *rinfo, unsigned long ms)
+{
+	if (rinfo->no_schedule || oops_in_progress)
+		mdelay(ms);
+	else
+		msleep(ms);
+}
+
+
 #define INREG8(addr)		readb((rinfo->mmio_base)+addr)
 #define OUTREG8(addr,val)	writeb(val, (rinfo->mmio_base)+addr)
 #define INREG(addr)		readl((rinfo->mmio_base)+addr)
@@ -390,107 +414,85 @@
 
 #define OUTREGP(addr,val,mask)	_OUTREGP(rinfo, addr, val,mask)
 
-/* This function is required to workaround a hardware bug in some (all?)
- * revisions of the R300.  This workaround should be called after every
- * CLOCK_CNTL_INDEX register access.  If not, register reads afterward
- * may not be correct.
- */
-static inline void R300_cg_workardound(struct radeonfb_info *rinfo)
-{
-	u32 save, tmp;
-	save = INREG(CLOCK_CNTL_INDEX);
-	tmp = save & ~(0x3f | PLL_WR_EN);
-	OUTREG(CLOCK_CNTL_INDEX, tmp);
-	tmp = INREG(CLOCK_CNTL_DATA);
-	OUTREG(CLOCK_CNTL_INDEX, save);
-}
-
 /*
- * PLL accesses suffer from various HW issues on the different chip
- * families. Some R300's need the above workaround, rv200 & friends
- * need a couple of dummy reads after any write of CLOCK_CNTL_INDEX,
- * and some RS100/200 need a dummy read before writing to
- * CLOCK_CNTL_INDEX as well. Instead of testing every chip revision,
- * we just unconditionally do  the workarounds at once since PLL
- * accesses are far from beeing performance critical. Except for R300
- * one which stays separate for now
+ * Note about PLL register accesses:
+ *
+ * I have removed the spinlock on them on purpose. The driver now
+ * expects that it will only manipulate the PLL registers in normal
+ * task environment, where radeon_msleep() will be called, protected
+ * by a semaphore (currently the console semaphore) so that no conflict
+ * will happen on the PLL register index.
+ *
+ * With the latest changes to the VT layer, this is guaranteed for all
+ * calls except the actual drawing/blits which aren't supposed to use
+ * the PLL registers anyway
+ *
+ * This is very important for the workarounds to work properly. The only
+ * possible exception to this rule is the call to unblank(), which may
+ * be done at irq time if an oops is in progress.
  */
-
-static inline void radeon_pll_workaround_before(struct radeonfb_info *rinfo)
+static inline void radeon_pll_errata_after_index(struct radeonfb_info *rinfo)
 {
+	if (!(rinfo->errata & CHIP_ERRATA_PLL_DUMMYREADS))
+		return;
+
+	(void)INREG(CLOCK_CNTL_DATA);
 	(void)INREG(CRTC_GEN_CNTL);
 }
 
-static inline void radeon_pll_workaround_after(struct radeonfb_info *rinfo)
+static inline void radeon_pll_errata_after_data(struct radeonfb_info *rinfo)
 {
-	(void)INREG(CLOCK_CNTL_DATA);
-	(void)INREG(CRTC_GEN_CNTL);
+	if (rinfo->errata & CHIP_ERRATA_PLL_DELAY) {
+		/* we can't deal with posted writes here ... */
+		_radeon_msleep(rinfo, 5);
+	}
+	if (rinfo->errata & CHIP_ERRATA_R300_CG) {
+		u32 save, tmp;
+		save = INREG(CLOCK_CNTL_INDEX);
+		tmp = save & ~(0x3f | PLL_WR_EN);
+		OUTREG(CLOCK_CNTL_INDEX, tmp);
+		tmp = INREG(CLOCK_CNTL_DATA);
+		OUTREG(CLOCK_CNTL_INDEX, save);
+	}
 }
 
 static inline u32 __INPLL(struct radeonfb_info *rinfo, u32 addr)
 {
 	u32 data;
 
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, addr & 0x0000003f);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	data = INREG(CLOCK_CNTL_DATA);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 	return data;
 }
 
-static inline u32 _INPLL(struct radeonfb_info *rinfo, u32 addr)
-{
-       	unsigned long flags;
-	u32 data;
-
-	spin_lock_irqsave(&rinfo->reg_lock, flags);
-	data = __INPLL(rinfo, addr);
-	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
-	return data;
-}
-
-#define INPLL(addr)		_INPLL(rinfo, addr)
-
-
 static inline void __OUTPLL(struct radeonfb_info *rinfo, unsigned int index,
 			    u32 val)
 {
 
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, (index & 0x0000003f) | 0x00000080);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG(CLOCK_CNTL_DATA, val);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 }
 
-static inline void _OUTPLL(struct radeonfb_info *rinfo, unsigned int index, u32 val)
-{
-       	unsigned long flags;
-	spin_lock_irqsave(&rinfo->reg_lock, flags);
-	__OUTPLL(rinfo, index, val);
-	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
-}
 
-static inline void _OUTPLLP(struct radeonfb_info *rinfo, unsigned int index,
-			    u32 val, u32 mask)
+static inline void __OUTPLLP(struct radeonfb_info *rinfo, unsigned int index,
+			     u32 val, u32 mask)
 {
-	unsigned long flags;
 	unsigned int tmp;
 
-	spin_lock_irqsave(&rinfo->reg_lock, flags);
 	tmp  = __INPLL(rinfo, index);
 	tmp &= (mask);
 	tmp |= (val);
 	__OUTPLL(rinfo, index, tmp);
-	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
 }
 
 
-#define OUTPLL(index, val)		_OUTPLL(rinfo, index, val)
-#define OUTPLLP(index, val, mask)	_OUTPLLP(rinfo, index, val, mask)
+#define INPLL(addr)			__INPLL(rinfo, addr)
+#define OUTPLL(index, val)		__OUTPLL(rinfo, index, val)
+#define OUTPLLP(index, val, mask)	__OUTPLLP(rinfo, index, val, mask)
 
 
 #define BIOS_IN8(v)  	(readb(rinfo->bios_seg + (v)))
@@ -582,20 +584,6 @@
 	printk(KERN_ERR "radeonfb: Idle Timeout !\n");
 }
 
-/* Note about this function: we have some rare cases where we must not schedule,
- * this typically happen with our special "wake up early" hook which allows us to
- * wake up the graphic chip (and thus get the console back) before everything else
- * on some machines that support that mecanism. At this point, interrupts are off
- * and scheduling is not permitted
- */
-static inline void _radeon_msleep(struct radeonfb_info *rinfo, unsigned long ms)
-{
-	if (rinfo->no_schedule)
-		mdelay(ms);
-	else
-		msleep(ms);
-}
-
 
 #define radeon_engine_idle()		_radeon_engine_idle(rinfo)
 #define radeon_fifo_wait(entries)	_radeon_fifo_wait(rinfo,entries)
Index: linux-work/drivers/video/aty/radeon_monitor.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_monitor.c	2005-03-11 15:05:34.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_monitor.c	2005-03-11 15:19:02.000000000 +1100
@@ -657,11 +657,8 @@
 	    && rinfo->is_mobility) {
 		int ppll_div_sel;
 		u32 ppll_divn;
-		radeon_pll_workaround_before(rinfo);
 		ppll_div_sel = INREG8(CLOCK_CNTL_INDEX + 1) & 0x3;
-		radeon_pll_workaround_after(rinfo);
-		if (rinfo->R300_cg_workaround)
-			R300_cg_workardound(rinfo);
+		radeon_pll_errata_after_index(rinfo);
 		ppll_divn = INPLL(PPLL_DIV_0 + ppll_div_sel);
 		rinfo->panel_info.ref_divider = rinfo->pll.ref_div;
 		rinfo->panel_info.fbk_divider = ppll_divn & 0x7ff;
Index: linux-work/drivers/video/aty/radeon_base.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_base.c	2005-03-11 15:05:34.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_base.c	2005-03-11 15:37:44.000000000 +1100
@@ -1,4 +1,3 @@
-
 /*
  *	drivers/video/aty/radeon_base.c
  *
@@ -530,11 +529,8 @@
         break;
 	}
 
-	radeon_pll_workaround_before(rinfo);
 	ppll_div_sel = INREG8(CLOCK_CNTL_INDEX + 1) & 0x3;
-	radeon_pll_workaround_after(rinfo);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 
 	n = (INPLL(PPLL_DIV_0 + ppll_div_sel) & 0x7ff);
 	m = (INPLL(PPLL_REF_DIV) & 0x3ff);
@@ -1173,11 +1169,8 @@
 	save->vclk_ecp_cntl = INPLL(VCLK_ECP_CNTL);
 
 	/* PLL regs */
-	radeon_pll_workaround_before(rinfo);
 	save->clk_cntl_index = INREG(CLOCK_CNTL_INDEX) & ~0x3f;
-	radeon_pll_workaround_after(rinfo);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	save->ppll_div_3 = INPLL(PPLL_DIV_3);
 	save->ppll_ref_div = INPLL(PPLL_REF_DIV);
 }
@@ -1204,13 +1197,11 @@
 			/* We still have to force a switch to selected PPLL div thanks to
 			 * an XFree86 driver bug which will switch it away in some cases
 			 * even when using UseFDev */
-			radeon_pll_workaround_before(rinfo);
 			OUTREGP(CLOCK_CNTL_INDEX,
 				mode->clk_cntl_index & PPLL_DIV_SEL_MASK,
 				~PPLL_DIV_SEL_MASK);
-			radeon_pll_workaround_after(rinfo);
-			if (rinfo->R300_cg_workaround)
-				R300_cg_workardound(rinfo);
+			radeon_pll_errata_after_index(rinfo);
+			radeon_pll_errata_after_data(rinfo);
             		return;
 		}
 	}
@@ -1224,13 +1215,11 @@
 		~(PPLL_RESET | PPLL_ATOMIC_UPDATE_EN | PPLL_VGA_ATOMIC_UPDATE_EN));
 
 	/* Switch to selected PPLL divider */
-	radeon_pll_workaround_before(rinfo);
 	OUTREGP(CLOCK_CNTL_INDEX,
 		mode->clk_cntl_index & PPLL_DIV_SEL_MASK,
 		~PPLL_DIV_SEL_MASK);
-	radeon_pll_workaround_after(rinfo);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_index(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	/* Set PPLL ref. div */
 	if (rinfo->family == CHIP_FAMILY_R300 ||
@@ -2248,11 +2237,29 @@
 	rinfo->has_CRTC2 = (ent->driver_data & CHIP_HAS_CRTC2) != 0;
 	rinfo->is_mobility = (ent->driver_data & CHIP_IS_MOBILITY) != 0;
 	rinfo->is_IGP = (ent->driver_data & CHIP_IS_IGP) != 0;
-		
+	
 	/* Set base addrs */
 	rinfo->fb_base_phys = pci_resource_start (pdev, 0);
 	rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
 
+	/*
+	 * Check for errata
+	 */
+	rinfo->errata = 0;
+	if (rinfo->family == CHIP_FAMILY_R300 &&
+	    (INREG(CONFIG_CNTL) & CFG_ATI_REV_ID_MASK)
+	    == CFG_ATI_REV_A11)
+		rinfo->errata |= CHIP_ERRATA_R300_CG;
+
+	if (rinfo->family == CHIP_FAMILY_RV200 ||
+	    rinfo->family == CHIP_FAMILY_RS200)
+		rinfo->errata |= CHIP_ERRATA_PLL_DUMMYREADS;
+
+	if (rinfo->family == CHIP_FAMILY_RV100 ||
+	    rinfo->family == CHIP_FAMILY_RS100 ||
+	    rinfo->family == CHIP_FAMILY_RS200)
+		rinfo->errata |= CHIP_ERRATA_PLL_DELAY;
+
 	/* request the mem regions */
 	ret = pci_request_regions(pdev, "radeonfb");
 	if (ret < 0) {
@@ -2310,13 +2317,6 @@
 	       rinfo->mapped_vram/1024);
 
 	/*
-	 * Check for required workaround for PLL accesses
-	 */
-	rinfo->R300_cg_workaround = (rinfo->family == CHIP_FAMILY_R300 &&
-				     (INREG(CONFIG_CNTL) & CFG_ATI_REV_ID_MASK)
-				     == CFG_ATI_REV_A11);
-
-	/*
 	 * Map the BIOS ROM if any and retreive PLL parameters from
 	 * the BIOS. We skip that on mobility chips as the real panel
 	 * values we need aren't in the ROM but in the BIOS image in
Index: linux-work/drivers/video/aty/radeon_pm.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_pm.c	2005-03-11 15:05:34.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_pm.c	2005-03-11 15:19:02.000000000 +1100
@@ -1372,12 +1372,10 @@
 
 	/* Reconfigure SPLL charge pump, VCO gain, duty cycle */
 	tmp = INPLL(pllSPLL_CNTL);
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, pllSPLL_CNTL + PLL_WR_EN);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	/* Set SPLL feedback divider */
 	tmp = INPLL(pllM_SPLL_REF_FB_DIV);
@@ -1409,12 +1407,10 @@
 
 	/* Reconfigure MPLL charge pump, VCO gain, duty cycle */
 	tmp = INPLL(pllMPLL_CNTL);
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, pllMPLL_CNTL + PLL_WR_EN);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	/* Set MPLL feedback divider */
 	tmp = INPLL(pllM_SPLL_REF_FB_DIV);
@@ -1532,12 +1528,10 @@
 {
 	u32 tmp;
 
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, pllHTOTAL_CNTL + PLL_WR_EN);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA, 0);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	tmp = INPLL(pllVCLK_ECP_CNTL);
 	OUTPLL(pllVCLK_ECP_CNTL, tmp | 0x80);
@@ -1552,12 +1546,10 @@
 	 * probably useless since we already did it ...
 	 */
 	tmp = INPLL(pllPPLL_CNTL);
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, pllSPLL_CNTL + PLL_WR_EN);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	/* Restore our "reference" PPLL divider set by firmware
 	 * according to proper spread spectrum calculations
@@ -1581,11 +1573,9 @@
 	mdelay(5);
 
 	/* Switch pixel clock to firmware default div 0 */
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX+1, 0);
-	radeon_pll_workaround_after(rinfo);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_index(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 }
 
 static void radeon_pm_m10_reconfigure_mc(struct radeonfb_info *rinfo)
@@ -2173,7 +2163,9 @@
 
 	tmp = INPLL(MPLL_CNTL);
 	OUTREG8(CLOCK_CNTL_INDEX, MPLL_CNTL + PLL_WR_EN);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
+	radeon_pll_errata_after_data(rinfo);
 
 	tmp = INPLL(M_SPLL_REF_FB_DIV);
 	OUTPLL(M_SPLL_REF_FB_DIV, tmp | 0x5900);
@@ -2194,7 +2186,9 @@
 
 	tmp = INPLL(SPLL_CNTL);
 	OUTREG8(CLOCK_CNTL_INDEX, SPLL_CNTL + PLL_WR_EN);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
+	radeon_pll_errata_after_data(rinfo);
 
 	tmp = INPLL(M_SPLL_REF_FB_DIV);
 	OUTPLL(M_SPLL_REF_FB_DIV, tmp | 0x780000);
@@ -2322,7 +2316,9 @@
 	OUTREG(CRTC_H_SYNC_STRT_WID, 0x008e0580);
 	OUTREG(CRTC_H_TOTAL_DISP, 0x009f00d2);
 	OUTREG8(CLOCK_CNTL_INDEX, HTOTAL_CNTL + PLL_WR_EN);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA, 0);
+	radeon_pll_errata_after_data(rinfo);
 	OUTREG(CRTC_V_SYNC_STRT_WID, 0x00830403);
 	OUTREG(CRTC_V_TOTAL_DISP, 0x03ff0429);
 	OUTREG(FP_CRTC_H_TOTAL_DISP, 0x009f0033);
@@ -2344,10 +2340,15 @@
 	INPLL(PPLL_REF_DIV);
 
 	OUTREG8(CLOCK_CNTL_INDEX, PPLL_CNTL + PLL_WR_EN);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, 0xbc);
+	radeon_pll_errata_after_data(rinfo);
 
 	tmp = INREG(CLOCK_CNTL_INDEX);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG(CLOCK_CNTL_INDEX, tmp & 0xff);
+	radeon_pll_errata_after_index(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	OUTPLL(PPLL_DIV_0, 0x48090);
 
Index: linux-work/drivers/video/aty/radeon_accel.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_accel.c	2005-02-24 08:37:56.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_accel.c	2005-03-11 15:43:33.000000000 +1100
@@ -189,32 +189,6 @@
 
 	radeon_engine_flush (rinfo);
 
-    	/* Some ASICs have bugs with dynamic-on feature, which are  
-     	 * ASIC-version dependent, so we force all blocks on for now
-     	 * -- from XFree86
-     	 * We don't do that on macs, things just work here with dynamic
-     	 * clocking... --BenH
-	 */
-#ifdef CONFIG_ALL_PPC
-	if (_machine != _MACH_Pmac && rinfo->hasCRTC2)
-#else
-	if (rinfo->has_CRTC2)
-#endif	
-	{
-		u32 tmp;
-
-		tmp = INPLL(SCLK_CNTL);
-		OUTPLL(SCLK_CNTL, ((tmp & ~DYN_STOP_LAT_MASK) |
-				   CP_MAX_DYN_STOP_LAT |
-				   SCLK_FORCEON_MASK));
-
-		if (rinfo->family == CHIP_FAMILY_RV200)
-		{
-			tmp = INPLL(SCLK_MORE_CNTL);
-			OUTPLL(SCLK_MORE_CNTL, tmp | SCLK_MORE_FORCEON);
-		}
-	}
-
 	clock_cntl_index = INREG(CLOCK_CNTL_INDEX);
 	mclk_cntl = INPLL(MCLK_CNTL);
 
@@ -274,8 +248,6 @@
 
 	OUTREG(CLOCK_CNTL_INDEX, clock_cntl_index);
 	OUTPLL(MCLK_CNTL, mclk_cntl);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
 }
 
 void radeonfb_engine_init (struct radeonfb_info *rinfo)

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

[Benjamin Herrenschmidt]

On Fri, 2005-03-11 at 16:42 +1100, Benjamin Herrenschmidt wrote:
> Hi !
> 
> After discussion with ATIs, it seems that the workarounds they initially
> gave me were not completely correct.
> 
> This patch implements the proper ones, which includes sleeping in PLL
> accesses, and thus requires the previous patch to make sure we do not
> call unblank at interrupt time (unless oops_in_progress is set, in which
> case I use an mdelay).
> 
> It also removes obsolete code that used to disable some power management
> features in the accel init code.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

And without a stupidity where I would use a register before ioremap'ing
them on some cards:

---


After discussion with ATIs, it seems that the workarounds they initially
gave me were not completely correct.

This patch implements the proper ones, which includes sleeping in PLL
accesses, and thus requires the previous patch to make sure we do not
call unblank at interrupt time (unless oops_in_progress is set, in which
case I use an mdelay).

It also removes obsolete code that used to disable some power management
features in the accel init code.

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


Index: linux-work/drivers/video/aty/radeonfb.h
===================================================================
--- linux-work.orig/drivers/video/aty/radeonfb.h	2005-03-11 11:08:32.000000000 +1100
+++ linux-work/drivers/video/aty/radeonfb.h	2005-03-13 11:09:30.000000000 +1100
@@ -77,6 +77,15 @@
 	CHIP_HAS_CRTC2		= 0x00040000UL,	
 };
 
+/*
+ * Errata workarounds
+ */
+enum radeon_errata {
+	CHIP_ERRATA_R300_CG		= 0x00000001,
+	CHIP_ERRATA_PLL_DUMMYREADS	= 0x00000002,
+	CHIP_ERRATA_PLL_DELAY		= 0x00000004,
+};
+
 
 /*
  * Monitor types
@@ -295,6 +304,7 @@
 	int			chipset;
 	u8			family;
 	u8			rev;
+	unsigned int		errata;
 	unsigned long		video_ram;
 	unsigned long		mapped_vram;
 	int			vram_width;
@@ -305,7 +315,6 @@
 	int			has_CRTC2;
 	int			is_mobility;
 	int			is_IGP;
-	int			R300_cg_workaround;
 	int			reversed_DAC;
 	int			reversed_TMDS;
 	struct panel_info	panel_info;
@@ -369,6 +378,21 @@
  * IO macros
  */
 
+/* Note about this function: we have some rare cases where we must not schedule,
+ * this typically happen with our special "wake up early" hook which allows us to
+ * wake up the graphic chip (and thus get the console back) before everything else
+ * on some machines that support that mecanism. At this point, interrupts are off
+ * and scheduling is not permitted
+ */
+static inline void _radeon_msleep(struct radeonfb_info *rinfo, unsigned long ms)
+{
+	if (rinfo->no_schedule || oops_in_progress)
+		mdelay(ms);
+	else
+		msleep(ms);
+}
+
+
 #define INREG8(addr)		readb((rinfo->mmio_base)+addr)
 #define OUTREG8(addr,val)	writeb(val, (rinfo->mmio_base)+addr)
 #define INREG(addr)		readl((rinfo->mmio_base)+addr)
@@ -390,107 +414,85 @@
 
 #define OUTREGP(addr,val,mask)	_OUTREGP(rinfo, addr, val,mask)
 
-/* This function is required to workaround a hardware bug in some (all?)
- * revisions of the R300.  This workaround should be called after every
- * CLOCK_CNTL_INDEX register access.  If not, register reads afterward
- * may not be correct.
- */
-static inline void R300_cg_workardound(struct radeonfb_info *rinfo)
-{
-	u32 save, tmp;
-	save = INREG(CLOCK_CNTL_INDEX);
-	tmp = save & ~(0x3f | PLL_WR_EN);
-	OUTREG(CLOCK_CNTL_INDEX, tmp);
-	tmp = INREG(CLOCK_CNTL_DATA);
-	OUTREG(CLOCK_CNTL_INDEX, save);
-}
-
 /*
- * PLL accesses suffer from various HW issues on the different chip
- * families. Some R300's need the above workaround, rv200 & friends
- * need a couple of dummy reads after any write of CLOCK_CNTL_INDEX,
- * and some RS100/200 need a dummy read before writing to
- * CLOCK_CNTL_INDEX as well. Instead of testing every chip revision,
- * we just unconditionally do  the workarounds at once since PLL
- * accesses are far from beeing performance critical. Except for R300
- * one which stays separate for now
+ * Note about PLL register accesses:
+ *
+ * I have removed the spinlock on them on purpose. The driver now
+ * expects that it will only manipulate the PLL registers in normal
+ * task environment, where radeon_msleep() will be called, protected
+ * by a semaphore (currently the console semaphore) so that no conflict
+ * will happen on the PLL register index.
+ *
+ * With the latest changes to the VT layer, this is guaranteed for all
+ * calls except the actual drawing/blits which aren't supposed to use
+ * the PLL registers anyway
+ *
+ * This is very important for the workarounds to work properly. The only
+ * possible exception to this rule is the call to unblank(), which may
+ * be done at irq time if an oops is in progress.
  */
-
-static inline void radeon_pll_workaround_before(struct radeonfb_info *rinfo)
+static inline void radeon_pll_errata_after_index(struct radeonfb_info *rinfo)
 {
+	if (!(rinfo->errata & CHIP_ERRATA_PLL_DUMMYREADS))
+		return;
+
+	(void)INREG(CLOCK_CNTL_DATA);
 	(void)INREG(CRTC_GEN_CNTL);
 }
 
-static inline void radeon_pll_workaround_after(struct radeonfb_info *rinfo)
+static inline void radeon_pll_errata_after_data(struct radeonfb_info *rinfo)
 {
-	(void)INREG(CLOCK_CNTL_DATA);
-	(void)INREG(CRTC_GEN_CNTL);
+	if (rinfo->errata & CHIP_ERRATA_PLL_DELAY) {
+		/* we can't deal with posted writes here ... */
+		_radeon_msleep(rinfo, 5);
+	}
+	if (rinfo->errata & CHIP_ERRATA_R300_CG) {
+		u32 save, tmp;
+		save = INREG(CLOCK_CNTL_INDEX);
+		tmp = save & ~(0x3f | PLL_WR_EN);
+		OUTREG(CLOCK_CNTL_INDEX, tmp);
+		tmp = INREG(CLOCK_CNTL_DATA);
+		OUTREG(CLOCK_CNTL_INDEX, save);
+	}
 }
 
 static inline u32 __INPLL(struct radeonfb_info *rinfo, u32 addr)
 {
 	u32 data;
 
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, addr & 0x0000003f);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	data = INREG(CLOCK_CNTL_DATA);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 	return data;
 }
 
-static inline u32 _INPLL(struct radeonfb_info *rinfo, u32 addr)
-{
-       	unsigned long flags;
-	u32 data;
-
-	spin_lock_irqsave(&rinfo->reg_lock, flags);
-	data = __INPLL(rinfo, addr);
-	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
-	return data;
-}
-
-#define INPLL(addr)		_INPLL(rinfo, addr)
-
-
 static inline void __OUTPLL(struct radeonfb_info *rinfo, unsigned int index,
 			    u32 val)
 {
 
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, (index & 0x0000003f) | 0x00000080);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG(CLOCK_CNTL_DATA, val);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 }
 
-static inline void _OUTPLL(struct radeonfb_info *rinfo, unsigned int index, u32 val)
-{
-       	unsigned long flags;
-	spin_lock_irqsave(&rinfo->reg_lock, flags);
-	__OUTPLL(rinfo, index, val);
-	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
-}
 
-static inline void _OUTPLLP(struct radeonfb_info *rinfo, unsigned int index,
-			    u32 val, u32 mask)
+static inline void __OUTPLLP(struct radeonfb_info *rinfo, unsigned int index,
+			     u32 val, u32 mask)
 {
-	unsigned long flags;
 	unsigned int tmp;
 
-	spin_lock_irqsave(&rinfo->reg_lock, flags);
 	tmp  = __INPLL(rinfo, index);
 	tmp &= (mask);
 	tmp |= (val);
 	__OUTPLL(rinfo, index, tmp);
-	spin_unlock_irqrestore(&rinfo->reg_lock, flags);
 }
 
 
-#define OUTPLL(index, val)		_OUTPLL(rinfo, index, val)
-#define OUTPLLP(index, val, mask)	_OUTPLLP(rinfo, index, val, mask)
+#define INPLL(addr)			__INPLL(rinfo, addr)
+#define OUTPLL(index, val)		__OUTPLL(rinfo, index, val)
+#define OUTPLLP(index, val, mask)	__OUTPLLP(rinfo, index, val, mask)
 
 
 #define BIOS_IN8(v)  	(readb(rinfo->bios_seg + (v)))
@@ -582,20 +584,6 @@
 	printk(KERN_ERR "radeonfb: Idle Timeout !\n");
 }
 
-/* Note about this function: we have some rare cases where we must not schedule,
- * this typically happen with our special "wake up early" hook which allows us to
- * wake up the graphic chip (and thus get the console back) before everything else
- * on some machines that support that mecanism. At this point, interrupts are off
- * and scheduling is not permitted
- */
-static inline void _radeon_msleep(struct radeonfb_info *rinfo, unsigned long ms)
-{
-	if (rinfo->no_schedule)
-		mdelay(ms);
-	else
-		msleep(ms);
-}
-
 
 #define radeon_engine_idle()		_radeon_engine_idle(rinfo)
 #define radeon_fifo_wait(entries)	_radeon_fifo_wait(rinfo,entries)
Index: linux-work/drivers/video/aty/radeon_monitor.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_monitor.c	2005-03-11 11:08:32.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_monitor.c	2005-03-13 11:09:33.000000000 +1100
@@ -657,11 +657,8 @@
 	    && rinfo->is_mobility) {
 		int ppll_div_sel;
 		u32 ppll_divn;
-		radeon_pll_workaround_before(rinfo);
 		ppll_div_sel = INREG8(CLOCK_CNTL_INDEX + 1) & 0x3;
-		radeon_pll_workaround_after(rinfo);
-		if (rinfo->R300_cg_workaround)
-			R300_cg_workardound(rinfo);
+		radeon_pll_errata_after_index(rinfo);
 		ppll_divn = INPLL(PPLL_DIV_0 + ppll_div_sel);
 		rinfo->panel_info.ref_divider = rinfo->pll.ref_div;
 		rinfo->panel_info.fbk_divider = ppll_divn & 0x7ff;
Index: linux-work/drivers/video/aty/radeon_base.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_base.c	2005-03-11 11:08:32.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_base.c	2005-03-13 11:10:12.000000000 +1100
@@ -1,4 +1,3 @@
-
 /*
  *	drivers/video/aty/radeon_base.c
  *
@@ -530,11 +529,8 @@
         break;
 	}
 
-	radeon_pll_workaround_before(rinfo);
 	ppll_div_sel = INREG8(CLOCK_CNTL_INDEX + 1) & 0x3;
-	radeon_pll_workaround_after(rinfo);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 
 	n = (INPLL(PPLL_DIV_0 + ppll_div_sel) & 0x7ff);
 	m = (INPLL(PPLL_REF_DIV) & 0x3ff);
@@ -1173,11 +1169,8 @@
 	save->vclk_ecp_cntl = INPLL(VCLK_ECP_CNTL);
 
 	/* PLL regs */
-	radeon_pll_workaround_before(rinfo);
 	save->clk_cntl_index = INREG(CLOCK_CNTL_INDEX) & ~0x3f;
-	radeon_pll_workaround_after(rinfo);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	save->ppll_div_3 = INPLL(PPLL_DIV_3);
 	save->ppll_ref_div = INPLL(PPLL_REF_DIV);
 }
@@ -1204,13 +1197,11 @@
 			/* We still have to force a switch to selected PPLL div thanks to
 			 * an XFree86 driver bug which will switch it away in some cases
 			 * even when using UseFDev */
-			radeon_pll_workaround_before(rinfo);
 			OUTREGP(CLOCK_CNTL_INDEX,
 				mode->clk_cntl_index & PPLL_DIV_SEL_MASK,
 				~PPLL_DIV_SEL_MASK);
-			radeon_pll_workaround_after(rinfo);
-			if (rinfo->R300_cg_workaround)
-				R300_cg_workardound(rinfo);
+			radeon_pll_errata_after_index(rinfo);
+			radeon_pll_errata_after_data(rinfo);
             		return;
 		}
 	}
@@ -1224,13 +1215,11 @@
 		~(PPLL_RESET | PPLL_ATOMIC_UPDATE_EN | PPLL_VGA_ATOMIC_UPDATE_EN));
 
 	/* Switch to selected PPLL divider */
-	radeon_pll_workaround_before(rinfo);
 	OUTREGP(CLOCK_CNTL_INDEX,
 		mode->clk_cntl_index & PPLL_DIV_SEL_MASK,
 		~PPLL_DIV_SEL_MASK);
-	radeon_pll_workaround_after(rinfo);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_index(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	/* Set PPLL ref. div */
 	if (rinfo->family == CHIP_FAMILY_R300 ||
@@ -2248,7 +2237,7 @@
 	rinfo->has_CRTC2 = (ent->driver_data & CHIP_HAS_CRTC2) != 0;
 	rinfo->is_mobility = (ent->driver_data & CHIP_IS_MOBILITY) != 0;
 	rinfo->is_IGP = (ent->driver_data & CHIP_IS_IGP) != 0;
-		
+	
 	/* Set base addrs */
 	rinfo->fb_base_phys = pci_resource_start (pdev, 0);
 	rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
@@ -2271,6 +2260,24 @@
 
 	rinfo->fb_local_base = INREG(MC_FB_LOCATION) << 16;
 
+	/*
+	 * Check for errata
+	 */
+	rinfo->errata = 0;
+	if (rinfo->family == CHIP_FAMILY_R300 &&
+	    (INREG(CONFIG_CNTL) & CFG_ATI_REV_ID_MASK)
+	    == CFG_ATI_REV_A11)
+		rinfo->errata |= CHIP_ERRATA_R300_CG;
+
+	if (rinfo->family == CHIP_FAMILY_RV200 ||
+	    rinfo->family == CHIP_FAMILY_RS200)
+		rinfo->errata |= CHIP_ERRATA_PLL_DUMMYREADS;
+
+	if (rinfo->family == CHIP_FAMILY_RV100 ||
+	    rinfo->family == CHIP_FAMILY_RS100 ||
+	    rinfo->family == CHIP_FAMILY_RS200)
+		rinfo->errata |= CHIP_ERRATA_PLL_DELAY;
+
 #ifdef CONFIG_PPC_OF
 	/* On PPC, we obtain the OF device-node pointer to the firmware
 	 * data for this chip
@@ -2310,13 +2317,6 @@
 	       rinfo->mapped_vram/1024);
 
 	/*
-	 * Check for required workaround for PLL accesses
-	 */
-	rinfo->R300_cg_workaround = (rinfo->family == CHIP_FAMILY_R300 &&
-				     (INREG(CONFIG_CNTL) & CFG_ATI_REV_ID_MASK)
-				     == CFG_ATI_REV_A11);
-
-	/*
 	 * Map the BIOS ROM if any and retreive PLL parameters from
 	 * the BIOS. We skip that on mobility chips as the real panel
 	 * values we need aren't in the ROM but in the BIOS image in
Index: linux-work/drivers/video/aty/radeon_pm.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_pm.c	2005-03-11 11:08:32.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_pm.c	2005-03-13 10:54:01.000000000 +1100
@@ -1372,12 +1372,10 @@
 
 	/* Reconfigure SPLL charge pump, VCO gain, duty cycle */
 	tmp = INPLL(pllSPLL_CNTL);
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, pllSPLL_CNTL + PLL_WR_EN);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	/* Set SPLL feedback divider */
 	tmp = INPLL(pllM_SPLL_REF_FB_DIV);
@@ -1409,12 +1407,10 @@
 
 	/* Reconfigure MPLL charge pump, VCO gain, duty cycle */
 	tmp = INPLL(pllMPLL_CNTL);
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, pllMPLL_CNTL + PLL_WR_EN);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	/* Set MPLL feedback divider */
 	tmp = INPLL(pllM_SPLL_REF_FB_DIV);
@@ -1532,12 +1528,10 @@
 {
 	u32 tmp;
 
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, pllHTOTAL_CNTL + PLL_WR_EN);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA, 0);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	tmp = INPLL(pllVCLK_ECP_CNTL);
 	OUTPLL(pllVCLK_ECP_CNTL, tmp | 0x80);
@@ -1552,12 +1546,10 @@
 	 * probably useless since we already did it ...
 	 */
 	tmp = INPLL(pllPPLL_CNTL);
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX, pllSPLL_CNTL + PLL_WR_EN);
-	radeon_pll_workaround_after(rinfo);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	/* Restore our "reference" PPLL divider set by firmware
 	 * according to proper spread spectrum calculations
@@ -1581,11 +1573,9 @@
 	mdelay(5);
 
 	/* Switch pixel clock to firmware default div 0 */
-	radeon_pll_workaround_before(rinfo);
 	OUTREG8(CLOCK_CNTL_INDEX+1, 0);
-	radeon_pll_workaround_after(rinfo);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
+	radeon_pll_errata_after_index(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 }
 
 static void radeon_pm_m10_reconfigure_mc(struct radeonfb_info *rinfo)
@@ -2173,7 +2163,9 @@
 
 	tmp = INPLL(MPLL_CNTL);
 	OUTREG8(CLOCK_CNTL_INDEX, MPLL_CNTL + PLL_WR_EN);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
+	radeon_pll_errata_after_data(rinfo);
 
 	tmp = INPLL(M_SPLL_REF_FB_DIV);
 	OUTPLL(M_SPLL_REF_FB_DIV, tmp | 0x5900);
@@ -2194,7 +2186,9 @@
 
 	tmp = INPLL(SPLL_CNTL);
 	OUTREG8(CLOCK_CNTL_INDEX, SPLL_CNTL + PLL_WR_EN);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, (tmp >> 8) & 0xff);
+	radeon_pll_errata_after_data(rinfo);
 
 	tmp = INPLL(M_SPLL_REF_FB_DIV);
 	OUTPLL(M_SPLL_REF_FB_DIV, tmp | 0x780000);
@@ -2322,7 +2316,9 @@
 	OUTREG(CRTC_H_SYNC_STRT_WID, 0x008e0580);
 	OUTREG(CRTC_H_TOTAL_DISP, 0x009f00d2);
 	OUTREG8(CLOCK_CNTL_INDEX, HTOTAL_CNTL + PLL_WR_EN);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA, 0);
+	radeon_pll_errata_after_data(rinfo);
 	OUTREG(CRTC_V_SYNC_STRT_WID, 0x00830403);
 	OUTREG(CRTC_V_TOTAL_DISP, 0x03ff0429);
 	OUTREG(FP_CRTC_H_TOTAL_DISP, 0x009f0033);
@@ -2344,10 +2340,15 @@
 	INPLL(PPLL_REF_DIV);
 
 	OUTREG8(CLOCK_CNTL_INDEX, PPLL_CNTL + PLL_WR_EN);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG8(CLOCK_CNTL_DATA + 1, 0xbc);
+	radeon_pll_errata_after_data(rinfo);
 
 	tmp = INREG(CLOCK_CNTL_INDEX);
+	radeon_pll_errata_after_index(rinfo);
 	OUTREG(CLOCK_CNTL_INDEX, tmp & 0xff);
+	radeon_pll_errata_after_index(rinfo);
+	radeon_pll_errata_after_data(rinfo);
 
 	OUTPLL(PPLL_DIV_0, 0x48090);
 
Index: linux-work/drivers/video/aty/radeon_accel.c
===================================================================
--- linux-work.orig/drivers/video/aty/radeon_accel.c	2005-02-24 08:37:56.000000000 +1100
+++ linux-work/drivers/video/aty/radeon_accel.c	2005-03-13 10:54:01.000000000 +1100
@@ -189,32 +189,6 @@
 
 	radeon_engine_flush (rinfo);
 
-    	/* Some ASICs have bugs with dynamic-on feature, which are  
-     	 * ASIC-version dependent, so we force all blocks on for now
-     	 * -- from XFree86
-     	 * We don't do that on macs, things just work here with dynamic
-     	 * clocking... --BenH
-	 */
-#ifdef CONFIG_ALL_PPC
-	if (_machine != _MACH_Pmac && rinfo->hasCRTC2)
-#else
-	if (rinfo->has_CRTC2)
-#endif	
-	{
-		u32 tmp;
-
-		tmp = INPLL(SCLK_CNTL);
-		OUTPLL(SCLK_CNTL, ((tmp & ~DYN_STOP_LAT_MASK) |
-				   CP_MAX_DYN_STOP_LAT |
-				   SCLK_FORCEON_MASK));
-
-		if (rinfo->family == CHIP_FAMILY_RV200)
-		{
-			tmp = INPLL(SCLK_MORE_CNTL);
-			OUTPLL(SCLK_MORE_CNTL, tmp | SCLK_MORE_FORCEON);
-		}
-	}
-
 	clock_cntl_index = INREG(CLOCK_CNTL_INDEX);
 	mclk_cntl = INPLL(MCLK_CNTL);
 
@@ -274,8 +248,6 @@
 
 	OUTREG(CLOCK_CNTL_INDEX, clock_cntl_index);
 	OUTPLL(MCLK_CNTL, mclk_cntl);
-	if (rinfo->R300_cg_workaround)
-		R300_cg_workardound(rinfo);
 }
 
 void radeonfb_engine_init (struct radeonfb_info *rinfo)

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

[Andreas Schwab]

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

> After discussion with ATIs, it seems that the workarounds they initially
> gave me were not completely correct.
>
> This patch implements the proper ones, which includes sleeping in PLL
> accesses, and thus requires the previous patch to make sure we do not
> call unblank at interrupt time (unless oops_in_progress is set, in which
> case I use an mdelay).
>
> It also removes obsolete code that used to disable some power management
> features in the accel init code.

This patch does no good on Radeon M6 (iBook G3).  It makes mode switching
to take an extraordinary amount of time, ie. when switching away from X it
takes about 2-3 seconds until the console is restored.

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 Tue, 2005-04-05 at 23:44 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > After discussion with ATIs, it seems that the workarounds they initially
> > gave me were not completely correct.
> >
> > This patch implements the proper ones, which includes sleeping in PLL
> > accesses, and thus requires the previous patch to make sure we do not
> > call unblank at interrupt time (unless oops_in_progress is set, in which
> > case I use an mdelay).
> >
> > It also removes obsolete code that used to disable some power management
> > features in the accel init code.
> 
> This patch does no good on Radeon M6 (iBook G3).  It makes mode switching
> to take an extraordinary amount of time, ie. when switching away from X it
> takes about 2-3 seconds until the console is restored.

Hrm... it should only add a few ms, maybe about 20 ms to the mode
switching... If you remove the radeon_msleep(5) call from the
radeon_pll_errata_after_data() routine in radeonfb.h, does it make a
difference ?

Ben.

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

[Andreas Schwab]

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

> Hrm... it should only add a few ms, maybe about 20 ms to the mode
> switching... If you remove the radeon_msleep(5) call from the
> radeon_pll_errata_after_data() routine in radeonfb.h, does it make a
> difference ?

Yes, it does.  Without the sleep, switching is as fast as before.

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 Thu, 2005-04-07 at 00:35 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > Hrm... it should only add a few ms, maybe about 20 ms to the mode
> > switching... If you remove the radeon_msleep(5) call from the
> > radeon_pll_errata_after_data() routine in radeonfb.h, does it make a
> > difference ?
> 
> Yes, it does.  Without the sleep, switching is as fast as before.

It is weird tho. Could you try adding a printk or something to figure
out how much this is called during a typical swich ? It will seriously
spam your console though ... I would expect only 5 or 6 calls during a
normal switch (since the PLL value shouldn't change for a flat panel),
that is no more than maybe 50ms, while it seems you are having a much
bigger delay.

Ben.

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

[Andreas Schwab]

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

> It is weird tho. Could you try adding a printk or something to figure
> out how much this is called during a typical swich ?

There are 1694 calls to radeon_pll_errata_after_data during a switch from
X to the console and 393 calls the other way.

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

[Dave Airlie]

> There are 1694 calls to radeon_pll_errata_after_data during a switch from
> X to the console and 393 calls the other way.

Wow... Ben that seems a bit extreme... there's not even close to 393 plls :-)

Dave.

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

[Benjamin Herrenschmidt]

On Thu, 2005-04-07 at 22:21 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > It is weird tho. Could you try adding a printk or something to figure
> > out how much this is called during a typical swich ?
> 
> There are 1694 calls to radeon_pll_errata_after_data during a switch from
> X to the console and 393 calls the other way.

Wow ! That is plain wrong !

Can you cound how many times radeonfb_set_par is called and dump your
"counter" at the beginning and end of each of these calls ?

Ben.

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

[Benjamin Herrenschmidt]

On Fri, 2005-04-08 at 07:22 +1000, Dave Airlie wrote:
> > There are 1694 calls to radeon_pll_errata_after_data during a switch from
> > X to the console and 393 calls the other way.
> 
> Wow... Ben that seems a bit extreme... there's not even close to 393 plls :-)

Yes, that's very extreme, I suspect somebody is banging on set_par or
something like that. Let me count a normal set_par operation:

 - blank. That can do INPLL, OUTPLLP and OUTPLL on MT_LCD, that is 4
calls to the errata (OUTPLLP has two). 

 - write_pll_regs which does
    * on mobility chips: 2x INPLL to test the PLL value, and if it
matches, just writes the PLL selector with a call to the errata, that
is only 3 calls to the errata. Can you check we actually get in that
case ? Normally, on the internal LCD, we should never change the PLL
registers, or only one (they should stay the same all the time after
that) and thus we should get into this case. If not (CRT), indeed, we
end up doing more accesses:

    * OUTPLLP (2), OUTPLLP (2), manual errata (1), OUTPLLP (2),
OUTPLLP (2), OUTPLLP (2), an INPLL loop (hrm...), OUTPLLP (2), another
INPLL loop, OUTPLL (1), OUTPLLP (2), OUTPLLP (2). That is 18 calls to
the errata plus the 2 INPLL loops. It would be useful to instrument
those loops and see what happens there, but I don't see why they would
have any impact unless something wrong is going on there with the PLL
locking...

   * One last call to OUTPLLP (2).

 - reset the engine, that is 3 calls to the errata

So that means that one call to raeonfb_set_par() should be in the normal
internal flat panel case about 12 calls to the errata, and in the case
where we actually write the PLL registers, about 29, plus the ones
called by the INPLL loops waiting for the PLL to lock.

As you can see, we are far below the measured counts. So I would need
more instrumentation of the driver to figure out what's going on there.

Ben.

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

[Andreas Schwab]

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

> Can you cound how many times radeonfb_set_par is called and dump your
> "counter" at the beginning and end of each of these calls ?

Switch from X to console:

kernel: radeonfb_set_par
kernel: radeon_pll_errata_after_data
last message repeated 774 times
kernel: radeonfb_set_par
kernel: radeon_pll_errata_after_data
last message repeated 918 times

Switch from console to X:

kernel: radeonfb_set_par
kernel: radeon_pll_errata_after_data
last message repeated 200 times
kernel: agpgart: Putting AGP V2 device at 0000:00:0b.0 into 1x mode
kernel: agpgart: Putting AGP V2 device at 0000:00:10.0 into 1x mode
kernel: radeon_pll_errata_after_data
last message repeated 191 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 Fri, 2005-04-08 at 01:13 +0200, Andreas Schwab wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> 
> > Can you cound how many times radeonfb_set_par is called and dump your
> > "counter" at the beginning and end of each of these calls ?
> 
> Switch from X to console:
> 
> kernel: radeonfb_set_par
> kernel: radeon_pll_errata_after_data
> last message repeated 774 times
> kernel: radeonfb_set_par
> kernel: radeon_pll_errata_after_data
> last message repeated 918 times

Ok, so somebody is calling set_par twice ... I suppose I know why but
it's not a very nice thing to do. Still, it doesn't explain why there
are so many calls to the errata. Please read my other email and try to
figure out where those big numbers come from...

Ben.

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

[Andreas Schwab]

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.

kernel: radeonfb_set_par
kernel: radeon_write_pll_regs: radeon_pll_errata_after_data
kernel: radeon_write_pll_regs: radeon_pll_errata_after_data
kernel: radeon_write_mode: radeon_pll_errata_after_data
kernel: radeonfb_engine_reset: radeon_pll_errata_after_data
last message repeated 2 times
kernel: radeonfb_setcolreg: radeon_pll_errata_after_data
last message repeated 767 times
kernel: radeonfb_set_par
kernel: radeon_write_pll_regs: radeon_pll_errata_after_data
kernel: radeon_write_pll_regs: radeon_pll_errata_after_data
kernel: radeon_write_mode: radeon_pll_errata_after_data
kernel: radeonfb_engine_reset: radeon_pll_errata_after_data
last message repeated 2 times
kernel: radeonfb_setcolreg: radeon_pll_errata_after_data
last message repeated 911 times

kernel: radeonfb_set_par
kernel: radeon_write_pll_regs: radeon_pll_errata_after_data
kernel: radeon_write_pll_regs: radeon_pll_errata_after_data
kernel: radeon_write_mode: radeon_pll_errata_after_data
kernel: radeonfb_engine_reset: radeon_pll_errata_after_data
last message repeated 2 times
kernel: radeonfb_setcolreg: radeon_pll_errata_after_data
last message repeated 193 times
kernel: agpgart: Putting AGP V2 device at 0000:00:0b.0 into 1x mode
kernel: agpgart: Putting AGP V2 device at 0000:00:10.0 into 1x mode
kernel: radeonfb_setcolreg: radeon_pll_errata_after_data
last message repeated 191 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 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.

Ahhh... interesting. I'll see if I can find a way to work around that
one. Best would be to "cache" the current PLL register index in fact,
but I'm afraid that may not work terribly well with userland apps
hacking it ...

Ben.

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) ||

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

[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.

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

[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.

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.

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.

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

[Brice Goglin]

Benjamin Herrenschmidt a écrit :
>>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...

In case your interested, I tried your last patch 
(http://lkml.org/lkml/2005/4/7/308)
on top of 2.6.12-rc2-mm2 on my Radeon Mobility M6 LY.
It works great. Switching was at least one second long with plain mm2.
Now it's almost immediat as in Linus' kernel. No more dirty colors
during the switch from X to fbcon. Not annoying at all, here.

Thanks,
Brice