Fix For Korg Bug #14897 (GM45 Display Flicker)

Fix For Korg Bug #14897 (GM45 Display Flicker)

[David John]

Hi Jesse, Yakui,

Sorry for the delay, but I was busy with other work last week. I got
some time to look into this on Monday and the problem seems to be this:

With the external display connected on VGA, intel_update_watermarks
calls update_wm a couple of times in sequence with the following params:

1) planea_clock = 148500 planeb_clock = 0, sr_hdisplay=1920 (VGA)
2) planea_clock = 148500 planeb_clock = 72330, sr_hdisplay=1366 (LVDS)

In update_wm (in my case g4x_update_wm), for 1) sr_entries is calculated
correctly and self refresh is enabled. For 2) sr_entries remains zero
and the SR watermark is set as such. However, SR remains
active. On mode switch to console, this causes a FIFO underrun and the
display flicker.

The solution that I see is to simply disable SR if more than one pipe is
enabled. I've written a patch that does this. Also another point is that
the G45 docs state that a particular sequence has to be followed if SR
is to be used when only one pipe is active for Cantiga (Pg 30, G45 Vol
3). I didn't see any code doing this, but I did not see any untoward
behaviour either, with only my patch applied.

I've updated the patch to fix i965 and i9xx as I assume the same problem
will occur on those chipsets as well, but this has _not_been tested as I
don't have the relevant platforms.

Patch follows this mail. Please let me know if any changes are required.

The VGA output powering-on on mode change mentioned in my bug report is
a separate DRM bug for which I will send a patch later.

Regards,
David.

[PATCH] Disable SR when more than one pipe is enabled

[David John]

Self Refresh should be disabled on dual plane configs.
Otherwise, as the SR watermark is not calculated for
such configs, switching to non VGA mode causes FIFO
underrun and display flicker.

This fixes Korg Bug # 14897.

Signed-off-by: David John <davidjon@xenontk.org>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 45da78e..46d7c0c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2519,6 +2519,10 @@ static void g4x_update_wm(struct drm_device *dev,  int planea_clock,
 		sr_entries = roundup(sr_entries / cacheline_size, 1);
 		DRM_DEBUG("self-refresh entries: %d\n", sr_entries);
 		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
+	} else {
+		/* Turn off self refresh if both pipes are enabled */
+		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
+					& ~FW_BLC_SELF_EN);
 	}
 
 	DRM_DEBUG("Setting FIFO watermarks - A: %d, B: %d, SR %d\n",
@@ -2562,6 +2566,10 @@ static void i965_update_wm(struct drm_device *dev, int planea_clock,
 			srwm = 1;
 		srwm &= 0x3f;
 		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
+	} else {
+		/* Turn off self refresh if both pipes are enabled */
+		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
+					& ~FW_BLC_SELF_EN);
 	}
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - A: 8, B: 8, C: 8, SR %d\n",
@@ -2630,6 +2638,10 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
 		if (srwm < 0)
 			srwm = 1;
 		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN | (srwm & 0x3f));
+	} else {
+		/* Turn off self refresh if both pipes are enabled */
+		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
+					& ~FW_BLC_SELF_EN);
 	}
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d, B: %d, C: %d, SR %d\n",

Re: [PATCH] Disable SR when more than one pipe is enabled

[Jesse Barnes]

On Wed, 27 Jan 2010 15:19:08 +0530
David John <davidjon@xenontk.org> wrote:

> Self Refresh should be disabled on dual plane configs.
> Otherwise, as the SR watermark is not calculated for
> such configs, switching to non VGA mode causes FIFO
> underrun and display flicker.
> 
> This fixes Korg Bug # 14897.
> 
> Signed-off-by: David John <davidjon@xenontk.org>
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 45da78e..46d7c0c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2519,6 +2519,10 @@ static void g4x_update_wm(struct drm_device *dev,  int planea_clock,
>  		sr_entries = roundup(sr_entries / cacheline_size, 1);
>  		DRM_DEBUG("self-refresh entries: %d\n", sr_entries);
>  		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
> +	} else {
> +		/* Turn off self refresh if both pipes are enabled */
> +		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
> +					& ~FW_BLC_SELF_EN);
>  	}
>  
>  	DRM_DEBUG("Setting FIFO watermarks - A: %d, B: %d, SR %d\n",
> @@ -2562,6 +2566,10 @@ static void i965_update_wm(struct drm_device *dev, int planea_clock,
>  			srwm = 1;
>  		srwm &= 0x3f;
>  		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
> +	} else {
> +		/* Turn off self refresh if both pipes are enabled */
> +		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
> +					& ~FW_BLC_SELF_EN);
>  	}
>  
>  	DRM_DEBUG_KMS("Setting FIFO watermarks - A: 8, B: 8, C: 8, SR %d\n",
> @@ -2630,6 +2638,10 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
>  		if (srwm < 0)
>  			srwm = 1;
>  		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN | (srwm & 0x3f));
> +	} else {
> +		/* Turn off self refresh if both pipes are enabled */
> +		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
> +					& ~FW_BLC_SELF_EN);
>  	}
>  
>  	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d, B: %d, C: %d, SR %d\n",
> 

Great!  Looks like the docs were a bit misleading here.  Disabling
self-refresh for dual plane configs is the safest thing to do.

Eric, please apply asap.  Should probably go to stable as well.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] Disable SR when more than one pipe is enabled

[David John]

On 01/27/2010 10:21 PM, Jesse Barnes wrote:
> On Wed, 27 Jan 2010 15:19:08 +0530
> David John <davidjon@xenontk.org> wrote:
> 
>> Self Refresh should be disabled on dual plane configs.
>> Otherwise, as the SR watermark is not calculated for
>> such configs, switching to non VGA mode causes FIFO
>> underrun and display flicker.
>>
>> This fixes Korg Bug # 14897.
>>
>> Signed-off-by: David John <davidjon@xenontk.org>
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 45da78e..46d7c0c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2519,6 +2519,10 @@ static void g4x_update_wm(struct drm_device *dev,  int planea_clock,
>>  		sr_entries = roundup(sr_entries / cacheline_size, 1);
>>  		DRM_DEBUG("self-refresh entries: %d\n", sr_entries);
>>  		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
>> +	} else {
>> +		/* Turn off self refresh if both pipes are enabled */
>> +		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
>> +					& ~FW_BLC_SELF_EN);
>>  	}
>>  
>>  	DRM_DEBUG("Setting FIFO watermarks - A: %d, B: %d, SR %d\n",
>> @@ -2562,6 +2566,10 @@ static void i965_update_wm(struct drm_device *dev, int planea_clock,
>>  			srwm = 1;
>>  		srwm &= 0x3f;
>>  		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
>> +	} else {
>> +		/* Turn off self refresh if both pipes are enabled */
>> +		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
>> +					& ~FW_BLC_SELF_EN);
>>  	}
>>  
>>  	DRM_DEBUG_KMS("Setting FIFO watermarks - A: 8, B: 8, C: 8, SR %d\n",
>> @@ -2630,6 +2638,10 @@ static void i9xx_update_wm(struct drm_device *dev, int planea_clock,
>>  		if (srwm < 0)
>>  			srwm = 1;
>>  		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN | (srwm & 0x3f));
>> +	} else {
>> +		/* Turn off self refresh if both pipes are enabled */
>> +		I915_WRITE(FW_BLC_SELF, I915_READ(FW_BLC_SELF)
>> +					& ~FW_BLC_SELF_EN);
>>  	}
>>  
>>  	DRM_DEBUG_KMS("Setting FIFO watermarks - A: %d, B: %d, C: %d, SR %d\n",
>>
> 
> Great!  Looks like the docs were a bit misleading here.  Disabling
> self-refresh for dual plane configs is the safest thing to do.
> 
> Eric, please apply asap.  Should probably go to stable as well.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 

[ Adding Eric and stable (for 2.6.32 series) to CC]

Regards,
David.

Re: [PATCH] Disable SR when more than one pipe is enabled

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

On Wed, 27 Jan 2010 15:19:08 +0530, David John <davidjon@xenontk.org> wrote:
> Self Refresh should be disabled on dual plane configs.
> Otherwise, as the SR watermark is not calculated for
> such configs, switching to non VGA mode causes FIFO
> underrun and display flicker.
> 
> This fixes Korg Bug # 14897.

Applied.  Thanks!

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Fix For Korg Bug #14897 (GM45 Display Flicker)

[ykzhao]

On Wed, 2010-01-27 at 17:31 +0800, David John wrote:
> Hi Jesse, Yakui,
> 
> Sorry for the delay, but I was busy with other work last week. I got
> some time to look into this on Monday and the problem seems to be this:

Hi, David
	Very good work. Thanks for looking at this issue.
	Do you have an opportunity to try the following patch set on 2.6.33-rc5
kernel and see whether the display flicker issue can be fixed?

>http://lists.freedesktop.org/archives/intel-gfx/2010-January/005505.html	

(The first patch can be skipped as it is already in 2.6.33-rc5 kernel).

Thanks.
	Yakui

> 
> With the external display connected on VGA, intel_update_watermarks
> calls update_wm a couple of times in sequence with the following params:
> 
> 1) planea_clock = 148500 planeb_clock = 0, sr_hdisplay=1920 (VGA)
> 2) planea_clock = 148500 planeb_clock = 72330, sr_hdisplay=1366 (LVDS)
> 
> In update_wm (in my case g4x_update_wm), for 1) sr_entries is calculated
> correctly and self refresh is enabled. For 2) sr_entries remains zero
> and the SR watermark is set as such. However, SR remains
> active. On mode switch to console, this causes a FIFO underrun and the
> display flicker.
> 
> The solution that I see is to simply disable SR if more than one pipe is
> enabled. I've written a patch that does this. Also another point is that
> the G45 docs state that a particular sequence has to be followed if SR
> is to be used when only one pipe is active for Cantiga (Pg 30, G45 Vol
> 3). I didn't see any code doing this, but I did not see any untoward
> behaviour either, with only my patch applied.
> 
> I've updated the patch to fix i965 and i9xx as I assume the same problem
> will occur on those chipsets as well, but this has _not_been tested as I
> don't have the relevant platforms.
> 
> Patch follows this mail. Please let me know if any changes are required.
> 
> The VGA output powering-on on mode change mentioned in my bug report is
> a separate DRM bug for which I will send a patch later.
> 
> Regards,
> David.

Re: Fix For Korg Bug #14897 (GM45 Display Flicker)

[David John]

On 01/27/2010 07:22 PM, ykzhao wrote:
> 
> Hi, David
> 	Very good work. Thanks for looking at this issue.
> 	Do you have an opportunity to try the following patch set on 2.6.33-rc5
> kernel and see whether the display flicker issue can be fixed?
> 
>> http://lists.freedesktop.org/archives/intel-gfx/2010-January/005505.html	
> 
> (The first patch can be skipped as it is already in 2.6.33-rc5 kernel).
> 
> Thanks.
> 	Yakui
> 

Hi Yakui,

Your patchset (specifically patch #8) also fixes the problem.

Can this be merged for 2.6.33?

Thanks,
David.

Re: Fix For Korg Bug #14897 (GM45 Display Flicker)

[Jesse Barnes]

On Wed, 27 Jan 2010 20:29:38 +0530
David John <davidjon@xenontk.org> wrote:

> On 01/27/2010 07:22 PM, ykzhao wrote:
> > 
> > Hi, David
> > 	Very good work. Thanks for looking at this issue.
> > 	Do you have an opportunity to try the following patch set on 2.6.33-rc5
> > kernel and see whether the display flicker issue can be fixed?
> > 
> >> http://lists.freedesktop.org/archives/intel-gfx/2010-January/005505.html	
> > 
> > (The first patch can be skipped as it is already in 2.6.33-rc5 kernel).
> > 
> > Thanks.
> > 	Yakui
> > 
> 
> Hi Yakui,
> 
> Your patchset (specifically patch #8) also fixes the problem.
> 
> Can this be merged for 2.6.33?

For .33 your patch is probably best.  For .34 Yakui's cleanups would be
good to apply.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: Fix For Korg Bug #14897 (GM45 Display Flicker)

[Jesse Barnes]

On Wed, 27 Jan 2010 15:01:58 +0530
David John <davidjon@xenontk.org> wrote:

> Hi Jesse, Yakui,
> 
> Sorry for the delay, but I was busy with other work last week. I got
> some time to look into this on Monday and the problem seems to be this:
> 
> With the external display connected on VGA, intel_update_watermarks
> calls update_wm a couple of times in sequence with the following params:
> 
> 1) planea_clock = 148500 planeb_clock = 0, sr_hdisplay=1920 (VGA)
> 2) planea_clock = 148500 planeb_clock = 72330, sr_hdisplay=1366 (LVDS)
> 
> In update_wm (in my case g4x_update_wm), for 1) sr_entries is calculated
> correctly and self refresh is enabled. For 2) sr_entries remains zero
> and the SR watermark is set as such. However, SR remains
> active. On mode switch to console, this causes a FIFO underrun and the
> display flicker.

Ugg that's not supposed to happen.  With both planes enabled
self-refresh shouldn't be active afaik.  I guess we'll actually have to
flip the bit and turn it off in that case.

> The solution that I see is to simply disable SR if more than one pipe is
> enabled. I've written a patch that does this. Also another point is that
> the G45 docs state that a particular sequence has to be followed if SR
> is to be used when only one pipe is active for Cantiga (Pg 30, G45 Vol
> 3). I didn't see any code doing this, but I did not see any untoward
> behaviour either, with only my patch applied.

Great, I'll look for it and ack it.

> I've updated the patch to fix i965 and i9xx as I assume the same problem
> will occur on those chipsets as well, but this has _not_been tested as I
> don't have the relevant platforms.

Yeah, it's probably the safe thing to do.

> 
> Patch follows this mail. Please let me know if any changes are required.
> 
> The VGA output powering-on on mode change mentioned in my bug report is
> a separate DRM bug for which I will send a patch later.

Thanks a lot.

-- 
Jesse Barnes, Intel Open Source Technology Center