[PATCH 0/3] More PSR improvements

[PATCH 0/3] More PSR improvements

[Rodrigo Vivi]

During the review/test cycle from the previous versions
I've captured few suggestions and tips and did more 3 patches
to make it more clean and avoid any kind of confusion.

Thanks,
Rodrigo.

*** BLURB HERE ***

Rodrigo Vivi (3):
  drm/i915: Also delay first activation for SKL+
  drm/i915: Remove PSR Perf Counter for SKL+
  drm/i915: Also disable PSR on Sink when disabling it on Source.

 drivers/gpu/drm/i915/i915_debugfs.c |  7 +++++--
 drivers/gpu/drm/i915/intel_psr.c    | 13 ++++++-------
 2 files changed, 11 insertions(+), 9 deletions(-)

-- 
2.4.3

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

[PATCH 1/3] drm/i915: Also delay first activation for SKL+

[Rodrigo Vivi]

In certain platforms we face strange and different issues
when activating PSR right after a modeset so quickly.
So we delayed the first activation for the platforms
where we saw the issues with 'commit d0ac896a477d
("drm/i915: Delay first PSR activation.")'.

So, let's apply the same delay on first activation
for SKL+ so we avoid any kind of confusion and
handle similar implentations in the same way.

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 465d36b..38ea4d0 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -420,9 +420,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
-
-		if (INTEL_INFO(dev)->gen >= 9)
-			intel_psr_activate(intel_dp);
 	} else {
 		vlv_psr_setup_vsc(intel_dp);
 
@@ -441,15 +438,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 	/*
 	 * FIXME: Activation should happen immediately since this function
 	 * is just called after pipe is fully trained and enabled.
-	 * However on every platform we face issues when first activation
+	 * However on some 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(intel_dp->panel_power_cycle_delay * 5));
+	schedule_delayed_work(&dev_priv->psr.work,
+			      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
 
 	dev_priv->psr.enabled = intel_dp;
 unlock:
-- 
2.4.3

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

[PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+

[Rodrigo Vivi]

Whenever DMC firmware put the HW into DC State a bunch
of registers including this perf counter is reset to 0 and
never restored.

So, even with PSR active and working we could still read
"Performance_Counter: 0" what will misslead people to believe
PSR is broken.

So, it is better to remove this counter information while
we don't have a better way to track PSR residency.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 038d5c6..71e1666 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2580,8 +2580,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		}
 	seq_puts(m, "\n");
 
-	/* CHV PSR has no kind of performance counter */
-	if (HAS_DDI(dev)) {
+	/*
+	 * VLV/CHV PSR has no kind of performance counter
+	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
+	 */
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
 			EDP_PSR_PERF_CNT_MASK;
 
-- 
2.4.3

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

[PATCH 3/3] drm/i915: Also disable PSR on Sink when disabling it on Source.

[Rodrigo Vivi]

It is not a bad idea to disable the PSR feature on Sink
when we are disabling on the Source.

Cc: Sonika Jindal <sonika.jindal@intel.com>
Suggested-by: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 38ea4d0..de31162 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -527,6 +527,9 @@ void intel_psr_disable(struct intel_dp *intel_dp)
 	mutex_unlock(&dev_priv->psr.lock);
 
 	cancel_delayed_work_sync(&dev_priv->psr.work);
+
+	/* Disable PSR on Sink */
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
 }
 
 static void intel_psr_work(struct work_struct *work)
-- 
2.4.3

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

Re: [PATCH 3/3] drm/i915: Also disable PSR on Sink when disabling it on Source.

[Jindal, Sonika]

Why not move it inside the mutex_lock/unlock on psr.lock ?

Regards,
Sonika

-----Original Message-----
From: Vivi, Rodrigo 
Sent: Thursday, November 19, 2015 6:10 AM
To: intel-gfx@lists.freedesktop.org
Cc: Vivi, Rodrigo; Jindal, Sonika
Subject: [PATCH 3/3] drm/i915: Also disable PSR on Sink when disabling it on Source.

It is not a bad idea to disable the PSR feature on Sink when we are disabling on the Source.

Cc: Sonika Jindal <sonika.jindal@intel.com>
Suggested-by: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 38ea4d0..de31162 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -527,6 +527,9 @@ void intel_psr_disable(struct intel_dp *intel_dp)
 	mutex_unlock(&dev_priv->psr.lock);
 
 	cancel_delayed_work_sync(&dev_priv->psr.work);
+
+	/* Disable PSR on Sink */
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
 }
 
 static void intel_psr_work(struct work_struct *work)
--
2.4.3

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

Re: [PATCH 1/3] drm/i915: Also delay first activation for SKL+

[R, Durgadoss]

>-----Original Message-----
>From: Vivi, Rodrigo
>Sent: Thursday, November 19, 2015 6:10 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo; R, Durgadoss
>Subject: [PATCH 1/3] drm/i915: Also delay first activation for SKL+
>
>In certain platforms we face strange and different issues
>when activating PSR right after a modeset so quickly.
>So we delayed the first activation for the platforms
>where we saw the issues with 'commit d0ac896a477d
>("drm/i915: Delay first PSR activation.")'.
>
>So, let's apply the same delay on first activation
>for SKL+ so we avoid any kind of confusion and
>handle similar implentations in the same way.

Thanks for the quick change Rodrigo!! 

Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga

>
>Cc: Durgadoss R <durgadoss.r@intel.com>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>---
> drivers/gpu/drm/i915/intel_psr.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>index 465d36b..38ea4d0 100644
>--- a/drivers/gpu/drm/i915/intel_psr.c
>+++ b/drivers/gpu/drm/i915/intel_psr.c
>@@ -420,9 +420,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>
> 		/* Enable PSR on the panel */
> 		hsw_psr_enable_sink(intel_dp);
>-
>-		if (INTEL_INFO(dev)->gen >= 9)
>-			intel_psr_activate(intel_dp);
> 	} else {
> 		vlv_psr_setup_vsc(intel_dp);
>
>@@ -441,15 +438,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
> 	/*
> 	 * FIXME: Activation should happen immediately since this function
> 	 * is just called after pipe is fully trained and enabled.
>-	 * However on every platform we face issues when first activation
>+	 * However on some 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(intel_dp->panel_power_cycle_delay * 5));
>+	schedule_delayed_work(&dev_priv->psr.work,
>+			      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
>
> 	dev_priv->psr.enabled = intel_dp;
> unlock:
>--
>2.4.3

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

Re: [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+

[R, Durgadoss]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Rodrigo Vivi
>Sent: Thursday, November 19, 2015 6:10 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo
>Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+
>
>Whenever DMC firmware put the HW into DC State a bunch
>of registers including this perf counter is reset to 0 and
>never restored.
>
>So, even with PSR active and working we could still read
>"Performance_Counter: 0" what will misslead people to believe
>PSR is broken.
>
>So, it is better to remove this counter information while
>we don't have a better way to track PSR residency.

Agreed..

Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga

>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>---
> drivers/gpu/drm/i915/i915_debugfs.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>index 038d5c6..71e1666 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -2580,8 +2580,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> 		}
> 	seq_puts(m, "\n");
>
>-	/* CHV PSR has no kind of performance counter */
>-	if (HAS_DDI(dev)) {
>+	/*
>+	 * VLV/CHV PSR has no kind of performance counter
>+	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
>+	 */
>+	if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> 		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> 			EDP_PSR_PERF_CNT_MASK;
>
>--
>2.4.3
>
>_______________________________________________
>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 2/3] drm/i915: Remove PSR Perf Counter for SKL+

[Jindal, Sonika]

Hi Rodrigo,

Which platform have you observed this issue on?
This looks really strange.
Have you checked whether we are able to enter PSR at sink side or not in such cases?
Are we sure we are not entering PSR? I mean the PSR_STATE register says it correctly?

Regards,
Sonika

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of R, Durgadoss
Sent: Thursday, November 19, 2015 11:39 AM
To: Vivi, Rodrigo; intel-gfx@lists.freedesktop.org
Cc: Vivi, Rodrigo
Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+



>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
>Behalf Of Rodrigo Vivi
>Sent: Thursday, November 19, 2015 6:10 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo
>Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for 
>SKL+
>
>Whenever DMC firmware put the HW into DC State a bunch of registers 
>including this perf counter is reset to 0 and never restored.
>
>So, even with PSR active and working we could still read
>"Performance_Counter: 0" what will misslead people to believe PSR is 
>broken.
>
>So, it is better to remove this counter information while we don't have 
>a better way to track PSR residency.

Agreed..

Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga

>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>---
> drivers/gpu/drm/i915/i915_debugfs.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>b/drivers/gpu/drm/i915/i915_debugfs.c
>index 038d5c6..71e1666 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -2580,8 +2580,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> 		}
> 	seq_puts(m, "\n");
>
>-	/* CHV PSR has no kind of performance counter */
>-	if (HAS_DDI(dev)) {
>+	/*
>+	 * VLV/CHV PSR has no kind of performance counter
>+	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
>+	 */
>+	if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> 		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> 			EDP_PSR_PERF_CNT_MASK;
>
>--
>2.4.3
>
>_______________________________________________
>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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Also delay first activation for SKL+

[Daniel Stone]

Hi Rodrigo,

On 19 November 2015 at 00:39, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> @@ -441,15 +438,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>         /*
>          * FIXME: Activation should happen immediately since this function
>          * is just called after pipe is fully trained and enabled.
> -        * However on every platform we face issues when first activation
> +        * However on some 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(intel_dp->panel_power_cycle_delay * 5));
> +       schedule_delayed_work(&dev_priv->psr.work,
> +                             msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));

The comment change here seems to be the exact opposite of the code
change: aren't we now seeing these issues on every platform? If not,
it would be good to elaborate on the issues seen in SKL/BSW.

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

Re: [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+

[Rodrigo Vivi]

On Wed, Nov 18, 2015 at 11:45 PM, Jindal, Sonika
<sonika.jindal@intel.com> wrote:
> Hi Rodrigo,
>
> Which platform have you observed this issue on?

Skylake and Kabylake

> This looks really strange.

It is expected actually.
On DC5/6 states all read-only registers are reset and cannot be
restored. including this counter.

> Have you checked whether we are able to enter PSR at sink side or not in such cases?
> Are we sure we are not entering PSR? I mean the PSR_STATE register says it correctly?

I'm sure PSR is working well in entry state with links off and Perf
counter goes to zero when DC5/6 states enters.
So this patch just removes the counter to avoid confusion. People
would think PSR isn't work, when it is.

>
> Regards,
> Sonika
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of R, Durgadoss
> Sent: Thursday, November 19, 2015 11:39 AM
> To: Vivi, Rodrigo; intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo
> Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+
>
>
>
>>-----Original Message-----
>>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>>Behalf Of Rodrigo Vivi
>>Sent: Thursday, November 19, 2015 6:10 AM
>>To: intel-gfx@lists.freedesktop.org
>>Cc: Vivi, Rodrigo
>>Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for
>>SKL+
>>
>>Whenever DMC firmware put the HW into DC State a bunch of registers
>>including this perf counter is reset to 0 and never restored.
>>
>>So, even with PSR active and working we could still read
>>"Performance_Counter: 0" what will misslead people to believe PSR is
>>broken.
>>
>>So, it is better to remove this counter information while we don't have
>>a better way to track PSR residency.
>
> Agreed..
>
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
>
> Thanks,
> Durga
>
>>
>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>---
>> drivers/gpu/drm/i915/i915_debugfs.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>b/drivers/gpu/drm/i915/i915_debugfs.c
>>index 038d5c6..71e1666 100644
>>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>@@ -2580,8 +2580,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>>               }
>>       seq_puts(m, "\n");
>>
>>-      /* CHV PSR has no kind of performance counter */
>>-      if (HAS_DDI(dev)) {
>>+      /*
>>+       * VLV/CHV PSR has no kind of performance counter
>>+       * SKL+ Perf counter is reset to 0 everytime DC state is entered
>>+       */
>>+      if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>>               psrperf = I915_READ(EDP_PSR_PERF_CNT) &
>>                       EDP_PSR_PERF_CNT_MASK;
>>
>>--
>>2.4.3
>>
>>_______________________________________________
>>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
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Also delay first activation for SKL+

[Vivi, Rodrigo]

On Thu, 2015-11-19 at 08:44 +0000, Daniel Stone wrote:
> Hi Rodrigo,
> 
> On 19 November 2015 at 00:39, Rodrigo Vivi <rodrigo.vivi@intel.com> 
> wrote:
> > @@ -441,15 +438,14 @@ void intel_psr_enable(struct intel_dp 
> > *intel_dp)
> >         /*
> >          * FIXME: Activation should happen immediately since this 
> > function
> >          * is just called after pipe is fully trained and enabled.
> > -        * However on every platform we face issues when first 
> > activation
> > +        * However on some 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(intel_dp
> > ->panel_power_cycle_delay * 5));
> > +       schedule_delayed_work(&dev_priv->psr.work,
> > +                             msecs_to_jiffies(intel_dp
> > ->panel_power_cycle_delay * 5));
> 
> The comment change here seems to be the exact opposite of the code
> change:

Indeed, this is why I'll keep the fixme on the comment.

>  aren't we now seeing these issues on every platform? If not,
> it would be good to elaborate on the issues seen in SKL/BSW.

So far no known issue on SKL/KBL. (BSW is CHV)
But since we have no idea why these issues are happening and separated
handling was causing confusion I preferred to handle all with the
protection.

Regarding the issue itself, the Hardware needs to communicate with the
Sink using AUX channels. I believe there is just kind of strange
conflict going on there because the failures are exactly when we are
also using aux channels for our modeset/link-trainings.
This is also why I used panel_power_cycle_delay * 5 there because it is
the same value we use to force VDD on while we are doing the aux
transactions.

Please let me know if you have further questions, concerns or
suggestions here.

Thanks
> 
> Cheers,
> Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+

[Jindal, Sonika]

Hmm, please help me understand more about it.
As per bspec this is a RW register.
Also, restoring the register values, isn't it part of the firmware's job?
Do we have any communication from HW team on this?

Regards,
Sonika

-----Original Message-----
From: Rodrigo Vivi [mailto:rodrigo.vivi@gmail.com] 
Sent: Friday, November 20, 2015 12:01 AM
To: Jindal, Sonika
Cc: R, Durgadoss; Vivi, Rodrigo; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+

On Wed, Nov 18, 2015 at 11:45 PM, Jindal, Sonika <sonika.jindal@intel.com> wrote:
> Hi Rodrigo,
>
> Which platform have you observed this issue on?

Skylake and Kabylake

> This looks really strange.

It is expected actually.
On DC5/6 states all read-only registers are reset and cannot be restored. including this counter.

> Have you checked whether we are able to enter PSR at sink side or not in such cases?
> Are we sure we are not entering PSR? I mean the PSR_STATE register says it correctly?

I'm sure PSR is working well in entry state with links off and Perf counter goes to zero when DC5/6 states enters.
So this patch just removes the counter to avoid confusion. People would think PSR isn't work, when it is.

>
> Regards,
> Sonika
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> Behalf Of R, Durgadoss
> Sent: Thursday, November 19, 2015 11:39 AM
> To: Vivi, Rodrigo; intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo
> Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter 
> for SKL+
>
>
>
>>-----Original Message-----
>>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
>>Behalf Of Rodrigo Vivi
>>Sent: Thursday, November 19, 2015 6:10 AM
>>To: intel-gfx@lists.freedesktop.org
>>Cc: Vivi, Rodrigo
>>Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf Counter for
>>SKL+
>>
>>Whenever DMC firmware put the HW into DC State a bunch of registers 
>>including this perf counter is reset to 0 and never restored.
>>
>>So, even with PSR active and working we could still read
>>"Performance_Counter: 0" what will misslead people to believe PSR is 
>>broken.
>>
>>So, it is better to remove this counter information while we don't 
>>have a better way to track PSR residency.
>
> Agreed..
>
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
>
> Thanks,
> Durga
>
>>
>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>---
>> drivers/gpu/drm/i915/i915_debugfs.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>b/drivers/gpu/drm/i915/i915_debugfs.c
>>index 038d5c6..71e1666 100644
>>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>@@ -2580,8 +2580,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>>               }
>>       seq_puts(m, "\n");
>>
>>-      /* CHV PSR has no kind of performance counter */
>>-      if (HAS_DDI(dev)) {
>>+      /*
>>+       * VLV/CHV PSR has no kind of performance counter
>>+       * SKL+ Perf counter is reset to 0 everytime DC state is entered
>>+       */
>>+      if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>>               psrperf = I915_READ(EDP_PSR_PERF_CNT) &
>>                       EDP_PSR_PERF_CNT_MASK;
>>
>>--
>>2.4.3
>>
>>_______________________________________________
>>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
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Remove PSR Perf Counter for SKL+

[Vivi, Rodrigo]

On Fri, 2015-11-20 at 06:01 +0000, Jindal, Sonika wrote:
> Hmm, please help me understand more about it.

Well, PSR is working because we are seeing PC10 with screen on. But the
registers are zeroed. Regarding it is restored properly or not.

I can send a v2 with a much better commit message and explanation now I
know it better after talking to HW guys.

> As per bspec this is a RW register.
> Also, restoring the register values, isn't it part of the firmware's 
> job?

Oh  indeed. I'm sorry for my confusion.

> Do we have any communication from HW team on this?

Today when I was explaining them what I was seeing I realized that this
register is reset to 0 when entering DC5/6. Even if restored properly
on exit, we would still see 0 on the full idle scenario and people
would think PSR is not working. So I still want to remove that.

> 
> Regards,
> Sonika
> 
> -----Original Message-----
> From: Rodrigo Vivi [mailto:rodrigo.vivi@gmail.com] 
> Sent: Friday, November 20, 2015 12:01 AM
> To: Jindal, Sonika
> Cc: R, Durgadoss; Vivi, Rodrigo; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf 
> Counter for SKL+
> 
> On Wed, Nov 18, 2015 at 11:45 PM, Jindal, Sonika <
> sonika.jindal@intel.com> wrote:
> > Hi Rodrigo,
> > 
> > Which platform have you observed this issue on?
> 
> Skylake and Kabylake
> 
> > This looks really strange.
> 
> It is expected actually.
> On DC5/6 states all read-only registers are reset and cannot be 
> restored. including this counter.
> 
> > Have you checked whether we are able to enter PSR at sink side or 
> > not in such cases?
> > Are we sure we are not entering PSR? I mean the PSR_STATE register 
> > says it correctly?
> 
> I'm sure PSR is working well in entry state with links off and Perf 
> counter goes to zero when DC5/6 states enters.
> So this patch just removes the counter to avoid confusion. People 
> would think PSR isn't work, when it is.
> 
> > 
> > Regards,
> > Sonika
> > 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> > Behalf Of R, Durgadoss
> > Sent: Thursday, November 19, 2015 11:39 AM
> > To: Vivi, Rodrigo; intel-gfx@lists.freedesktop.org
> > Cc: Vivi, Rodrigo
> > Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf 
> > Counter 
> > for SKL+
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] 
> > > On 
> > > Behalf Of Rodrigo Vivi
> > > Sent: Thursday, November 19, 2015 6:10 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: Vivi, Rodrigo
> > > Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Remove PSR Perf 
> > > Counter for
> > > SKL+
> > > 
> > > Whenever DMC firmware put the HW into DC State a bunch of 
> > > registers 
> > > including this perf counter is reset to 0 and never restored.
> > > 
> > > So, even with PSR active and working we could still read
> > > "Performance_Counter: 0" what will misslead people to believe PSR 
> > > is 
> > > broken.
> > > 
> > > So, it is better to remove this counter information while we 
> > > don't 
> > > have a better way to track PSR residency.
> > 
> > Agreed..
> > 
> > Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
> > 
> > Thanks,
> > Durga
> > 
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_debugfs.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 038d5c6..71e1666 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2580,8 +2580,11 @@ static int i915_edp_psr_status(struct 
> > > seq_file *m, void *data)
> > >               }
> > >       seq_puts(m, "\n");
> > > 
> > > -      /* CHV PSR has no kind of performance counter */
> > > -      if (HAS_DDI(dev)) {
> > > +      /*
> > > +       * VLV/CHV PSR has no kind of performance counter
> > > +       * SKL+ Perf counter is reset to 0 everytime DC state is 
> > > entered
> > > +       */
> > > +      if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > >               psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> > >                       EDP_PSR_PERF_CNT_MASK;
> > > 
> > > --
> > > 2.4.3
> > > 
> > > _______________________________________________
> > > 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
> > _______________________________________________
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Remove PSR Perf Counter for SKL+

[Rodrigo Vivi]

Whenever DMC firmware put the HW into DC State a bunch
of registers including this perf counter is reset to 0.

Even with PSR active and working we could still read
"Performance_Counter: 0" what will misslead people to believe
PSR is broken. For instance on SKL we can only see PC10
residency with screen on if PSR is working properly.
However Performance_Counter was showing 0.

Even if it restored properly on DC6 exit we don't want to
give users the wrong impression that PSR is not working
while we know for sure it is.

So, it is better to remove this counter information while
we don't have a better way to track PSR residency.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b28da6f..a728ff1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2582,8 +2582,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		}
 	seq_puts(m, "\n");
 
-	/* CHV PSR has no kind of performance counter */
-	if (HAS_DDI(dev)) {
+	/*
+	 * VLV/CHV PSR has no kind of performance counter
+	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
+	 */
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
 			EDP_PSR_PERF_CNT_MASK;
 
-- 
2.4.3

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

[PATCH] drm/i915: Also disable PSR on Sink when disabling it on Source.

[Rodrigo Vivi]

It is not a bad idea to disable the PSR feature on Sink
when we are disabling on the Source.

v2: Move dpcd write inside mutex protected area as suggested by Sonika.

Cc: Sonika Jindal <sonika.jindal@intel.com>
Suggested-by: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2b2f84d..3bbb270 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -518,11 +518,15 @@ void intel_psr_disable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Disable PSR on Source */
 	if (HAS_DDI(dev))
 		hsw_psr_disable(intel_dp);
 	else
 		vlv_psr_disable(intel_dp);
 
+	/* Disable PSR on Sink */
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
+
 	dev_priv->psr.enabled = NULL;
 	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] drm/i915: Also disable PSR on Sink when disabling it on Source.

[Jindal, Sonika]

Reviewed-by: Sonika Jindal <sonika.jindal@intel.com>

-----Original Message-----
From: Vivi, Rodrigo 
Sent: Tuesday, November 24, 2015 3:50 AM
To: intel-gfx@lists.freedesktop.org
Cc: Vivi, Rodrigo; Jindal, Sonika
Subject: [PATCH] drm/i915: Also disable PSR on Sink when disabling it on Source.

It is not a bad idea to disable the PSR feature on Sink when we are disabling on the Source.

v2: Move dpcd write inside mutex protected area as suggested by Sonika.

Cc: Sonika Jindal <sonika.jindal@intel.com>
Suggested-by: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2b2f84d..3bbb270 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -518,11 +518,15 @@ void intel_psr_disable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Disable PSR on Source */
 	if (HAS_DDI(dev))
 		hsw_psr_disable(intel_dp);
 	else
 		vlv_psr_disable(intel_dp);
 
+	/* Disable PSR on Sink */
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
+
 	dev_priv->psr.enabled = NULL;
 	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] drm/i915: Remove PSR Perf Counter for SKL+

[Daniel Vetter]

On Mon, Nov 23, 2015 at 02:16:40PM -0800, Rodrigo Vivi wrote:
> Whenever DMC firmware put the HW into DC State a bunch
> of registers including this perf counter is reset to 0.
> 
> Even with PSR active and working we could still read
> "Performance_Counter: 0" what will misslead people to believe
> PSR is broken. For instance on SKL we can only see PC10
> residency with screen on if PSR is working properly.
> However Performance_Counter was showing 0.
> 
> Even if it restored properly on DC6 exit we don't want to
> give users the wrong impression that PSR is not working
> while we know for sure it is.
> 
> So, it is better to remove this counter information while
> we don't have a better way to track PSR residency.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index b28da6f..a728ff1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2582,8 +2582,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  		}
>  	seq_puts(m, "\n");
>  
> -	/* CHV PSR has no kind of performance counter */
> -	if (HAS_DDI(dev)) {
> +	/*
> +	 * VLV/CHV PSR has no kind of performance counter
> +	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
> +	 */
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		psrperf = I915_READ(EDP_PSR_PERF_CNT) &
>  			EDP_PSR_PERF_CNT_MASK;
>  
> -- 
> 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: [PATCH] drm/i915: Also disable PSR on Sink when disabling it on Source.

[Daniel Vetter]

On Tue, Nov 24, 2015 at 04:40:08AM +0000, Jindal, Sonika wrote:
> Reviewed-by: Sonika Jindal <sonika.jindal@intel.com>
> 
> -----Original Message-----
> From: Vivi, Rodrigo 
> Sent: Tuesday, November 24, 2015 3:50 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo; Jindal, Sonika
> Subject: [PATCH] drm/i915: Also disable PSR on Sink when disabling it on Source.
> 
> It is not a bad idea to disable the PSR feature on Sink when we are disabling on the Source.
> 
> v2: Move dpcd write inside mutex protected area as suggested by Sonika.
> 
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> Suggested-by: Sonika Jindal <sonika.jindal@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2b2f84d..3bbb270 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -518,11 +518,15 @@ void intel_psr_disable(struct intel_dp *intel_dp)
>  		return;
>  	}
>  
> +	/* Disable PSR on Source */
>  	if (HAS_DDI(dev))
>  		hsw_psr_disable(intel_dp);
>  	else
>  		vlv_psr_disable(intel_dp);
>  
> +	/* Disable PSR on Sink */
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
> +
>  	dev_priv->psr.enabled = NULL;
>  	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

-- 
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