[PATCH 1/7] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.

[PATCH 1/7] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.

[Rodrigo Vivi]

This is wrong since my commit (89251b17). The intention of that
commit was to remove this one here that is also wrong anyway,
but it was forgotten.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index a04b4dc..51f0514 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -170,9 +170,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
-	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-			   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
-
 	/* Enable AUX frame sync at sink */
 	if (dev_priv->psr.aux_frame_sync)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
-- 
2.4.3

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

[PATCH 2/7] drm/i915: Fix PSR disable sequence on core platforms.

[Rodrigo Vivi]

According to spec the disable sequence is:
Driver will do the following on PSR Disable.
1. Disable PSR in PSR control register, SRD_CTL[bit 31].
2. Poll on PSR idle
3. Wait for VBlank
4. Disable VSC DIP.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 51f0514..92e2b467 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -459,6 +459,10 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->config->cpu_transcoder);
 
 	if (dev_priv->psr.active) {
 		I915_WRITE(EDP_PSR_CTL(dev),
@@ -469,6 +473,12 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
 			       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
 			DRM_ERROR("Timed out waiting for PSR Idle State\n");
 
+		intel_wait_for_vblank(dev, pipe);
+
+		I915_WRITE(ctl_reg, I915_READ(ctl_reg)
+			   & ~VIDEO_DIP_ENABLE_VSC_HSW);
+		POSTING_READ(ctl_reg);
+
 		dev_priv->psr.active = false;
 	} else {
 		WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE);
-- 
2.4.3

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

Re: [PATCH 2/7] drm/i915: Fix PSR disable sequence on core platforms.

[Zanoni, Paulo R]

Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> According to spec the disable sequence is:
> Driver will do the following on PSR Disable.
> 1. Disable PSR in PSR control register, SRD_CTL[bit 31].
> 2. Poll on PSR idle
> 3. Wait for VBlank
> 4. Disable VSC DIP.

Shouldn't this be done at intel_psr_exit() instead  of
intel_psr_disable()?

In case it's yes (which is my guess), then the wait_for_vblank() is
probably going to slow down everything, so we may need to use some sort
of delayed work.

In case it's no, then I don't think we need the wait_for_vblank() since
the encoder disable only happens after the pipe disable.

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index 51f0514..92e2b467 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -459,6 +459,10 @@ static void hsw_psr_disable(struct intel_dp 
> *intel_dp)
>  	struct intel_digital_port *intel_dig_port = 
> dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->config
> ->cpu_transcoder);
>  
>  	if (dev_priv->psr.active) {
>  		I915_WRITE(EDP_PSR_CTL(dev),
> @@ -469,6 +473,12 @@ static void hsw_psr_disable(struct intel_dp 
> *intel_dp)
>  			       EDP_PSR_STATUS_STATE_MASK) == 0, 
> 2000, 10))
>  			DRM_ERROR("Timed out waiting for PSR Idle 
> State\n");
>  
> +		intel_wait_for_vblank(dev, pipe);
> +
> +		I915_WRITE(ctl_reg, I915_READ(ctl_reg)
> +			   & ~VIDEO_DIP_ENABLE_VSC_HSW);
> +		POSTING_READ(ctl_reg);
> +
>  		dev_priv->psr.active = false;
>  	} else {
>  		WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & 
> EDP_PSR_ENABLE);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Fix PSR disable sequence on core platforms.

[Vivi, Rodrigo]

On Mon, 2015-08-24 at 14:14 +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> > According to spec the disable sequence is:
> > Driver will do the following on PSR Disable.
> > 1. Disable PSR in PSR control register, SRD_CTL[bit 31].
> > 2. Poll on PSR idle
> > 3. Wait for VBlank
> > 4. Disable VSC DIP.
> 
> Shouldn't this be done at intel_psr_exit() instead  of
> intel_psr_disable()?

don't think this is a good idea...

> 
> In case it's yes (which is my guess), then the wait_for_vblank() is
> probably going to slow down everything, so we may need to use some 
> sort
> of delayed work.

delayed work wouldn't help because locks would still block the exit...

> 
> In case it's no, then I don't think we need the wait_for_vblank() 
> since
> the encoder disable only happens after the pipe disable.

hm indeed... so I believe just ignore this patch for now since it works
without this...

> 
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 51f0514..92e2b467 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -459,6 +459,10 @@ static void hsw_psr_disable(struct intel_dp 
> > *intel_dp)
> >  	struct intel_digital_port *intel_dig_port = 
> > dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc = intel_dig_port->base.base.crtc;
> > +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->config
> > ->cpu_transcoder);
> >  
> >  	if (dev_priv->psr.active) {
> >  		I915_WRITE(EDP_PSR_CTL(dev),
> > @@ -469,6 +473,12 @@ static void hsw_psr_disable(struct intel_dp 
> > *intel_dp)
> >  			       EDP_PSR_STATUS_STATE_MASK) == 0, 
> > 2000, 10))
> >  			DRM_ERROR("Timed out waiting for PSR Idle 
> > State\n");
> >  
> > +		intel_wait_for_vblank(dev, pipe);
> > +
> > +		I915_WRITE(ctl_reg, I915_READ(ctl_reg)
> > +			   & ~VIDEO_DIP_ENABLE_VSC_HSW);
> > +		POSTING_READ(ctl_reg);
> > +
> >  		dev_priv->psr.active = false;
> >  	} else {
> >  		WARN_ON(I915_READ(EDP_PSR_CTL(dev)) & 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Rodrigo Vivi]

Many reasons here:

- Hardware tracking also has hidden corner cases
- Frontbuffer tracking is mature and reliable now
- Our sw exit by unseting bit 31 is really fast and reliable.

Also frontbuffer tracking flush means invalidate and flush.

So, let's rely more and do the proper meaning of flush for
all cases without any workaround.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 92e2b467..63bbab2 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -719,25 +719,9 @@ void intel_psr_flush(struct drm_device *dev,
 	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
-	if (HAS_DDI(dev)) {
-		/*
-		 * By definition every flush should mean invalidate + flush,
-		 * however on core platforms let's minimize the
-		 * disable/re-enable so we can avoid the invalidate when flip
-		 * originated the flush.
-		 */
-		if (frontbuffer_bits && origin != ORIGIN_FLIP)
-			intel_psr_exit(dev);
-	} else {
-		/*
-		 * On Valleyview and Cherryview we don't use hardware tracking
-		 * so any plane updates or cursor moves don't result in a PSR
-		 * invalidating. Which means we need to manually fake this in
-		 * software for all flushes.
-		 */
-		if (frontbuffer_bits)
-			intel_psr_exit(dev);
-	}
+	/* By definition flush = invalidate + flush */
+	if (frontbuffer_bits)
+		intel_psr_exit(dev);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->psr.work,
-- 
2.4.3

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

Re: [PATCH 3/7] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Zanoni, Paulo R]

Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> Many reasons here:
> 
> - Hardware tracking also has hidden corner cases

Can you please elaborate more on that? I really really really really
really think we should try as hard as possible to cook some IGT cases
if something is affecting us :)

> - Frontbuffer tracking is mature and reliable now
> - Our sw exit by unseting bit 31 is really fast and reliable.

But doesn't it trigger an automatic link retraining?

> 
> Also frontbuffer tracking flush means invalidate and flush.

I don't know what are the implications of this in the current context.

> 
> So, let's rely more and do the proper meaning of flush for
> all cases without any workaround.

I'm really in favor of the idea that if the HW can properly handle the
flips, we should rely on it, since in a lot of modern desktop
environments we basically do one flip per frame. Did we study how this
patch affects the PSR residency on the different cases we care about?

(yes, I know FBC is not relying on the HW for flips, but this is on the
"optimization" TODO list after we finally merge the bug fixes)

Due to the benefits of relying on the HW tracking, I think you'll have
to bring some good arguments to sell this patch to me. But a
"Testcase:" tag would totally do it :)

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index 92e2b467..63bbab2 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -719,25 +719,9 @@ void intel_psr_flush(struct drm_device *dev,
>  	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>  
> -	if (HAS_DDI(dev)) {
> -		/*
> -		 * By definition every flush should mean invalidate 
> + flush,
> -		 * however on core platforms let's minimize the
> -		 * disable/re-enable so we can avoid the invalidate 
> when flip
> -		 * originated the flush.
> -		 */
> -		if (frontbuffer_bits && origin != ORIGIN_FLIP)
> -			intel_psr_exit(dev);
> -	} else {
> -		/*
> -		 * On Valleyview and Cherryview we don't use 
> hardware tracking
> -		 * so any plane updates or cursor moves don't result 
> in a PSR
> -		 * invalidating. Which means we need to manually 
> fake this in
> -		 * software for all flushes.
> -		 */
> -		if (frontbuffer_bits)
> -			intel_psr_exit(dev);
> -	}
> +	/* By definition flush = invalidate + flush */
> +	if (frontbuffer_bits)
> +		intel_psr_exit(dev);
>  
>  	if (!dev_priv->psr.active && !dev_priv
> ->psr.busy_frontbuffer_bits)
>  		schedule_delayed_work(&dev_priv->psr.work,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: PSR: Let's rely more on frontbuffer tracking.

[Vivi, Rodrigo]

On Mon, 2015-08-24 at 14:29 +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> > Many reasons here:
> > 
> > - Hardware tracking also has hidden corner cases
> 
> Can you please elaborate more on that? I really really really really
> really think we should try as hard as possible to cook some IGT cases
> if something is affecting us :)

Nothing much to be elaborated... Just that case I mentioned with
unrecoverable frozen screen after dpms off that I couldn't reproduce
with igt.

> 
> > - Frontbuffer tracking is mature and reliable now
> > - Our sw exit by unseting bit 31 is really fast and reliable.
> 
> But doesn't it trigger an automatic link retraining?

afaik nothing different from what hw needs to do when entering PSR
already. But if this is not true we need to stop trying SW tracking at
all for all core platforms but also let userspace to control when to
enable disable PSR otherwise it would broken half of the world.


> 
> > 
> > Also frontbuffer tracking flush means invalidate and flush.
> 
> I don't know what are the implications of this in the current 
> context.
> 
> > 
> > So, let's rely more and do the proper meaning of flush for
> > all cases without any workaround.
> 
> I'm really in favor of the idea that if the HW can properly handle 
> the
> flips, we should rely on it, since in a lot of modern desktop
> environments we basically do one flip per frame. Did we study how 
> this
> patch affects the PSR residency on the different cases we care about?

I believe what affects the PSR residency for good is to remove the
100ms delay on re-enable.


> (yes, I know FBC is not relying on the HW for flips, but this is on 
> the
> "optimization" TODO list after we finally merge the bug fixes)
> 
> Due to the benefits of relying on the HW tracking, I think you'll 
> have
> to bring some good arguments to sell this patch to me. But a
> "Testcase:" tag would totally do it :)

I'm not convinced without this patch we can mask again the LPSP,
otherwise that issue Mathew Garret faced with strange scroll on firefox
on Gnome will be seen, but with LPSP we have a higher risk of
unrecoverable screens.

There is no test case for visual feeling. I could reproduce what Mathew
had seen when opened firefox in a gnome and scroolled down and up a big
page... I'm not able to create an IGT that gets this kind of feeling,
sorry...

> 
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 22 +++-------------------
> >  1 file changed, 3 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 92e2b467..63bbab2 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -719,25 +719,9 @@ void intel_psr_flush(struct drm_device *dev,
> >  	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> >  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> >  
> > -	if (HAS_DDI(dev)) {
> > -		/*
> > -		 * By definition every flush should mean 
> > invalidate 
> > + flush,
> > -		 * however on core platforms let's minimize the
> > -		 * disable/re-enable so we can avoid the 
> > invalidate 
> > when flip
> > -		 * originated the flush.
> > -		 */
> > -		if (frontbuffer_bits && origin != ORIGIN_FLIP)
> > -			intel_psr_exit(dev);
> > -	} else {
> > -		/*
> > -		 * On Valleyview and Cherryview we don't use 
> > hardware tracking
> > -		 * so any plane updates or cursor moves don't 
> > result 
> > in a PSR
> > -		 * invalidating. Which means we need to manually 
> > fake this in
> > -		 * software for all flushes.
> > -		 */
> > -		if (frontbuffer_bits)
> > -			intel_psr_exit(dev);
> > -	}
> > +	/* By definition flush = invalidate + flush */
> > +	if (frontbuffer_bits)
> > +		intel_psr_exit(dev);
> >  
> >  	if (!dev_priv->psr.active && !dev_priv
> > ->psr.busy_frontbuffer_bits)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915: PSR: Mask LPSP hw tracking back again.

[Rodrigo Vivi]

At the beginning it was masked to allow PSR at all.
Than it got removed later by my
commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking mask.")
in order to trying fixing one case reported at intel-gfx mailing list
where we were missing screen updates when runtime_pm was enabled.

However I verified that other patch that makes flush to force
invalidate also fixes this issue by itself.
commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + flush")

Mainly now that we are relying more on frontbuffer tracking it is a
good idea to mask this hw tracking again.

But besides all this above it is important to hightligh that with LPSP
unmasked we started seeing some screen freezings as reported at fd.o.

This patch fixes the unrecoverable frozen screens reported:

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436
Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91437

Cc: Ivan Mitev <ivan.mitev@gmail.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 63bbab2..d02d4e2 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -398,9 +398,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 				skl_psr_setup_su_vsc(intel_dp);
 		}
 
-		/* Avoid continuous PSR exit by masking memup and hpd */
+		/*
+		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
+		 * Also mask LPSP to avoid dependency on other drivers that
+		 * might block runtime_pm besides preventing other hw tracking
+		 * issues now we can rely on frontbuffer tracking.
+		 */
 		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD);
+			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
-- 
2.4.3

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

Re: [PATCH 4/7] drm/i915: PSR: Mask LPSP hw tracking back again.

[Daniel Vetter]

On Thu, Aug 20, 2015 at 05:55:41PM -0700, Rodrigo Vivi wrote:
> At the beginning it was masked to allow PSR at all.
> Than it got removed later by my
> commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking mask.")
> in order to trying fixing one case reported at intel-gfx mailing list
> where we were missing screen updates when runtime_pm was enabled.
> 
> However I verified that other patch that makes flush to force
> invalidate also fixes this issue by itself.
> commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + flush")
> 
> Mainly now that we are relying more on frontbuffer tracking it is a
> good idea to mask this hw tracking again.
> 
> But besides all this above it is important to hightligh that with LPSP
> unmasked we started seeing some screen freezings as reported at fd.o.
> 
> This patch fixes the unrecoverable frozen screens reported:
> 
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91437
> 
> Cc: Ivan Mitev <ivan.mitev@gmail.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Not sure whether you missed it, but did you look into pipe A interrupts
when the screen freezes? Apparently we should try to do manual link
training in that case, and failing to do so with a PSR panel pretty much
fits the description of frozen panel.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 63bbab2..d02d4e2 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -398,9 +398,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  				skl_psr_setup_su_vsc(intel_dp);
>  		}
>  
> -		/* Avoid continuous PSR exit by masking memup and hpd */
> +		/*
> +		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
> +		 * Also mask LPSP to avoid dependency on other drivers that
> +		 * might block runtime_pm besides preventing other hw tracking
> +		 * issues now we can rely on frontbuffer tracking.
> +		 */
>  		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> -			   EDP_PSR_DEBUG_MASK_HPD);
> +			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>  
>  		/* Enable PSR on the panel */
>  		hsw_psr_enable_sink(intel_dp);
> -- 
> 2.4.3
> 
> _______________________________________________
> 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: [4/7] drm/i915: PSR: Mask LPSP hw tracking back again.

[Brian Norris]

On Thu, Aug 20, 2015 at 05:55:41PM -0700, Rodrigo Vivi wrote:
> At the beginning it was masked to allow PSR at all.
> Than it got removed later by my
> commit 09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking mask.")
> in order to trying fixing one case reported at intel-gfx mailing list
> where we were missing screen updates when runtime_pm was enabled.
> 
> However I verified that other patch that makes flush to force
> invalidate also fixes this issue by itself.
> commit 169de1316c1e ("drm/i915: PSR: Flush means invalidate + flush")
> 
> Mainly now that we are relying more on frontbuffer tracking it is a
> good idea to mask this hw tracking again.
> 
> But besides all this above it is important to hightligh that with LPSP
> unmasked we started seeing some screen freezings as reported at fd.o.
> 
> This patch fixes the unrecoverable frozen screens reported:
> 
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91437
> 
> Cc: Ivan Mitev <ivan.mitev@gmail.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

FWIW:

Tested-by: Brian Norris <briannorris@chromium.org>

After patching up another regression locally, I bisected down to commit
09108b90f040 ("drm/i915: PSR: Remove Low Power HW tracking mask.") when
searching for why I could rarely get into PSR. I guess that's intended
behavior from the commit message?

    WARNING: With this patch if snd_intel_hda driver is
    running and not releasing power well properly PSR will
    constant Exit and Performance Counter will be 0.

Anyway, I guess I'm saying I'm interested in whether there is a sane
path to getting PSR going again (without disabling sound!).

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

[PATCH 5/7] drm/i915: Delay first PSR activation.

[Rodrigo Vivi]

This affects PSR on VLV, CHV, HSW and BDW.

When debuging the frozen screen caused by HW tracking with low
power state I noticed that if we keep moving the mouse non stop
you will miss the screen updates for a while. At least
until we stop moving the mouse for a small time and move again.

The actual enabling should happen immediately after
Display Port enabling sequence finished with links trained and
everything enabled. However we face many issues when enabling PSR
right after a modeset.

On VLV/CHV we face blank screens on this scenario and on HSW+
we face a recoverable frozen screen, at least until next
exit-activate sequence.

Another workaround for the same issue here would be to increase
re-enable idle time from 100 to 500 as we did for VLV/CHV.
However this patch workaround this issue in a better
way since it doesn't reduce PSR residency and also
allow us to reduce the delay time between re-enables at least
on VLV/CHV.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index d02d4e2..2be4a62 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -427,6 +427,19 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 		vlv_psr_enable_source(intel_dp);
 	}
 
+	/*
+	 * FIXME: Activation should happen immediately since this function
+	 * is just called after pipe is fully trained and enabled.
+	 * However on previous platforms we face issues when first activation
+	 * follows a modeset so quickly.
+	 *     - On VLV/CHV we get bank screen on first activation
+	 *     - On HSW/BDW we get a recoverable frozen screen until next
+	 *       exit-activate sequence.
+	 */
+	if (INTEL_INFO(dev)->gen < 9)
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(500));
+
 	dev_priv->psr.enabled = intel_dp;
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
@@ -729,8 +742,9 @@ void intel_psr_flush(struct drm_device *dev,
 		intel_psr_exit(dev);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
-		schedule_delayed_work(&dev_priv->psr.work,
-				      msecs_to_jiffies(delay_ms));
+		if (!work_busy(&dev_priv->psr.work.work))
+			schedule_delayed_work(&dev_priv->psr.work,
+					      msecs_to_jiffies(delay_ms));
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-- 
2.4.3

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

Re: [PATCH 5/7] drm/i915: Delay first PSR activation.

[Zanoni, Paulo R]

Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> This affects PSR on VLV, CHV, HSW and BDW.
> 
> When debuging the frozen screen caused by HW tracking with low
> power state I noticed that if we keep moving the mouse non stop
> you will miss the screen updates for a while. At least
> until we stop moving the mouse for a small time and move again.
> 
> The actual enabling should happen immediately after
> Display Port enabling sequence finished with links trained and
> everything enabled. However we face many issues when enabling PSR
> right after a modeset.
> 
> On VLV/CHV we face blank screens on this scenario and on HSW+
> we face a recoverable frozen screen, at least until next
> exit-activate sequence.
> 
> Another workaround for the same issue here would be to increase
> re-enable idle time from 100 to 500 as we did for VLV/CHV.
> However this patch workaround this issue in a better
> way since it doesn't reduce PSR residency and also
> allow us to reduce the delay time between re-enables at least
> on VLV/CHV.

It sounds like this could use a little more debugging. Have we tried
moving the psr enable to after intel_post_plane_update()?

The "move the cursor to reproduce bug" sounds very weird.

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index d02d4e2..2be4a62 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -427,6 +427,19 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  		vlv_psr_enable_source(intel_dp);
>  	}
>  
> +	/*
> +	 * FIXME: Activation should happen immediately since this 
> function
> +	 * is just called after pipe is fully trained and enabled.
> +	 * However on previous platforms we face issues when first 
> activation
> +	 * follows a modeset so quickly.
> +	 *     - On VLV/CHV we get bank screen on first activation
> +	 *     - On HSW/BDW we get a recoverable frozen screen until 
> next
> +	 *       exit-activate sequence.
> +	 */
> +	if (INTEL_INFO(dev)->gen < 9)
> +		schedule_delayed_work(&dev_priv->psr.work,
> +				      msecs_to_jiffies(500));
> +
>  	dev_priv->psr.enabled = intel_dp;
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
> @@ -729,8 +742,9 @@ void intel_psr_flush(struct drm_device *dev,
>  		intel_psr_exit(dev);
>  
>  	if (!dev_priv->psr.active && !dev_priv
> ->psr.busy_frontbuffer_bits)
> -		schedule_delayed_work(&dev_priv->psr.work,
> -				      msecs_to_jiffies(delay_ms));
> +		if (!work_busy(&dev_priv->psr.work.work))
> +			schedule_delayed_work(&dev_priv->psr.work,
> +					     
>  msecs_to_jiffies(delay_ms));
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Delay first PSR activation.

[Vivi, Rodrigo]

On Mon, 2015-08-24 at 17:03 +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> > This affects PSR on VLV, CHV, HSW and BDW.
> > 
> > When debuging the frozen screen caused by HW tracking with low
> > power state I noticed that if we keep moving the mouse non stop
> > you will miss the screen updates for a while. At least
> > until we stop moving the mouse for a small time and move again.
> > 
> > The actual enabling should happen immediately after
> > Display Port enabling sequence finished with links trained and
> > everything enabled. However we face many issues when enabling PSR
> > right after a modeset.
> > 
> > On VLV/CHV we face blank screens on this scenario and on HSW+
> > we face a recoverable frozen screen, at least until next
> > exit-activate sequence.
> > 
> > Another workaround for the same issue here would be to increase
> > re-enable idle time from 100 to 500 as we did for VLV/CHV.
> > However this patch workaround this issue in a better
> > way since it doesn't reduce PSR residency and also
> > allow us to reduce the delay time between re-enables at least
> > on VLV/CHV.
> 
> It sounds like this could use a little more debugging. Have we tried
> moving the psr enable to after intel_post_plane_update()?

I'm not sure if it is a good idea to enable psr to late directly, on
some attempts to enable psr late I was having trouble with fbdev
cases... but I could try... but also here we would need to be carefull
to avoid multiple unecessary calls to psr_enable...

> 
> The "move the cursor to reproduce bug" sounds very weird.

yes, it is weird indeed. And hard to explain unless you are seeing it,
so if you want I could upload a video somewhere to show you... 

> 
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index d02d4e2..2be4a62 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -427,6 +427,19 @@ void intel_psr_enable(struct intel_dp 
> > *intel_dp)
> >  		vlv_psr_enable_source(intel_dp);
> >  	}
> >  
> > +	/*
> > +	 * FIXME: Activation should happen immediately since this 
> > function
> > +	 * is just called after pipe is fully trained and enabled.
> > +	 * However on previous platforms we face issues when first 
> > activation
> > +	 * follows a modeset so quickly.
> > +	 *     - On VLV/CHV we get bank screen on first activation
> > +	 *     - On HSW/BDW we get a recoverable frozen screen 
> > until 
> > next
> > +	 *       exit-activate sequence.
> > +	 */
> > +	if (INTEL_INFO(dev)->gen < 9)
> > +		schedule_delayed_work(&dev_priv->psr.work,
> > +				      msecs_to_jiffies(500));
> > +
> >  	dev_priv->psr.enabled = intel_dp;
> >  unlock:
> >  	mutex_unlock(&dev_priv->psr.lock);
> > @@ -729,8 +742,9 @@ void intel_psr_flush(struct drm_device *dev,
> >  		intel_psr_exit(dev);
> >  
> >  	if (!dev_priv->psr.active && !dev_priv
> > ->psr.busy_frontbuffer_bits)
> > -		schedule_delayed_work(&dev_priv->psr.work,
> > -				      msecs_to_jiffies(delay_ms));
> > +		if (!work_busy(&dev_priv->psr.work.work))
> > +			schedule_delayed_work(&dev_priv->psr.work,
> > +					     
> >  msecs_to_jiffies(delay_ms));
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm/i915: Remove psr re-activation delay on HSW+.

[Rodrigo Vivi]

commit 97173eaf5f3 ("drm/i915: PSR: Increase idle_frames")
incresed the number of idle frames making PSR entry more
reliable on many panels. Also commit
"drm/i915: Delay first PSR activation." move add the
delay workaround to the right place since all errors that
made us to introduce this delay here was the first
activation after the modeset.

So, now we are able to remove the delay on re-activation
getting better PSR residency.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2be4a62..a850b7d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -723,7 +723,7 @@ void intel_psr_flush(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	enum pipe pipe;
-	int delay_ms = HAS_DDI(dev) ? 100 : 500;
+	int delay_ms = HAS_DDI(dev) ? 0 : 500;
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
-- 
2.4.3

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

[PATCH 7/7] drm/i915: Reduce PSR re-activation time for VLV/CHV.

[Rodrigo Vivi]

With commit 30886c5a ("drm/i915: VLV/CHV PSR: Increase wait delay
time before active PSR.") we fixed a blank screen when first
activation was happening immediatelly after PSR being enabled.
There we gave more time for idleness by increasing the delay
between re-activating sequences.

However, commit "drm/i915: Delay first PSR activation."
delay the first activation in a better way keeping a good psr
residency. So, we can now reduce the delay on re-enable.

Unfortunately we cannot reduce to 0 as on core platforms
because on SW mode the transiction to active state
happens immediatelly and panel needs some idle frames.

However instead to reduce it back to 100ms let's propperly
calculate the frame time and wait at least 2 frame time so
we assure 1 entire vblank period.

I also avoided wait_for_vblank to avoid long period block
on psr.lock. And also I calculated the time only once per
enable so we avoid re-calculating this time every exit.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e0f3f05..0e2cb35 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -984,6 +984,7 @@ struct i915_psr {
 	unsigned busy_frontbuffer_bits;
 	bool psr2_support;
 	bool aux_frame_sync;
+	int sw_idle_frame_delay;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index a850b7d..7517207 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -364,6 +364,8 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc);
+	struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
+	int frame_time, idle_frames;
 
 	if (!HAS_PSR(dev)) {
 		DRM_DEBUG_KMS("PSR not supported on this platform\n");
@@ -425,6 +427,15 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 		 * to active transition, i.e. here.
 		 */
 		vlv_psr_enable_source(intel_dp);
+
+		/*
+		 * On VLV/CHV the transition to PSR active happens
+		 * immediatelly on SW mode. So we need to simulate
+		 * idle_frames to respect panel limitations.
+		 */
+		frame_time = DIV_ROUND_UP(1000, drm_mode_vrefresh(mode));
+		idle_frames = dev_priv->vbt.psr.idle_frames + 2;
+		dev_priv->psr.sw_idle_frame_delay = idle_frames * frame_time;
 	}
 
 	/*
@@ -723,7 +734,7 @@ void intel_psr_flush(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	enum pipe pipe;
-	int delay_ms = HAS_DDI(dev) ? 0 : 500;
+	int delay_ms = dev_priv->psr.sw_idle_frame_delay;
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
-- 
2.4.3

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

Re: [PATCH 7/7] drm/i915: Reduce PSR re-activation time for VLV/CHV.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7240
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
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 1/7] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.

[Zanoni, Paulo R]

Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> This is wrong since my commit (89251b17). The intention of that
> commit was to remove this one here that is also wrong anyway,
> but it was forgotten.

You mentioned the current code is wrong, but it would be really nice if
you could describe why it is wrong and what are the implications of it
being wrong. Was this causing some specific problem or was it just
found by code inspection? In case it was found just by code inspection,
you could try to elaborate on the possible problems brought by the bad
commit.

A more elaborate description helps not only the reviewers, but also the
possible backporters and even helps us judging whether it should be
merged by Daniel or Jani.

Anyway, as far as I see the patch looks correct, so if you could
provide another paragraph for Daniel to amend on the commit message:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index a04b4dc..51f0514 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -170,9 +170,6 @@ static void hsw_psr_enable_sink(struct intel_dp 
> *intel_dp)
>  
>  	aux_clock_divider = intel_dp
> ->get_aux_clock_divider(intel_dp, 0);
>  
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> -			   DP_PSR_ENABLE & 
> ~DP_PSR_MAIN_LINK_ACTIVE);
> -
>  	/* Enable AUX frame sync at sink */
>  	if (dev_priv->psr.aux_frame_sync)
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.

[Vivi, Rodrigo]

On Mon, 2015-08-24 at 14:04 +0000, Zanoni, Paulo R wrote:
> Em Qui, 2015-08-20 às 17:55 -0700, Rodrigo Vivi escreveu:
> > This is wrong since my commit (89251b17). The intention of that
> > commit was to remove this one here that is also wrong anyway,
> > but it was forgotten.
> 
> You mentioned the current code is wrong, but it would be really nice 
> if
> you could describe why it is wrong and what are the implications of 
> it
> being wrong. Was this causing some specific problem or was it just
> found by code inspection? In case it was found just by code 
> inspection,
> you could try to elaborate on the possible problems brought by the 
> bad
> commit.

nothing critical, by wrong I meant "& ~DP_PSR_MAIN_LINK_ACTIVE" doesn't
do anything at all.... I don't remember if when I added I intended to
be informative or I removed the val read from dpdc and forgot to remove
this part...

> 
> A more elaborate description helps not only the reviewers, but also 
> the
> possible backporters and even helps us judging whether it should be
> merged by Daniel or Jani.
> 
> Anyway, as far as I see the patch looks correct, so if you could
> provide another paragraph for Daniel to amend on the commit message:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks

> 
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index a04b4dc..51f0514 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -170,9 +170,6 @@ static void hsw_psr_enable_sink(struct intel_dp 
> > *intel_dp)
> >  
> >  	aux_clock_divider = intel_dp
> > ->get_aux_clock_divider(intel_dp, 0);
> >  
> > -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> > -			   DP_PSR_ENABLE & 
> > ~DP_PSR_MAIN_LINK_ACTIVE);
> > -
> >  	/* Enable AUX frame sync at sink */
> >  	if (dev_priv->psr.aux_frame_sync)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [1/7] drm/i915: Remove duplicated dpcd write on hsw_psr_enable_sink.

[Brian Norris]

On Thu, Aug 20, 2015 at 05:55:38PM -0700, Rodrigo Vivi wrote:
> This is wrong since my commit (89251b17). The intention of that
> commit was to remove this one here that is also wrong anyway,
> but it was forgotten.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Tested-by: Brian Norris <briannorris@chromium.org>

(caveat below)

I was just debugging some PSR issues on Broadwell and ran across your
commit 89251b17. I was also left wondering:
(1) why the duplicate DPCD write to DP_PSR_EN_CFG and
(2) why the '& ~DP_PSR_MAIN_LINK_ACTIVE' mask

Anyway, I was going to send a similar patch soon :)

*** Testing caveat: ***

PSR is still broken on my Broadwell system as of commit 3301d4092106
("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic"). I've done a
partial revert of this commit (but made sure I avoid link standby, as
commit 89251b177b58 ("drm/i915: PSR: deprecate link_standby support for
core platforms.") rightly argues).

But I'll send a proper mail for this issue separately, rather than
carrying on about it here on an unrelated patch.

*** End caveat ***

Regards,
Brian

> ---
> drivers/gpu/drm/i915/intel_psr.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index a04b4dc..51f0514 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -170,9 +170,6 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>  
>  	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
>  
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> -			   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
> -
>  	/* Enable AUX frame sync at sink */
>  	if (dev_priv->psr.aux_frame_sync)
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx