[Intel-gfx] [PATCH 2/3] drm/i915: Instrument PSR parameter for possible quirks with link standby.

[Daniel Vetter]

On Thu, Dec 10, 2015 at 08:28:23AM -0800, Rodrigo Vivi wrote:
> Link standby support has been deprecated with 'commit 89251b177
> ("drm/i915: PSR: deprecate link_standby support for core platforms.")'
> 
> The reason for that is that main link in full off offers more power
> savings and some platforms implementations on source side had known
> bugs with link standby.
> 
> However we don't know all panels out there and we don't fully rely
> on the VBT information after the case found with the commit that
> made us to deprecate link standby.
> 
> So, before enable PSR by default let's instrument the PSR parameter
> in a way that we can identify different panels out there that might
> require or work better with link standby mode.
> 
> It is also useful to say that for backward compatibility I'm not
> changing the meaning of this flag. So "0" still means disabled
> and "1" means enabled with full support and maximum power savings.
> 
> Negative values are being used for debug purposes since I'd like
> to save positive number for future use like PSR2 for instance.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  5 +++++
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/i915_params.c  |  7 ++++++-
>  drivers/gpu/drm/i915/intel_psr.c    | 13 ++++++++++++-
>  4 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 24318b7..efe973b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2567,6 +2567,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  				enabled = true;
>  		}
>  	}
> +
> +	seq_printf(m, "Forcing main link standby: %s\n",
> +		   yesno(dev_priv->psr.link_standby));
> +
>  	seq_printf(m, "HW Enabled & Active bit: %s", yesno(enabled));
>  
>  	if (!HAS_DDI(dev))
> @@ -2587,6 +2591,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  
>  		seq_printf(m, "Performance_Counter: %u\n", psrperf);
>  	}
> +
>  	mutex_unlock(&dev_priv->psr.lock);
>  
>  	intel_runtime_pm_put(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5edd393..de086f0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -969,6 +969,7 @@ struct i915_psr {
>  	unsigned busy_frontbuffer_bits;
>  	bool psr2_support;
>  	bool aux_frame_sync;
> +	bool link_standby;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 835d609..def8802 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -126,7 +126,12 @@ MODULE_PARM_DESC(enable_execlists,
>  	"(-1=auto [default], 0=disabled, 1=enabled)");
>  
>  module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600);
> -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
> +MODULE_PARM_DESC(enable_psr, "Enable PSR "
> +		 "(-1=force link standby for debug, 0=disabled [default], 1=enabled)"

-1 usually means debug, and values >= 1 are for different enabling modes.
Otherwise makes sense I'd say.
-Daniel

> +		 "In case you needed to force debug mode, please "
> +		 "report PCI device ID, subsystem vendor and subsystem device ID "
> +		 "to intel-gfx at lists.freedesktop.org, if your machine needs it. "
> +		 "It will then be included in an upcoming module version.");
>  
>  module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
>  MODULE_PARM_DESC(preliminary_hw_support,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 9ccff30..7529f93 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -225,7 +225,12 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>  		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
>  	}
>  
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE);
> +	if (dev_priv->psr.link_standby)
> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> +				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
> +	else
> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
> +				   DP_PSR_ENABLE);
>  }
>  
>  static void vlv_psr_enable_source(struct intel_dp *intel_dp)
> @@ -280,6 +285,9 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>  	if (IS_HASWELL(dev))
>  		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>  
> +	if (dev_priv->psr.link_standby)
> +		val |= EDP_PSR_LINK_STANDBY;
> +
>  	I915_WRITE(EDP_PSR_CTL, val |
>  		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
>  		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
> @@ -763,6 +771,9 @@ void intel_psr_init(struct drm_device *dev)
>  	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
>  		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
>  
> +	if (i915.enable_psr == -1)
> +		dev_priv->psr.link_standby = true;
> +
>  	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>  	mutex_init(&dev_priv->psr.lock);
>  }
> -- 
> 2.4.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch