diff mbox

Flicker caused by "drm/i915: Eliminate usage of plane_wm_parameters from ILK-style WM code (v2)"

Message ID 20160105124932.GA3628@x61s.reliablesolutions.de (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Niehusmann Jan. 5, 2016, 12:49 p.m. UTC
Hi,

On Thu, Sep 24, 2015 at 03:53:07PM -0700, Matt Roper wrote:
> Just pull the info out of the plane state structure rather than staging
> it in an additional structure.
> 
> v2: Add 'visible' condition to sprites_scaled so that we don't limit the
>     WM level when the sprite isn't enabled.  (Ville)

It looks like this patch - specifically the visible condition in
ilk_compute_cur_wm - causes screen flicker when moving the cursor from
one screen to another one in a multi-screen setup.

My hardware is a Thinkpad x201s
(http://www.thinkwiki.org/wiki/Category:X201s)
00:02.0 VGA compatible controller [0300]: Intel Corporation Core Processor Integrated Graphics Controller [8086:0046] (rev 02),
CPU is an i7-620LM.

The screen flickering is the one the coursor is leaving. In most cases,
parts of the screen just blank for a very short moment. In some rare
cases, the screen even stays blanked until I move the cursor back to
that screen. No stability issues were observed, this seems to be a
purely cosmetic issue.

I bisected the issue to 43d59eda1f69631c267e06ab6b94ed3c14f1f6d1. As I
knew the flickering was cursor-related, I tried the following minimal
patch, which actually fixed the symptom, when applied to 4.4-rc8:



I have no idea at all if this is actually fixing anything or if it's
just hiding the real bug. All I can say is that the flickering doesn't
occur any longer.

Jan

Comments

Matt Roper Jan. 5, 2016, 9:58 p.m. UTC | #1
On Tue, Jan 05, 2016 at 01:49:33PM +0100, Jan Niehusmann wrote:
> Hi,
> 
> On Thu, Sep 24, 2015 at 03:53:07PM -0700, Matt Roper wrote:
> > Just pull the info out of the plane state structure rather than staging
> > it in an additional structure.
> > 
> > v2: Add 'visible' condition to sprites_scaled so that we don't limit the
> >     WM level when the sprite isn't enabled.  (Ville)
> 
> It looks like this patch - specifically the visible condition in
> ilk_compute_cur_wm - causes screen flicker when moving the cursor from
> one screen to another one in a multi-screen setup.
> 
> My hardware is a Thinkpad x201s
> (http://www.thinkwiki.org/wiki/Category:X201s)
> 00:02.0 VGA compatible controller [0300]: Intel Corporation Core Processor Integrated Graphics Controller [8086:0046] (rev 02),
> CPU is an i7-620LM.
> 
> The screen flickering is the one the coursor is leaving. In most cases,
> parts of the screen just blank for a very short moment. In some rare
> cases, the screen even stays blanked until I move the cursor back to
> that screen. No stability issues were observed, this seems to be a
> purely cosmetic issue.
> 
> I bisected the issue to 43d59eda1f69631c267e06ab6b94ed3c14f1f6d1. As I
> knew the flickering was cursor-related, I tried the following minimal
> patch, which actually fixed the symptom, when applied to 4.4-rc8:
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f091ad1..1ef0c54 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1791,7 +1791,7 @@ static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
>  {
>  	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
>  
> -	if (!cstate->base.active || !pstate->visible)
> +	if (!cstate->base.active)
>  		return 0;
>  
>  	return ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> 
> 
> I have no idea at all if this is actually fixing anything or if it's
> just hiding the real bug. All I can say is that the flickering doesn't
> occur any longer.
> 
> Jan

Hi Jan.  I think the flicker you're seeing is caused by our current lack
of two-stage watermark updates on platforms that use ILK-style
watermarks.  I have a patch series at
http://patchwork.freedesktop.org/bundle/mattrope/atomic_wm_ilk/ that's
supposed to address this, but it's still awaiting review at the moment
so it isn't yet available upstream.  Now that folks are coming back from
holiday vacations and such, hopefully we'll get the review finished soon
and be able to merge the patches.

The change you propose above would cause us to calculate watermarks as
if the cursor plane was on full-time (even when it's actually off
because the cursor is on the other display), so your watermarks wouldn't
need to change when you move your mouse to the other display and you
wouldn't see a flicker at that point.  If the review process for atomic
WM carries on too long, we may want to consider merging a patch like
yours as a short term workaround for the issue you're seeing.  There
would still be plenty of other ways to trigger flickering (changing
cursor size, using sprite planes (e.g., via xvideo), etc., but at least
a cursor getting enabled/disabled wouldn't cause a flicker.

Thanks.


Matt
Jan Niehusmann Jan. 6, 2016, 12:46 a.m. UTC | #2
Hi Matt,

On Tue, Jan 05, 2016 at 01:58:19PM -0800, Matt Roper wrote:
> Hi Jan.  I think the flicker you're seeing is caused by our current lack
> of two-stage watermark updates on platforms that use ILK-style
> watermarks.  I have a patch series at
> http://patchwork.freedesktop.org/bundle/mattrope/atomic_wm_ilk/ that's
> supposed to address this, but it's still awaiting review at the moment

Yes, thanks! That patch set fixes the flickering, as well.

Applied the patches, some of them manually, to 4.4-rc8,
after re-appling the reverted watermark commits
from 2791a16ca43302d07ac74cbe7c048e367c4632c4 and
261a27d11fa1dec47c47ece6968eaaba55861eca.

I didn't do much testing, but the resulting kernel boots fine and the
flickering is gone.

Jan
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f091ad1..1ef0c54 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1791,7 +1791,7 @@  static uint32_t ilk_compute_cur_wm(const struct intel_crtc_state *cstate,
 {
 	int bpp = pstate->base.fb ? pstate->base.fb->bits_per_pixel / 8 : 0;
 
-	if (!cstate->base.active || !pstate->visible)
+	if (!cstate->base.active)
 		return 0;
 
 	return ilk_wm_method2(ilk_pipe_pixel_rate(cstate),