Re: [Intel-gfx] [PATCH 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Tarun Vyas]

On Mon, Aug 13, 2018 at 11:10:00AM -0700, Pandiyan, Dhinakaran wrote:
> On Mon, 2018-08-13 at 09:57 -0700, Rodrigo Vivi wrote:
> > On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan wrote:
> > > CI runs show PSR2 does not go to IDLE with selective update enabled
> > > on
> > > all PSR exit triggers. Specifically, logs indicate the hardware
> > > enters
> > > "SLEEP Selective Update" and not "IDLE Reset state' like the kernel
> > > expects. This check was added for PSR1 but incorrectly extended to
> > > PSR2,
> > > remove this check for PSR2 as there is a plan to test only PSR1 on
> > > PSR2
> > > panels.
> > > 
> > > Also add bspec reference to the comment about idle timeout.
> > > 
> > > Cc: Tarun Vyas 
> > > Cc: José Roberto de Souza 
> > > Cc: Rodrigo Vivi 
> > > Signed-off-by: Dhinakaran Pandiyan 
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 39 --
> > > --
> > >  1 file changed, 14 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 5686ddaa6a72..09be9bfee2be 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct
> > > intel_crtc_state *new_crtc_state,
> > >  {
> > >   struct intel_crtc *crtc = to_intel_crtc(new_crtc_state-
> > > >base.crtc);
> > >   struct drm_i915_private *dev_priv = to_i915(crtc-
> > > >base.dev);
> > > - i915_reg_t reg;
> > > - u32 mask;
> > > -
> > > - if (!new_crtc_state->has_psr)
> > > - return 0;
> > >  
> > >   /*
> > > -  * The sole user right now is intel_pipe_update_start(),
> > > -  * which won't race with psr_enable/disable, which is
> > > -  * where psr2_enabled is written to. So, we don't need
> > > -  * to acquire the psr.lock. More importantly, we want the
> > > -  * latency inside intel_pipe_update_start() to be as low
> > > -  * as possible, so no need to acquire psr.lock when it is
> > > -  * not needed and will induce latencies in the atomic
> > > -  * update path.
> > > +  * The sole user right now is intel_pipe_update_start(),
> > > which won't
> > > +  * race with psr_enable/disable where psr2_enabled is
> > > written to. So, we
> > > +  * don't need to acquire the psr.lock. More importantly,
> > > we want the
> > > +  * latency inside intel_pipe_update_start() to be as low
> > > as possible, so
> > > +  * no need to acquire psr.lock when it is not needed and
> > > will induce
> > > +  * latencies in the atomic update path.
> > >*/
> > 
> > I think we shouldn't change this format here to keep patch cleaner...
> > if there is any change here I couldn't see because it is changing all
> > lines and if there is no change I think it is better not to touch
> > because
> > it removes the focus of the real changes.
> 
> Okay.
> > 
> > > - if (dev_priv->psr.psr2_enabled) {
> > > - reg = EDP_PSR2_STATUS;
> > > - mask = EDP_PSR2_STATUS_STATE_MASK;
> > > - } else {
> > > - reg = EDP_PSR_STATUS;
> > > - mask = EDP_PSR_STATUS_STATE_MASK;
> > > - }
> > > + if (!new_crtc_state->has_psr || READ_ONCE(dev_priv-
> > > >psr.psr2_enabled))
> > 
> > I now see that we are removing psr2 of the picture, but I don't see
> > how we are
> > improving psr2 situation here.
> > what am I missing?
> > 
> When the patch was written, we did not have sufficient tests to tell us
> the wait_for_idle condition was wrong for PSR2. It was not known
> whether the wait was *necessary* for PSR2, think of this as a partial
> revert. Now that CI has pointed out, (and I checked with a PSR2 panel)
> that the condition is wrong, we should be removing it for PSR2. If you
> think about it, it does improve PSR2 my removing irrelevant code.
Do we have another way to ensure that we dont try to do a pipe update or rather
check for the PIPE DSL when still in a PSR2 sleep state ?
> 
> 
> > > + return 0;
> > >  
> > >   /*
> > > -  * Max time for PSR to idle = Inverse of the refresh rate
> > > +
> > > -  * 6 ms of exit training time + 1.5 ms

[Intel-gfx] [PATCH v5] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

[Tarun Vyas]

In commit "drm/i915: Wait for PSR exit before checking for vblank
evasion", the idea was to limit the PSR IDLE checks when PSR is
actually supported. While CAN_PSR does do that check, it doesn't
applies on a per-crtc basis. crtc_state->has_psr is a more granular
check that only applies to pipe(s) that have PSR enabled.

Without the has_psr check, we end up waiting on the eDP transcoder's
PSR_STATUS register irrespective of whether the pipe being updated is
driving it or not.

Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for
vblank evasion")

v2: Remove unnecessary parantheses, make checkpatch happy.

v3: Move the has_psr check to intel_psr_wait_for_idle and commit
message changes (DK).

v4: Derive dev_priv from intel_crtc_state (DK)

v5: Commit message changes to reflect the HW behavior (DK)

Reviewed-by: Dhinakaran Pandiyan 
Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h| 2 +-
 drivers/gpu/drm/i915/intel_psr.c| 7 ++-
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61e715ddd0d5..699073fbecb1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1923,7 +1923,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
 void intel_psr_short_pulse(struct intel_dp *intel_dp);
-int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
+int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 23acc9ac8d4d..e97db5dd75b1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -717,11 +717,16 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
-int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state)
 {
+   struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
+   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
i915_reg_t reg;
u32 mask;
 
+   if (!new_crtc_state->has_psr)
+   return 0;
+
/*
 * The sole user right now is intel_pipe_update_start(),
 * which won't race with psr_enable/disable, which is
diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 4990d6e84ddf..9d6d1ac149da 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
 * VBL interrupts will start the PSR exit and prevent a PSR
 * re-entry as well.
 */
-   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   if (intel_psr_wait_for_idle(new_crtc_state))
DRM_ERROR("PSR idle timed out, atomic update may fail\n");
 
local_irq_disable();
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

[Tarun Vyas]

In commit "drm/i915: Wait for PSR exit before checking for vblank
evasion", the idea was to limit the PSR IDLE checks when PSR is
actually supported. While CAN_PSR does do that check, it doesn't
applies on a per-crtc basis. crtc_state->has_psr is a more granular
check that only applies to pipe(s) that have PSR enabled.

Currently, the driver supports PSR on port A + transcoder eDP, so
only pipe A will wait for PSR to go IDLE, as it should, and other
pipes should return immediately.

Without the has_psr check, non-PSR pipe_updates (pipe B/C in this
case), end up waiting on PSR pipe (pipe A in this case) to exit PSR,
which may incur substantial delays for non-PSR pipe updates alongwith
the fact the it doesn't makes any sense.

Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for
vblank evasion")

v2: Remove unnecessary parantheses, make checkpatch happy.

v3: Move the has_psr check to intel_psr_wait_for_idle and commit
message changes (DK).

v4: Derive dev_priv from intel_crtc_state (DK)

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h| 2 +-
 drivers/gpu/drm/i915/intel_psr.c| 7 ++-
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61e715ddd0d5..699073fbecb1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1923,7 +1923,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
 void intel_psr_short_pulse(struct intel_dp *intel_dp);
-int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
+int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 23acc9ac8d4d..e97db5dd75b1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -717,11 +717,16 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
-int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state)
 {
+   struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
+   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
i915_reg_t reg;
u32 mask;
 
+   if (!new_crtc_state->has_psr)
+   return 0;
+
/*
 * The sole user right now is intel_pipe_update_start(),
 * which won't race with psr_enable/disable, which is
diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 4990d6e84ddf..9d6d1ac149da 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
 * VBL interrupts will start the PSR exit and prevent a PSR
 * re-entry as well.
 */
-   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   if (intel_psr_wait_for_idle(new_crtc_state))
DRM_ERROR("PSR idle timed out, atomic update may fail\n");
 
local_irq_disable();
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

[Tarun Vyas]

In commit "drm/i915: Wait for PSR exit before checking for vblank
evasion", the idea was to limit the PSR IDLE checks when PSR is
actually supported. While CAN_PSR does do that check, it doesn't
applies on a per-crtc basis. crtc_state->has_psr is a more granular
check that only applies to pipe(s) that have PSR enabled.

Currently, the driver supports PSR on port A + transcoder eDP, so
only pipe A will wait for PSR to go IDLE, as it should, and other
pipes should return immediately.

Without the has_psr check, non-PSR pipe_updates (pipe B/C in this
case), end up waiting on PSR pipe (pipe A in this case) to exit PSR,
which may incur substantial delays for non-PSR pipe updates alongwith
the fact the it doesn't makes any sense.

Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for
vblank evasion")

v2: Remove unnecessary parantheses, make checkpatch happy.

v3: Move the has_psr check to intel_psr_wait_for_idle and commit
message changes (DK).

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h| 2 +-
 drivers/gpu/drm/i915/intel_psr.c| 5 -
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61e715ddd0d5..3f7fbaeb1ee9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1923,7 +1923,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
 void intel_psr_short_pulse(struct intel_dp *intel_dp);
-int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv, const struct 
intel_crtc_state *crtc_state);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 23acc9ac8d4d..3691c857cc91 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -717,11 +717,14 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
-int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv, const struct 
intel_crtc_state *crtc_state)
 {
i915_reg_t reg;
u32 mask;
 
+   if(!crtc_state->has_psr)
+   return 0;
+
/*
 * The sole user right now is intel_pipe_update_start(),
 * which won't race with psr_enable/disable, which is
diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 4990d6e84ddf..fcd96c52cf5b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
 * VBL interrupts will start the PSR exit and prevent a PSR
 * re-entry as well.
 */
-   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   if (intel_psr_wait_for_idle(dev_priv, new_crtc_state))
DRM_ERROR("PSR idle timed out, atomic update may fail\n");
 
local_irq_disable();
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

[Tarun Vyas]

On Mon, Jul 09, 2018 at 01:31:52PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-07-09 at 12:52 -0700, Tarun Vyas wrote:
> > On Mon, Jul 09, 2018 at 11:58:52AM -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > On Mon, 2018-07-09 at 11:16 -0700, Tarun Vyas wrote:
> > > > 
> > > > On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan
> > > > wrote:
> > > > > 
> > > > > 
> > > > > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote:
> > > > > > 
> > > > > > 
> > > > > > In commit "drm/i915: Wait for PSR exit before checking for
> > > > > > vblank
> > > > > > evasion", the idea was to limit the PSR IDLE checks when PSR
> > > > > > is
> > > > > > actually supported. While CAN_PSR does do that check, it
> > > > > > doesn't
> > > > > > applies on a per-crtc basis. crtc_state->has_psr is a more
> > > > > > granular
> > > > > > check that avoids everything but pipe A, for the PSR IDLE
> > > > > > check.
> 
> I looked at the code and spec again, PSR isn't tied to "pipe A" The
> driver allows PSR only on "port A" + "transcoder eDP", but the pipe
> itself can be any one of the possible options.
>
I'll remove the pipe A part from the message and specify that at the moment we 
assume port A + eDP, b/c at least in the code we populate the registers with 
EDP_PSR base directly. 
> > > > > > 
> > > > > > With this, the PSR IDLE check should be a *no-op* for all but
> > > > > > pipe A
> > > > > > which is what was intended originally.
> > > > > > 
> > > > > So, the problem is when we update a non-PSR pipe (B or C) and
> > > > > PSR
> > > > > is
> > > > > active on another pipe(A, specifically), we end up waiting for
> > > > > the
> > > > > pipe
> > > > > A MMIO to become idle.
> > > > > 
> > > > > Can you please update the commit message as the commit message
> > > > > makes
> > > > > the per-pipe check sound like an optimization? 
> > > > > 
> > > > > This also points to a gap in our testing, I don't see a two
> > > > > pipe
> > > > > PSR
> > > > > related IGT.
> > > > > 
> > > > That's right. On my KBL chromebook that's running the drm-tip,
> > > > when I
> > > > plug-in an external display, so pipe B,
> > > > I see "[drm:intel_pipe_update_start] *ERROR* PSR idle timed out,
> > > > atomic update may fail on pipe B", Iadded the pipe
> > > > name in the DRM_ERROR, may be I should make that change in the v3
> > > > of
> > > > this patch along with updating the commit message.
> > > > 
> > > > But, yea, this proves that with the CAN_PSR check, the non-PSR
> > > > pipes
> > > > (B/C) wait on pipe-A to exit PSR which doesn't have
> > > > any reason to do so at that moment, hence the error.
> > > > 
> > > > I'll make the commit message changes and add the pipe name in the
> > > > DRM_ERROR as well ?
> > > I am thinking you could pass crtc_state
> > > to intel_psr_wait_for_idle()
> > > and then check inside the implementation if the argument is the
> > > same as
> > > the pipe PSR was enabled on and then wait.
> > > 
> > > intel_psr_wait_for_idle(crtc_state) {
> > >   if (!CAN_PSR() || !crtc_state->has_psr)
> > >   return;
> > >   ...
> > > }
> > Hmm, but the CAN_PSR check is already taken care of by
> > intel_psr_compute_config() which then sets has_psr, so just
> > if (!crtc_state->has_psr)
> > return;
> > should suffice, right ?
> > 
> Yeah, we can assume at this point state->has_psr is set correctly.
> 
> > But then, we incur a function call for non-PSR pipes, which will
> > return right away.
> 
> That should be okay, having the PSR related check inside the function
> looks cleaner IMO. 
Sounds good.
> 
> > > 
> > > 
> > > I don't like how intel_psr_wait_for_idle() doesn't care which pipe
> > > (transcoder actually) MMIO it should wait on.
> > >   
> > > 
> > > > 
> > > > > 
> > > > > 
> >

Re: [Intel-gfx] [PATCH v2] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

[Tarun Vyas]

On Mon, Jul 09, 2018 at 11:58:52AM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-07-09 at 11:16 -0700, Tarun Vyas wrote:
> > On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote:
> > > > 
> > > > In commit "drm/i915: Wait for PSR exit before checking for vblank
> > > > evasion", the idea was to limit the PSR IDLE checks when PSR is
> > > > actually supported. While CAN_PSR does do that check, it doesn't
> > > > applies on a per-crtc basis. crtc_state->has_psr is a more
> > > > granular
> > > > check that avoids everything but pipe A, for the PSR IDLE check.
> > > > 
> > > > With this, the PSR IDLE check should be a *no-op* for all but
> > > > pipe A
> > > > which is what was intended originally.
> > > > 
> > > So, the problem is when we update a non-PSR pipe (B or C) and PSR
> > > is
> > > active on another pipe(A, specifically), we end up waiting for the
> > > pipe
> > > A MMIO to become idle.
> > > 
> > > Can you please update the commit message as the commit message
> > > makes
> > > the per-pipe check sound like an optimization? 
> > > 
> > > This also points to a gap in our testing, I don't see a two pipe
> > > PSR
> > > related IGT.
> > > 
> > That's right. On my KBL chromebook that's running the drm-tip, when I
> > plug-in an external display, so pipe B,
> > I see "[drm:intel_pipe_update_start] *ERROR* PSR idle timed out,
> > atomic update may fail on pipe B", Iadded the pipe
> > name in the DRM_ERROR, may be I should make that change in the v3 of
> > this patch along with updating the commit message.
> > 
> > But, yea, this proves that with the CAN_PSR check, the non-PSR pipes
> > (B/C) wait on pipe-A to exit PSR which doesn't have
> > any reason to do so at that moment, hence the error.
> > 
> > I'll make the commit message changes and add the pipe name in the
> > DRM_ERROR as well ?
> 
> I am thinking you could pass crtc_state to intel_psr_wait_for_idle()
> and then check inside the implementation if the argument is the same as
> the pipe PSR was enabled on and then wait.
> 
> intel_psr_wait_for_idle(crtc_state) {
>   if (!CAN_PSR() || !crtc_state->has_psr)
>   return;
>   ...
> }
Hmm, but the CAN_PSR check is already taken care of by 
intel_psr_compute_config() which then sets has_psr, so just
if (!crtc_state->has_psr)
return;
should suffice, right ?

But then, we incur a function call for non-PSR pipes, which will return right 
away.
> 
> I don't like how intel_psr_wait_for_idle() doesn't care which pipe
> (transcoder actually) MMIO it should wait on.
>   
> 
> > > 
> > > > 
> > > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking
> > > > for
> > > > vblank evasion")
> > > > 
> > > > v2: Remove unnecessary parantheses, make checkpatch happy.
> > > > 
> > > > Reviewed-by: Rodrigo Vivi 
> > > > Signed-off-by: Tarun Vyas 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sprite.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index 4990d6e84ddf..83880e3a5f3d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct
> > > > intel_crtc_state *new_crtc_state)
> > > >      * VBL interrupts will start the PSR exit and prevent a
> > > > PSR
> > > >      * re-entry as well.
> > > >      */
> > > > -   if (CAN_PSR(dev_priv) &&
> > > > intel_psr_wait_for_idle(dev_priv))
> > > > +   if (new_crtc_state->has_psr &&
> > > > intel_psr_wait_for_idle(dev_priv))
> > > >     DRM_ERROR("PSR idle timed out, atomic update may
> > > > fail\n");
> > > >  
> > > >     local_irq_disable();
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

[Tarun Vyas]

On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan wrote:
> On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote:
> > In commit "drm/i915: Wait for PSR exit before checking for vblank
> > evasion", the idea was to limit the PSR IDLE checks when PSR is
> > actually supported. While CAN_PSR does do that check, it doesn't
> > applies on a per-crtc basis. crtc_state->has_psr is a more granular
> > check that avoids everything but pipe A, for the PSR IDLE check.
> > 
> > With this, the PSR IDLE check should be a *no-op* for all but pipe A
> > which is what was intended originally.
> > 
> 
> So, the problem is when we update a non-PSR pipe (B or C) and PSR is
> active on another pipe(A, specifically), we end up waiting for the pipe
> A MMIO to become idle.
> 
> Can you please update the commit message as the commit message makes
> the per-pipe check sound like an optimization? 
> 
> This also points to a gap in our testing, I don't see a two pipe PSR
> related IGT.
>
That's right. On my KBL chromebook that's running the drm-tip, when I plug-in 
an external display, so pipe B,
I see "[drm:intel_pipe_update_start] *ERROR* PSR idle timed out, atomic update 
may fail on pipe B", Iadded the pipe
name in the DRM_ERROR, may be I should make that change in the v3 of this patch 
along with updating the commit message.

But, yea, this proves that with the CAN_PSR check, the non-PSR pipes (B/C) wait 
on pipe-A to exit PSR which doesn't have
any reason to do so at that moment, hence the error.

I'll make the commit message changes and add the pipe name in the DRM_ERROR as 
well ?
> > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for
> > vblank evasion")
> > 
> > v2: Remove unnecessary parantheses, make checkpatch happy.
> > 
> > Reviewed-by: Rodrigo Vivi 
> > Signed-off-by: Tarun Vyas 
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 4990d6e84ddf..83880e3a5f3d 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct
> > intel_crtc_state *new_crtc_state)
> >      * VBL interrupts will start the PSR exit and prevent a PSR
> >      * re-entry as well.
> >      */
> > -   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
> > +   if (new_crtc_state->has_psr &&
> > intel_psr_wait_for_idle(dev_priv))
> >     DRM_ERROR("PSR idle timed out, atomic update may
> > fail\n");
> >  
> >     local_irq_disable();
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

[Tarun Vyas]

In commit "drm/i915: Wait for PSR exit before checking for vblank
evasion", the idea was to limit the PSR IDLE checks when PSR is
actually supported. While CAN_PSR does do that check, it doesn't
applies on a per-crtc basis. crtc_state->has_psr is a more granular
check that avoids everything but pipe A, for the PSR IDLE check.

With this, the PSR IDLE check should be a *no-op* for all but pipe A
which is what was intended originally.

Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for
vblank evasion")

v2: Remove unnecessary parantheses, make checkpatch happy.

Reviewed-by: Rodrigo Vivi 
Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 4990d6e84ddf..83880e3a5f3d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
 * VBL interrupts will start the PSR exit and prevent a PSR
 * re-entry as well.
 */
-   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   if (new_crtc_state->has_psr && intel_psr_wait_for_idle(dev_priv))
DRM_ERROR("PSR idle timed out, atomic update may fail\n");
 
local_irq_disable();
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Use crtc_state->has_psr instead of CAN_PSR for pipe update

[Tarun Vyas]

In commit "drm/i915: Wait for PSR exit before checking for vblank
evasion", the idea was to limit the PSR IDLE checks when PSR is
actually supported. While CAN_PSR does do that check, it doesn't
applies on a per-crtc basis. crtc_state->has_psr is a more granular
check that avoids everything but pipe A, for the PSR IDLE check.

With this, the PSR IDLE check should be a *no-op* for all but pipe A
which is what was intended originally.

Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for
vblank evasion")

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 4990d6e84ddf..c27720847672 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
 * VBL interrupts will start the PSR exit and prevent a PSR
 * re-entry as well.
 */
-   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   if ((new_crtc_state->has_psr) && intel_psr_wait_for_idle(dev_priv))
DRM_ERROR("PSR idle timed out, atomic update may fail\n");
 
local_irq_disable();
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v8 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is disabled, we incur
a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

v4: Comment explaining psr_wait after enabling VBL interrupts (DK)

v5: CAN_PSR() to handle platforms that don't support PSR.

v6: Handle local_irq_disable on early return (Chris)

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_sprite.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..4990d6e84ddf 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,13 +107,21 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
  VBLANK_EVASION_TIME_US);
max = vblank_start - 1;
 
-   local_irq_disable();
-
if (min <= 0 || max <= 0)
-   return;
+   goto irq_disable;
 
if (WARN_ON(drm_crtc_vblank_get(>base)))
-   return;
+   goto irq_disable;
+
+   /*
+* Wait for psr to idle out after enabling the VBL interrupts
+* VBL interrupts will start the PSR exit and prevent a PSR
+* re-entry as well.
+*/
+   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   DRM_ERROR("PSR idle timed out, atomic update may fail\n");
+
+   local_irq_disable();
 
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
@@ -171,6 +179,10 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
 
trace_i915_pipe_update_vblank_evaded(crtc);
+   return;
+
+irq_disable:
+   local_irq_disable();
 }
 
 /**
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v8 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

This is a lockless version of the exisiting psr_wait_for_idle().
We want to wait for PSR to idle out inside intel_pipe_update_start.
At the time of a pipe update, we should never race with any psr
enable or disable code, which is a part of crtc enable/disable.
The follow up patch will use this lockless wait inside pipe_update_
start to wait for PSR to idle out before checking for vblank evasion.
We need to keep the wait in pipe_update_start to as less as it can be.
So,we can live and flourish w/o taking any psr locks at all.

Even if psr is never enabled, psr2_enabled will be false and this
function will wait for PSR1 to idle out, which should just return
immediately, so a very short (~1-2 usec) wait for cases where PSR
is disabled.

v2: Add comment to explain the 25msec timeout (DK)

v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
naming conflicts and propagate err (if any) to the caller (Chris)

v5: Form a series with the next patch

v7: Better explain the need for lockless wait and increase the max
timeout to handle refresh rates < 60 Hz (Daniel Vetter)

v8: Rebase

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 36 ++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a6ff2600a3a0..b9b70321c054 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1922,6 +1922,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
 void intel_psr_short_pulse(struct intel_dp *intel_dp);
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 45f1cb7d6c04..23acc9ac8d4d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -717,7 +717,39 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
-static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+{
+   i915_reg_t reg;
+   u32 mask;
+
+   /*
+* The sole user right now is intel_pipe_update_start(),
+* which won't race with psr_enable/disable, which is
+* where psr2_enabled is written to. So, we don't need
+* to acquire the psr.lock. More importantly, we want the
+* latency inside intel_pipe_update_start() to be as low
+* as possible, so no need to acquire psr.lock when it is
+* not needed and will induce latencies in the atomic
+* update path.
+*/
+   if (dev_priv->psr.psr2_enabled) {
+   reg = EDP_PSR2_STATUS;
+   mask = EDP_PSR2_STATUS_STATE_MASK;
+   } else {
+   reg = EDP_PSR_STATUS;
+   mask = EDP_PSR_STATUS_STATE_MASK;
+   }
+
+   /*
+* Max time for PSR to idle = Inverse of the refresh rate +
+* 6 ms of exit training time + 1.5 ms of aux channel
+* handshake. 50 msec is defesive enough to cover everything.
+*/
+   return intel_wait_for_register(dev_priv, reg, mask,
+  EDP_PSR_STATUS_STATE_IDLE, 50);
+}
+
+static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
i915_reg_t reg;
@@ -763,7 +795,7 @@ static void intel_psr_work(struct work_struct *work)
 * PSR might take some time to get fully disabled
 * and be ready for re-enable.
 */
-   if (!psr_wait_for_idle(dev_priv))
+   if (!__psr_wait_for_idle_locked(dev_priv))
goto unlock;
 
/*
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v7 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

This is a lockless version of the exisiting psr_wait_for_idle().
We want to wait for PSR to idle out inside intel_pipe_update_start.
At the time of a pipe update, we should never race with any psr
enable or disable code, which is a part of crtc enable/disable.
The follow up patch will use this lockless wait inside pipe_update_
start to wait for PSR to idle out before checking for vblank evasion.
We need to keep the wait in pipe_update_start to as less as it can be.
So,we can live and flourish w/o taking any psr locks at all.

Even if psr is never enabled, psr2_enabled will be false and this
function will wait for PSR1 to idle out, which should just return
immediately, so a very short (~1-2 usec) wait for cases where PSR
is disabled.

v2: Add comment to explain the 25msec timeout (DK)

v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
naming conflicts and propagate err (if any) to the caller (Chris)

v5: Form a series with the next patch

v7: Better explain the need for lockless wait and increase the max
timeout to handle refresh rates < 60 Hz (Daniel Vetter)

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 36 ++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578346b8d7e2..9cb2b8afdd3e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
  struct intel_crtc_state *crtc_state);
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index aea81ace854b..d11fd8a01d98 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -757,7 +757,39 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
-static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+{
+   i915_reg_t reg;
+   u32 mask;
+
+   /*
+* The sole user right now is intel_pipe_update_start(),
+* which won't race with psr_enable/disable, which is
+* where psr2_enabled is written to. So, we don't need
+* to acquire the psr.lock. More importantly, we want the
+* latency inside intel_pipe_update_start() to be as low
+* as possible, so no need to acquire psr.lock when it is
+* not needed and will induce latencies in the atomic
+* update path.
+*/
+   if (dev_priv->psr.psr2_enabled) {
+   reg = EDP_PSR2_STATUS;
+   mask = EDP_PSR2_STATUS_STATE_MASK;
+   } else {
+   reg = EDP_PSR_STATUS;
+   mask = EDP_PSR_STATUS_STATE_MASK;
+   }
+
+   /*
+* Max time for PSR to idle = Inverse of the refresh rate +
+* 6 ms of exit training time + 1.5 ms of aux channel
+* handshake. 50 msec is defesive enough to cover everything.
+*/
+   return intel_wait_for_register(dev_priv, reg, mask,
+  EDP_PSR_STATUS_STATE_IDLE, 50);
+}
+
+static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
i915_reg_t reg;
@@ -803,7 +835,7 @@ static void intel_psr_work(struct work_struct *work)
 * PSR might take some time to get fully disabled
 * and be ready for re-enable.
 */
-   if (!psr_wait_for_idle(dev_priv))
+   if (!__psr_wait_for_idle_locked(dev_priv))
goto unlock;
 
/*
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v7 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is disabled, we incur
a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

v4: Comment explaining psr_wait after enabling VBL interrupts (DK)

v5: CAN_PSR() to handle platforms that don't support PSR.

v6: Handle local_irq_disable on early return (Chris)

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_sprite.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..4990d6e84ddf 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,13 +107,21 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
  VBLANK_EVASION_TIME_US);
max = vblank_start - 1;
 
-   local_irq_disable();
-
if (min <= 0 || max <= 0)
-   return;
+   goto irq_disable;
 
if (WARN_ON(drm_crtc_vblank_get(>base)))
-   return;
+   goto irq_disable;
+
+   /*
+* Wait for psr to idle out after enabling the VBL interrupts
+* VBL interrupts will start the PSR exit and prevent a PSR
+* re-entry as well.
+*/
+   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   DRM_ERROR("PSR idle timed out, atomic update may fail\n");
+
+   local_irq_disable();
 
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
@@ -171,6 +179,10 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
 
trace_i915_pipe_update_vblank_evaded(crtc);
+   return;
+
+irq_disable:
+   local_irq_disable();
 }
 
 /**
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

On Tue, Jun 26, 2018 at 09:42:11AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2018-06-26 09:26:57)
> > On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> > > This is a lockless version of the exisiting psr_wait_for_idle().
> > > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > > At the time of a pipe update, we should never race with any psr
> > > enable or disable code, which is a part of crtc enable/disable. So,
> > > we can live w/o taking any psr locks at all.
> > > The follow up patch will use this lockless wait inside pipe_update_
> > > start to wait for PSR to idle out before checking for vblank evasion.
> > 
> > What's the upside of the lockless wait? The patch seems to be entirely
> > missing the motivation for the change. "Make it lockless" isn't a good
> > justification on itself, there needs to be data about overhead or stalls
> > included if that's the reason for doing this change.
> > 
> > > Even if psr is never enabled, psr2_enabled will be false and this
> > > function will wait for PSR1 to idle out, which should just return
> > > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > > is disabled.
> > > 
> > > v2: Add comment to explain the 25msec timeout (DK)
> > > 
> > > v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
> > > naming conflicts and propagate err (if any) to the caller (Chris)
> > > 
> > > v5: Form a series with the next patch
> > > 
> > > Signed-off-by: Tarun Vyas 
> > > ---
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c | 25 +++--
> > >  2 files changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 578346b8d7e2..9cb2b8afdd3e 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp 
> > > *intel_dp,
> > > struct intel_crtc_state *crtc_state);
> > >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool 
> > > debug);
> > >  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 
> > > psr_iir);
> > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
> > >  
> > >  /* intel_runtime_pm.c */
> > >  int intel_power_domains_init(struct drm_i915_private *);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index aea81ace854b..41e6962923ae 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> > >   cancel_work_sync(_priv->psr.work);
> > >  }
> > >  
> > > -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > +{
> > > + i915_reg_t reg;
> > > + u32 mask;
> > > +
> > 
> > I think a comment here explaining why the lockless access is correct is
> > justified here.
> > 
> > > + if (dev_priv->psr.psr2_enabled) {
> 
> In particular, it is this 'psr2_enabled' and which register we need that
> is serialised in the locked version. The important question to answer is
> why can we lift that here and not there.
> 
> In this case (and even in the other case), you could simply say "do both".
> -Chris
With the locked case, the concern was that intel_psr_work can race with 
psr_enable/disable paths but we have no such concern here, as we are already 
too far in the commit path. psr2_enabled should be stable by the time we do 
pipe_update(s), so we can do a fast check in pipe_update w/o grabbing and 
subsequently dropping locks.

Also, another difference here is that, "intel_dp = dev_priv->psr.enabled;" has 
been dropped in the lockless version as we don't really care if psr is enabled 
or disabled. If it is disabled, then the intel_wait_for_register should be 
close to a noop, ~1usec hit as per the preliminary measurements. 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

On Tue, Jun 26, 2018 at 12:43:42PM -0700, Dhinakaran Pandiyan wrote:
> On Tue, 2018-06-26 at 10:26 +0200, Daniel Vetter wrote:
> > On Mon, Jun 25, 2018 at 10:57:23PM -0700, Tarun Vyas wrote:
> > > 
> > > This is a lockless version of the exisiting psr_wait_for_idle().
> > > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > > At the time of a pipe update, we should never race with any psr
> > > enable or disable code, which is a part of crtc enable/disable. So,
> > > we can live w/o taking any psr locks at all.
> > > The follow up patch will use this lockless wait inside pipe_update_
> > > start to wait for PSR to idle out before checking for vblank
> > > evasion.
> > What's the upside of the lockless wait? The patch seems to be
> > entirely
> > missing the motivation for the change. "Make it lockless" isn't a
> > good
> > justification on itself, there needs to be data about overhead or
> > stalls
> > included if that's the reason for doing this change.
> > 
> Acquiring the PSR mutex means potential stalls due to PSR work having
> already acquired it. The idea was to keep PSR changes in
> pipe_update_start() less invasive latency wise.
> 
> But yeah, the commit has to add the explanation.
> 
> 
>
Yea, will explain it better in the commit message. 
> > > 
> > > Even if psr is never enabled, psr2_enabled will be false and this
> > > function will wait for PSR1 to idle out, which should just return
> > > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > > is disabled.
> > > 
> > > v2: Add comment to explain the 25msec timeout (DK)
> > > 
> > > v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
> > > naming conflicts and propagate err (if any) to the caller
> > > (Chris)
> > > 
> > > v5: Form a series with the next patch
> > > 
> > > Signed-off-by: Tarun Vyas 
> > > ---
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_psr.c | 25 +++--
> > >  2 files changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h
> > > index 578346b8d7e2..9cb2b8afdd3e 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > >     struct intel_crtc_state
> > > *crtc_state);
> > >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool
> > > debug);
> > >  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > > psr_iir);
> > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
> > >  
> > >  /* intel_runtime_pm.c */
> > >  int intel_power_domains_init(struct drm_i915_private *);
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index aea81ace854b..41e6962923ae 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp
> > > *intel_dp,
> > >   cancel_work_sync(_priv->psr.work);
> > >  }
> > >  
> > > -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > +int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> > > +{
> > > + i915_reg_t reg;
> > > + u32 mask;
> > > +
> > I think a comment here explaining why the lockless access is correct
> > is
> > justified here.
> > 
> > > 
> > > + if (dev_priv->psr.psr2_enabled) {
> > > + reg = EDP_PSR2_STATUS;
> > > + mask = EDP_PSR2_STATUS_STATE_MASK;
> > > + } else {
> > > + reg = EDP_PSR_STATUS;
> > > + mask = EDP_PSR_STATUS_STATE_MASK;
> > > + }
> > > +
> > > + /*
> > > +  * The  25 msec timeout accounts for a frame @ 60Hz
> > > refresh rate,
> > > +  * exit training an aux handshake time.
> > > +  */
> > So this goes boom if the panel is running at e.g. 50Hz? Please either
> > calculate this from the current mode (but that's a bit tricky, due to
> > DRRS), or go with a more defensive timeout. Also small typo,
> > s/an/and/.
> > 
> > Would also be good to have 

[Intel-gfx] [PATCH v6 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

This is a lockless version of the exisiting psr_wait_for_idle().
We want to wait for PSR to idle out inside intel_pipe_update_start.
At the time of a pipe update, we should never race with any psr
enable or disable code, which is a part of crtc enable/disable. So,
we can live w/o taking any psr locks at all.
The follow up patch will use this lockless wait inside pipe_update_
start to wait for PSR to idle out before checking for vblank evasion.

Even if psr is never enabled, psr2_enabled will be false and this
function will wait for PSR1 to idle out, which should just return
immediately, so a very short (~1-2 usec) wait for cases where PSR
is disabled.

v2: Add comment to explain the 25msec timeout (DK)

v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
naming conflicts and propagate err (if any) to the caller (Chris)

v5: Form a series with the next patch

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 25 +++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578346b8d7e2..9cb2b8afdd3e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
  struct intel_crtc_state *crtc_state);
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index aea81ace854b..41e6962923ae 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
-static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+{
+   i915_reg_t reg;
+   u32 mask;
+
+   if (dev_priv->psr.psr2_enabled) {
+   reg = EDP_PSR2_STATUS;
+   mask = EDP_PSR2_STATUS_STATE_MASK;
+   } else {
+   reg = EDP_PSR_STATUS;
+   mask = EDP_PSR_STATUS_STATE_MASK;
+   }
+
+   /*
+* The  25 msec timeout accounts for a frame @ 60Hz refresh rate,
+* exit training an aux handshake time.
+*/
+   return intel_wait_for_register(dev_priv, reg, mask,
+  EDP_PSR_STATUS_STATE_IDLE, 25);
+}
+
+static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
i915_reg_t reg;
@@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct *work)
 * PSR might take some time to get fully disabled
 * and be ready for re-enable.
 */
-   if (!psr_wait_for_idle(dev_priv))
+   if (!__psr_wait_for_idle_locked(dev_priv))
goto unlock;
 
/*
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v6 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is disabled, we incur
a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

v4: Comment explaining psr_wait after enabling VBL interrupts (DK)

v5: CAN_PSR() to handle platforms that don't support PSR.

v6: Handle local_irq_disable on early return (Chris)

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_sprite.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..4990d6e84ddf 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,13 +107,21 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
  VBLANK_EVASION_TIME_US);
max = vblank_start - 1;
 
-   local_irq_disable();
-
if (min <= 0 || max <= 0)
-   return;
+   goto irq_disable;
 
if (WARN_ON(drm_crtc_vblank_get(>base)))
-   return;
+   goto irq_disable;
+
+   /*
+* Wait for psr to idle out after enabling the VBL interrupts
+* VBL interrupts will start the PSR exit and prevent a PSR
+* re-entry as well.
+*/
+   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   DRM_ERROR("PSR idle timed out, atomic update may fail\n");
+
+   local_irq_disable();
 
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
@@ -171,6 +179,10 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
 
trace_i915_pipe_update_vblank_evaded(crtc);
+   return;
+
+irq_disable:
+   local_irq_disable();
 }
 
 /**
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

On Mon, Jun 25, 2018 at 01:56:24PM +0100, Chris Wilson wrote:
> Quoting Tarun Vyas (2018-06-25 08:09:18)
> > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
> > the pipe_update_start call schedules itself out to check back later.
> > 
> > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
> > lags w.r.t core kernel code, hot plugging an external display triggers
> > tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
> > closer analysis reveals that we try to read the scanline 3 times and
> > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
> > stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
> > reason we loop inside intel_pipe_update start for ~2+ msec which in this
> > case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
> > counter, hence no error. On the other hand, the ChromeOS kernel spends
> > ~1.1 msec looping inside intel_pipe_update_start and hence errors out
> > b/c the source is still in PSR.
> > 
> > Regardless, we should wait for PSR exit (if PSR is disabled, we incur
> > a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't
> > fully exited PSR, then checking for vblank evasion isn't actually
> > applicable.
> > 
> > v4: Comment explaining psr_wait after enabling VBL interrupts (DK)
> > 
> > v5: CAN_PSR() to handle platforms that don't support PSR.
> > 
> > Signed-off-by: Tarun Vyas 
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 12 ++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 344c0e709b19..8982a69a13dd 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -107,14 +107,22 @@ void intel_pipe_update_start(const struct 
> > intel_crtc_state *new_crtc_state)
> >   
> > VBLANK_EVASION_TIME_US);
> > max = vblank_start - 1;
> >  
> > -   local_irq_disable();
> > -
> > if (min <= 0 || max <= 0)
> > return;
> >  
> > if (WARN_ON(drm_crtc_vblank_get(>base)))
> > return;
> >  
> > +   /*
> > +* Wait for psr to idle out after enabling the VBL interrupts
> > +* VBL interrupts will start the PSR exit and prevent a PSR
> > +* re-entry as well.
> > +*/
> > +   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
> > +   DRM_ERROR("PSR idle timed out, atomic update may fail\n");
> > +
> > +   local_irq_disable();
> 
> The function must return with irqs disabled as the update_end is always
> called to re-enable irqs.
> 
> The pair of early returns may be unjustified, but they still do exist...
> -Chris
Oops ! Missed it, will handle in v6 :( . I am curious as to how the atomic 
update will go ahead in the event of these early returns.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v5 2/2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is disabled, we incur
a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

v4: Comment explaining psr_wait after enabling VBL interrupts (DK)

v5: CAN_PSR() to handle platforms that don't support PSR.

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_sprite.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..8982a69a13dd 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,14 +107,22 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
  VBLANK_EVASION_TIME_US);
max = vblank_start - 1;
 
-   local_irq_disable();
-
if (min <= 0 || max <= 0)
return;
 
if (WARN_ON(drm_crtc_vblank_get(>base)))
return;
 
+   /*
+* Wait for psr to idle out after enabling the VBL interrupts
+* VBL interrupts will start the PSR exit and prevent a PSR
+* re-entry as well.
+*/
+   if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv))
+   DRM_ERROR("PSR idle timed out, atomic update may fail\n");
+
+   local_irq_disable();
+
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
trace_i915_pipe_update_start(crtc);
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v5 1/2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

This is a lockless version of the exisiting psr_wait_for_idle().
We want to wait for PSR to idle out inside intel_pipe_update_start.
At the time of a pipe update, we should never race with any psr
enable or disable code, which is a part of crtc enable/disable. So,
we can live w/o taking any psr locks at all.
The follow up patch will use this lockless wait inside pipe_update_
start to wait for PSR to idle out before checking for vblank evasion.

Even if psr is never enabled, psr2_enabled will be false and this
function will wait for PSR1 to idle out, which should just return
immediately, so a very short (~1-2 usec) wait for cases where PSR
is disabled.

v2: Add comment to explain the 25msec timeout (DK)

v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
naming conflicts and propagate err (if any) to the caller (Chris)

v5: Form a series with the next patch

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 25 +++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578346b8d7e2..9cb2b8afdd3e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
  struct intel_crtc_state *crtc_state);
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index aea81ace854b..41e6962923ae 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
-static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+{
+   i915_reg_t reg;
+   u32 mask;
+
+   if (dev_priv->psr.psr2_enabled) {
+   reg = EDP_PSR2_STATUS;
+   mask = EDP_PSR2_STATUS_STATE_MASK;
+   } else {
+   reg = EDP_PSR_STATUS;
+   mask = EDP_PSR_STATUS_STATE_MASK;
+   }
+
+   /*
+* The  25 msec timeout accounts for a frame @ 60Hz refresh rate,
+* exit training an aux handshake time.
+*/
+   return intel_wait_for_register(dev_priv, reg, mask,
+  EDP_PSR_STATUS_STATE_IDLE, 25);
+}
+
+static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
i915_reg_t reg;
@@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct *work)
 * PSR might take some time to get fully disabled
 * and be ready for re-enable.
 */
-   if (!psr_wait_for_idle(dev_priv))
+   if (!__psr_wait_for_idle_locked(dev_priv))
goto unlock;
 
/*
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is disabled, we incur
a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

v4: Comment explaining psr_wait after enabling VBL interrupts (DK)

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_sprite.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..3a4ad6fed246 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,14 +107,22 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
  VBLANK_EVASION_TIME_US);
max = vblank_start - 1;
 
-   local_irq_disable();
-
if (min <= 0 || max <= 0)
return;
 
if (WARN_ON(drm_crtc_vblank_get(>base)))
return;
 
+   /*
+* Wait for psr to idle out after enabling the VBL interrupts
+* VBL interrupts will start the PSR exit and prevent a PSR
+* re-entry as well.
+*/
+   if (intel_psr_wait_for_idle(dev_priv))
+   DRM_ERROR("PSR idle timed out, atomic update may fail\n");
+
+   local_irq_disable();
+
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
trace_i915_pipe_update_start(crtc);
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

This is a lockless version of the exisiting psr_wait_for_idle().
We want to wait for PSR to idle out inside intel_pipe_update_start.
At the time of a pipe update, we should never race with any psr
enable or disable code, which is a part of crtc enable/disable. So,
we can live w/o taking any psr locks at all.
The follow up patch will use this lockless wait inside pipe_update_
start to wait for PSR to idle out before checking for vblank evasion.

Even if psr is never enabled, psr2_enabled will be false and this
function will wait for PSR1 to idle out, which should just return
immediately, so a very short (~1-2 usec) wait for cases where PSR
is disabled.

v2: Add comment to explain the 25msec timeout (DK)

v3: Rename psr_wait_for_idle to __psr_wait_for_idle_locked to avoid
naming conflicts and propagate err (if any) to the caller (Chris)

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 25 +++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578346b8d7e2..9cb2b8afdd3e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
  struct intel_crtc_state *crtc_state);
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index aea81ace854b..41e6962923ae 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -757,7 +757,28 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
-static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
+int intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+{
+   i915_reg_t reg;
+   u32 mask;
+
+   if (dev_priv->psr.psr2_enabled) {
+   reg = EDP_PSR2_STATUS;
+   mask = EDP_PSR2_STATUS_STATE_MASK;
+   } else {
+   reg = EDP_PSR_STATUS;
+   mask = EDP_PSR_STATUS_STATE_MASK;
+   }
+
+   /*
+* The  25 msec timeout accounts for a frame @ 60Hz refresh rate,
+* exit training an aux handshake time.
+*/
+   return intel_wait_for_register(dev_priv, reg, mask,
+  EDP_PSR_STATUS_STATE_IDLE, 25);
+}
+
+static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
i915_reg_t reg;
@@ -803,7 +824,7 @@ static void intel_psr_work(struct work_struct *work)
 * PSR might take some time to get fully disabled
 * and be ready for re-enable.
 */
-   if (!psr_wait_for_idle(dev_priv))
+   if (!__psr_wait_for_idle_locked(dev_priv))
goto unlock;
 
/*
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Lockless wait for PSR idle.

[Tarun Vyas]

On Fri, Jun 22, 2018 at 01:51:08AM -0700, Tarun Vyas wrote:
> Before checking for vblank evasion in pipe_update_start, we
> need to wait for PSR idle. intel_psr.c already has psr_wait_for_idle
> but we don't need any psr locks in pipe_update_start anyway b/c
> psr_enable/disable won't race there. There is some code duplication
> here but can't help it (borrowed from psr_wait_for_idle).
> 
> Signed-off-by: Tarun Vyas 
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 17 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index d7dbca1aabff..935eb3d5d9bb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1919,6 +1919,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
> struct intel_crtc_state *crtc_state);
>  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
>  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
> +void intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index db27f2faa1de..75e2035ba29f 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -883,6 +883,23 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>   cancel_delayed_work_sync(_priv->psr.work);
>  }
>  
> +void intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
> +{
> + i915_reg_t reg;
> + u32 mask;
> +
> + if (dev_priv->psr.psr2_enabled) {
> + reg = EDP_PSR2_STATUS;
> + mask = EDP_PSR2_STATUS_STATE_MASK;
> + } else {
> + reg = EDP_PSR_STATUS;
> + mask = EDP_PSR_STATUS_STATE_MASK;
> + }
> +
> + if (intel_wait_for_register(dev_priv, reg, mask, 0, 25))
> + DRM_ERROR("Timed out waiting for PSR Idle\n");
> +}
> +
>  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
>  {
>   struct intel_dp *intel_dp;

Wrong patch. Please ignore.
> -- 
> 2.13.5
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 1/2] drm/i915: Lockless wait for PSR idle.

[Tarun Vyas]

Before checking for vblank evasion in pipe_update_start, we
need to wait for PSR idle. intel_psr.c already has psr_wait_for_idle
but we don't need any psr locks in pipe_update_start anyway b/c
psr_enable/disable won't race there. There is some code duplication
here but can't help it (borrowed from psr_wait_for_idle).

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 17 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d7dbca1aabff..935eb3d5d9bb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1919,6 +1919,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
  struct intel_crtc_state *crtc_state);
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
+void intel_psr_wait_for_idle(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index db27f2faa1de..75e2035ba29f 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -883,6 +883,23 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_delayed_work_sync(_priv->psr.work);
 }
 
+void intel_psr_wait_for_idle(struct drm_i915_private *dev_priv)
+{
+   i915_reg_t reg;
+   u32 mask;
+
+   if (dev_priv->psr.psr2_enabled) {
+   reg = EDP_PSR2_STATUS;
+   mask = EDP_PSR2_STATUS_STATE_MASK;
+   } else {
+   reg = EDP_PSR_STATUS;
+   mask = EDP_PSR_STATUS_STATE_MASK;
+   }
+
+   if (intel_wait_for_register(dev_priv, reg, mask, 0, 25))
+   DRM_ERROR("Timed out waiting for PSR Idle\n");
+}
+
 static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

On Fri, Jun 22, 2018 at 08:12:00AM +0100, Chris Wilson wrote:
> Quoting Tarun Vyas (2018-06-22 08:05:21)
> > This is a lockless version of the exisiting psr_wait_for_idle().
> > We want to wait for PSR to idle out inside intel_pipe_update_start.
> > At the time of a pipe update, we should never race with any psr
> > enable or disable code, which is a part of crtc enable/disable. So,
> > we can live w/o taking any psr locks at all.
> > The follow up patch will use this lockless wait inside pipe_update_
> > start to wait for PSR to idle out before checking for vblank evasion.
> > 
> > Even if psr is never enabled, psr2_enabled will be false and this
> > function will wait for PSR1 to idle out, which should just return
> > immediately, so a very short (~1-2 usec) wait for cases where PSR
> > is disabled.
> > 
> > v2: Add comment to explain the 25msec timeout (DK)
> > 
> > Signed-off-by: Tarun Vyas 
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 24 
> >  2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 578346b8d7e2..a48aad0f99bf 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp 
> > *intel_dp,
> >   struct intel_crtc_state *crtc_state);
> >  void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
> >  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
> > +void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv);
> >  
> >  /* intel_runtime_pm.c */
> >  int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index aea81ace854b..8e69e6193063 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -757,6 +757,30 @@ void intel_psr_disable(struct intel_dp *intel_dp,
> > cancel_work_sync(_priv->psr.work);
> >  }
> >  
> > +void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv)
> 
> intel_psr_
> 
> No need for lockless here you don't export anything else. Rename one of
> the two to avoid the conflict. Probably __psr_wait_for_idle_locked,
> though you would expect some sort of reuse between very similarly named
> functions.
>
Thanks for the review Chris.

Yes, there is a lot duplication between the two, but the locking drop in 
psr_wait_for_idle(), before the wait_for_register is making it harder to reuse 
it.
Will rename the original wait function.  
> > +{
> > +   i915_reg_t reg;
> > +   u32 mask;
> > +   int err;
> > +
> > +   if (dev_priv->psr.psr2_enabled) {
> > +   reg = EDP_PSR2_STATUS;
> > +   mask = EDP_PSR2_STATUS_STATE_MASK;
> > +   } else {
> > +   reg = EDP_PSR_STATUS;
> > +   mask = EDP_PSR_STATUS_STATE_MASK;
> > +   }
> > +
> > +   /*
> > +* The  25 msec timeout accounts for a frame @ 60Hz refresh rate,
> > +* exit training an aux handshake time.
> > +*/
> > +   err = intel_wait_for_register(dev_priv, reg, mask,
> > + EDP_PSR_STATUS_STATE_IDLE, 25);
> > +   if (err)
> > +   DRM_ERROR("Timed out waiting for PSR Idle for pipe 
> > update\n");
> 
> Propagate the error, let the caller decide if it's an error or not.
> -Chrisa
Will do, in the next version.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

This is a lockless version of the exisiting psr_wait_for_idle().
We want to wait for PSR to idle out inside intel_pipe_update_start.
At the time of a pipe update, we should never race with any psr
enable or disable code, which is a part of crtc enable/disable. So,
we can live w/o taking any psr locks at all.
The follow up patch will use this lockless wait inside pipe_update_
start to wait for PSR to idle out before checking for vblank evasion.

Even if psr is never enabled, psr2_enabled will be false and this
function will wait for PSR1 to idle out, which should just return
immediately, so a very short (~1-2 usec) wait for cases where PSR
is disabled.

v2: Add comment to explain the 25msec timeout (DK)

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 24 
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578346b8d7e2..a48aad0f99bf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
  struct intel_crtc_state *crtc_state);
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
+void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index aea81ace854b..8e69e6193063 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -757,6 +757,30 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
+void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv)
+{
+   i915_reg_t reg;
+   u32 mask;
+   int err;
+
+   if (dev_priv->psr.psr2_enabled) {
+   reg = EDP_PSR2_STATUS;
+   mask = EDP_PSR2_STATUS_STATE_MASK;
+   } else {
+   reg = EDP_PSR_STATUS;
+   mask = EDP_PSR_STATUS_STATE_MASK;
+   }
+
+   /*
+* The  25 msec timeout accounts for a frame @ 60Hz refresh rate,
+* exit training an aux handshake time.
+*/
+   err = intel_wait_for_register(dev_priv, reg, mask,
+ EDP_PSR_STATUS_STATE_IDLE, 25);
+   if (err)
+   DRM_ERROR("Timed out waiting for PSR Idle for pipe update\n");
+}
+
 static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is disabled, we incur
a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_sprite.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index 344c0e709b19..34754771d7a7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,14 +107,16 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
  VBLANK_EVASION_TIME_US);
max = vblank_start - 1;
 
-   local_irq_disable();
-
if (min <= 0 || max <= 0)
return;
 
if (WARN_ON(drm_crtc_vblank_get(>base)))
return;
 
+   psr_wait_for_idle_lockless(dev_priv);
+
+   local_irq_disable();
+
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
trace_i915_pipe_update_start(crtc);
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/psr: Lockless version of psr_wait_for_idle

[Tarun Vyas]

This is a lockless version of the exisiting psr_wait_for_idle().
We want to wait for PSR to idle out inside intel_pipe_update_start.
At the time of a pipe update, we should never race with any psr
enable or disable code, which is a part of crtc enable/disable. So,
we can live w/o taking any psr locks at all.
The follow up patch will use this lockless wait inside pipe_update_
start to wait for PSR to idle out before checking for vblank evasion.

Even if psr is never enabled, psr2_enabled will be false and this
function will wait for PSR1 to idle out, which should just return
immediately, so a very short (~1-2 usec) wait for cases where PSR
is disabled.

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 19 +++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 578346b8d7e2..a48aad0f99bf 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1920,6 +1920,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
  struct intel_crtc_state *crtc_state);
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
+void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index aea81ace854b..425147444f69 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -757,6 +757,25 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_work_sync(_priv->psr.work);
 }
 
+void psr_wait_for_idle_lockless(struct drm_i915_private *dev_priv)
+{
+   i915_reg_t reg;
+   u32 mask;
+   int err;
+
+   if (dev_priv->psr.psr2_enabled) {
+   reg = EDP_PSR2_STATUS;
+   mask = EDP_PSR2_STATUS_STATE_MASK;
+   } else {
+   reg = EDP_PSR_STATUS;
+   mask = EDP_PSR_STATUS_STATE_MASK;
+   }
+
+   err = intel_wait_for_register(dev_priv, reg, mask, 
EDP_PSR_STATUS_STATE_IDLE, 25);
+   if (err)
+   DRM_ERROR("Timed out waiting for PSR Idle for pipe update\n");
+}
+
 static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

On Tue, Jun 19, 2018 at 02:59:54PM -0700, Tarun Vyas wrote:
> On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> > > On Mon, 2018-05-14 at 13:49 -0700, Tarun Vyas wrote:
> > > > 
> > > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited,
> > > > then
> > > > the pipe_update_start call schedules itself out to check back
> > > > later.
> > > > 
> > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915
> > > > but
> > > > lags w.r.t core kernel code, hot plugging an external display
> > > > triggers
> > > > tons of "potential atomic update errors" in the dmesg, on *pipe A*.
> > > > A
> > > > closer analysis reveals that we try to read the scanline 3 times
> > > > and
> > > > eventually timeout, b/c PSR hasn't exited fully leading to a
> > > > PIPEDSL
> > > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for
> > > > *some*
> > > > reason we loop inside intel_pipe_update start for ~2+ msec which in
> > > > this
> > > > case is more than enough to exit PSR fully, hence an *unstuck*
> > > > PIPEDSL
> > > > counter, hence no error. On the other hand, the ChromeOS kernel
> > > > spends
> > > > ~1.1 msec looping inside intel_pipe_update_start and hence errors
> > > > out
> > > > b/c the source is still in PSR.
> > > > 
> > > > Regardless, we should wait for PSR exit (if PSR is supported and
> > > > active
> > > > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > > > fully exited PSR, then checking for vblank evasion isn't actually
> > > > applicable.
> > > > 
> > > > This scenario applies to a configuration with an additional pipe,
> > > > as of now
> > > > 
> > > > Signed-off-by: Tarun Vyas 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sprite.c | 7 +--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index ee23613f9fd4..481d310e5c3b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct
> > > > intel_crtc_state *new_crtc_state)
> > > >       VBLANK_EVASI
> > > > ON
> > > > _TIME_US);
> > > >     max = vblank_start - 1;
> > > >  
> > > > -   local_irq_disable();
> > > > -
> > > >     if (min <= 0 || max <= 0)
> > > >     return;
> > > >  
> > > >     if (WARN_ON(drm_crtc_vblank_get(>base)))
> > > >     return;
> > > >  
> > > > +   if(new_crtc_state->has_psr && dev_priv->psr.active)
> > > > +   psr_wait_for_idle(dev_priv);
> > > How about just waiting for PSR_STATUS to idle without grabbing any
> > > locks or checking whether PSR is active?
> > > 
> > > Status should be idle if PSR was disabled or on it's way to becoming
> > > idle if it was enabled. Even if PSR did get enabled while we are in
> > > pipe_update_start(), it will not be active as long as VBIs are
> > > enabled.
> > > 
> Right, if we are OK with some duplication (of psr_wait_for_idle) inside 
> intel_psr.c, then we can duplicate the PSR2 vs. PSR check that's being done 
> in psr_wait_for_idle and then just wait without grabbing any locks, so 
> essentially a lockless version of psr_wait_for_idle()
> > Correct me if this was already considered, why not wait until the
> > scanline counter starts moving? I see we have a 
> > intel_wait_for_pipe_scanline_moving(crtc) that's used when the
> > pipe is enabled.
> > 
> > -DK
> 
> Didn't consider this before, but, pipe_scanline_is_moving waits for a minimum 
> of 5 msec. Are we OK with a min wait of 5 msec inside pipe_update_start ? 
> Heuristically, waiting for PSR idle has almost always returned within < 2 
> msec. Occasionally it takes upto 1 full frame.
As per some preliminary measurements
Approach 1:
Wait *unconditionally* (so no need to check for PSR enabled/disabled 
and hence no locks) for PSR_STATUS to IDLE out. 
This takes ~7msec when PSR is active and ~2 usec when PSR is inactive/disabled.

Approach 2:
Use intel_wait_for_pipe_scanline_moving to wait for PIPEDSL to start 
moving after PSR exit. Currently, this ends up waiting for a minimum of 5 msec 
but I changed this to accept a caller defined value for the delay.
After the above changes, this approach takes ~7msec when PSR is active and 
~25-40 usec with PSR disabled, b/c we still need to check for at least 10+ usec 
and see if PIPEDSL moved, if it did, we wait for longer, otherwise we move on.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote:
> On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-05-14 at 13:49 -0700, Tarun Vyas wrote:
> > > 
> > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited,
> > > then
> > > the pipe_update_start call schedules itself out to check back
> > > later.
> > > 
> > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915
> > > but
> > > lags w.r.t core kernel code, hot plugging an external display
> > > triggers
> > > tons of "potential atomic update errors" in the dmesg, on *pipe A*.
> > > A
> > > closer analysis reveals that we try to read the scanline 3 times
> > > and
> > > eventually timeout, b/c PSR hasn't exited fully leading to a
> > > PIPEDSL
> > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for
> > > *some*
> > > reason we loop inside intel_pipe_update start for ~2+ msec which in
> > > this
> > > case is more than enough to exit PSR fully, hence an *unstuck*
> > > PIPEDSL
> > > counter, hence no error. On the other hand, the ChromeOS kernel
> > > spends
> > > ~1.1 msec looping inside intel_pipe_update_start and hence errors
> > > out
> > > b/c the source is still in PSR.
> > > 
> > > Regardless, we should wait for PSR exit (if PSR is supported and
> > > active
> > > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > > fully exited PSR, then checking for vblank evasion isn't actually
> > > applicable.
> > > 
> > > This scenario applies to a configuration with an additional pipe,
> > > as of now
> > > 
> > > Signed-off-by: Tarun Vyas 
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c | 7 +--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > index ee23613f9fd4..481d310e5c3b 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct
> > > intel_crtc_state *new_crtc_state)
> > >     VBLANK_EVASI
> > > ON
> > > _TIME_US);
> > >   max = vblank_start - 1;
> > >  
> > > - local_irq_disable();
> > > -
> > >   if (min <= 0 || max <= 0)
> > >   return;
> > >  
> > >   if (WARN_ON(drm_crtc_vblank_get(>base)))
> > >   return;
> > >  
> > > + if(new_crtc_state->has_psr && dev_priv->psr.active)
> > > + psr_wait_for_idle(dev_priv);
> > How about just waiting for PSR_STATUS to idle without grabbing any
> > locks or checking whether PSR is active?
> > 
> > Status should be idle if PSR was disabled or on it's way to becoming
> > idle if it was enabled. Even if PSR did get enabled while we are in
> > pipe_update_start(), it will not be active as long as VBIs are
> > enabled.
> > 
Right, if we are OK with some duplication (of psr_wait_for_idle) inside 
intel_psr.c, then we can duplicate the PSR2 vs. PSR check that's being done in 
psr_wait_for_idle and then just wait without grabbing any locks, so essentially 
a lockless version of psr_wait_for_idle()
> Correct me if this was already considered, why not wait until the
> scanline counter starts moving? I see we have a 
>   intel_wait_for_pipe_scanline_moving(crtc) that's used when the
> pipe is enabled.
> 
> -DK

Didn't consider this before, but, pipe_scanline_is_moving waits for a minimum 
of 5 msec. Are we OK with a min wait of 5 msec inside pipe_update_start ? 
Heuristically, waiting for PSR idle has almost always returned within < 2 msec. 
Occasionally it takes upto 1 full frame. 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915/psr: Check for partial PSR dpcd reads

[Tarun Vyas]

The dpcd read during psr_dpcd_init may not always return the
requested number of bytes. No known cases yet, but good to
put that check in place.

v2: Fix checkpatch warnings.

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_psr.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index ebc483f06c6f..ed5591ed038c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -234,8 +234,12 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
struct drm_i915_private *dev_priv =
to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
 
-   drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
-sizeof(intel_dp->psr_dpcd));
+   if (drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
+sizeof(intel_dp->psr_dpcd)) !=
+sizeof(intel_dp->psr_dpcd)) {
+   DRM_ERROR("PSR DPCD init failed\n");
+   return;
+   }
 
if (!intel_dp->psr_dpcd[0])
return;
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/psr: Check for partial PSR dpcd reads

[Tarun Vyas]

The dpcd read during psr_dpcd_init may not always return the
requested number of bytes. No known cases yet, but good to
put that check in place.

Signed-off-by: Tarun Vyas 
---
 drivers/gpu/drm/i915/intel_psr.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index ebc483f06c6f..877ed763a12c 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -234,8 +234,11 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
struct drm_i915_private *dev_priv =
to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
 
-   drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
-sizeof(intel_dp->psr_dpcd));
+   if (drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
+sizeof(intel_dp->psr_dpcd)) != 
sizeof(intel_dp->psr_dpcd)) {
+   DRM_ERROR("PSR DPCD init failed \n");
+   return;
+   }
 
if (!intel_dp->psr_dpcd[0])
return;
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/6] drm/i915/psr: Fix ALPM cap check for PSR2

[Tarun Vyas]

On Fri, May 11, 2018 at 12:51:45PM -0700, Dhinakaran Pandiyan wrote:
> While touching the code around this, I noticed that absence of ALPM
> capability does not stop us from enabling PSR2. But, the spec
> unambiguously states that ALPM is required for PSR2 and so does this
> commit that introduced this code
> 
> drm/i915/psr: enable ALPM for psr2
> 
> As per edp1.4 spec , alpm is required for psr2 operation as it's
> used for all psr2  main link power down management and alpm enable
> bit must be set for psr2 operation.
>
Since, the code introduced by "drm/i915/psr: enable ALPM for psr2" enables PSR2 
even if ALPM isn't supported, can we add the "Fixes" tag here ? Rest looks good.

Reviewed-by: Tarun Vyas <tarun.v...@intel.com> 
> Cc: Jose Roberto de Souza <jose.so...@intel.com>
> Cc: Vathsala Nagaraju <vathsala.nagar...@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index b4a4f5d3a2bb..92abf61e234c 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -254,6 +254,10 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  
>   if (INTEL_GEN(dev_priv) >= 9 &&
>   (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> + bool y_req = intel_dp->psr_dpcd[1] &
> +  DP_PSR2_SU_Y_COORDINATE_REQUIRED;
> + bool alpm = intel_dp_get_alpm_status(intel_dp);
> +
>   /*
>* All panels that supports PSR version 03h (PSR2 +
>* Y-coordinate) can handle Y-coordinates in VSC but we are
> @@ -265,16 +269,13 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>* Y-coordinate requirement panels we would need to enable
>* GTC first.
>*/
> - dev_priv->psr.sink_psr2_support =
> - intel_dp->psr_dpcd[1] & 
> DP_PSR2_SU_Y_COORDINATE_REQUIRED;
> + dev_priv->psr.sink_psr2_support = y_req && alpm;
>   DRM_DEBUG_KMS("PSR2 %ssupported\n",
> dev_priv->psr.sink_psr2_support ? "" : "not ");
>  
>   if (dev_priv->psr.sink_psr2_support) {
>   dev_priv->psr.colorimetry_support =
>   intel_dp_get_colorimetry_status(intel_dp);
> - dev_priv->psr.alpm =
> - intel_dp_get_alpm_status(intel_dp);
>   dev_priv->psr.sink_sync_latency =
>   intel_dp_get_sink_sync_latency(intel_dp);
>   }
> @@ -386,13 +387,12 @@ static void hsw_psr_enable_sink(struct intel_dp 
> *intel_dp)
>   u8 dpcd_val = DP_PSR_ENABLE;
>  
>   /* Enable ALPM at sink for psr2 */
> - if (dev_priv->psr.psr2_enabled && dev_priv->psr.alpm)
> - drm_dp_dpcd_writeb(_dp->aux,
> - DP_RECEIVER_ALPM_CONFIG,
> - DP_ALPM_ENABLE);
> -
> - if (dev_priv->psr.psr2_enabled)
> + if (dev_priv->psr.psr2_enabled) {
> + drm_dp_dpcd_writeb(_dp->aux, DP_RECEIVER_ALPM_CONFIG,
> +DP_ALPM_ENABLE);
>   dpcd_val |= DP_PSR_ENABLE_PSR2;
> + }
> +
>   if (dev_priv->psr.link_standby)
>   dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
>   drm_dp_dpcd_writeb(_dp->aux, DP_PSR_EN_CFG, dpcd_val);
> -- 
> 2.14.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/6] drm/i915/psr: Avoid unnecessary DPCD read of DP_PSR_CAPS

[Tarun Vyas]

On Fri, May 11, 2018 at 12:51:43PM -0700, Dhinakaran Pandiyan wrote:
> intel_dp->psr_dpcd already has the required values.
> 
> Cc: Jose Roberto de Souza <jose.so...@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 11 +--
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index 61ade81576f5..381dbdbf30f4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -201,15 +201,6 @@ void intel_psr_irq_handler(struct drm_i915_private 
> *dev_priv, u32 psr_iir)
>   }
>  }
>  
> -static bool intel_dp_get_y_coord_required(struct intel_dp *intel_dp)
> -{
> - uint8_t psr_caps = 0;
> -
> - if (drm_dp_dpcd_readb(_dp->aux, DP_PSR_CAPS, _caps) != 1)
> - return false;
> - return psr_caps & DP_PSR2_SU_Y_COORDINATE_REQUIRED;
> -}
> -
>  static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
>  {
>   uint8_t dprx = 0;
> @@ -275,7 +266,7 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>* GTC first.
>*/
>   dev_priv->psr.sink_psr2_support =
> - intel_dp_get_y_coord_required(intel_dp);
> + intel_dp->psr_dpcd[1] & 
> DP_PSR2_SU_Y_COORDINATE_REQUIRED;
>   DRM_DEBUG_KMS("PSR2 %ssupported\n",
> dev_priv->psr.sink_psr2_support ? "" : "not ");
The drm_dp_dpcd_read itself reads the first 2 PSR DPCD bytes which is what is 
needed. Also, no other callers of intel_dp_get_y_coord_required exist. So,

Reviewed-by: Tarun Vyas <tarun.v...@intel.com>
>  
> -- 
> 2.14.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/psr: Fix missed entry in PSR setup time table.

[Tarun Vyas]

On Thu, May 10, 2018 at 05:54:19PM -0700, Dhinakaran Pandiyan wrote:
> Entry corresponding to 220 us setup time was missing. I am not aware of
> any specific bug this fixes, but this could potentially result in enabling
> PSR on a panel with a higher setup time requirement than supported by the
> hardware.
> 
> I verified the value is present in eDP spec versions 1.3, 1.4 and 1.4a.
> 
> Fixes: 6608804b3d7f ("drm/dp: Add drm_dp_psr_setup_time()")
> Cc: sta...@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Jose Roberto de Souza <jose.so...@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 36c7609a4bd5..a7ba602a43a8 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1159,6 +1159,7 @@ int drm_dp_psr_setup_time(const u8 
> psr_cap[EDP_PSR_RECEIVER_CAP_SIZE])
>   static const u16 psr_setup_time_us[] = {
>   PSR_SETUP_TIME(330),
>   PSR_SETUP_TIME(275),
> +     PSR_SETUP_TIME(220),
Verified the PSR set up time table in the eDP 1.4b spec
Reviewed-by: Tarun Vyas <tarun.v...@intel.com>
>   PSR_SETUP_TIME(165),
>   PSR_SETUP_TIME(110),
>   PSR_SETUP_TIME(55),
> -- 
> 2.14.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/6] drm/i915/psr: Avoid DPCD reads when panel does not support PSR

[Tarun Vyas]

On Fri, May 11, 2018 at 12:51:40PM -0700, Dhinakaran Pandiyan wrote:
> Ville noticed that we are unncessarily reading DPCD's after knowing
> panel did not support PSR. Looks like this check that was present
> earlier got removed unintentionally, let's put it back.
> 
> While we do this, add the PSR version number in the debug print.
> 
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index db27f2faa1de..8fe6d2f9ab2b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -250,10 +250,12 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>   drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT, intel_dp->psr_dpcd,
>sizeof(intel_dp->psr_dpcd));
>  
> - if (intel_dp->psr_dpcd[0]) {
> - dev_priv->psr.sink_support = true;
> - DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> - }
> + if (!intel_dp->psr_dpcd[0])
> + return;
> +
> + DRM_DEBUG_KMS("eDP panel supports PSR version %x\n",
> +   intel_dp->psr_dpcd[0]);
> + dev_priv->psr.sink_support = true;
>  
>   if (INTEL_GEN(dev_priv) >= 9 &&
>   (intel_dp->psr_dpcd[0] == DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> @@ -270,8 +272,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>*/
>   dev_priv->psr.sink_psr2_support =
>   intel_dp_get_y_coord_required(intel_dp);
> - DRM_DEBUG_KMS("PSR2 %s on sink", dev_priv->psr.sink_psr2_support
> -   ? "supported" : "not supported");
> + DRM_DEBUG_KMS("PSR2 %ssupported\n",
> +   dev_priv->psr.sink_psr2_support ? "" : "not ");
Would it make sense to make it clearer that PSR2 is not supported b/c of lack 
of y-coordinate support on the sink ?

Reviewed-by: Tarun Vyas <tarun.v...@intel.com>
>  
>   if (dev_priv->psr.sink_psr2_support) {
>   dev_priv->psr.colorimetry_support =
> -- 
> 2.14.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[Tarun Vyas]

On Mon, May 14, 2018 at 03:00:15PM -0700, Tarun Vyas wrote:
> On Mon, May 14, 2018 at 10:15:19PM +0100, Chris Wilson wrote:
> > Quoting Tarun Vyas (2018-05-14 21:49:20)
> > > intel_pipe_update_start also needs to wait for PSR to idle
> > > out. Need some minor modifications in psr_wait_for_idle in
> > > order to reuse it.
> > > 
> > > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > > Signed-off-by: Tarun Vyas <tarun.v...@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 29 ++---
> > >  1 file changed, 18 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index db27f2faa1de..40aafc0f4513 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct 
> > > drm_i915_private *dev_priv)
> > > i915_reg_t reg;
> > > u32 mask;
> > > int err;
> > > +   bool wait = false;
> > > +
> > > +   mutex_lock(_priv->psr.lock);
> > >  
> > > intel_dp = dev_priv->psr.enabled;
> > > if (!intel_dp)
> > > -   return false;
> > > +   goto unlock;
> > >  
> > > +   wait = true;
> > > if (HAS_DDI(dev_priv)) {
> > > if (dev_priv->psr.psr2_enabled) {
> > > reg = EDP_PSR2_STATUS;
> > > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct 
> > > drm_i915_private *dev_priv)
> > > mask = VLV_EDP_PSR_IN_TRANS;
> > > }
> > >  
> > > +unlock:
> > > mutex_unlock(_priv->psr.lock);
> > >  
> > > -   err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> > > -   if (err)
> > > -   DRM_ERROR("Timed out waiting for PSR Idle for 
> > > re-enable\n");
> > > +   if(wait) {
> > > +   err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> > > +   if (err) {
> > > +   DRM_ERROR("Timed out waiting for PSR Idle for 
> > > re-enable\n");
> > > +   wait = false;
> > > +   }
> > > +   }
> > >  
> > > -   /* After the unlocked wait, verify that PSR is still wanted! */
> > > -   mutex_lock(_priv->psr.lock);
> > > -   return err == 0 && dev_priv->psr.enabled;
> > > +   return wait;
> I wanted to avoid taking this additional lock b/c all we need inside 
> intel_pipe_update_start is for PSR to go idle. So can we retain moving it to 
> intel_psr_work ?
> > >  }
> > >  
> > >  static void intel_psr_work(struct work_struct *work)
> > > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work)
> > > struct drm_i915_private *dev_priv =
> > > container_of(work, typeof(*dev_priv), psr.work.work);
> > >  
> > > -   mutex_lock(_priv->psr.lock);
> > >  
> > > /*
> > >  * We have to make sure PSR is ready for re-enable
> > > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work)
> > >  * and be ready for re-enable.
> > >  */
> > > if (!psr_wait_for_idle(dev_priv))
> > > -   goto unlock;
> > > +   return;
> > >  
> > > -   /*
> > > +   /* After the unlocked wait, verify that PSR is still wanted!
> > >  * The delayed work can race with an invalidate hence we need to
> > >  * recheck. Since psr_flush first clears this and then 
> > > reschedules we
> > >  * won't ever miss a flush when bailing out here.
> > >  */
> > > -   if (dev_priv->psr.busy_frontbuffer_bits)
> > > +   mutex_lock(_priv->psr.lock);
> > > +   if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits)
> > > goto unlock;
> > 
> > I'm not sold on the locking dropping here, doing so inside the wait is
> > bad enough. (And do we need to there anyway?)
> > 
Per the commit message in "daeb725e drm/i915/psr: Chase psr.enabled only under 
the psr.lock",
the wait_for_register is d

Re: [Intel-gfx] [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

On Mon, May 14, 2018 at 10:16:38PM +0100, Chris Wilson wrote:
> Quoting Tarun Vyas (2018-05-14 21:49:22)
> > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
> > the pipe_update_start call schedules itself out to check back later.
> > 
> > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
> > lags w.r.t core kernel code, hot plugging an external display triggers
> > tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
> > closer analysis reveals that we try to read the scanline 3 times and
> > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
> > stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
> > reason we loop inside intel_pipe_update start for ~2+ msec which in this
> > case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
> > counter, hence no error. On the other hand, the ChromeOS kernel spends
> > ~1.1 msec looping inside intel_pipe_update_start and hence errors out
> > b/c the source is still in PSR.
> > 
> > Regardless, we should wait for PSR exit (if PSR is supported and active
> > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > fully exited PSR, then checking for vblank evasion isn't actually
> > applicable.
> > 
> > This scenario applies to a configuration with an additional pipe,
> > as of now
> > 
> > Signed-off-by: Tarun Vyas <tarun.v...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index ee23613f9fd4..481d310e5c3b 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct 
> > intel_crtc_state *new_crtc_state)
> >   
> > VBLANK_EVASION_TIME_US);
> > max = vblank_start - 1;
> >  
> > -   local_irq_disable();
> > -
> > if (min <= 0 || max <= 0)
> > return;
> >  
> > if (WARN_ON(drm_crtc_vblank_get(>base)))
> > return;
> >  
> > +   if(new_crtc_state->has_psr && dev_priv->psr.active)
> > +   psr_wait_for_idle(dev_priv);
> > +
> > +   local_irq_disable();
> 
> Pop quiz, does intel_pipe_update_finish() unconditionally assume it is
> called with irqs disabled?
> -Chris
Unless local_irq_disable() fails, intel_pipe_update_end() should always get 
called with irqs disabled, from what it looks like to me.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[Tarun Vyas]

On Mon, May 14, 2018 at 10:15:19PM +0100, Chris Wilson wrote:
> Quoting Tarun Vyas (2018-05-14 21:49:20)
> > intel_pipe_update_start also needs to wait for PSR to idle
> > out. Need some minor modifications in psr_wait_for_idle in
> > order to reuse it.
> > 
> > Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> > Signed-off-by: Tarun Vyas <tarun.v...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 29 ++---
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index db27f2faa1de..40aafc0f4513 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private 
> > *dev_priv)
> > i915_reg_t reg;
> > u32 mask;
> > int err;
> > +   bool wait = false;
> > +
> > +   mutex_lock(_priv->psr.lock);
> >  
> > intel_dp = dev_priv->psr.enabled;
> > if (!intel_dp)
> > -   return false;
> > +   goto unlock;
> >  
> > +   wait = true;
> > if (HAS_DDI(dev_priv)) {
> > if (dev_priv->psr.psr2_enabled) {
> > reg = EDP_PSR2_STATUS;
> > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private 
> > *dev_priv)
> > mask = VLV_EDP_PSR_IN_TRANS;
> > }
> >  
> > +unlock:
> > mutex_unlock(_priv->psr.lock);
> >  
> > -   err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> > -   if (err)
> > -   DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> > +   if(wait) {
> > +   err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
> > +   if (err) {
> > +   DRM_ERROR("Timed out waiting for PSR Idle for 
> > re-enable\n");
> > +   wait = false;
> > +   }
> > +   }
> >  
> > -   /* After the unlocked wait, verify that PSR is still wanted! */
> > -   mutex_lock(_priv->psr.lock);
> > -   return err == 0 && dev_priv->psr.enabled;
> > +   return wait;
I wanted to avoid taking this additional lock b/c all we need inside 
intel_pipe_update_start is for PSR to go idle. So can we retain moving it to 
intel_psr_work ?
> >  }
> >  
> >  static void intel_psr_work(struct work_struct *work)
> > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work)
> > struct drm_i915_private *dev_priv =
> > container_of(work, typeof(*dev_priv), psr.work.work);
> >  
> > -   mutex_lock(_priv->psr.lock);
> >  
> > /*
> >  * We have to make sure PSR is ready for re-enable
> > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work)
> >  * and be ready for re-enable.
> >  */
> > if (!psr_wait_for_idle(dev_priv))
> > -   goto unlock;
> > +   return;
> >  
> > -   /*
> > +   /* After the unlocked wait, verify that PSR is still wanted!
> >  * The delayed work can race with an invalidate hence we need to
> >  * recheck. Since psr_flush first clears this and then reschedules 
> > we
> >  * won't ever miss a flush when bailing out here.
> >  */
> > -   if (dev_priv->psr.busy_frontbuffer_bits)
> > +   mutex_lock(_priv->psr.lock);
> > +   if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits)
> > goto unlock;
> 
> I'm not sold on the locking dropping here, doing so inside the wait is
> bad enough. (And do we need to there anyway?)
> 
Thanks for the comments Chris.
In that case, as suggested by Rodrigo, can we assert that the lock is held, 
inside psr_wait_for_idle() ?
> Since you need to introduce intel_psr_wait_for_idle() anyway, how about
> 
> void intel_psr_wait_for_idle(...)
> {
>   mutex_lock(>psr.lock);
>   psr_wait_for_idle();
>   mutex_unlock(>psr.lock);
> }
> -Chris
>>/* After the unlocked wait, verify that PSR is still wanted! */
>>  mutex_lock(_priv->psr.lock); 
>>  return err == 0 && dev_priv->psr.enabled; 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Tarun Vyas]

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is supported and active
on the current pipe) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

This scenario applies to a configuration with an additional pipe,
as of now

Signed-off-by: Tarun Vyas <tarun.v...@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index ee23613f9fd4..481d310e5c3b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
  VBLANK_EVASION_TIME_US);
max = vblank_start - 1;
 
-   local_irq_disable();
-
if (min <= 0 || max <= 0)
return;
 
if (WARN_ON(drm_crtc_vblank_get(>base)))
return;
 
+   if(new_crtc_state->has_psr && dev_priv->psr.active)
+   psr_wait_for_idle(dev_priv);
+
+   local_irq_disable();
+
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
trace_i915_pipe_update_start(crtc);
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915: Un-statify psr_wait_for_idle

[Tarun Vyas]

We have new users (follow up patch). So, un-statify it

Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Signed-off-by: Tarun Vyas <tarun.v...@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d7dbca1aabff..d8f8282f1fad 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1919,6 +1919,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
  struct intel_crtc_state *crtc_state);
 void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug);
 void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
+bool psr_wait_for_idle(struct drm_i915_private *dev_priv);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 40aafc0f4513..e69859fa782f 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -883,7 +883,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
cancel_delayed_work_sync(_priv->psr.work);
 }
 
-static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
+bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
 {
struct intel_dp *intel_dp;
i915_reg_t reg;
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[Tarun Vyas]

intel_pipe_update_start also needs to wait for PSR to idle
out. Need some minor modifications in psr_wait_for_idle in
order to reuse it.

Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Signed-off-by: Tarun Vyas <tarun.v...@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index db27f2faa1de..40aafc0f4513 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private 
*dev_priv)
i915_reg_t reg;
u32 mask;
int err;
+   bool wait = false;
+
+   mutex_lock(_priv->psr.lock);
 
intel_dp = dev_priv->psr.enabled;
if (!intel_dp)
-   return false;
+   goto unlock;
 
+   wait = true;
if (HAS_DDI(dev_priv)) {
if (dev_priv->psr.psr2_enabled) {
reg = EDP_PSR2_STATUS;
@@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private 
*dev_priv)
mask = VLV_EDP_PSR_IN_TRANS;
}
 
+unlock:
mutex_unlock(_priv->psr.lock);
 
-   err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
-   if (err)
-   DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
+   if(wait) {
+   err = intel_wait_for_register(dev_priv, reg, mask, 0, 50);
+   if (err) {
+   DRM_ERROR("Timed out waiting for PSR Idle for 
re-enable\n");
+   wait = false;
+   }
+   }
 
-   /* After the unlocked wait, verify that PSR is still wanted! */
-   mutex_lock(_priv->psr.lock);
-   return err == 0 && dev_priv->psr.enabled;
+   return wait;
 }
 
 static void intel_psr_work(struct work_struct *work)
@@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work)
struct drm_i915_private *dev_priv =
container_of(work, typeof(*dev_priv), psr.work.work);
 
-   mutex_lock(_priv->psr.lock);
 
/*
 * We have to make sure PSR is ready for re-enable
@@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work)
 * and be ready for re-enable.
 */
if (!psr_wait_for_idle(dev_priv))
-   goto unlock;
+   return;
 
-   /*
+   /* After the unlocked wait, verify that PSR is still wanted!
 * The delayed work can race with an invalidate hence we need to
 * recheck. Since psr_flush first clears this and then reschedules we
 * won't ever miss a flush when bailing out here.
 */
-   if (dev_priv->psr.busy_frontbuffer_bits)
+   mutex_lock(_priv->psr.lock);
+   if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits)
goto unlock;
 
intel_psr_activate(dev_priv->psr.enabled);
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Wait for PSR exit before checking for vblank evasion for an atomic update

[Tarun Vyas]

On Thu, May 03, 2018 at 09:58:56AM -0700, Rodrigo Vivi wrote:
> On Wed, May 02, 2018 at 03:31:15PM -0700, Tarun Vyas wrote:
> > On Wed, May 02, 2018 at 01:04:06PM -0700, Vivi, Rodrigo wrote:
> > > On Wed, May 02, 2018 at 09:51:43PM +0300, Ville Syrjälä wrote:
> > > > On Wed, May 02, 2018 at 11:19:14AM -0700, Tarun Vyas wrote:
> > > > > On Mon, Apr 30, 2018 at 10:19:33AM -0700, Rodrigo Vivi wrote:
> > > > > > On Sun, Apr 29, 2018 at 09:00:18PM -0700, Tarun Vyas wrote:
> > > > > > > From: Tarun <tarun.v...@intel.com>
> > > > > > >
> > > > > > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, 
> > > > > > > then
> > > > > > > the pipe_update_start call schedules itself out to check back 
> > > > > > > later.
> > > > > > >
> > > > > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 
> > > > > > > but
> > > > > > > lags w.r.t core kernel code, hot plugging an external display 
> > > > > > > triggers
> > > > > > > tons of "potential atomic update errors" in the dmesg, on *pipe 
> > > > > > > A*. A
> > > > > > > closer analysis reveals that we try to read the scanline 3 times 
> > > > > > > and
> > > > > > > eventually timeout, b/c PSR hasn't exited fully leading to a 
> > > > > > > PIPEDSL
> > > > > > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for 
> > > > > > > *some*
> > > > > > > reason we loop inside intel_pipe_update start for ~2+ msec which 
> > > > > > > in this
> > > > > > > case is more than enough to exit PSR fully, hence an *unstuck* 
> > > > > > > PIPEDSL
> > > > > > > counter, hence no error. On the other hand, the ChromeOS kernel 
> > > > > > > spends
> > > > > > > ~1.1 msec looping inside intel_pipe_update_start and hence errors 
> > > > > > > out
> > > > > > > b/c the source is still in PSR.
> > > > > > >
> > > > > > > Regardless, we should wait for PSR exit (if PSR is supported and 
> > > > > > > active
> > > > > > > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > > > > > > fully exited PSR, then checking for vblank evasion isn't actually
> > > > > > > applicable.
> > > > > > >
> > > > > > > This scenario applies to a configuration with an additional pipe,
> > > > > > > as of now.
> > > > > >
> > > > > > I honestly believe you picking the wrong culprit here. By 
> > > > > > "coincidence".
> > > > > > PSR will allow DC state with screen on and DC state will mess up 
> > > > > > with all
> > > > > > registers reads
> > > > > >
> > > > > > probably what you are missing you your kernel is some power domain
> > > > > > grab that would keep DC_OFF and consequently a sane read of these
> > > > > > registers.
> > > > > >
> > > > > > Maybe Imre has a quick idea of what you could be missing on your 
> > > > > > kernel
> > > > > > that we already have on upstream one.
> > > > > >
> > > > > > Thanks,
> > > > > > Rodrigo.
> > > > > >
> > > > > Thanks for the quick response Rodrigo !
> > > > > Some key observations based on my experiments so far:
> > > > >for (;;) {
> > > > > /*
> > > > >  * prepare_to_wait() has a memory barrier, which 
> > > > > guarantees
> > > > >  * other CPUs can see the task state update by the 
> > > > > time we
> > > > >  * read the scanline.
> > > > >  */
> > > > > prepare_to_wait(wq, , TASK_UNINTERRUPTIBLE);
> > > > >
> > > > > scanline = intel_get_crtc_scanline(crtc);
> > > > > if (scanline < min || scanline > max)
> > > > > break;
>

[Intel-gfx] [PATCH v2] drm/i915: Remove redundant check for negative timeout while doing an atomic pipe update

[Tarun Vyas]

From: Tarun <tarun.v...@intel.com>

No functional changes, just a minor knit. Stumbled across the kernel doc for
schedule_timeout() which quotes "In all cases the return value is guaranteed
to be non-negative". Also, the return code of schedule_timeout() already checks
for negative values "return timeout < 0 ? 0 : timeout;" and returns 0
in such cases. Furthermore, the msec_to_jiffies returns an ungined long
value. So, let's do away with the redundant check for an atomic
pipe update.

v2: Commit message changes (Manasi).

Reviewed-by: Manasi Navare <manasi.d.nav...@intel.com>
Signed-off-by: Tarun Vyas <tarun.v...@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index aa1dfaa692b9..9cd4be020840 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -131,7 +131,7 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
if (scanline < min || scanline > max)
break;
 
-   if (timeout <= 0) {
+   if (!timeout) {
DRM_ERROR("Potential atomic update failure on pipe 
%c\n",
  pipe_name(crtc->pipe));
break;
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Wait for PSR exit before checking for vblank evasion for an atomic update

[Tarun Vyas]

On Wed, May 02, 2018 at 01:04:06PM -0700, Vivi, Rodrigo wrote:
> On Wed, May 02, 2018 at 09:51:43PM +0300, Ville Syrjälä wrote:
> > On Wed, May 02, 2018 at 11:19:14AM -0700, Tarun Vyas wrote:
> > > On Mon, Apr 30, 2018 at 10:19:33AM -0700, Rodrigo Vivi wrote:
> > > > On Sun, Apr 29, 2018 at 09:00:18PM -0700, Tarun Vyas wrote:
> > > > > From: Tarun <tarun.v...@intel.com>
> > > > >
> > > > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
> > > > > the pipe_update_start call schedules itself out to check back later.
> > > > >
> > > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
> > > > > lags w.r.t core kernel code, hot plugging an external display triggers
> > > > > tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
> > > > > closer analysis reveals that we try to read the scanline 3 times and
> > > > > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
> > > > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for 
> > > > > *some*
> > > > > reason we loop inside intel_pipe_update start for ~2+ msec which in 
> > > > > this
> > > > > case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
> > > > > counter, hence no error. On the other hand, the ChromeOS kernel spends
> > > > > ~1.1 msec looping inside intel_pipe_update_start and hence errors out
> > > > > b/c the source is still in PSR.
> > > > >
> > > > > Regardless, we should wait for PSR exit (if PSR is supported and 
> > > > > active
> > > > > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > > > > fully exited PSR, then checking for vblank evasion isn't actually
> > > > > applicable.
> > > > >
> > > > > This scenario applies to a configuration with an additional pipe,
> > > > > as of now.
> > > >
> > > > I honestly believe you picking the wrong culprit here. By "coincidence".
> > > > PSR will allow DC state with screen on and DC state will mess up with 
> > > > all
> > > > registers reads
> > > >
> > > > probably what you are missing you your kernel is some power domain
> > > > grab that would keep DC_OFF and consequently a sane read of these
> > > > registers.
> > > >
> > > > Maybe Imre has a quick idea of what you could be missing on your kernel
> > > > that we already have on upstream one.
> > > >
> > > > Thanks,
> > > > Rodrigo.
> > > >
> > > Thanks for the quick response Rodrigo !
> > > Some key observations based on my experiments so far:
> > >for (;;) {
> > > /*
> > >  * prepare_to_wait() has a memory barrier, which 
> > > guarantees
> > >  * other CPUs can see the task state update by the time we
> > >  * read the scanline.
> > >  */
> > > prepare_to_wait(wq, , TASK_UNINTERRUPTIBLE);
> > >
> > > scanline = intel_get_crtc_scanline(crtc);
> > > if (scanline < min || scanline > max)
> > > break;
> > >
> > > if (timeout <= 0) {
> > > DRM_ERROR("Potential atomic update failure on 
> > > pipe %c\n",
> > >   pipe_name(crtc->pipe));
> > > break;
> > > }
> > >
> > > local_irq_enable();
> > >
> > > timeout = schedule_timeout(timeout);
> > >
> > > local_irq_disable();
> > > }
> > > 1. In the above loop inside pipe_update_start, the *first time*, we read 
> > > the PIPEDSL, with PSR1 and external display connected, it always reads 
> > > 1599, for *both* the kernels(upstream and ChromeOS-4.4) . The PSR_STATUS 
> > > also reads the exact same for *both* kernels and shows that we haven't 
> > > *fully* exited PSR.
> > >
> > > 2. The difference between the two kernels comes after this first read of 
> > > the PIPEDSL. ChromeOS-4.4 spends ~1 msec inside that loop and upstream 
> > > spends ~2msec. I suspe

Re: [Intel-gfx] [PATCH] drm/i915: Wait for PSR exit before checking for vblank evasion for an atomic update

[Tarun Vyas]

On Mon, Apr 30, 2018 at 10:19:33AM -0700, Rodrigo Vivi wrote:
> On Sun, Apr 29, 2018 at 09:00:18PM -0700, Tarun Vyas wrote:
> > From: Tarun <tarun.v...@intel.com>
> > 
> > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
> > the pipe_update_start call schedules itself out to check back later.
> > 
> > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
> > lags w.r.t core kernel code, hot plugging an external display triggers
> > tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
> > closer analysis reveals that we try to read the scanline 3 times and
> > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
> > stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
> > reason we loop inside intel_pipe_update start for ~2+ msec which in this
> > case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
> > counter, hence no error. On the other hand, the ChromeOS kernel spends
> > ~1.1 msec looping inside intel_pipe_update_start and hence errors out
> > b/c the source is still in PSR.
> > 
> > Regardless, we should wait for PSR exit (if PSR is supported and active
> > on the current pipe) before reading the PIPEDSL, b/c if we haven't
> > fully exited PSR, then checking for vblank evasion isn't actually
> > applicable.
> > 
> > This scenario applies to a configuration with an additional pipe,
> > as of now.
> 
> I honestly believe you picking the wrong culprit here. By "coincidence".
> PSR will allow DC state with screen on and DC state will mess up with all
> registers reads
> 
> probably what you are missing you your kernel is some power domain
> grab that would keep DC_OFF and consequently a sane read of these
> registers.
> 
> Maybe Imre has a quick idea of what you could be missing on your kernel
> that we already have on upstream one.
> 
> Thanks,
> Rodrigo.
>
Thanks for the quick response Rodrigo !
Some key observations based on my experiments so far:
   for (;;) {   
  
/*  
  
 * prepare_to_wait() has a memory barrier, which guarantees 
  
 * other CPUs can see the task state update by the time we  
  
 * read the scanline.   
  
 */ 
  
prepare_to_wait(wq, , TASK_UNINTERRUPTIBLE);   
  

  
scanline = intel_get_crtc_scanline(crtc);   
  
if (scanline < min || scanline > max)   
  
break;  
  

  
if (timeout <= 0) { 
  
DRM_ERROR("Potential atomic update failure on pipe 
%c\n", 
  pipe_name(crtc->pipe));   
  
break;  
  
}   
  

  
local_irq_enable(); 
  

  
timeout = schedule_timeout(timeout);
  

  
local_irq_disable();
  
}
1. In the above loop inside pipe_update_start, the *first time*, we read the 
PIPEDSL, with PSR1 and external display connected, it always reads 1599, for 
*both* the kernels(upstream and ChromeOS-4.4) . The PSR_STATUS also reads the 
exact same for *both* kernels and shows that we haven't *fully* exited PSR.

2. The difference between the two kernels comes after this first read of the 
PIPEDSL. ChromeOS-4.4 spends ~1 msec inside that loop and upstream spends 
~2msec. I suspect that it is because of the scheduling changes between the two 
kernels, b/c I can't find any i915 specific code running in that loop, except 
for vblank processing.

3. So to summarize it, both the kernels are in the same state w.r.t PSR and 
PIPEDSL value when they read the PIPEDSL for the first time inside the loop. 
*When* the ke

[Intel-gfx] [PATCH] drm/i915: Remove redundant check for negative timeout while doing an atomic pipe update

[Tarun Vyas]

Just a minor knit. Stumbled across the kernel doc for schedule_timeout() which
quotes "In all cases the return value is guaranteed to be non-negative". Also,
the return code of schedule_timeout() already checks for negative values
"return timeout < 0 ? 0 : timeout;" and returns 0 in such cases. So,
let's do away with the redundant check for an atomic pipe update.

Signed-off-by: Tarun Vyas <tarun.v...@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index aa1dfaa692b9..9cd4be020840 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -131,7 +131,7 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
if (scanline < min || scanline > max)
break;
 
-   if (timeout <= 0) {
+   if (!timeout) {
DRM_ERROR("Potential atomic update failure on pipe 
%c\n",
  pipe_name(crtc->pipe));
break;
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Wait for PSR exit before checking for vblank evasion for an atomic update

[Tarun Vyas]

From: Tarun 

The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
the pipe_update_start call schedules itself out to check back later.

On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some*
reason we loop inside intel_pipe_update start for ~2+ msec which in this
case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL
counter, hence no error. On the other hand, the ChromeOS kernel spends
~1.1 msec looping inside intel_pipe_update_start and hence errors out
b/c the source is still in PSR.

Regardless, we should wait for PSR exit (if PSR is supported and active
on the current pipe) before reading the PIPEDSL, b/c if we haven't
fully exited PSR, then checking for vblank evasion isn't actually
applicable.

This scenario applies to a configuration with an additional pipe,
as of now.

---
 drivers/gpu/drm/i915/intel_sprite.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index aa1dfaa692b9..135b41568503 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
  VBLANK_EVASION_TIME_US);
max = vblank_start - 1;
 
-   local_irq_disable();
-
if (min <= 0 || max <= 0)
return;
 
if (WARN_ON(drm_crtc_vblank_get(>base)))
return;
 
+   if(new_crtc_state->has_psr && dev_priv->psr.active)
+   intel_wait_for_register(dev_priv, EDP_PSR_STATUS, 
EDP_PSR_STATUS_STATE_MASK, EDP_PSR_STATUS_STATE_IDLE, 5);
+
+   local_irq_disable();
+
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
trace_i915_pipe_update_start(crtc);
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] drm/i915: Rework "Potential atomic update error" to handle PSR exit

[Tarun Vyas]

On Thu, Apr 26, 2018 at 02:39:04PM -0700, Tarun Vyas wrote:
> On Thu, Apr 26, 2018 at 10:47:40AM -0700, Dhinakaran Pandiyan wrote:
> > 
> > 
> > 
> > On Thu, 2018-04-26 at 16:41 +0300, Ville Syrjälä wrote:
> > > On Wed, Apr 25, 2018 at 07:10:09PM -0700, tarun.v...@intel.com wrote:
> > > > From: Tarun <tarun.v...@intel.com>
> > > > 
> > > > The Display scanline counter freezes on PSR entry. Inside
> > > > intel_pipe_update_start, once Vblank interrupts are enabled, we start
> > > > exiting PSR, but by the time the scanline counter is read, we may not
> > > > have completely exited PSR which leads us to schedule out and check back
> > > > later.
> > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
> > > > lags w.r.t core kernel code, hot plugging an external display triggers
> > > > tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
> > > > closer analysis reveals that we try to read the scanline 3 times and
> > > > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL 
> > > > stuck @
> > > > 1599.
> > > > This issue is not seen on upstream kernels, b/c for *some* reason we
> > > > loop inside intel_pipe_update start for ~2+ msec which in this case is
> > > > more than enough to exit PSR fully, hence an *unstuck* PIPEDSL counter,
> > > > hence no error. On the other hand, the ChromeOS kernel spends ~1.1 msec
> > > > looping inside intel_pipe_update_start and hence errors out b/c the
> > > > source is still in PSR.
> > > > 
> > > > If PSR is enabled, then we should *wait* for  the PSR
> > > > state to move to IDLE before re-reading the PIPEDSL so as to avoid bogus
> > > > and annoying "potential atomic update error" messages.
> > > > 
> > > > P.S: This scenario applies to a configuration with an additional pipe,
> > > > as of now.
> > > > 
> > 
> > Ville, 
> > 
> > Any idea what could be the reason the warnings start appearing when an
> > external display is connected? We couldn't come up with an explanation.
> > 
> Another source of confusion for me is that on the upstream kernels, it 
> *appears* to take more time for us to get *re-scheduled* after we call 
> schedule_timeout(). So with ~2+msec spent in the loop, it seems to be not 
> working as intended b/c we end up spending a lot more time in the loop, which 
> in turn contributes to this issue not being seen on upstream kernels.
> > 
> > > > Signed-off-by: Tarun <tarun.v...@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sprite.c | 19 +++
> > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> > > > b/drivers/gpu/drm/i915/intel_sprite.c
> > > > index aa1dfaa692b9..77dd3b936131 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > > @@ -92,11 +92,13 @@ void intel_pipe_update_start(const struct 
> > > > intel_crtc_state *new_crtc_state)
> > > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > const struct drm_display_mode *adjusted_mode = 
> > > > _crtc_state->base.adjusted_mode;
> > > > long timeout = msecs_to_jiffies_timeout(1);
> > > > -   int scanline, min, max, vblank_start;
> > > > +   int scanline, min, max, vblank_start, old_scanline, 
> > > > new_scanline;
> > > > +   bool retried = false;
> > > > wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(>base);
> > > > bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || 
> > > > IS_CHERRYVIEW(dev_priv)) &&
> > > > intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
> > > > DEFINE_WAIT(wait);
> > > > +   old_scanline = new_scanline = -1;
> > > >  
> > > > vblank_start = adjusted_mode->crtc_vblank_start;
> > > > if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> > > > @@ -126,15 +128,24 @@ void intel_pipe_update_start(const struct 
> > > > intel_crtc_state *new_crtc_state)
> > > >  * read the scanline.
> > > >  */
> > > > prepare_to

[Intel-gfx] [RFC] drm/i915: Rework "Potential atomic update error" to handle PSR exit

[tarun . vyas]

From: Tarun 

The Display scanline counter freezes on PSR entry. Inside
intel_pipe_update_start, once Vblank interrupts are enabled, we start
exiting PSR, but by the time the scanline counter is read, we may not
have completely exited PSR which leads us to schedule out and check back
later.
On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
lags w.r.t core kernel code, hot plugging an external display triggers
tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
closer analysis reveals that we try to read the scanline 3 times and
eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL stuck @
1599.
This issue is not seen on upstream kernels, b/c for *some* reason we
loop inside intel_pipe_update start for ~2+ msec which in this case is
more than enough to exit PSR fully, hence an *unstuck* PIPEDSL counter,
hence no error. On the other hand, the ChromeOS kernel spends ~1.1 msec
looping inside intel_pipe_update_start and hence errors out b/c the
source is still in PSR.

If PSR is enabled, then we should *wait* for  the PSR
state to move to IDLE before re-reading the PIPEDSL so as to avoid bogus
and annoying "potential atomic update error" messages.

P.S: This scenario applies to a configuration with an additional pipe,
as of now.

Signed-off-by: Tarun 
---
 drivers/gpu/drm/i915/intel_sprite.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
b/drivers/gpu/drm/i915/intel_sprite.c
index aa1dfaa692b9..77dd3b936131 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -92,11 +92,13 @@ void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
const struct drm_display_mode *adjusted_mode = 
_crtc_state->base.adjusted_mode;
long timeout = msecs_to_jiffies_timeout(1);
-   int scanline, min, max, vblank_start;
+   int scanline, min, max, vblank_start, old_scanline, new_scanline;
+   bool retried = false;
wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(>base);
bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || 
IS_CHERRYVIEW(dev_priv)) &&
intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
DEFINE_WAIT(wait);
+   old_scanline = new_scanline = -1;
 
vblank_start = adjusted_mode->crtc_vblank_start;
if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
@@ -126,15 +128,24 @@ void intel_pipe_update_start(const struct 
intel_crtc_state *new_crtc_state)
 * read the scanline.
 */
prepare_to_wait(wq, , TASK_UNINTERRUPTIBLE);
-
+retry:
scanline = intel_get_crtc_scanline(crtc);
+   old_scanline = new_scanline, new_scanline = scanline;
+
if (scanline < min || scanline > max)
break;
 
if (timeout <= 0) {
-   DRM_ERROR("Potential atomic update failure on pipe 
%c\n",
+   if(!i915.enable_psr || retried) {
+   DRM_ERROR("Potential atomic update failure on 
pipe %c\n",
  pipe_name(crtc->pipe));
-   break;
+   break;
+   }
+   else if(old_scanline == new_scanline && !retried) {
+   retried = true;
+   intel_wait_for_register(dev_priv, 
EDP_PSR_STATUS_CTL, EDP_PSR_STATUS_STATE_MASK, EDP_PSR_STATUS_STATE_IDLE, 10);
+   goto retry;
+   }
}
 
local_irq_enable();
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx