[PATCH 0/4] PSR Critical fixes

[PATCH 0/4] PSR Critical fixes

[Rodrigo Vivi]

Let's split critical PSR fixes from the series that contains other
reworks, stabilization and improvements.

The second patch in this series isn't considered critical in terms
of functionality, but it depends on the first one and it can be consider
a fix for PSR residency on VLV/CHV.

Thanks,
Rodrigo.

Rodrigo Vivi (4):
  drm/i915: Delay first PSR activation.
  drm/i915: Reduce PSR re-activation time for VLV/CHV.
  drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT.
  drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT.

 drivers/gpu/drm/i915/intel_psr.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
2.4.3

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

[PATCH 1/4] drm/i915: Delay first PSR activation.

[Rodrigo Vivi]

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.

This is also important to make the sysfs toggle working properly.

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 213581c..6b24c24 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 every platform 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));
+
 	dev_priv->psr.enabled = intel_dp;
 unlock:
 	mutex_unlock(&dev_priv->psr.lock);
@@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device *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

[PATCH 2/4] 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 immediately 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.

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 6b24c24..6d0a64b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -713,7 +713,6 @@ 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;
 
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
@@ -750,7 +749,7 @@ void intel_psr_flush(struct drm_device *dev,
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
 		if (!work_busy(&dev_priv->psr.work.work))
 			schedule_delayed_work(&dev_priv->psr.work,
-					      msecs_to_jiffies(delay_ms));
+					      msecs_to_jiffies(100));
 	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

[PATCH 3/4] drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT.

[Rodrigo Vivi]

Since the beginning there is a confusion on the meaning of this bit.

A previous patch had identified this already and fixed it partially:
'commit 3301d409 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic")

DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the
training, but it doesn't tell to avoid TP patterns or to skip
aux handshake.

This patch fixes the hard freeze 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 | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 6d0a64b..a80adf4 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -271,7 +271,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 		   send the minimal TP1 possible and skip TP2. */
 		val |= EDP_PSR_TP1_TIME_100us;
 		val |= EDP_PSR_TP2_TP3_TIME_0us;
-		val |= EDP_PSR_SKIP_AUX_EXIT;
 		/* Sink should be able to train with the 5 or 6 idle patterns */
 		idle_frames += 4;
 	}
-- 
2.4.3

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

[PATCH 4/4] drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT.

[Rodrigo Vivi]

On the commit 3301d4092106 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic")'
we already had identified that DP_PSR_NO_TRAIN_ON_EXIT
doesn't mean we shouldn't send TPS patterns, however we start sending the
minimal TP1 as possible and no TP2.

For most of the panels this is ok, but we found a reported case where
this is not true and panel keeps frozen without updating the screen for a while.

We could just get this case after patch "PSR: Don't Skip aux handshake on
DP_PSR_NO_TRAIN_ON_EXIT." is applied since that one fix the
hard freeze on this kind of panels.

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

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index a80adf4..7984b8c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -267,10 +267,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
 	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
-		/* It doesn't mean we shouldn't send TPS patters, so let's
-		   send the minimal TP1 possible and skip TP2. */
-		val |= EDP_PSR_TP1_TIME_100us;
-		val |= EDP_PSR_TP2_TP3_TIME_0us;
 		/* Sink should be able to train with the 5 or 6 idle patterns */
 		idle_frames += 4;
 	}
-- 
2.4.3

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

Re: [PATCH 1/4] drm/i915: Delay first PSR activation.

[R, Durgadoss]

Hi Rodrigo,

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Rodrigo Vivi
>Sent: Thursday, November 12, 2015 1:07 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo
>Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR activation.
>
>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.
>
>This is also important to make the sysfs toggle working properly.
>
>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 213581c..6b24c24 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 every platform 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));
>+
> 	dev_priv->psr.enabled = intel_dp;
> unlock:
> 	mutex_unlock(&dev_priv->psr.lock);
>@@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device *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));

Agree with the theory of the patch as such.. But, Is there any specific reason for
the !work_busy() check here ?

I believe when the later work runs, it will anyway bail out in _activate
function, if it sees PSR_ENABLE bit set already. So, is this check just to
prevent scheduling one more work item when there is one pending
already ? (or it helps in something else also ?)

Thanks,
Durga

> 	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/4] PSR Critical fixes

[R, Durgadoss]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Rodrigo Vivi
>Sent: Thursday, November 12, 2015 1:07 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo
>Subject: [Intel-gfx] [PATCH 0/4] PSR Critical fixes
>
>Let's split critical PSR fixes from the series that contains other
>reworks, stabilization and improvements.

That really helped in review ;-)

For patches 2,3,4 in this series:

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

Thanks,
Durga

>
>The second patch in this series isn't considered critical in terms
>of functionality, but it depends on the first one and it can be consider
>a fix for PSR residency on VLV/CHV.
>
>Thanks,
>Rodrigo.
>
>Rodrigo Vivi (4):
>  drm/i915: Delay first PSR activation.
>  drm/i915: Reduce PSR re-activation time for VLV/CHV.
>  drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT.
>  drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT.
>
> drivers/gpu/drm/i915/intel_psr.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
>--
>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 1/4] drm/i915: Delay first PSR activation.

[Vivi, Rodrigo]

On Thu, 2015-11-12 at 13:50 +0000, R, Durgadoss wrote:
> Hi Rodrigo,
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> > Behalf Of Rodrigo Vivi
> > Sent: Thursday, November 12, 2015 1:07 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Vivi, Rodrigo
> > Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR 
> > activation.
> > 
> > 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.
> > 
> > This is also important to make the sysfs toggle working properly.
> > 
> > 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 213581c..6b24c24 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 every platform 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));
> > +
> > 	dev_priv->psr.enabled = intel_dp;
> > unlock:
> > 	mutex_unlock(&dev_priv->psr.lock);
> > @@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device *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));
> 
> Agree with the theory of the patch as such.. But, Is there any 
> specific reason for
> the !work_busy() check here ?
> 
> I believe when the later work runs, it will anyway bail out in 
> _activate
> function, if it sees PSR_ENABLE bit set already. So, is this check 
> just to
> prevent scheduling one more work item when there is one pending
> already ? (or it helps in something else also ?)

The !work_busy is to prevent that eventual _activate call reduce the
first activation time.

for instance:

0s - we enable and schedule first activation to 2.5s
1s - we got a page flip that flushed fb tracking and called
psr_activation to 0.1s 
1.1s - psr is activated

while we want

0s - we enable and schedule first activation to 2.5s
1s - we got a page
flip that flushed fb tracking and called psr_activation to 0.1s # just
ignore and move ahead since we are going to activate it soon.
2.5s - psr
is activated

I'm open to hear ideas to make it better or more clear.


> 
> Thanks,
> Durga

Thank you very much for all the reviews!

> 
> > 	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Delay first PSR activation.

[R, Durgadoss]

>-----Original Message-----
>From: Vivi, Rodrigo
>Sent: Friday, November 13, 2015 3:08 AM
>To: intel-gfx@lists.freedesktop.org; R, Durgadoss
>Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR activation.
>
>On Thu, 2015-11-12 at 13:50 +0000, R, Durgadoss wrote:
>> Hi Rodrigo,
>>
>> > -----Original Message-----
>> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>> > Behalf Of Rodrigo Vivi
>> > Sent: Thursday, November 12, 2015 1:07 AM
>> > To: intel-gfx@lists.freedesktop.org
>> > Cc: Vivi, Rodrigo
>> > Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR
>> > activation.
>> >
>> > 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.
>> >
>> > This is also important to make the sysfs toggle working properly.
>> >
>> > 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 213581c..6b24c24 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 every platform 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));
>> > +
>> > 	dev_priv->psr.enabled = intel_dp;

Should we set this before scheduling the delayed work ?

>> > unlock:
>> > 	mutex_unlock(&dev_priv->psr.lock);
>> > @@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device *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));
>>
>> Agree with the theory of the patch as such.. But, Is there any
>> specific reason for
>> the !work_busy() check here ?
>>
>> I believe when the later work runs, it will anyway bail out in
>> _activate
>> function, if it sees PSR_ENABLE bit set already. So, is this check
>> just to
>> prevent scheduling one more work item when there is one pending
>> already ? (or it helps in something else also ?)
>
>The !work_busy is to prevent that eventual _activate call reduce the
>first activation time.

Yes, this is what I understood from the code. Just wanted to confirm whether
you meant the same.

The other thing I am thinking is:
Inside intel_psr_enable() we call _activate() for DDI platforms & gen>=9.
For others, we schedule a work.

May be we should have only the worker thread do _activate() for every
Platform .. I believe this would simplify things a lot. Not sure whether this
Will impact gen>=9 platforms in any way..

Anyway, we can have that as a separate change if required and valid.
So, for this patch:
Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga

>
>for instance:
>
>0s - we enable and schedule first activation to 2.5s
>1s - we got a page flip that flushed fb tracking and called
>psr_activation to 0.1s
>1.1s - psr is activated
>
>while we want
>
>0s - we enable and schedule first activation to 2.5s
>1s - we got a page
>flip that flushed fb tracking and called psr_activation to 0.1s # just
>ignore and move ahead since we are going to activate it soon.
>2.5s - psr
>is activated
>
>I'm open to hear ideas to make it better or more clear.
>
>
>>
>> Thanks,
>> Durga
>
>Thank you very much for all the reviews!
>
>>
>> > 	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/4] PSR Critical fixes

[Ville Syrjälä]

On Wed, Nov 11, 2015 at 11:37:06AM -0800, Rodrigo Vivi wrote:
> Let's split critical PSR fixes from the series that contains other
> reworks, stabilization and improvements.
> 
> The second patch in this series isn't considered critical in terms
> of functionality, but it depends on the first one and it can be consider
> a fix for PSR residency on VLV/CHV.

FYI I recently glanced at the psr code and a few things that left me
scratching my head:
- hsw_psr_enable_sink() frobs at the AUX registers without holding the hw_mutex.
  On SKL+ it seems to use the normal AUX registers here. Before it used
  the special PSR registers, so that may have been OK, but the SKL+
  thing seems rather questionable.
- intel_psr_enable() calls intel_psr_activate() on SKL+ but not on
  HSW/BDW. I'm thinking there should be a comment there to make it clear
  why, if it's even correct.

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

Re: [PATCH 0/4] PSR Critical fixes

[Vivi, Rodrigo]

On Fri, 2015-11-13 at 17:08 +0200, Ville Syrjälä wrote:
> On Wed, Nov 11, 2015 at 11:37:06AM -0800, Rodrigo Vivi wrote:
> > Let's split critical PSR fixes from the series that contains other
> > reworks, stabilization and improvements.
> > 
> > The second patch in this series isn't considered critical in terms
> > of functionality, but it depends on the first one and it can be 
> > consider
> > a fix for PSR residency on VLV/CHV.
> 
> FYI I recently glanced at the psr code and a few things that left me
> scratching my head:

Thanks for spotting this.

> - hsw_psr_enable_sink() frobs at the AUX registers without holding 
> the hw_mutex.
>   On SKL+ it seems to use the normal AUX registers here. Before it 
> used
>   the special PSR registers, so that may have been OK, but the SKL+
>   thing seems rather questionable.

Yes, I agree. I'll take a look.

> - intel_psr_enable() calls intel_psr_activate() on SKL+ but not on
>   HSW/BDW. I'm thinking there should be a comment there to make it 
> clear
>   why, if it's even correct.

Indeed. Also Durga when reviewing mentioned it would be good to make
only worker calling _activate(). So I will follow-up with a patch to
fix this.


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

Re: [PATCH 1/4] drm/i915: Delay first PSR activation.

[Vivi, Rodrigo]

On Fri, 2015-11-13 at 09:09 +0000, R, Durgadoss wrote:
> > -----Original Message-----
> > From: Vivi, Rodrigo
> > Sent: Friday, November 13, 2015 3:08 AM
> > To: intel-gfx@lists.freedesktop.org; R, Durgadoss
> > Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR 
> > activation.
> > 
> > On Thu, 2015-11-12 at 13:50 +0000, R, Durgadoss wrote:
> > > Hi Rodrigo,
> > > 
> > > > -----Original Message-----
> > > > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org
> > > > ] On
> > > > Behalf Of Rodrigo Vivi
> > > > Sent: Thursday, November 12, 2015 1:07 AM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Cc: Vivi, Rodrigo
> > > > Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR
> > > > activation.
> > > > 
> > > > 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.
> > > > 
> > > > This is also important to make the sysfs toggle working 
> > > > properly.
> > > > 
> > > > 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 213581c..6b24c24 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 every platform 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));
> > > > +
> > > > 	dev_priv->psr.enabled = intel_dp;
> 
> Should we set this before scheduling the delayed work ?
> 
> > > > unlock:
> > > > 	mutex_unlock(&dev_priv->psr.lock);
> > > > @@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device 
> > > > *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));
> > > 
> > > Agree with the theory of the patch as such.. But, Is there any
> > > specific reason for
> > > the !work_busy() check here ?
> > > 
> > > I believe when the later work runs, it will anyway bail out in
> > > _activate
> > > function, if it sees PSR_ENABLE bit set already. So, is this 
> > > check
> > > just to
> > > prevent scheduling one more work item when there is one pending
> > > already ? (or it helps in something else also ?)
> > 
> > The !work_busy is to prevent that eventual _activate call reduce 
> > the
> > first activation time.
> 
> Yes, this is what I understood from the code. Just wanted to confirm 
> whether
> you meant the same.
> 
> The other thing I am thinking is:
> Inside intel_psr_enable() we call _activate() for DDI platforms & 
> gen>=9.
> For others, we schedule a work.
> 
> May be we should have only the worker thread do _activate() for every
> Platform .. I believe this would simplify things a lot. Not sure 
> whether this
> Will impact gen>=9 platforms in any way..

Yeap, good idea. It will make things standardized and simpler with
almost no impact on the feature and better for stability in case we
start facing this issue on newer platforms again...
I'll do an extra/separated patch for that.

> 
> Anyway, we can have that as a separate change if required and valid.
> So, for this patch:
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks

> 
> Thanks,
> Durga
> 
> > 
> > for instance:
> > 
> > 0s - we enable and schedule first activation to 2.5s
> > 1s - we got a page flip that flushed fb tracking and called
> > psr_activation to 0.1s
> > 1.1s - psr is activated
> > 
> > while we want
> > 
> > 0s - we enable and schedule first activation to 2.5s
> > 1s - we got a page
> > flip that flushed fb tracking and called psr_activation to 0.1s # 
> > just
> > ignore and move ahead since we are going to activate it soon.
> > 2.5s - psr
> > is activated
> > 
> > I'm open to hear ideas to make it better or more clear.
> > 
> > 
> > > 
> > > Thanks,
> > > Durga
> > 
> > Thank you very much for all the reviews!
> > 
> > > 
> > > > 	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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/4] PSR Critical fixes

[Rodrigo Vivi]

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

Hi Daniel,

All 4 patches in this series are reviewed and ready to merge,
could you please merge them?

Thanks,
Rodrigo.

On Fri, Nov 13, 2015 at 10:42 AM Vivi, Rodrigo <rodrigo.vivi@intel.com>
wrote:

> On Fri, 2015-11-13 at 17:08 +0200, Ville Syrjälä wrote:
> > On Wed, Nov 11, 2015 at 11:37:06AM -0800, Rodrigo Vivi wrote:
> > > Let's split critical PSR fixes from the series that contains other
> > > reworks, stabilization and improvements.
> > >
> > > The second patch in this series isn't considered critical in terms
> > > of functionality, but it depends on the first one and it can be
> > > consider
> > > a fix for PSR residency on VLV/CHV.
> >
> > FYI I recently glanced at the psr code and a few things that left me
> > scratching my head:
>
> Thanks for spotting this.
>
> > - hsw_psr_enable_sink() frobs at the AUX registers without holding
> > the hw_mutex.
> >   On SKL+ it seems to use the normal AUX registers here. Before it
> > used
> >   the special PSR registers, so that may have been OK, but the SKL+
> >   thing seems rather questionable.
>
> Yes, I agree. I'll take a look.
>
> > - intel_psr_enable() calls intel_psr_activate() on SKL+ but not on
> >   HSW/BDW. I'm thinking there should be a comment there to make it
> > clear
> >   why, if it's even correct.
>
> Indeed. Also Durga when reviewing mentioned it would be good to make
> only worker calling _activate(). So I will follow-up with a patch to
> fix this.
>
>
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 2346 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: [PATCH 0/4] PSR Critical fixes

[Daniel Vetter]

On Mon, Nov 16, 2015 at 04:00:35PM +0000, Rodrigo Vivi wrote:
> Hi Daniel,
> 
> All 4 patches in this series are reviewed and ready to merge,
> could you please merge them?

All merged to dinq, thanks for patches&review.
-Daniel

> 
> Thanks,
> Rodrigo.
> 
> On Fri, Nov 13, 2015 at 10:42 AM Vivi, Rodrigo <rodrigo.vivi@intel.com>
> wrote:
> 
> > On Fri, 2015-11-13 at 17:08 +0200, Ville Syrjälä wrote:
> > > On Wed, Nov 11, 2015 at 11:37:06AM -0800, Rodrigo Vivi wrote:
> > > > Let's split critical PSR fixes from the series that contains other
> > > > reworks, stabilization and improvements.
> > > >
> > > > The second patch in this series isn't considered critical in terms
> > > > of functionality, but it depends on the first one and it can be
> > > > consider
> > > > a fix for PSR residency on VLV/CHV.
> > >
> > > FYI I recently glanced at the psr code and a few things that left me
> > > scratching my head:
> >
> > Thanks for spotting this.
> >
> > > - hsw_psr_enable_sink() frobs at the AUX registers without holding
> > > the hw_mutex.
> > >   On SKL+ it seems to use the normal AUX registers here. Before it
> > > used
> > >   the special PSR registers, so that may have been OK, but the SKL+
> > >   thing seems rather questionable.
> >
> > Yes, I agree. I'll take a look.
> >
> > > - intel_psr_enable() calls intel_psr_activate() on SKL+ but not on
> > >   HSW/BDW. I'm thinking there should be a comment there to make it
> > > clear
> > >   why, if it's even correct.
> >
> > Indeed. Also Durga when reviewing mentioned it would be good to make
> > only worker calling _activate(). So I will follow-up with a patch to
> > fix this.
> >
> >
> > >
> > _______________________________________________
> > 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