Re: [PATCH] drm/i915/psr: Kill delays when activating psr back.

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Wed, 2018-06-13 at 12:26 -0700, Rodrigo Vivi wrote:
> The immediate enabling was actually not an issue for the
> HW perspective for core platforms that have HW tracking.
> HW will wait few identical idle frames before transitioning
> to actual psr active anyways.
> 
> Now that we removed VLV/CHV out of the picture completely
> we can safely remove any delays.
> 
> Note that this patch also remove the delayed activation
> on HSW and BDW introduced by commit 'd0ac896a477d
> ("drm/i915: Delay first PSR activation.")'. This was
> introduced to fix a blank screen on VLV/CHV and also
> masked some frozen screens on other core platforms.
> Probably the same that we are now properly hunting and fixing.
> 
> v2:(DK): Remove unnecessary WARN_ONs and make some other
>          VLV | CHV more readable.
> v3: Do it regardless the timer rework.
> v4: (DK/CI): Add VLV || CHV check on cancel work at psr_disable.
> v5: Kill remaining items and fully rework activation functions.
> v6: Rebase on top of VLV/CHV clean-up and keep the reactivation
>     on a regular non-delayed work to avoid extra delays on exit
>     calls and allow us to add few more safety checks before
>     real activation.

We have to implement this bspec step in the disable sequence now that
you are removing the delay - 
"Wait for SRD_STATUS to show SRD is Idle. This will take up to one full
frame time (1/refresh rate), plus SRD exit training time (max of 6ms),
plus SRD aux channel handshake (max of 1.5ms)."

Otherwise, we can end up re-enabling right after a disable in
psr_exit()


> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 --
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
>  drivers/gpu/drm/i915/intel_psr.c    | 29 +++++++------------------
> ----
>  3 files changed, 8 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 769ab9745834..948b973af067 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2660,8 +2660,6 @@ static int i915_edp_psr_status(struct seq_file
> *m, void *data)
>  	seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv-
> >psr.enabled));
>  	seq_printf(m, "Busy frontbuffer bits: 0x%03x\n",
>  		   dev_priv->psr.busy_frontbuffer_bits);
> -	seq_printf(m, "Re-enable work scheduled: %s\n",
> -		   yesno(work_busy(&dev_priv->psr.work.work)));
>  
>  	if (dev_priv->psr.psr2_enabled)
>  		enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index be8c2f0823c4..19defe73b156 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -613,7 +613,7 @@ struct i915_psr {
>  	bool sink_support;
>  	struct intel_dp *enabled;
>  	bool active;
> -	struct delayed_work work;
> +	struct work_struct work;
>  	unsigned busy_frontbuffer_bits;
>  	bool sink_psr2_support;
>  	bool link_standby;
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 71dfe541740f..ef0f4741a95d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -671,21 +671,7 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	dev_priv->psr.enable_source(intel_dp, crtc_state);
>  	dev_priv->psr.enabled = intel_dp;
>  
> -	if (INTEL_GEN(dev_priv) >= 9) {
> -		intel_psr_activate(intel_dp);
> -	} else {
> -		/*
> -		 * FIXME: Activation should happen immediately since
> this
> -		 * function is just called after pipe is fully
> trained and
> -		 * enabled.
> -		 * However on some platforms we face issues when
> first
> -		 * activation follows a modeset so quickly.
> -		 *     - On HSW/BDW we get a recoverable frozen
> screen until
> -		 *       next exit-activate sequence.
> -		 */
> -		schedule_delayed_work(&dev_priv->psr.work,
> -				      msecs_to_jiffies(intel_dp-
> >panel_power_cycle_delay * 5));
> -	}
> +	intel_psr_activate(intel_dp);
>  
>  unlock:
>  	mutex_unlock(&dev_priv->psr.lock);
> @@ -768,8 +754,6 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>  
>  	dev_priv->psr.enabled = NULL;
>  	mutex_unlock(&dev_priv->psr.lock);
> -
> -	cancel_delayed_work_sync(&dev_priv->psr.work);
>  }
>  
>  static bool psr_wait_for_idle(struct drm_i915_private *dev_priv)
> @@ -805,10 +789,13 @@ static bool psr_wait_for_idle(struct
> drm_i915_private *dev_priv)
>  static void intel_psr_work(struct work_struct *work)
>  {
>  	struct drm_i915_private *dev_priv =
> -		container_of(work, typeof(*dev_priv),
> psr.work.work);
> +		container_of(work, typeof(*dev_priv), psr.work);
>  
>  	mutex_lock(&dev_priv->psr.lock);
>  
> +	if (!dev_priv->psr.enabled)
> +		goto unlock;
> +

I thought flush_work() was missing in psr_disable(), but this check
should take care of not enabling PSR after psr_disable()


>  	/*
>  	 * We have to make sure PSR is ready for re-enable
>  	 * otherwise it keeps disabled until next full
> enable/disable cycle.
> @@ -949,9 +936,7 @@ void intel_psr_flush(struct drm_i915_private
> *dev_priv,
>  	}
>  
>  	if (!dev_priv->psr.active && !dev_priv-
> >psr.busy_frontbuffer_bits)
> -		if (!work_busy(&dev_priv->psr.work.work))
> -			schedule_delayed_work(&dev_priv->psr.work,
> -					      msecs_to_jiffies(100))
> ;
> +		schedule_work(&dev_priv->psr.work);
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -998,7 +983,7 @@ void intel_psr_init(struct drm_i915_private
> *dev_priv)
>  		dev_priv->psr.link_standby = false;
>  	}
>  
> -	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> +	INIT_WORK(&dev_priv->psr.work, intel_psr_work);
>  	mutex_init(&dev_priv->psr.lock);
>  
>  	dev_priv->psr.enable_source = hsw_psr_enable_source;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx