| Message ID | 20180626055724.167534-1-tarun.vyas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <tarun.vyas@intel.com> > --- > 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(&dev_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 numbers for the exit training/aux handshake time. -Daniel > + 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
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 <tarun.vyas@intel.com> > > --- > > 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(&dev_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
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. > > > > 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 <tarun.vyas@intel.com> > > --- > > 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(&dev_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 numbers for the exit training/aux > handshake > time. bspec says exit should be compelete in "one full frame time (1/refresh rate), plus SRD exit training time (max of 6ms), plus SRD aux channel handshake (max of 1.5ms)." > -Daniel > > > > > + 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
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 <tarun.vyas@intel.com> > > > --- > > > 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(&dev_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 numbers for the exit training/aux > > handshake > > time. > > bspec says exit should be compelete in "one full frame time (1/refresh > rate), plus SRD exit training time (max of 6ms), plus SRD aux channel > handshake (max of 1.5ms)." > > > So should we use 50 Hz as the lower limit for the refresh rate to calc our max timeout here. Can eDP go down to 30 Hz ? > > -Daniel > > > > > > > > + 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
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 <tarun.vyas@intel.com> > > > --- > > > 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(&dev_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.
On Tue, Jun 26, 2018 at 01:20:58PM -0700, Tarun Vyas wrote: > 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. Have we measured these stalls? Is it actually faster? Directly poking hw registers because our own software tracking is a bit funny still feels like a rather bad hack. And without gathering data I'd assume that the mutex_lock is contended only when the there's a state transition going on in psr, and in that case the register wait_for will also take quite a while (equally long really, until the psr hw settles). -Daniel > > > > > > > > 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 <tarun.vyas@intel.com> > > > > --- > > > > 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(&dev_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 numbers for the exit training/aux > > > handshake > > > time. > > > > bspec says exit should be compelete in "one full frame time (1/refresh > > rate), plus SRD exit training time (max of 6ms), plus SRD aux channel > > handshake (max of 1.5ms)." > > > > > > > So should we use 50 Hz as the lower limit for the refresh rate to calc our max timeout here. Can eDP go down to 30 Hz ? > > > -Daniel > > > > > > > > > > > + 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
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(&dev_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; /*
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 <tarun.vyas@intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_psr.c | 25 +++++++++++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-)