radeonfb: Implement proper workarounds for PLL accesses
PATCH 1/1 radeonfb: Implement proper workarounds for PLL accesses
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.
drivers/video/aty/radeonfb.h +67 -79
@@ -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)
drivers/video/aty/radeon_monitor.c +1 -4
@@ -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 ;
drivers/video/aty/radeon_base.c +25 -25
101 lines not on this page · raw message
drivers/video/aty/radeon_pm.c +21 -20
111 lines not on this page · raw message
drivers/video/aty/radeon_accel.c +0 -28
@@ -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 1/1 radeonfb: (#2) Implement proper workarounds for PLL accesses
Quoted 10 lines · 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_progressis set, in which case I use an mdelay).
Quoted 4 lines
It also removes obsolete code that used to disable some power management features in the accel init code.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
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.
drivers/video/aty/radeonfb.h +67 -79
@@ -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)
drivers/video/aty/radeon_monitor.c +1 -4
@@ -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 ;
drivers/video/aty/radeon_base.c +25 -25
103 lines not on this page · raw message
drivers/video/aty/radeon_pm.c +21 -20
111 lines not on this page · raw message
drivers/video/aty/radeon_accel.c +0 -28
@@ -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
Quoted 10 lines · Benjamin Herrenschmidt
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_progressis 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.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] 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
Quoted 10 lines · Andreas Schwab
Quoted 8 lines · Benjamin Herrenschmidt
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_progressis set, in which case I use an mdelay).
Quoted 6 lines
Quoted 2 lines
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
Quoted 4 lines · Benjamin Herrenschmidt
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 theradeon_pll_errata_after_data()routine inradeonfb.h, does it make a difference ?
Yes, it does. Without the sleep, switching is as fast as before.
Andreas.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] 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
Quoted 8 lines · Andreas Schwab
Quoted 4 lines · Benjamin Herrenschmidt
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 theradeon_pll_errata_after_data()routine inradeonfb.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
Quoted 2 lines · Benjamin Herrenschmidt
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.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] 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
Quoted 2 lines
There are 1694 calls to
radeon_pll_errata_after_dataduring 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
Quoted 4 lines · Dave Airlie
Quoted 2 lines
There are 1694 calls to
radeon_pll_errata_after_dataduring 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
Quoted 2 lines · Benjamin Herrenschmidt
Yes, that's very extreme, I suspect somebody is banging on
set_paror 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.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] 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
Quoted 6 lines · Andreas Schwab
Quoted 2 lines · Benjamin Herrenschmidt
Yes, that's very extreme, I suspect somebody is banging on
set_paror something like that.
fb_setcolregis 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
In reply to Andreas SchwabQuoted 6 lines · Andreas Schwab
Quoted 2 lines · Benjamin Herrenschmidt
Yes, that's very extreme, I suspect somebody is banging on
set_paror something like that.
fb_setcolregis 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.
include/linux/fb.h +3 -0
@@ -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);
drivers/video/aty/radeon_base.c +94 -24
163 lines not on this page · raw message
drivers/video/fbcmap.c +31 -3
@@ -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 &©_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
Quoted 8 lines · Benjamin Herrenschmidt
Quoted 6 lines · Andreas Schwab
Quoted 2 lines · Benjamin Herrenschmidt
Yes, that's very extreme, I suspect somebody is banging on
set_paror something like that.
fb_setcolregis it.
Quoted 8 lines
Ok, what about that patch:
---
This patch adds to the fbdev interface a
set_cmapcallback 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.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] 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
Quoted 8 lines
Quoted 5 lines
This patch adds to the fbdev interface a
set_cmapcallback 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
Quoted 1 line · Benjamin Herrenschmidt
Can you redo the counting of the workarounds with the patch ?
Switching from X to console:
radeon_write_pll_regs: INPLLradeon_write_pll_regs: INPLLradeon_write_mode: OUTPLLradeonfb_engine_reset: INPLLradeonfb_engine_reset: OUTPLLradeonfb_engine_reset: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: OUTPLLradeon_write_pll_regs: INPLLradeon_write_pll_regs: INPLLradeon_write_mode: OUTPLLradeonfb_engine_reset: INPLLradeonfb_engine_reset: OUTPLLradeonfb_engine_reset: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLL
Switching from console to X:
radeonfb_setcmap: OUTPLLradeon_write_pll_regs: INPLLradeon_write_pll_regs: INPLLradeon_write_mode: OUTPLLradeonfb_engine_reset: INPLLradeonfb_engine_reset: OUTPLLradeonfb_engine_reset: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_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 moderadeonfb_setcolreg: INPLLradeonfb_setcolreg: OUTPLLradeonfb_setcolreg: OUTPLL
... last three lines repeated 63 times
Andreas.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] 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
Quoted 24 lines · Andreas Schwab
Quoted 1 line · Benjamin Herrenschmidt
Can you redo the counting of the workarounds with the patch ?
Switching from X to console:
radeon_write_pll_regs: INPLLradeon_write_pll_regs: INPLLradeon_write_mode: OUTPLLradeonfb_engine_reset: INPLLradeonfb_engine_reset: OUTPLLradeonfb_engine_reset: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: OUTPLLradeon_write_pll_regs: INPLLradeon_write_pll_regs: INPLLradeon_write_mode: OUTPLLradeonfb_engine_reset: INPLLradeonfb_engine_reset: OUTPLLradeonfb_engine_reset: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: OUTPLL
Quoted 8 lines
radeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: OUTPLLradeonfb_setcmap: INPLLradeonfb_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.
Quoted 18 lines
Switching from console to X:
radeonfb_setcmap: OUTPLLradeon_write_pll_regs: INPLLradeon_write_pll_regs: INPLLradeon_write_mode: OUTPLLradeonfb_engine_reset: INPLLradeonfb_engine_reset: OUTPLLradeonfb_engine_reset: OUTPLLradeonfb_setcmap: INPLLradeonfb_setcmap: OUTPLLradeonfb_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 moderadeonfb_setcolreg: INPLLradeonfb_setcolreg: OUTPLLradeonfb_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
Quoted 9 lines · Benjamin Herrenschmidt
Quoted 4 lines
radeonfb_setcolreg: INPLLradeonfb_setcolreg: OUTPLLradeonfb_setcolreg: OUTPLL
... last three lines repeated 63 timesHrm... 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...
Quoted 6 lines
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
Quoted 9 lines
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
Quoted 11 lines · Benjamin Herrenschmidt
Quoted 8 lines
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
Re: [PATCH] radeonfb: (#2) Implement proper workarounds for PLL accesses
In reply to Andreas SchwabQuoted 7 lines · Andreas Schwab
Quoted 2 lines · Benjamin Herrenschmidt
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_dataduring 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
Quoted 2 lines · Benjamin Herrenschmidt
Can you cound how many times
radeonfb_set_paris 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.
Signature · 4 lines
Andreas Schwab, SuSE Labs, [email protected] 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
Quoted 5 lines · Andreas Schwab
Quoted 2 lines · Benjamin Herrenschmidt
Can you cound how many times
radeonfb_set_paris called and dump your "counter" at the beginning and end of each of these calls ?
Quoted 8 lines
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.