On Thu, 2018-04-05 at 12:49 +0100, Chris Wilson wrote:
> Inside the psr work function, we want to wait for PSR to idle first
> and
> wish to do so without blocking the normal modeset path, so we do so
> without holding the PSR lock. However, we first have to find which
> pipe
> PSR was enabled on, which requires chasing into the PSR struct and
> requires locking to prevent intel_psr_disable() from concurrently
> setting our pointer to NULL.
>
> Fixes: 995d30477496 ("drm/i915: VLV/CHV PSR Software timer mode")
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Durgadoss R <durgadoss.r@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.0+
Feel free to add:
Reviewed-by: Jose Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_psr.c | 82 +++++++++++++++++++++---------
> ----------
> 1 file changed, 44 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2d53f7398a6d..69a5b276f4d8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -775,53 +775,59 @@ void intel_psr_disable(struct intel_dp
> *intel_dp,
> cancel_delayed_work_sync(&dev_priv->psr.work);
> }
>
> -static void intel_psr_work(struct work_struct *work)
> +static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> {
> - struct drm_i915_private *dev_priv =
> - container_of(work, typeof(*dev_priv),
> psr.work.work);
> - struct intel_dp *intel_dp = dev_priv->psr.enabled;
> - struct drm_crtc *crtc = dp_to_dig_port(intel_dp)-
> >base.base.crtc;
> - enum pipe pipe = to_intel_crtc(crtc)->pipe;
> + struct intel_dp *intel_dp;
nitpick: Why not already set it?
struct intel_dp *intel_dp = dev_priv->psr.enabled;
> + i915_reg_t reg;
> + u32 mask;
> + int err;
> +
> + intel_dp = dev_priv->psr.enabled;
> + if (!intel_dp)
> + return false;
>
> - /* We have to make sure PSR is ready for re-enable
> - * otherwise it keeps disabled until next full
> enable/disable cycle.
> - * PSR might take some time to get fully disabled
> - * and be ready for re-enable.
> - */
> if (HAS_DDI(dev_priv)) {
nitpick: While on that you could replace this for:
if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))) {
> if (dev_priv->psr.psr2_enabled) {
> - if (intel_wait_for_register(dev_priv,
> - EDP_PSR2_STATUS,
> - EDP_PSR2_STATUS_
> STATE_MASK,
> - 0,
> - 50)) {
> - DRM_ERROR("Timed out waiting for
> PSR2 Idle for re-enable\n");
> - return;
> - }
> + reg = EDP_PSR2_STATUS;
> + mask = EDP_PSR2_STATUS_STATE_MASK;
> } else {
> - if (intel_wait_for_register(dev_priv,
> - EDP_PSR_STATUS,
> - EDP_PSR_STATUS_S
> TATE_MASK,
> - 0,
> - 50)) {
> - DRM_ERROR("Timed out waiting for PSR
> Idle for re-enable\n");
> - return;
> - }
> + reg = EDP_PSR_STATUS;
> + mask = EDP_PSR_STATUS_STATE_MASK;
> }
> } else {
> - if (intel_wait_for_register(dev_priv,
> - VLV_PSRSTAT(pipe),
> - VLV_EDP_PSR_IN_TRANS,
> - 0,
> - 1)) {
> - DRM_ERROR("Timed out waiting for PSR Idle
> for re-enable\n");
> - return;
> - }
> + struct drm_crtc *crtc =
> + dp_to_dig_port(intel_dp)->base.base.crtc;
> + enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +
> + reg = VLV_PSRSTAT(pipe);
> + mask = VLV_EDP_PSR_IN_TRANS;
> }
> +
> + mutex_unlock(&dev_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");
> +
> + /* After the unlocked wait, verify that PSR is still wanted!
> */
> mutex_lock(&dev_priv->psr.lock);
> - intel_dp = dev_priv->psr.enabled;
> + return err == 0 && dev_priv->psr.enabled;
> +}
>
> - if (!intel_dp)
> +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(&dev_priv->psr.lock);
> +
> + /*
> + * We have to make sure PSR is ready for re-enable
> + * otherwise it keeps disabled until next full
> enable/disable cycle.
> + * PSR might take some time to get fully disabled
> + * and be ready for re-enable.
> + */
> + if (!psr_wait_for_idle(dev_priv))
> goto unlock;
>
> /*
> @@ -832,7 +838,7 @@ static void intel_psr_work(struct work_struct
> *work)
> if (dev_priv->psr.busy_frontbuffer_bits)
> goto unlock;
>
> - intel_psr_activate(intel_dp);
> + intel_psr_activate(dev_priv->psr.enabled);
> unlock:
> mutex_unlock(&dev_priv->psr.lock);
> }