Re: [PATCH] drm/i915/psr: Kill scheduled work for Core platforms.

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

On Thu, 2018-03-08 at 17:17 -0800, Rodrigo Vivi wrote:
> The immediate enabling is 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.
> 
> 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.
> 
> Furthermore, if we stop using delayed activation on core
> platforms we will be able, on following up patches,
> to use available workarounds to make HW tracking properly
> exit PSR instead of the big nuke of disabling psr and
> re-enable on exit and activate respectively.
> At least on few reliable cases.
> 
> v2:(DK): Remove unnecessary WARN_ONs and make some other
> 	 VLV | CHV more readable.
> v3: Do it regardless the timer rework.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++--------
>  drivers/gpu/drm/i915/intel_psr.c    | 22 ++++++++++++++--------
>  2 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index e838c765b251..2fd6f98364d3 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2572,15 +2572,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>  	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 (HAS_DDI(dev_priv)) {
> -		if (dev_priv->psr.psr2_support)
> -			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> -		else
> -			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
> -	} else {
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		seq_printf(m, "Re-enable work scheduled: %s\n",
> +			   yesno(work_busy(&dev_priv->psr.work.work)));
> +
>  		for_each_pipe(dev_priv, pipe) {
>  			enum transcoder cpu_transcoder =
>  				intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> @@ -2599,6 +2595,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  			intel_display_power_put(dev_priv, power_domain);
>  		}
> +	} else {
> +		if (dev_priv->psr.psr2_support)
> +			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
> +		else
> +			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
>  	}
>  
>  	seq_printf(m, "Main link in standby mode: %s\n",
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index b0286722a72f..9705d89c5ebf 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -652,9 +652,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 {
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>  		/*
>  		 * FIXME: Activation should happen immediately since this
>  		 * function is just called after pipe is fully trained and
> @@ -667,6 +665,8 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  		 */
>  		schedule_delayed_work(&dev_priv->psr.work,
>  				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
> +	} else {
> +		intel_psr_activate(intel_dp);
>  	}
>  

I think we should split this into a separate patch.

>  unlock:
> @@ -1045,10 +1045,15 @@ 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));
> +	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits) {
> +		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +			if (!work_busy(&dev_priv->psr.work.work))


These conditions have become extremely hard to read, we need to rework
them into functions and/or re-evaluate some of the conditions. I don't
follow the logic every time I read it after a couple of days.


For instance, if frontbuffer_bits == 0, why do we need to
activate/schedule psr. Isn't that a nop for this function?


> +				schedule_delayed_work(&dev_priv->psr.work,
> +						      msecs_to_jiffies(100));
> +		} else {
> +			intel_psr_activate(dev_priv->psr.enabled);
> +		}
> +	}
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
>  
> @@ -1095,7 +1100,8 @@ 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);
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>  	mutex_init(&dev_priv->psr.lock);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx