[PATCH] drm/i915: Fix IPS related flicker

[PATCH] drm/i915: Fix IPS related flicker

[Rodrigo Vivi]

We cannot let IPS enabled with no plane on the pipe:

BSpec: "IPS cannot be enabled until after at least one plane has
been enabled for at least one vertical blank." and "IPS must be
disabled while there is still at least one plane enabled on the
same pipe as IPS.

However this shortcut path to make primary plane invisible when
updating primary plane was leting IPS enabled while there was no
other plane enabled on the pipe causing flickerings that we were
believing that it was caused by that other restriction where
ips cannot be used when pixel rate is greater than 95% of cdclok.

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
Cc: Joe Konno <joe.konno@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9d2d6fb..5519d56 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2789,6 +2789,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 	int pixel_size;
 
 	if (!visible || !fb) {
+		/*
+		 * This shortcut path disables the primary plane making
+		 * IPS really sad and lost when it is enabled alone with no
+		 * plane around on the pipe. So, for now it is safe to disable
+		 * IPS here.
+		 /
+		 /*
+		 * FIXME: Any other plane enabled should be enough so it would
+		 * be better to check if there is really no sprite or
+		 * cursor around.
+		 */
+		hsw_disable_ips(intel_crtc);
+
 		I915_WRITE(reg, 0);
 		I915_WRITE(DSPSURF(plane), 0);
 		POSTING_READ(reg);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[Daniel Vetter]

On Thu, May 21, 2015 at 12:53:03PM -0700, Rodrigo Vivi wrote:
> We cannot let IPS enabled with no plane on the pipe:
> 
> BSpec: "IPS cannot be enabled until after at least one plane has
> been enabled for at least one vertical blank." and "IPS must be
> disabled while there is still at least one plane enabled on the
> same pipe as IPS.
> 
> However this shortcut path to make primary plane invisible when
> updating primary plane was leting IPS enabled while there was no
> other plane enabled on the pipe causing flickerings that we were
> believing that it was caused by that other restriction where
> ips cannot be used when pixel rate is greater than 95% of cdclok.
> 
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> Cc: Joe Konno <joe.konno@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Broken since forever or regression? cc: stable? Issue on bdw only or also
hsw? Please help out your poor clueless maintainers ;-)

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9d2d6fb..5519d56 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2789,6 +2789,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	int pixel_size;
>  
>  	if (!visible || !fb) {
> +		/*
> +		 * This shortcut path disables the primary plane making
> +		 * IPS really sad and lost when it is enabled alone with no
> +		 * plane around on the pipe. So, for now it is safe to disable
> +		 * IPS here.
> +		 /
> +		 /*
> +		 * FIXME: Any other plane enabled should be enough so it would
> +		 * be better to check if there is really no sprite or
> +		 * cursor around.
> +		 */
> +		hsw_disable_ips(intel_crtc);
> +
>  		I915_WRITE(reg, 0);
>  		I915_WRITE(DSPSURF(plane), 0);
>  		POSTING_READ(reg);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[Vivi, Rodrigo]

On Thu, 2015-05-21 at 23:33 +0200, Daniel Vetter wrote:
> On Thu, May 21, 2015 at 12:53:03PM -0700, Rodrigo Vivi wrote:
> > We cannot let IPS enabled with no plane on the pipe:
> > 
> > BSpec: "IPS cannot be enabled until after at least one plane has
> > been enabled for at least one vertical blank." and "IPS must be
> > disabled while there is still at least one plane enabled on the
> > same pipe as IPS.
> > 
> > However this shortcut path to make primary plane invisible when
> > updating primary plane was leting IPS enabled while there was no
> > other plane enabled on the pipe causing flickerings that we were
> > believing that it was caused by that other restriction where
> > ips cannot be used when pixel rate is greater than 95% of cdclok.
> > 
> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> > Cc: Joe Konno <joe.konno@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Broken since forever or regression?

Not sure, but probably a regression since I was looking to a 3.14 based
kernel here and didn't find this shortcut setting DSPCNTRL to 0 on
update_primary_plane.

>  cc: stable?

Yes, I should've added, sorry!

> Issue on bdw only or also
> hsw? 

Also on HSW since this limitation is also documented on HSW.

> Please help out your poor clueless maintainers ;-)

Sorry for my poor comments...

> 
> Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9d2d6fb..5519d56 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2789,6 +2789,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
> >  	int pixel_size;
> >  
> >  	if (!visible || !fb) {
> > +		/*
> > +		 * This shortcut path disables the primary plane making
> > +		 * IPS really sad and lost when it is enabled alone with no
> > +		 * plane around on the pipe. So, for now it is safe to disable
> > +		 * IPS here.
> > +		 /
Bikesheding my on patch: I forgot a "*" here above

> > +		 /*
> > +		 * FIXME: Any other plane enabled should be enough so it would
> > +		 * be better to check if there is really no sprite or
> > +		 * cursor around.
Bikesheding my on patch: I'm not sure if we should add this FIXME
here...

> > +		 */
> > +		hsw_disable_ips(intel_crtc);
> > +
> >  		I915_WRITE(reg, 0);
> >  		I915_WRITE(DSPSURF(plane), 0);
> >  		POSTING_READ(reg);
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[Ville Syrjälä]

On Thu, May 21, 2015 at 12:53:03PM -0700, Rodrigo Vivi wrote:
> We cannot let IPS enabled with no plane on the pipe:
> 
> BSpec: "IPS cannot be enabled until after at least one plane has
> been enabled for at least one vertical blank." and "IPS must be
> disabled while there is still at least one plane enabled on the
> same pipe as IPS.
> 
> However this shortcut path to make primary plane invisible when
> updating primary plane was leting IPS enabled while there was no
> other plane enabled on the pipe causing flickerings that we were
> believing that it was caused by that other restriction where
> ips cannot be used when pixel rate is greater than 95% of cdclok.
> 
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> Cc: Joe Konno <joe.konno@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9d2d6fb..5519d56 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2789,6 +2789,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	int pixel_size;
>  
>  	if (!visible || !fb) {
> +		/*
> +		 * This shortcut path disables the primary plane making
> +		 * IPS really sad and lost when it is enabled alone with no
> +		 * plane around on the pipe. So, for now it is safe to disable
> +		 * IPS here.
> +		 /
> +		 /*
> +		 * FIXME: Any other plane enabled should be enough so it would
> +		 * be better to check if there is really no sprite or
> +		 * cursor around.
> +		 */
> +		hsw_disable_ips(intel_crtc);
> +

This isn't really the right place for it. It should be called by higher
level functions outside of the atomic critical section. I suppose we're
missing it from some legacy path.

>  		I915_WRITE(reg, 0);
>  		I915_WRITE(DSPSURF(plane), 0);
>  		POSTING_READ(reg);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Fix IPS related flicker

[Rodrigo Vivi]

We cannot let IPS enabled with no plane on the pipe:

BSpec: "IPS cannot be enabled until after at least one plane has
been enabled for at least one vertical blank." and "IPS must be
disabled while there is still at least one plane enabled on the
same pipe as IPS." This restriction apply to HSW and BDW.

However a shortcut path on update primary plane function
to make primary plane invisible by setting DSPCTRL to 0
was leting IPS enabled while there was no
other plane enabled on the pipe causing flickerings that we were
believing that it was caused by that other restriction where
ips cannot be used when pixel rate is greater than 95% of cdclok.

v2: Don't mess with Atomic path as pointed out by Ville.

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4e3f302..5a6b17b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
 				intel_crtc->atomic.wait_vblank = true;
 		}
 
+		/*
+		 * FIXME: Actually if we will still have any other plane enabled
+		 * on the pipe we could let IPS enabled still, but for
+		 * now lets consider that when we make primary invisible
+		 * by setting DSPCNTR to 0 on update_primary_plane function
+		 * IPS needs to be disable.
+		 */
+		if (!state->visible || !fb)
+			intel_crtc->atomic.disable_ips = true;
+
 		intel_crtc->atomic.fb_bits |=
 			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
 
@@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	if (intel_crtc->atomic.disable_fbc)
 		intel_fbc_disable(dev);
 
+	if (intel_crtc->atomic.disable_ips)
+		hsw_disable_ips(intel_crtc);
+
 	if (intel_crtc->atomic.pre_disable_primary)
 		intel_pre_disable_primary(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2afb31a..1059283 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool wait_for_flips;
 	bool disable_fbc;
+	bool disable_ips;
 	bool pre_disable_primary;
 	bool update_wm;
 	unsigned disabled_planes;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[Ville Syrjälä]

On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
> We cannot let IPS enabled with no plane on the pipe:
> 
> BSpec: "IPS cannot be enabled until after at least one plane has
> been enabled for at least one vertical blank." and "IPS must be
> disabled while there is still at least one plane enabled on the
> same pipe as IPS." This restriction apply to HSW and BDW.
> 
> However a shortcut path on update primary plane function
> to make primary plane invisible by setting DSPCTRL to 0
> was leting IPS enabled while there was no
> other plane enabled on the pipe causing flickerings that we were
> believing that it was caused by that other restriction where
> ips cannot be used when pixel rate is greater than 95% of cdclok.
> 
> v2: Don't mess with Atomic path as pointed out by Ville.
> 
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4e3f302..5a6b17b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
>  				intel_crtc->atomic.wait_vblank = true;
>  		}
>  
> +		/*
> +		 * FIXME: Actually if we will still have any other plane enabled
> +		 * on the pipe we could let IPS enabled still, but for
> +		 * now lets consider that when we make primary invisible
> +		 * by setting DSPCNTR to 0 on update_primary_plane function
> +		 * IPS needs to be disable.
> +		 */
> +		if (!state->visible || !fb)
> +			intel_crtc->atomic.disable_ips = true;
> +

How could it be visible without an fb?

>  		intel_crtc->atomic.fb_bits |=
>  			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>  
> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	if (intel_crtc->atomic.disable_fbc)
>  		intel_fbc_disable(dev);
>  
> +	if (intel_crtc->atomic.disable_ips)
> +		hsw_disable_ips(intel_crtc);
> +
>  	if (intel_crtc->atomic.pre_disable_primary)
>  		intel_pre_disable_primary(crtc);

intel_pre_disable_primary() would already disable IPS. Except no one
sets .pre_disable_primary=true. OTOH that thing mostly seems to do
stuff that has nothing to do with the primary plane (cxsr disable,
fifo underrun reporting disable on gen2), so I don't think we want
to use that.

In any case we should really have the IPS state as part of the crtc
state. These global disable_foo things should just be killed IMO.
Hmm, except to do this properly we'd then need to track the hw IPS
state separately somewhere.

I guess we can just go with this for now. At least it's not really
making things worse, so (maybe with the !fb check dropped):
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2afb31a..1059283 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool wait_for_flips;
>  	bool disable_fbc;
> +	bool disable_ips;
>  	bool pre_disable_primary;
>  	bool update_wm;
>  	unsigned disabled_planes;
> -- 
> 2.1.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Rodrigo Vivi]

I just noticed that I had forgotten to reply-all...

Jani, would you consider merge this fix with the explanation above
related to Ville's question?

or do you want/need any action here?

Thanks,
Rodrigo.


---------- Forwarded message ----------
From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Date: Fri, May 29, 2015 at 9:45 AM
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
To: Ville Syrjälä <ville.syrjala@linux.intel.com>


On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
>> We cannot let IPS enabled with no plane on the pipe:
>>
>> BSpec: "IPS cannot be enabled until after at least one plane has
>> been enabled for at least one vertical blank." and "IPS must be
>> disabled while there is still at least one plane enabled on the
>> same pipe as IPS." This restriction apply to HSW and BDW.
>>
>> However a shortcut path on update primary plane function
>> to make primary plane invisible by setting DSPCTRL to 0
>> was leting IPS enabled while there was no
>> other plane enabled on the pipe causing flickerings that we were
>> believing that it was caused by that other restriction where
>> ips cannot be used when pixel rate is greater than 95% of cdclok.
>>
>> v2: Don't mess with Atomic path as pointed out by Ville.
>>
>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4e3f302..5a6b17b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
>>                               intel_crtc->atomic.wait_vblank = true;
>>               }
>>
>> +             /*
>> +              * FIXME: Actually if we will still have any other plane enabled
>> +              * on the pipe we could let IPS enabled still, but for
>> +              * now lets consider that when we make primary invisible
>> +              * by setting DSPCNTR to 0 on update_primary_plane function
>> +              * IPS needs to be disable.
>> +              */
>> +             if (!state->visible || !fb)
>> +                     intel_crtc->atomic.disable_ips = true;
>> +
>
> How could it be visible without an fb?

I don't like this !fb here as well, but I just tried to keep exactly
same if statement that makes I915_WRITE(DSPCNTRL, 0) on update primary
plane func...

>
>>               intel_crtc->atomic.fb_bits |=
>>                       INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>>
>> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>       if (intel_crtc->atomic.disable_fbc)
>>               intel_fbc_disable(dev);
>>
>> +     if (intel_crtc->atomic.disable_ips)
>> +             hsw_disable_ips(intel_crtc);
>> +
>>       if (intel_crtc->atomic.pre_disable_primary)
>>               intel_pre_disable_primary(crtc);
>
> intel_pre_disable_primary() would already disable IPS. Except no one
> sets .pre_disable_primary=true. OTOH that thing mostly seems to do
> stuff that has nothing to do with the primary plane (cxsr disable,
> fifo underrun reporting disable on gen2), so I don't think we want
> to use that.
>
> In any case we should really have the IPS state as part of the crtc
> state. These global disable_foo things should just be killed IMO.
> Hmm, except to do this properly we'd then need to track the hw IPS
> state separately somewhere.

agree.

>
> I guess we can just go with this for now. At least it's not really
> making things worse, so (maybe with the !fb check dropped):
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks

>
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 2afb31a..1059283 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
>>       /* Sleepable operations to perform before commit */
>>       bool wait_for_flips;
>>       bool disable_fbc;
>> +     bool disable_ips;
>>       bool pre_disable_primary;
>>       bool update_wm;
>>       unsigned disabled_planes;
>> --
>> 2.1.0
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br


-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Jani Nikula]

On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> I just noticed that I had forgotten to reply-all...
>
> Jani, would you consider merge this fix with the explanation above
> related to Ville's question?
>
> or do you want/need any action here?

Ville's question, I'd like Ville's ack on it.

BR,
Jani.


>
> Thanks,
> Rodrigo.
>
>
> ---------- Forwarded message ----------
> From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Date: Fri, May 29, 2015 at 9:45 AM
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
> To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>
> On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
>>> We cannot let IPS enabled with no plane on the pipe:
>>>
>>> BSpec: "IPS cannot be enabled until after at least one plane has
>>> been enabled for at least one vertical blank." and "IPS must be
>>> disabled while there is still at least one plane enabled on the
>>> same pipe as IPS." This restriction apply to HSW and BDW.
>>>
>>> However a shortcut path on update primary plane function
>>> to make primary plane invisible by setting DSPCTRL to 0
>>> was leting IPS enabled while there was no
>>> other plane enabled on the pipe causing flickerings that we were
>>> believing that it was caused by that other restriction where
>>> ips cannot be used when pixel rate is greater than 95% of cdclok.
>>>
>>> v2: Don't mess with Atomic path as pointed out by Ville.
>>>
>>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 4e3f302..5a6b17b 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
>>>                               intel_crtc->atomic.wait_vblank = true;
>>>               }
>>>
>>> +             /*
>>> +              * FIXME: Actually if we will still have any other plane enabled
>>> +              * on the pipe we could let IPS enabled still, but for
>>> +              * now lets consider that when we make primary invisible
>>> +              * by setting DSPCNTR to 0 on update_primary_plane function
>>> +              * IPS needs to be disable.
>>> +              */
>>> +             if (!state->visible || !fb)
>>> +                     intel_crtc->atomic.disable_ips = true;
>>> +
>>
>> How could it be visible without an fb?
>
> I don't like this !fb here as well, but I just tried to keep exactly
> same if statement that makes I915_WRITE(DSPCNTRL, 0) on update primary
> plane func...
>
>>
>>>               intel_crtc->atomic.fb_bits |=
>>>                       INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>>>
>>> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>>       if (intel_crtc->atomic.disable_fbc)
>>>               intel_fbc_disable(dev);
>>>
>>> +     if (intel_crtc->atomic.disable_ips)
>>> +             hsw_disable_ips(intel_crtc);
>>> +
>>>       if (intel_crtc->atomic.pre_disable_primary)
>>>               intel_pre_disable_primary(crtc);
>>
>> intel_pre_disable_primary() would already disable IPS. Except no one
>> sets .pre_disable_primary=true. OTOH that thing mostly seems to do
>> stuff that has nothing to do with the primary plane (cxsr disable,
>> fifo underrun reporting disable on gen2), so I don't think we want
>> to use that.
>>
>> In any case we should really have the IPS state as part of the crtc
>> state. These global disable_foo things should just be killed IMO.
>> Hmm, except to do this properly we'd then need to track the hw IPS
>> state separately somewhere.
>
> agree.
>
>>
>> I guess we can just go with this for now. At least it's not really
>> making things worse, so (maybe with the !fb check dropped):
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Thanks
>
>>
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 2afb31a..1059283 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
>>>       /* Sleepable operations to perform before commit */
>>>       bool wait_for_flips;
>>>       bool disable_fbc;
>>> +     bool disable_ips;
>>>       bool pre_disable_primary;
>>>       bool update_wm;
>>>       unsigned disabled_planes;
>>> --
>>> 2.1.0
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
>
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Ville Syrjälä]

On Fri, Jun 05, 2015 at 11:51:42AM +0300, Jani Nikula wrote:
> On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> > I just noticed that I had forgotten to reply-all...
> >
> > Jani, would you consider merge this fix with the explanation above
> > related to Ville's question?
> >
> > or do you want/need any action here?
> 
> Ville's question, I'd like Ville's ack on it.

It's good enough for me. This part of the driver is a quite a mess
anyway currently, so doesn't matter too much what we stick in there.

> 
> BR,
> Jani.
> 
> 
> >
> > Thanks,
> > Rodrigo.
> >
> >
> > ---------- Forwarded message ----------
> > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > Date: Fri, May 29, 2015 at 9:45 AM
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
> > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >
> > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> >> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
> >>> We cannot let IPS enabled with no plane on the pipe:
> >>>
> >>> BSpec: "IPS cannot be enabled until after at least one plane has
> >>> been enabled for at least one vertical blank." and "IPS must be
> >>> disabled while there is still at least one plane enabled on the
> >>> same pipe as IPS." This restriction apply to HSW and BDW.
> >>>
> >>> However a shortcut path on update primary plane function
> >>> to make primary plane invisible by setting DSPCTRL to 0
> >>> was leting IPS enabled while there was no
> >>> other plane enabled on the pipe causing flickerings that we were
> >>> believing that it was caused by that other restriction where
> >>> ips cannot be used when pixel rate is greater than 95% of cdclok.
> >>>
> >>> v2: Don't mess with Atomic path as pointed out by Ville.
> >>>
> >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
> >>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >>>  2 files changed, 14 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 4e3f302..5a6b17b 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
> >>>                               intel_crtc->atomic.wait_vblank = true;
> >>>               }
> >>>
> >>> +             /*
> >>> +              * FIXME: Actually if we will still have any other plane enabled
> >>> +              * on the pipe we could let IPS enabled still, but for
> >>> +              * now lets consider that when we make primary invisible
> >>> +              * by setting DSPCNTR to 0 on update_primary_plane function
> >>> +              * IPS needs to be disable.
> >>> +              */
> >>> +             if (!state->visible || !fb)
> >>> +                     intel_crtc->atomic.disable_ips = true;
> >>> +
> >>
> >> How could it be visible without an fb?
> >
> > I don't like this !fb here as well, but I just tried to keep exactly
> > same if statement that makes I915_WRITE(DSPCNTRL, 0) on update primary
> > plane func...
> >
> >>
> >>>               intel_crtc->atomic.fb_bits |=
> >>>                       INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> >>>
> >>> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
> >>>       if (intel_crtc->atomic.disable_fbc)
> >>>               intel_fbc_disable(dev);
> >>>
> >>> +     if (intel_crtc->atomic.disable_ips)
> >>> +             hsw_disable_ips(intel_crtc);
> >>> +
> >>>       if (intel_crtc->atomic.pre_disable_primary)
> >>>               intel_pre_disable_primary(crtc);
> >>
> >> intel_pre_disable_primary() would already disable IPS. Except no one
> >> sets .pre_disable_primary=true. OTOH that thing mostly seems to do
> >> stuff that has nothing to do with the primary plane (cxsr disable,
> >> fifo underrun reporting disable on gen2), so I don't think we want
> >> to use that.
> >>
> >> In any case we should really have the IPS state as part of the crtc
> >> state. These global disable_foo things should just be killed IMO.
> >> Hmm, except to do this properly we'd then need to track the hw IPS
> >> state separately somewhere.
> >
> > agree.
> >
> >>
> >> I guess we can just go with this for now. At least it's not really
> >> making things worse, so (maybe with the !fb check dropped):
> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Thanks
> >
> >>
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>> index 2afb31a..1059283 100644
> >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
> >>>       /* Sleepable operations to perform before commit */
> >>>       bool wait_for_flips;
> >>>       bool disable_fbc;
> >>> +     bool disable_ips;
> >>>       bool pre_disable_primary;
> >>>       bool update_wm;
> >>>       unsigned disabled_planes;
> >>> --
> >>> 2.1.0
> >>
> >> --
> >> Ville Syrjälä
> >> Intel OTC
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
> >
> >
> > -- 
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Ander Conselvan De Oliveira]

On Fri, 2015-06-05 at 12:11 +0300, Ville Syrjälä wrote:
> On Fri, Jun 05, 2015 at 11:51:42AM +0300, Jani Nikula wrote:
> > On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> > > I just noticed that I had forgotten to reply-all...
> > >
> > > Jani, would you consider merge this fix with the explanation above
> > > related to Ville's question?
> > >
> > > or do you want/need any action here?
> > 
> > Ville's question, I'd like Ville's ack on it.
> 
> It's good enough for me. This part of the driver is a quite a mess
> anyway currently, so doesn't matter too much what we stick in there.

Ping. Seems like this still isn't merged. Does it need more work or did
it just fall through the cracks?

Thanks,
Ander

> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > >
> > > Thanks,
> > > Rodrigo.
> > >
> > >
> > > ---------- Forwarded message ----------
> > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > > Date: Fri, May 29, 2015 at 9:45 AM
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > >
> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote:
> > >> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
> > >>> We cannot let IPS enabled with no plane on the pipe:
> > >>>
> > >>> BSpec: "IPS cannot be enabled until after at least one plane has
> > >>> been enabled for at least one vertical blank." and "IPS must be
> > >>> disabled while there is still at least one plane enabled on the
> > >>> same pipe as IPS." This restriction apply to HSW and BDW.
> > >>>
> > >>> However a shortcut path on update primary plane function
> > >>> to make primary plane invisible by setting DSPCTRL to 0
> > >>> was leting IPS enabled while there was no
> > >>> other plane enabled on the pipe causing flickerings that we were
> > >>> believing that it was caused by that other restriction where
> > >>> ips cannot be used when pixel rate is greater than 95% of cdclok.
> > >>>
> > >>> v2: Don't mess with Atomic path as pointed out by Ville.
> > >>>
> > >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >>> ---
> > >>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
> > >>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> > >>>  2 files changed, 14 insertions(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >>> index 4e3f302..5a6b17b 100644
> > >>> --- a/drivers/gpu/drm/i915/intel_display.c
> > >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> > >>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
> > >>>                               intel_crtc->atomic.wait_vblank = true;
> > >>>               }
> > >>>
> > >>> +             /*
> > >>> +              * FIXME: Actually if we will still have any other plane enabled
> > >>> +              * on the pipe we could let IPS enabled still, but for
> > >>> +              * now lets consider that when we make primary invisible
> > >>> +              * by setting DSPCNTR to 0 on update_primary_plane function
> > >>> +              * IPS needs to be disable.
> > >>> +              */
> > >>> +             if (!state->visible || !fb)
> > >>> +                     intel_crtc->atomic.disable_ips = true;
> > >>> +
> > >>
> > >> How could it be visible without an fb?
> > >
> > > I don't like this !fb here as well, but I just tried to keep exactly
> > > same if statement that makes I915_WRITE(DSPCNTRL, 0) on update primary
> > > plane func...
> > >
> > >>
> > >>>               intel_crtc->atomic.fb_bits |=
> > >>>                       INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> > >>>
> > >>> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
> > >>>       if (intel_crtc->atomic.disable_fbc)
> > >>>               intel_fbc_disable(dev);
> > >>>
> > >>> +     if (intel_crtc->atomic.disable_ips)
> > >>> +             hsw_disable_ips(intel_crtc);
> > >>> +
> > >>>       if (intel_crtc->atomic.pre_disable_primary)
> > >>>               intel_pre_disable_primary(crtc);
> > >>
> > >> intel_pre_disable_primary() would already disable IPS. Except no one
> > >> sets .pre_disable_primary=true. OTOH that thing mostly seems to do
> > >> stuff that has nothing to do with the primary plane (cxsr disable,
> > >> fifo underrun reporting disable on gen2), so I don't think we want
> > >> to use that.
> > >>
> > >> In any case we should really have the IPS state as part of the crtc
> > >> state. These global disable_foo things should just be killed IMO.
> > >> Hmm, except to do this properly we'd then need to track the hw IPS
> > >> state separately somewhere.
> > >
> > > agree.
> > >
> > >>
> > >> I guess we can just go with this for now. At least it's not really
> > >> making things worse, so (maybe with the !fb check dropped):
> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Thanks
> > >
> > >>
> > >>>
> > >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > >>> index 2afb31a..1059283 100644
> > >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> > >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> > >>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
> > >>>       /* Sleepable operations to perform before commit */
> > >>>       bool wait_for_flips;
> > >>>       bool disable_fbc;
> > >>> +     bool disable_ips;
> > >>>       bool pre_disable_primary;
> > >>>       bool update_wm;
> > >>>       unsigned disabled_planes;
> > >>> --
> > >>> 2.1.0
> > >>
> > >> --
> > >> Ville Syrjälä
> > >> Intel OTC
> > >> _______________________________________________
> > >> Intel-gfx mailing list
> > >> Intel-gfx@lists.freedesktop.org
> > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > >
> > >
> > > --
> > > Rodrigo Vivi
> > > Blog: http://blog.vivi.eng.br
> > >
> > >
> > > -- 
> > > Rodrigo Vivi
> > > Blog: http://blog.vivi.eng.br
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Jani Nikula]

On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
> On Fri, 2015-06-05 at 12:11 +0300, Ville Syrjälä wrote:
>> On Fri, Jun 05, 2015 at 11:51:42AM +0300, Jani Nikula wrote:
>> > On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>> > > I just noticed that I had forgotten to reply-all...
>> > >
>> > > Jani, would you consider merge this fix with the explanation above
>> > > related to Ville's question?
>> > >
>> > > or do you want/need any action here?
>> > 
>> > Ville's question, I'd like Ville's ack on it.
>> 
>> It's good enough for me. This part of the driver is a quite a mess
>> anyway currently, so doesn't matter too much what we stick in there.
>
> Ping. Seems like this still isn't merged. Does it need more work or did
> it just fall through the cracks?

It fell between the cracks. I know the world isn't black and white, but
it doesn't help the maintainers when review is some shade of grey.

I've pushed this to drm-intel-next-fixes for now, but it has missed the
train for both the v4.1 release and the main drm-next feature pull
request for the v4.2 merge window. I expect this to land upstream in
v4.2-rc2, unless there's an additional drm-next pull request during the
merge window. I've added cc: stable.

Thanks for the patch, and I guess the review was, uh, "good enough for
me" now... :p

BR,
Jani.


>
> Thanks,
> Ander
>
>> 
>> > 
>> > BR,
>> > Jani.
>> > 
>> > 
>> > >
>> > > Thanks,
>> > > Rodrigo.
>> > >
>> > >
>> > > ---------- Forwarded message ----------
>> > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> > > Date: Fri, May 29, 2015 at 9:45 AM
>> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
>> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > >
>> > >
>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
>> > > <ville.syrjala@linux.intel.com> wrote:
>> > >> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
>> > >>> We cannot let IPS enabled with no plane on the pipe:
>> > >>>
>> > >>> BSpec: "IPS cannot be enabled until after at least one plane has
>> > >>> been enabled for at least one vertical blank." and "IPS must be
>> > >>> disabled while there is still at least one plane enabled on the
>> > >>> same pipe as IPS." This restriction apply to HSW and BDW.
>> > >>>
>> > >>> However a shortcut path on update primary plane function
>> > >>> to make primary plane invisible by setting DSPCTRL to 0
>> > >>> was leting IPS enabled while there was no
>> > >>> other plane enabled on the pipe causing flickerings that we were
>> > >>> believing that it was caused by that other restriction where
>> > >>> ips cannot be used when pixel rate is greater than 95% of cdclok.
>> > >>>
>> > >>> v2: Don't mess with Atomic path as pointed out by Ville.
>> > >>>
>> > >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
>> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > >>> ---
>> > >>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>> > >>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>> > >>>  2 files changed, 14 insertions(+)
>> > >>>
>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > >>> index 4e3f302..5a6b17b 100644
>> > >>> --- a/drivers/gpu/drm/i915/intel_display.c
>> > >>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> > >>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
>> > >>>                               intel_crtc->atomic.wait_vblank = true;
>> > >>>               }
>> > >>>
>> > >>> +             /*
>> > >>> +              * FIXME: Actually if we will still have any other plane enabled
>> > >>> +              * on the pipe we could let IPS enabled still, but for
>> > >>> +              * now lets consider that when we make primary invisible
>> > >>> +              * by setting DSPCNTR to 0 on update_primary_plane function
>> > >>> +              * IPS needs to be disable.
>> > >>> +              */
>> > >>> +             if (!state->visible || !fb)
>> > >>> +                     intel_crtc->atomic.disable_ips = true;
>> > >>> +
>> > >>
>> > >> How could it be visible without an fb?
>> > >
>> > > I don't like this !fb here as well, but I just tried to keep exactly
>> > > same if statement that makes I915_WRITE(DSPCNTRL, 0) on update primary
>> > > plane func...
>> > >
>> > >>
>> > >>>               intel_crtc->atomic.fb_bits |=
>> > >>>                       INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>> > >>>
>> > >>> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>> > >>>       if (intel_crtc->atomic.disable_fbc)
>> > >>>               intel_fbc_disable(dev);
>> > >>>
>> > >>> +     if (intel_crtc->atomic.disable_ips)
>> > >>> +             hsw_disable_ips(intel_crtc);
>> > >>> +
>> > >>>       if (intel_crtc->atomic.pre_disable_primary)
>> > >>>               intel_pre_disable_primary(crtc);
>> > >>
>> > >> intel_pre_disable_primary() would already disable IPS. Except no one
>> > >> sets .pre_disable_primary=true. OTOH that thing mostly seems to do
>> > >> stuff that has nothing to do with the primary plane (cxsr disable,
>> > >> fifo underrun reporting disable on gen2), so I don't think we want
>> > >> to use that.
>> > >>
>> > >> In any case we should really have the IPS state as part of the crtc
>> > >> state. These global disable_foo things should just be killed IMO.
>> > >> Hmm, except to do this properly we'd then need to track the hw IPS
>> > >> state separately somewhere.
>> > >
>> > > agree.
>> > >
>> > >>
>> > >> I guess we can just go with this for now. At least it's not really
>> > >> making things worse, so (maybe with the !fb check dropped):
>> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > >
>> > > Thanks
>> > >
>> > >>
>> > >>>
>> > >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > >>> index 2afb31a..1059283 100644
>> > >>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> > >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > >>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
>> > >>>       /* Sleepable operations to perform before commit */
>> > >>>       bool wait_for_flips;
>> > >>>       bool disable_fbc;
>> > >>> +     bool disable_ips;
>> > >>>       bool pre_disable_primary;
>> > >>>       bool update_wm;
>> > >>>       unsigned disabled_planes;
>> > >>> --
>> > >>> 2.1.0
>> > >>
>> > >> --
>> > >> Ville Syrjälä
>> > >> Intel OTC
>> > >> _______________________________________________
>> > >> Intel-gfx mailing list
>> > >> Intel-gfx@lists.freedesktop.org
>> > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> > >
>> > >
>> > >
>> > > --
>> > > Rodrigo Vivi
>> > > Blog: http://blog.vivi.eng.br
>> > >
>> > >
>> > > -- 
>> > > Rodrigo Vivi
>> > > Blog: http://blog.vivi.eng.br
>> > 
>> > -- 
>> > Jani Nikula, Intel Open Source Technology Center
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Jani Nikula]

On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
>> On Fri, 2015-06-05 at 12:11 +0300, Ville Syrjälä wrote:
>>> On Fri, Jun 05, 2015 at 11:51:42AM +0300, Jani Nikula wrote:
>>> > On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>>> > > I just noticed that I had forgotten to reply-all...
>>> > >
>>> > > Jani, would you consider merge this fix with the explanation above
>>> > > related to Ville's question?
>>> > >
>>> > > or do you want/need any action here?
>>> > 
>>> > Ville's question, I'd like Ville's ack on it.
>>> 
>>> It's good enough for me. This part of the driver is a quite a mess
>>> anyway currently, so doesn't matter too much what we stick in there.
>>
>> Ping. Seems like this still isn't merged. Does it need more work or did
>> it just fall through the cracks?
>
> It fell between the cracks. I know the world isn't black and white, but
> it doesn't help the maintainers when review is some shade of grey.
>
> I've pushed this to drm-intel-next-fixes for now, but it has missed the
> train for both the v4.1 release and the main drm-next feature pull
> request for the v4.2 merge window. I expect this to land upstream in
> v4.2-rc2, unless there's an additional drm-next pull request during the
> merge window. I've added cc: stable.
>
> Thanks for the patch, and I guess the review was, uh, "good enough for
> me" now... :p

Argh, I'll take that back. This conflicts with dinq, and while doing so
also confuses git rerere enough to uncover a previous much bigger
conflict that I have no intention of resolving again before the
weekend. I'll return to it next week. Sorry.

BR,
Jani.



>
> BR,
> Jani.
>
>
>>
>> Thanks,
>> Ander
>>
>>> 
>>> > 
>>> > BR,
>>> > Jani.
>>> > 
>>> > 
>>> > >
>>> > > Thanks,
>>> > > Rodrigo.
>>> > >
>>> > >
>>> > > ---------- Forwarded message ----------
>>> > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>> > > Date: Fri, May 29, 2015 at 9:45 AM
>>> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
>>> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> > >
>>> > >
>>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
>>> > > <ville.syrjala@linux.intel.com> wrote:
>>> > >> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
>>> > >>> We cannot let IPS enabled with no plane on the pipe:
>>> > >>>
>>> > >>> BSpec: "IPS cannot be enabled until after at least one plane has
>>> > >>> been enabled for at least one vertical blank." and "IPS must be
>>> > >>> disabled while there is still at least one plane enabled on the
>>> > >>> same pipe as IPS." This restriction apply to HSW and BDW.
>>> > >>>
>>> > >>> However a shortcut path on update primary plane function
>>> > >>> to make primary plane invisible by setting DSPCTRL to 0
>>> > >>> was leting IPS enabled while there was no
>>> > >>> other plane enabled on the pipe causing flickerings that we were
>>> > >>> believing that it was caused by that other restriction where
>>> > >>> ips cannot be used when pixel rate is greater than 95% of cdclok.
>>> > >>>
>>> > >>> v2: Don't mess with Atomic path as pointed out by Ville.
>>> > >>>
>>> > >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
>>> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> > >>> ---
>>> > >>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>>> > >>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>> > >>>  2 files changed, 14 insertions(+)
>>> > >>>
>>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> > >>> index 4e3f302..5a6b17b 100644
>>> > >>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> > >>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> > >>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
>>> > >>>                               intel_crtc->atomic.wait_vblank = true;
>>> > >>>               }
>>> > >>>
>>> > >>> +             /*
>>> > >>> +              * FIXME: Actually if we will still have any other plane enabled
>>> > >>> +              * on the pipe we could let IPS enabled still, but for
>>> > >>> +              * now lets consider that when we make primary invisible
>>> > >>> +              * by setting DSPCNTR to 0 on update_primary_plane function
>>> > >>> +              * IPS needs to be disable.
>>> > >>> +              */
>>> > >>> +             if (!state->visible || !fb)
>>> > >>> +                     intel_crtc->atomic.disable_ips = true;
>>> > >>> +
>>> > >>
>>> > >> How could it be visible without an fb?
>>> > >
>>> > > I don't like this !fb here as well, but I just tried to keep exactly
>>> > > same if statement that makes I915_WRITE(DSPCNTRL, 0) on update primary
>>> > > plane func...
>>> > >
>>> > >>
>>> > >>>               intel_crtc->atomic.fb_bits |=
>>> > >>>                       INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>>> > >>>
>>> > >>> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>> > >>>       if (intel_crtc->atomic.disable_fbc)
>>> > >>>               intel_fbc_disable(dev);
>>> > >>>
>>> > >>> +     if (intel_crtc->atomic.disable_ips)
>>> > >>> +             hsw_disable_ips(intel_crtc);
>>> > >>> +
>>> > >>>       if (intel_crtc->atomic.pre_disable_primary)
>>> > >>>               intel_pre_disable_primary(crtc);
>>> > >>
>>> > >> intel_pre_disable_primary() would already disable IPS. Except no one
>>> > >> sets .pre_disable_primary=true. OTOH that thing mostly seems to do
>>> > >> stuff that has nothing to do with the primary plane (cxsr disable,
>>> > >> fifo underrun reporting disable on gen2), so I don't think we want
>>> > >> to use that.
>>> > >>
>>> > >> In any case we should really have the IPS state as part of the crtc
>>> > >> state. These global disable_foo things should just be killed IMO.
>>> > >> Hmm, except to do this properly we'd then need to track the hw IPS
>>> > >> state separately somewhere.
>>> > >
>>> > > agree.
>>> > >
>>> > >>
>>> > >> I guess we can just go with this for now. At least it's not really
>>> > >> making things worse, so (maybe with the !fb check dropped):
>>> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> > >
>>> > > Thanks
>>> > >
>>> > >>
>>> > >>>
>>> > >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> > >>> index 2afb31a..1059283 100644
>>> > >>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> > >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> > >>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
>>> > >>>       /* Sleepable operations to perform before commit */
>>> > >>>       bool wait_for_flips;
>>> > >>>       bool disable_fbc;
>>> > >>> +     bool disable_ips;
>>> > >>>       bool pre_disable_primary;
>>> > >>>       bool update_wm;
>>> > >>>       unsigned disabled_planes;
>>> > >>> --
>>> > >>> 2.1.0
>>> > >>
>>> > >> --
>>> > >> Ville Syrjälä
>>> > >> Intel OTC
>>> > >> _______________________________________________
>>> > >> Intel-gfx mailing list
>>> > >> Intel-gfx@lists.freedesktop.org
>>> > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>> > >
>>> > >
>>> > >
>>> > > --
>>> > > Rodrigo Vivi
>>> > > Blog: http://blog.vivi.eng.br
>>> > >
>>> > >
>>> > > -- 
>>> > > Rodrigo Vivi
>>> > > Blog: http://blog.vivi.eng.br
>>> > 
>>> > -- 
>>> > Jani Nikula, Intel Open Source Technology Center
>>> > _______________________________________________
>>> > Intel-gfx mailing list
>>> > Intel-gfx@lists.freedesktop.org
>>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>> 
>>
>>
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Jani Nikula]

On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
>>> On Fri, 2015-06-05 at 12:11 +0300, Ville Syrjälä wrote:
>>>> On Fri, Jun 05, 2015 at 11:51:42AM +0300, Jani Nikula wrote:
>>>> > On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>>>> > > I just noticed that I had forgotten to reply-all...
>>>> > >
>>>> > > Jani, would you consider merge this fix with the explanation above
>>>> > > related to Ville's question?
>>>> > >
>>>> > > or do you want/need any action here?
>>>> > 
>>>> > Ville's question, I'd like Ville's ack on it.
>>>> 
>>>> It's good enough for me. This part of the driver is a quite a mess
>>>> anyway currently, so doesn't matter too much what we stick in there.
>>>
>>> Ping. Seems like this still isn't merged. Does it need more work or did
>>> it just fall through the cracks?
>>
>> It fell between the cracks. I know the world isn't black and white, but
>> it doesn't help the maintainers when review is some shade of grey.
>>
>> I've pushed this to drm-intel-next-fixes for now, but it has missed the
>> train for both the v4.1 release and the main drm-next feature pull
>> request for the v4.2 merge window. I expect this to land upstream in
>> v4.2-rc2, unless there's an additional drm-next pull request during the
>> merge window. I've added cc: stable.
>>
>> Thanks for the patch, and I guess the review was, uh, "good enough for
>> me" now... :p
>
> Argh, I'll take that back. This conflicts with dinq, and while doing so
> also confuses git rerere enough to uncover a previous much bigger
> conflict that I have no intention of resolving again before the
> weekend. I'll return to it next week. Sorry.

And keeps conflicting too badly for me to figure out what needs to be
done. Is this still needed in dinq?

BR,
Jani.


>
> BR,
> Jani.
>
>
>
>>
>> BR,
>> Jani.
>>
>>
>>>
>>> Thanks,
>>> Ander
>>>
>>>> 
>>>> > 
>>>> > BR,
>>>> > Jani.
>>>> > 
>>>> > 
>>>> > >
>>>> > > Thanks,
>>>> > > Rodrigo.
>>>> > >
>>>> > >
>>>> > > ---------- Forwarded message ----------
>>>> > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>>> > > Date: Fri, May 29, 2015 at 9:45 AM
>>>> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
>>>> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> > >
>>>> > >
>>>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
>>>> > > <ville.syrjala@linux.intel.com> wrote:
>>>> > >> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
>>>> > >>> We cannot let IPS enabled with no plane on the pipe:
>>>> > >>>
>>>> > >>> BSpec: "IPS cannot be enabled until after at least one plane has
>>>> > >>> been enabled for at least one vertical blank." and "IPS must be
>>>> > >>> disabled while there is still at least one plane enabled on the
>>>> > >>> same pipe as IPS." This restriction apply to HSW and BDW.
>>>> > >>>
>>>> > >>> However a shortcut path on update primary plane function
>>>> > >>> to make primary plane invisible by setting DSPCTRL to 0
>>>> > >>> was leting IPS enabled while there was no
>>>> > >>> other plane enabled on the pipe causing flickerings that we were
>>>> > >>> believing that it was caused by that other restriction where
>>>> > >>> ips cannot be used when pixel rate is greater than 95% of cdclok.
>>>> > >>>
>>>> > >>> v2: Don't mess with Atomic path as pointed out by Ville.
>>>> > >>>
>>>> > >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
>>>> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> > >>> ---
>>>> > >>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>>>> > >>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>>> > >>>  2 files changed, 14 insertions(+)
>>>> > >>>
>>>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> > >>> index 4e3f302..5a6b17b 100644
>>>> > >>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> > >>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> > >>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
>>>> > >>>                               intel_crtc->atomic.wait_vblank = true;
>>>> > >>>               }
>>>> > >>>
>>>> > >>> +             /*
>>>> > >>> +              * FIXME: Actually if we will still have any other plane enabled
>>>> > >>> +              * on the pipe we could let IPS enabled still, but for
>>>> > >>> +              * now lets consider that when we make primary invisible
>>>> > >>> +              * by setting DSPCNTR to 0 on update_primary_plane function
>>>> > >>> +              * IPS needs to be disable.
>>>> > >>> +              */
>>>> > >>> +             if (!state->visible || !fb)
>>>> > >>> +                     intel_crtc->atomic.disable_ips = true;
>>>> > >>> +
>>>> > >>
>>>> > >> How could it be visible without an fb?
>>>> > >
>>>> > > I don't like this !fb here as well, but I just tried to keep exactly
>>>> > > same if statement that makes I915_WRITE(DSPCNTRL, 0) on update primary
>>>> > > plane func...
>>>> > >
>>>> > >>
>>>> > >>>               intel_crtc->atomic.fb_bits |=
>>>> > >>>                       INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>>>> > >>>
>>>> > >>> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>>> > >>>       if (intel_crtc->atomic.disable_fbc)
>>>> > >>>               intel_fbc_disable(dev);
>>>> > >>>
>>>> > >>> +     if (intel_crtc->atomic.disable_ips)
>>>> > >>> +             hsw_disable_ips(intel_crtc);
>>>> > >>> +
>>>> > >>>       if (intel_crtc->atomic.pre_disable_primary)
>>>> > >>>               intel_pre_disable_primary(crtc);
>>>> > >>
>>>> > >> intel_pre_disable_primary() would already disable IPS. Except no one
>>>> > >> sets .pre_disable_primary=true. OTOH that thing mostly seems to do
>>>> > >> stuff that has nothing to do with the primary plane (cxsr disable,
>>>> > >> fifo underrun reporting disable on gen2), so I don't think we want
>>>> > >> to use that.
>>>> > >>
>>>> > >> In any case we should really have the IPS state as part of the crtc
>>>> > >> state. These global disable_foo things should just be killed IMO.
>>>> > >> Hmm, except to do this properly we'd then need to track the hw IPS
>>>> > >> state separately somewhere.
>>>> > >
>>>> > > agree.
>>>> > >
>>>> > >>
>>>> > >> I guess we can just go with this for now. At least it's not really
>>>> > >> making things worse, so (maybe with the !fb check dropped):
>>>> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> > >
>>>> > > Thanks
>>>> > >
>>>> > >>
>>>> > >>>
>>>> > >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> > >>> index 2afb31a..1059283 100644
>>>> > >>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> > >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> > >>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
>>>> > >>>       /* Sleepable operations to perform before commit */
>>>> > >>>       bool wait_for_flips;
>>>> > >>>       bool disable_fbc;
>>>> > >>> +     bool disable_ips;
>>>> > >>>       bool pre_disable_primary;
>>>> > >>>       bool update_wm;
>>>> > >>>       unsigned disabled_planes;
>>>> > >>> --
>>>> > >>> 2.1.0
>>>> > >>
>>>> > >> --
>>>> > >> Ville Syrjälä
>>>> > >> Intel OTC
>>>> > >> _______________________________________________
>>>> > >> Intel-gfx mailing list
>>>> > >> Intel-gfx@lists.freedesktop.org
>>>> > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>> > >
>>>> > >
>>>> > >
>>>> > > --
>>>> > > Rodrigo Vivi
>>>> > > Blog: http://blog.vivi.eng.br
>>>> > >
>>>> > >
>>>> > > -- 
>>>> > > Rodrigo Vivi
>>>> > > Blog: http://blog.vivi.eng.br
>>>> > 
>>>> > -- 
>>>> > Jani Nikula, Intel Open Source Technology Center
>>>> > _______________________________________________
>>>> > Intel-gfx mailing list
>>>> > Intel-gfx@lists.freedesktop.org
>>>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>> 
>>>
>>>
>>
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 9591 bytes --]

On Thu, Jun 25, 2015 at 4:58 AM Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com>
> wrote:
> >>> On Fri, 2015-06-05 at 12:11 +0300, Ville Syrjälä wrote:
> >>>> On Fri, Jun 05, 2015 at 11:51:42AM +0300, Jani Nikula wrote:
> >>>> > On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> >>>> > > I just noticed that I had forgotten to reply-all...
> >>>> > >
> >>>> > > Jani, would you consider merge this fix with the explanation above
> >>>> > > related to Ville's question?
> >>>> > >
> >>>> > > or do you want/need any action here?
> >>>> >
> >>>> > Ville's question, I'd like Ville's ack on it.
> >>>>
> >>>> It's good enough for me. This part of the driver is a quite a mess
> >>>> anyway currently, so doesn't matter too much what we stick in there.
> >>>
> >>> Ping. Seems like this still isn't merged. Does it need more work or did
> >>> it just fall through the cracks?
> >>
> >> It fell between the cracks. I know the world isn't black and white, but
> >> it doesn't help the maintainers when review is some shade of grey.
> >>
> >> I've pushed this to drm-intel-next-fixes for now, but it has missed the
> >> train for both the v4.1 release and the main drm-next feature pull
> >> request for the v4.2 merge window. I expect this to land upstream in
> >> v4.2-rc2, unless there's an additional drm-next pull request during the
> >> merge window. I've added cc: stable.
> >>
> >> Thanks for the patch, and I guess the review was, uh, "good enough for
> >> me" now... :p
> >
> > Argh, I'll take that back. This conflicts with dinq, and while doing so
> > also confuses git rerere enough to uncover a previous much bigger
> > conflict that I have no intention of resolving again before the
> > weekend. I'll return to it next week. Sorry.
>
> And keeps conflicting too badly for me to figure out what needs to be
> done. Is this still needed in dinq?
>

To be honest, on drm-intel-nightly: 2015y-06m-17d-12h-44m-01s
with ips enabled I'm not facing the bad flicker anymore.

However since we still have the following 2 lines on
update_primary_plane_function:

if (!visible || !fb) {
I915_WRITE(reg, 0);

I believe we need this protection. Let me refactor it..


>
> BR,
> Jani.
>
>
> >
> > BR,
> > Jani.
> >
> >
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >>>
> >>> Thanks,
> >>> Ander
> >>>
> >>>>
> >>>> >
> >>>> > BR,
> >>>> > Jani.
> >>>> >
> >>>> >
> >>>> > >
> >>>> > > Thanks,
> >>>> > > Rodrigo.
> >>>> > >
> >>>> > >
> >>>> > > ---------- Forwarded message ----------
> >>>> > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >>>> > > Date: Fri, May 29, 2015 at 9:45 AM
> >>>> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
> >>>> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> > >
> >>>> > >
> >>>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
> >>>> > > <ville.syrjala@linux.intel.com> wrote:
> >>>> > >> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
> >>>> > >>> We cannot let IPS enabled with no plane on the pipe:
> >>>> > >>>
> >>>> > >>> BSpec: "IPS cannot be enabled until after at least one plane has
> >>>> > >>> been enabled for at least one vertical blank." and "IPS must be
> >>>> > >>> disabled while there is still at least one plane enabled on the
> >>>> > >>> same pipe as IPS." This restriction apply to HSW and BDW.
> >>>> > >>>
> >>>> > >>> However a shortcut path on update primary plane function
> >>>> > >>> to make primary plane invisible by setting DSPCTRL to 0
> >>>> > >>> was leting IPS enabled while there was no
> >>>> > >>> other plane enabled on the pipe causing flickerings that we were
> >>>> > >>> believing that it was caused by that other restriction where
> >>>> > >>> ips cannot be used when pixel rate is greater than 95% of
> cdclok.
> >>>> > >>>
> >>>> > >>> v2: Don't mess with Atomic path as pointed out by Ville.
> >>>> > >>>
> >>>> > >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> >>>> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>>> > >>> ---
> >>>> > >>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
> >>>> > >>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >>>> > >>>  2 files changed, 14 insertions(+)
> >>>> > >>>
> >>>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> >>>> > >>> index 4e3f302..5a6b17b 100644
> >>>> > >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> > >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> > >>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct
> drm_plane *plane,
> >>>> > >>>                               intel_crtc->atomic.wait_vblank =
> true;
> >>>> > >>>               }
> >>>> > >>>
> >>>> > >>> +             /*
> >>>> > >>> +              * FIXME: Actually if we will still have any
> other plane enabled
> >>>> > >>> +              * on the pipe we could let IPS enabled still,
> but for
> >>>> > >>> +              * now lets consider that when we make primary
> invisible
> >>>> > >>> +              * by setting DSPCNTR to 0 on
> update_primary_plane function
> >>>> > >>> +              * IPS needs to be disable.
> >>>> > >>> +              */
> >>>> > >>> +             if (!state->visible || !fb)
> >>>> > >>> +                     intel_crtc->atomic.disable_ips = true;
> >>>> > >>> +
> >>>> > >>
> >>>> > >> How could it be visible without an fb?
> >>>> > >
> >>>> > > I don't like this !fb here as well, but I just tried to keep
> exactly
> >>>> > > same if statement that makes I915_WRITE(DSPCNTRL, 0) on update
> primary
> >>>> > > plane func...
> >>>> > >
> >>>> > >>
> >>>> > >>>               intel_crtc->atomic.fb_bits |=
> >>>> > >>>
>  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> >>>> > >>>
> >>>> > >>> @@ -13406,6 +13416,9 @@ static void
> intel_begin_crtc_commit(struct drm_crtc *crtc)
> >>>> > >>>       if (intel_crtc->atomic.disable_fbc)
> >>>> > >>>               intel_fbc_disable(dev);
> >>>> > >>>
> >>>> > >>> +     if (intel_crtc->atomic.disable_ips)
> >>>> > >>> +             hsw_disable_ips(intel_crtc);
> >>>> > >>> +
> >>>> > >>>       if (intel_crtc->atomic.pre_disable_primary)
> >>>> > >>>               intel_pre_disable_primary(crtc);
> >>>> > >>
> >>>> > >> intel_pre_disable_primary() would already disable IPS. Except no
> one
> >>>> > >> sets .pre_disable_primary=true. OTOH that thing mostly seems to
> do
> >>>> > >> stuff that has nothing to do with the primary plane (cxsr
> disable,
> >>>> > >> fifo underrun reporting disable on gen2), so I don't think we
> want
> >>>> > >> to use that.
> >>>> > >>
> >>>> > >> In any case we should really have the IPS state as part of the
> crtc
> >>>> > >> state. These global disable_foo things should just be killed IMO.
> >>>> > >> Hmm, except to do this properly we'd then need to track the hw
> IPS
> >>>> > >> state separately somewhere.
> >>>> > >
> >>>> > > agree.
> >>>> > >
> >>>> > >>
> >>>> > >> I guess we can just go with this for now. At least it's not
> really
> >>>> > >> making things worse, so (maybe with the !fb check dropped):
> >>>> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> > >
> >>>> > > Thanks
> >>>> > >
> >>>> > >>
> >>>> > >>>
> >>>> > >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> >>>> > >>> index 2afb31a..1059283 100644
> >>>> > >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>>> > >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>>> > >>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
> >>>> > >>>       /* Sleepable operations to perform before commit */
> >>>> > >>>       bool wait_for_flips;
> >>>> > >>>       bool disable_fbc;
> >>>> > >>> +     bool disable_ips;
> >>>> > >>>       bool pre_disable_primary;
> >>>> > >>>       bool update_wm;
> >>>> > >>>       unsigned disabled_planes;
> >>>> > >>> --
> >>>> > >>> 2.1.0
> >>>> > >>
> >>>> > >> --
> >>>> > >> Ville Syrjälä
> >>>> > >> Intel OTC
> >>>> > >> _______________________________________________
> >>>> > >> Intel-gfx mailing list
> >>>> > >> Intel-gfx@lists.freedesktop.org
> >>>> > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>> > >
> >>>> > >
> >>>> > >
> >>>> > > --
> >>>> > > Rodrigo Vivi
> >>>> > > Blog: http://blog.vivi.eng.br
> >>>> > >
> >>>> > >
> >>>> > > --
> >>>> > > Rodrigo Vivi
> >>>> > > Blog: http://blog.vivi.eng.br
> >>>> >
> >>>> > --
> >>>> > Jani Nikula, Intel Open Source Technology Center
> >>>> > _______________________________________________
> >>>> > Intel-gfx mailing list
> >>>> > Intel-gfx@lists.freedesktop.org
> >>>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>>
> >>>
> >>>
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 15952 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: Fwd: [PATCH] drm/i915: Fix IPS related flicker

[Jani Nikula]

On Thu, 25 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Thu, Jun 25, 2015 at 4:58 AM Jani Nikula <jani.nikula@linux.intel.com>
> wrote:
>
>> On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> > On Thu, 18 Jun 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> >> On Thu, 18 Jun 2015, Ander Conselvan De Oliveira <conselvan2@gmail.com>
>> wrote:
>> >>> On Fri, 2015-06-05 at 12:11 +0300, Ville Syrjälä wrote:
>> >>>> On Fri, Jun 05, 2015 at 11:51:42AM +0300, Jani Nikula wrote:
>> >>>> > On Thu, 04 Jun 2015, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>> >>>> > > I just noticed that I had forgotten to reply-all...
>> >>>> > >
>> >>>> > > Jani, would you consider merge this fix with the explanation above
>> >>>> > > related to Ville's question?
>> >>>> > >
>> >>>> > > or do you want/need any action here?
>> >>>> >
>> >>>> > Ville's question, I'd like Ville's ack on it.
>> >>>>
>> >>>> It's good enough for me. This part of the driver is a quite a mess
>> >>>> anyway currently, so doesn't matter too much what we stick in there.
>> >>>
>> >>> Ping. Seems like this still isn't merged. Does it need more work or did
>> >>> it just fall through the cracks?
>> >>
>> >> It fell between the cracks. I know the world isn't black and white, but
>> >> it doesn't help the maintainers when review is some shade of grey.
>> >>
>> >> I've pushed this to drm-intel-next-fixes for now, but it has missed the
>> >> train for both the v4.1 release and the main drm-next feature pull
>> >> request for the v4.2 merge window. I expect this to land upstream in
>> >> v4.2-rc2, unless there's an additional drm-next pull request during the
>> >> merge window. I've added cc: stable.
>> >>
>> >> Thanks for the patch, and I guess the review was, uh, "good enough for
>> >> me" now... :p
>> >
>> > Argh, I'll take that back. This conflicts with dinq, and while doing so
>> > also confuses git rerere enough to uncover a previous much bigger
>> > conflict that I have no intention of resolving again before the
>> > weekend. I'll return to it next week. Sorry.
>>
>> And keeps conflicting too badly for me to figure out what needs to be
>> done. Is this still needed in dinq?
>>
>
> To be honest, on drm-intel-nightly: 2015y-06m-17d-12h-44m-01s
> with ips enabled I'm not facing the bad flicker anymore.
>
> However since we still have the following 2 lines on
> update_primary_plane_function:
>
> if (!visible || !fb) {
> I915_WRITE(reg, 0);
>
> I believe we need this protection. Let me refactor it..

I think we need to apply this patch to drm-intel-next-fixes (for v4.2
and cc: stable), and the refactored version on drm-intel-next-queued
(for v4.3).

BR,
Jani.

>
>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > BR,
>> > Jani.
>> >
>> >
>> >
>> >>
>> >> BR,
>> >> Jani.
>> >>
>> >>
>> >>>
>> >>> Thanks,
>> >>> Ander
>> >>>
>> >>>>
>> >>>> >
>> >>>> > BR,
>> >>>> > Jani.
>> >>>> >
>> >>>> >
>> >>>> > >
>> >>>> > > Thanks,
>> >>>> > > Rodrigo.
>> >>>> > >
>> >>>> > >
>> >>>> > > ---------- Forwarded message ----------
>> >>>> > > From: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> >>>> > > Date: Fri, May 29, 2015 at 9:45 AM
>> >>>> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix IPS related flicker
>> >>>> > > To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>>> > >
>> >>>> > >
>> >>>> > > On Fri, May 29, 2015 at 1:47 AM, Ville Syrjälä
>> >>>> > > <ville.syrjala@linux.intel.com> wrote:
>> >>>> > >> On Thu, May 28, 2015 at 11:07:11AM -0700, Rodrigo Vivi wrote:
>> >>>> > >>> We cannot let IPS enabled with no plane on the pipe:
>> >>>> > >>>
>> >>>> > >>> BSpec: "IPS cannot be enabled until after at least one plane has
>> >>>> > >>> been enabled for at least one vertical blank." and "IPS must be
>> >>>> > >>> disabled while there is still at least one plane enabled on the
>> >>>> > >>> same pipe as IPS." This restriction apply to HSW and BDW.
>> >>>> > >>>
>> >>>> > >>> However a shortcut path on update primary plane function
>> >>>> > >>> to make primary plane invisible by setting DSPCTRL to 0
>> >>>> > >>> was leting IPS enabled while there was no
>> >>>> > >>> other plane enabled on the pipe causing flickerings that we were
>> >>>> > >>> believing that it was caused by that other restriction where
>> >>>> > >>> ips cannot be used when pixel rate is greater than 95% of
>> cdclok.
>> >>>> > >>>
>> >>>> > >>> v2: Don't mess with Atomic path as pointed out by Ville.
>> >>>> > >>>
>> >>>> > >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
>> >>>> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>>> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >>>> > >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> >>>> > >>> ---
>> >>>> > >>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>> >>>> > >>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>> >>>> > >>>  2 files changed, 14 insertions(+)
>> >>>> > >>>
>> >>>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> >>>> > >>> index 4e3f302..5a6b17b 100644
>> >>>> > >>> --- a/drivers/gpu/drm/i915/intel_display.c
>> >>>> > >>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> >>>> > >>> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct
>> drm_plane *plane,
>> >>>> > >>>                               intel_crtc->atomic.wait_vblank =
>> true;
>> >>>> > >>>               }
>> >>>> > >>>
>> >>>> > >>> +             /*
>> >>>> > >>> +              * FIXME: Actually if we will still have any
>> other plane enabled
>> >>>> > >>> +              * on the pipe we could let IPS enabled still,
>> but for
>> >>>> > >>> +              * now lets consider that when we make primary
>> invisible
>> >>>> > >>> +              * by setting DSPCNTR to 0 on
>> update_primary_plane function
>> >>>> > >>> +              * IPS needs to be disable.
>> >>>> > >>> +              */
>> >>>> > >>> +             if (!state->visible || !fb)
>> >>>> > >>> +                     intel_crtc->atomic.disable_ips = true;
>> >>>> > >>> +
>> >>>> > >>
>> >>>> > >> How could it be visible without an fb?
>> >>>> > >
>> >>>> > > I don't like this !fb here as well, but I just tried to keep
>> exactly
>> >>>> > > same if statement that makes I915_WRITE(DSPCNTRL, 0) on update
>> primary
>> >>>> > > plane func...
>> >>>> > >
>> >>>> > >>
>> >>>> > >>>               intel_crtc->atomic.fb_bits |=
>> >>>> > >>>
>>  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>> >>>> > >>>
>> >>>> > >>> @@ -13406,6 +13416,9 @@ static void
>> intel_begin_crtc_commit(struct drm_crtc *crtc)
>> >>>> > >>>       if (intel_crtc->atomic.disable_fbc)
>> >>>> > >>>               intel_fbc_disable(dev);
>> >>>> > >>>
>> >>>> > >>> +     if (intel_crtc->atomic.disable_ips)
>> >>>> > >>> +             hsw_disable_ips(intel_crtc);
>> >>>> > >>> +
>> >>>> > >>>       if (intel_crtc->atomic.pre_disable_primary)
>> >>>> > >>>               intel_pre_disable_primary(crtc);
>> >>>> > >>
>> >>>> > >> intel_pre_disable_primary() would already disable IPS. Except no
>> one
>> >>>> > >> sets .pre_disable_primary=true. OTOH that thing mostly seems to
>> do
>> >>>> > >> stuff that has nothing to do with the primary plane (cxsr
>> disable,
>> >>>> > >> fifo underrun reporting disable on gen2), so I don't think we
>> want
>> >>>> > >> to use that.
>> >>>> > >>
>> >>>> > >> In any case we should really have the IPS state as part of the
>> crtc
>> >>>> > >> state. These global disable_foo things should just be killed IMO.
>> >>>> > >> Hmm, except to do this properly we'd then need to track the hw
>> IPS
>> >>>> > >> state separately somewhere.
>> >>>> > >
>> >>>> > > agree.
>> >>>> > >
>> >>>> > >>
>> >>>> > >> I guess we can just go with this for now. At least it's not
>> really
>> >>>> > >> making things worse, so (maybe with the !fb check dropped):
>> >>>> > >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>>> > >
>> >>>> > > Thanks
>> >>>> > >
>> >>>> > >>
>> >>>> > >>>
>> >>>> > >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> >>>> > >>> index 2afb31a..1059283 100644
>> >>>> > >>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> >>>> > >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> >>>> > >>> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
>> >>>> > >>>       /* Sleepable operations to perform before commit */
>> >>>> > >>>       bool wait_for_flips;
>> >>>> > >>>       bool disable_fbc;
>> >>>> > >>> +     bool disable_ips;
>> >>>> > >>>       bool pre_disable_primary;
>> >>>> > >>>       bool update_wm;
>> >>>> > >>>       unsigned disabled_planes;
>> >>>> > >>> --
>> >>>> > >>> 2.1.0
>> >>>> > >>
>> >>>> > >> --
>> >>>> > >> Ville Syrjälä
>> >>>> > >> Intel OTC
>> >>>> > >> _______________________________________________
>> >>>> > >> Intel-gfx mailing list
>> >>>> > >> Intel-gfx@lists.freedesktop.org
>> >>>> > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >>>> > >
>> >>>> > >
>> >>>> > >
>> >>>> > > --
>> >>>> > > Rodrigo Vivi
>> >>>> > > Blog: http://blog.vivi.eng.br
>> >>>> > >
>> >>>> > >
>> >>>> > > --
>> >>>> > > Rodrigo Vivi
>> >>>> > > Blog: http://blog.vivi.eng.br
>> >>>> >
>> >>>> > --
>> >>>> > Jani Nikula, Intel Open Source Technology Center
>> >>>> > _______________________________________________
>> >>>> > Intel-gfx mailing list
>> >>>> > Intel-gfx@lists.freedesktop.org
>> >>>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >>>>
>> >>>
>> >>>
>> >>
>> >> --
>> >> Jani Nikula, Intel Open Source Technology Center
>> >
>> > --
>> > Jani Nikula, Intel Open Source Technology Center
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Fix IPS related flicker

[Rodrigo Vivi]

We cannot let IPS enabled with no plane on the pipe:

BSpec: "IPS cannot be enabled until after at least one plane has
been enabled for at least one vertical blank." and "IPS must be
disabled while there is still at least one plane enabled on the
same pipe as IPS." This restriction apply to HSW and BDW.

However a shortcut path on update primary plane function
to make primary plane invisible by setting DSPCTRL to 0
was leting IPS enabled while there was no
other plane enabled on the pipe causing flickerings that we were
believing that it was caused by that other restriction where
ips cannot be used when pixel rate is greater than 95% of cdclok.

v2: Don't mess with Atomic path as pointed out by Ville.

v3: Rebase after a long time and atomic path changes.
    Accept Ville suggestion of not check !fb

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cc68e41..9b0d10b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11618,6 +11618,16 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		if (visible)
 			intel_crtc->atomic.fb_bits |=
 			    INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
+		else
+			/*
+			 * FIXME: Actually if we will still have any other
+			 * plane enabled on the pipe we could let IPS enabled
+			 * still, but for now lets consider that when we make
+			 * primary invisible by setting DSPCNTR to 0 on
+			 * update_primary_plane function IPS needs to be
+			 * disable.
+			 */
+			intel_crtc->atomic.disable_ips = true;
 
 		intel_crtc->atomic.wait_for_flips = true;
 		intel_crtc->atomic.pre_disable_primary = turn_off;
@@ -13724,6 +13734,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	if (!needs_modeset(crtc->state))
 		intel_pre_plane_update(intel_crtc);
 
+	if (intel_crtc->atomic.disable_ips)
+		hsw_disable_ips(intel_crtc);
+
 	if (intel_crtc->atomic.update_wm)
 		intel_update_watermarks(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e90c743..6ffd245 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -498,6 +498,7 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool wait_for_flips;
 	bool disable_fbc;
+	bool disable_ips;
 	bool pre_disable_primary;
 	bool update_wm;
 	unsigned disabled_planes;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[Daniel Vetter]

On Thu, Jun 25, 2015 at 09:58:22AM -0700, Rodrigo Vivi wrote:
> We cannot let IPS enabled with no plane on the pipe:
> 
> BSpec: "IPS cannot be enabled until after at least one plane has
> been enabled for at least one vertical blank." and "IPS must be
> disabled while there is still at least one plane enabled on the
> same pipe as IPS." This restriction apply to HSW and BDW.
> 
> However a shortcut path on update primary plane function
> to make primary plane invisible by setting DSPCTRL to 0
> was leting IPS enabled while there was no
> other plane enabled on the pipe causing flickerings that we were
> believing that it was caused by that other restriction where
> ips cannot be used when pixel rate is greater than 95% of cdclok.
> 
> v2: Don't mess with Atomic path as pointed out by Ville.
> 
> v3: Rebase after a long time and atomic path changes.
>     Accept Ville suggestion of not check !fb
> 
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Tested-by: Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Already conflicts since I pulled in Ville's frontbuffer_bits rework. Can
you please rebase once more?

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cc68e41..9b0d10b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11618,6 +11618,16 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		if (visible)
>  			intel_crtc->atomic.fb_bits |=
>  			    INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
> +		else
> +			/*
> +			 * FIXME: Actually if we will still have any other
> +			 * plane enabled on the pipe we could let IPS enabled
> +			 * still, but for now lets consider that when we make
> +			 * primary invisible by setting DSPCNTR to 0 on
> +			 * update_primary_plane function IPS needs to be
> +			 * disable.
> +			 */
> +			intel_crtc->atomic.disable_ips = true;
>  
>  		intel_crtc->atomic.wait_for_flips = true;
>  		intel_crtc->atomic.pre_disable_primary = turn_off;
> @@ -13724,6 +13734,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	if (!needs_modeset(crtc->state))
>  		intel_pre_plane_update(intel_crtc);
>  
> +	if (intel_crtc->atomic.disable_ips)
> +		hsw_disable_ips(intel_crtc);
> +
>  	if (intel_crtc->atomic.update_wm)
>  		intel_update_watermarks(crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e90c743..6ffd245 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -498,6 +498,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool wait_for_flips;
>  	bool disable_fbc;
> +	bool disable_ips;
>  	bool pre_disable_primary;
>  	bool update_wm;
>  	unsigned disabled_planes;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[Jani Nikula]

On Fri, 26 Jun 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jun 25, 2015 at 09:58:22AM -0700, Rodrigo Vivi wrote:
>> We cannot let IPS enabled with no plane on the pipe:
>> 
>> BSpec: "IPS cannot be enabled until after at least one plane has
>> been enabled for at least one vertical blank." and "IPS must be
>> disabled while there is still at least one plane enabled on the
>> same pipe as IPS." This restriction apply to HSW and BDW.
>> 
>> However a shortcut path on update primary plane function
>> to make primary plane invisible by setting DSPCTRL to 0
>> was leting IPS enabled while there was no
>> other plane enabled on the pipe causing flickerings that we were
>> believing that it was caused by that other restriction where
>> ips cannot be used when pixel rate is greater than 95% of cdclok.
>> 
>> v2: Don't mess with Atomic path as pointed out by Ville.
>> 
>> v3: Rebase after a long time and atomic path changes.
>>     Accept Ville suggestion of not check !fb
>> 
>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Tested-by: Kenneth Graunke <kenneth@whitecape.org>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Already conflicts since I pulled in Ville's frontbuffer_bits rework. Can
> you please rebase once more?

Preferably on top of drm-intel-next-queued directly, I think. I already
applied the earlier version to drm-intel-next-fixes.

BR,
Jani.


>
> Thanks, Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>>  2 files changed, 14 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index cc68e41..9b0d10b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11618,6 +11618,16 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  		if (visible)
>>  			intel_crtc->atomic.fb_bits |=
>>  			    INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>> +		else
>> +			/*
>> +			 * FIXME: Actually if we will still have any other
>> +			 * plane enabled on the pipe we could let IPS enabled
>> +			 * still, but for now lets consider that when we make
>> +			 * primary invisible by setting DSPCNTR to 0 on
>> +			 * update_primary_plane function IPS needs to be
>> +			 * disable.
>> +			 */
>> +			intel_crtc->atomic.disable_ips = true;
>>  
>>  		intel_crtc->atomic.wait_for_flips = true;
>>  		intel_crtc->atomic.pre_disable_primary = turn_off;
>> @@ -13724,6 +13734,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>  	if (!needs_modeset(crtc->state))
>>  		intel_pre_plane_update(intel_crtc);
>>  
>> +	if (intel_crtc->atomic.disable_ips)
>> +		hsw_disable_ips(intel_crtc);
>> +
>>  	if (intel_crtc->atomic.update_wm)
>>  		intel_update_watermarks(crtc);
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e90c743..6ffd245 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -498,6 +498,7 @@ struct intel_crtc_atomic_commit {
>>  	/* Sleepable operations to perform before commit */
>>  	bool wait_for_flips;
>>  	bool disable_fbc;
>> +	bool disable_ips;
>>  	bool pre_disable_primary;
>>  	bool update_wm;
>>  	unsigned disabled_planes;
>> -- 
>> 2.1.0
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Fix IPS related flicker

[Rodrigo Vivi]

We cannot let IPS enabled with no plane on the pipe:

BSpec: "IPS cannot be enabled until after at least one plane has
been enabled for at least one vertical blank." and "IPS must be
disabled while there is still at least one plane enabled on the
same pipe as IPS." This restriction apply to HSW and BDW.

However a shortcut path on update primary plane function
to make primary plane invisible by setting DSPCTRL to 0
was leting IPS enabled while there was no
other plane enabled on the pipe causing flickerings that we were
believing that it was caused by that other restriction where
ips cannot be used when pixel rate is greater than 95% of cdclok.

v2: Don't mess with Atomic path as pointed out by Ville.

v3: Rebase after a long time and atomic path changes.
    Accept Ville suggestion of not check !fb

v4: Re-factore on dinq

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 01eaab8..f87704a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4766,6 +4766,9 @@ static void intel_pre_plane_update(struct intel_crtc *crtc)
 		mutex_unlock(&dev->struct_mutex);
 	}
 
+	if (intel_crtc->atomic.disable_ips)
+		hsw_disable_ips(intel_crtc);
+
 	if (atomic->pre_disable_primary)
 		intel_pre_disable_primary(&crtc->base);
 }
@@ -11616,8 +11619,19 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		intel_crtc->atomic.pre_disable_primary = turn_off;
 		intel_crtc->atomic.post_enable_primary = turn_on;
 
-		if (turn_off)
+		if (turn_off) {
+			/*
+			 * FIXME: Actually if we will still have any other
+			 * plane enabled on the pipe we could let IPS enabled
+			 * still, but for now lets consider that when we make
+			 * primary invisible by setting DSPCNTR to 0 on
+			 * update_primary_plane function IPS needs to be
+			 * disable.
+			 */
+			intel_crtc->atomic.disable_ips = true;
+
 			intel_crtc->atomic.disable_fbc = true;
+		}
 
 		/*
 		 * FBC does not work on some platforms for rotated
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e016d72..33cff9d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -498,6 +498,7 @@ struct intel_crtc_atomic_commit {
 	/* Sleepable operations to perform before commit */
 	bool wait_for_flips;
 	bool disable_fbc;
+	bool disable_ips;
 	bool pre_disable_primary;
 	bool update_wm;
 	unsigned disabled_planes;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6501
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                 -1              315/315              314/315
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                 -1              320/320              319/320
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
*BDW  igt@gem_flink_race@flink_name      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_display.c:#assert_plane[i915]()@WARNING:.* at .* assert_plane
assertion_failure@assertion failure
WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[Jani Nikula]

On Thu, 28 May 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> We cannot let IPS enabled with no plane on the pipe:
>
> BSpec: "IPS cannot be enabled until after at least one plane has
> been enabled for at least one vertical blank." and "IPS must be
> disabled while there is still at least one plane enabled on the
> same pipe as IPS." This restriction apply to HSW and BDW.
>
> However a shortcut path on update primary plane function
> to make primary plane invisible by setting DSPCTRL to 0
> was leting IPS enabled while there was no
> other plane enabled on the pipe causing flickerings that we were
> believing that it was caused by that other restriction where
> ips cannot be used when pixel rate is greater than 95% of cdclok.
>
> v2: Don't mess with Atomic path as pointed out by Ville.
>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Pushed to drm-intel-next-fixes, thanks for the patch and review. I
resolved the conflicts -nightly in favor of drm-intel-next-queued, so
your other patch is needed for that.

BR,
Jani.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4e3f302..5a6b17b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13309,6 +13309,16 @@ intel_check_primary_plane(struct drm_plane *plane,
>  				intel_crtc->atomic.wait_vblank = true;
>  		}
>  
> +		/*
> +		 * FIXME: Actually if we will still have any other plane enabled
> +		 * on the pipe we could let IPS enabled still, but for
> +		 * now lets consider that when we make primary invisible
> +		 * by setting DSPCNTR to 0 on update_primary_plane function
> +		 * IPS needs to be disable.
> +		 */
> +		if (!state->visible || !fb)
> +			intel_crtc->atomic.disable_ips = true;
> +
>  		intel_crtc->atomic.fb_bits |=
>  			INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe);
>  
> @@ -13406,6 +13416,9 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	if (intel_crtc->atomic.disable_fbc)
>  		intel_fbc_disable(dev);
>  
> +	if (intel_crtc->atomic.disable_ips)
> +		hsw_disable_ips(intel_crtc);
> +
>  	if (intel_crtc->atomic.pre_disable_primary)
>  		intel_pre_disable_primary(crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2afb31a..1059283 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -485,6 +485,7 @@ struct intel_crtc_atomic_commit {
>  	/* Sleepable operations to perform before commit */
>  	bool wait_for_flips;
>  	bool disable_fbc;
> +	bool disable_ips;
>  	bool pre_disable_primary;
>  	bool update_wm;
>  	unsigned disabled_planes;
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6453
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  234/234              234/234
ILK                                  262/262              262/262
SNB                 -1              282/282              281/282
IVB                                  300/300              300/300
BYT                                  254/254              254/254
BDW                                  275/275              275/275
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(18)PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix IPS related flicker

[Kenneth Graunke]

[-- Attachment #1.1: Type: text/plain, Size: 2114 bytes --]

On Thursday, May 21, 2015 12:53:03 PM Rodrigo Vivi wrote:
> We cannot let IPS enabled with no plane on the pipe:
> 
> BSpec: "IPS cannot be enabled until after at least one plane has
> been enabled for at least one vertical blank." and "IPS must be
> disabled while there is still at least one plane enabled on the
> same pipe as IPS.
> 
> However this shortcut path to make primary plane invisible when
> updating primary plane was leting IPS enabled while there was no
> other plane enabled on the pipe causing flickerings that we were
> believing that it was caused by that other restriction where
> ips cannot be used when pixel rate is greater than 95% of cdclok.
> 
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=85583
> Cc: Joe Konno <joe.konno@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9d2d6fb..5519d56 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2789,6 +2789,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	int pixel_size;
>  
>  	if (!visible || !fb) {
> +		/*
> +		 * This shortcut path disables the primary plane making
> +		 * IPS really sad and lost when it is enabled alone with no
> +		 * plane around on the pipe. So, for now it is safe to disable
> +		 * IPS here.
> +		 /
> +		 /*
> +		 * FIXME: Any other plane enabled should be enough so it would
> +		 * be better to check if there is really no sprite or
> +		 * cursor around.
> +		 */
> +		hsw_disable_ips(intel_crtc);
> +
>  		I915_WRITE(reg, 0);
>  		I915_WRITE(DSPSURF(plane), 0);
>  		POSTING_READ(reg);

This fixes a bug on my Lenovo X250 (Broadwell GT2).

1. Boot
2. startx using SNA and KDE 5
3. Quit X

Prior to this patch, the screen was all scrambled.  With this patch, it
works perfectly.

Thanks, Rodrigo!

Tested-by: Kenneth Graunke <kenneth@whitecape.org>

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx