| Message ID | 20180622070521.169995-1-tarun.vyas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <tarun.vyas@intel.com> > --- > 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(&dev_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. > +{ > + 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. -Chris
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 <tarun.vyas@intel.com> > > --- > > 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(&dev_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.
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(&dev_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;
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 <tarun.vyas@intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_psr.c | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+)