[Intel-gfx] [PATCH 2/4] drm/i915: Add PSR main link standby support back

[Zanoni, Paulo R]

Em Sex, 2015-12-11 às 08:39 -0800, Rodrigo Vivi escreveu:
> 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 on HSW and BDW implementations on source side had known
> bugs with link standby.
> 
> However that same HSD report only mentions BDW and HSW and tells that
> a fix was going to new platforms. Since I got a SKL working really
> well 

Please define what "working really well" means, especially when
comparing against the full-off mode that's supposed to be even better
:)


> with link in standby mode let's respect VBT again for this

I see this patch fixes some of the issues I pointed in the review of
patch 1/4. This patch should definitely be applied _before_ the patch
where we tell intel_psr_match_conditions() to start accepting link
standby mode.

> platforms.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  4 ++++
>  drivers/gpu/drm/i915/i915_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_psr.c    | 27 +++++++++++++++++++++++++--
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 24318b7..c15434b 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, "Main link in standby mode: %s\n",
> +		   yesno(dev_priv->psr.link_standby));
> +
>  	seq_printf(m, "HW Enabled & Active bit: %s",
> yesno(enabled));
>  
>  	if (!HAS_DDI(dev))
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 9124085..4b4ae3a 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/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 4a9c062..b84ec80 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 |
> @@ -327,7 +335,7 @@ static bool intel_psr_match_conditions(struct
> intel_dp *intel_dp)
>  		return false;
>  	}
>  
> -	if (HAS_DDI(dev) && !dev_priv->vbt.psr.full_link &&
> +	if (HAS_DDI(dev) && !dev_priv->psr.link_standby &&

Since I think this patch should come before patch 1, then I suppose
you'll need to rebase this chunk.


>  	    dig_port->port != PORT_A) {
>  		DRM_DEBUG_KMS("PSR condition failed: Link Off
> requested/needed but not supported on this port\n");
>  		return false;
> @@ -763,6 +771,21 @@ 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 (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		/*
> +		 * On HSW and BDW Source implementation as an issue
> with PSR
> +		 * exit that requires an workaround that doesn't
> exist on HSW
> +		 * and that on BDW requires single frame update that
> we don't
> +		 * implement because it needs 3 or 4 extra
> workarounds.
> +		 */

Bikeshed: since we don't mention the exact problems anywhere, we could
just go with something like "HSW and BDW require workarounds that we
don't implement".

Besides the patch ordering issue, everything else looks correct.

> +		dev_priv->psr.link_standby = false;
> +	else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> +		/* On VLV and CHV only standby mode is supported. */
> +		dev_priv->psr.link_standby = true;
> +	else
> +		/* For new platforms let's respect VBT back again */
> +		dev_priv->psr.link_standby = dev_priv-
> >vbt.psr.full_link;
> +
>  	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
>  	mutex_init(&dev_priv->psr.lock);
>  }