Re: [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Feb 07, 2014 at 04:09:47PM -0200, Rodrigo Vivi wrote:
> As pointed out by Ville we were using inverted logic here.
> According to spec:
> For link standby mode set 170h[1] = 1.
> For full link disabling set 170h[1] = 0.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 50381f7..4ecda72 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1661,12 +1661,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>  	/* Enable PSR in sink */
>  	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
>  		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> -					    DP_PSR_ENABLE &
> -					    ~DP_PSR_MAIN_LINK_ACTIVE);
> -	else
> -		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>  					    DP_PSR_ENABLE |
>  					    DP_PSR_MAIN_LINK_ACTIVE);
> +	else
> +		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> +					    DP_PSR_ENABLE &
> +					    ~DP_PSR_MAIN_LINK_ACTIVE);

I think this is now the opposite of what we want. Ie. if the sink
doesn't require training we should disable the main link. Otherwise we
should keep the main link on, and that way avoid the need to train on
PSR exit.

Actually I'm not sure that's really what we want. I think the hardware
can do the training on its own, so in theory we should just always disable
the main link. Although the PM guide has a comment indicating that the
hardware training can fail, in which case software must repeat it. We
don't have code to do that, so I guess leaving the main link on is the
safer option. Would be nice to have a comment in the code stating as
much, if this is indeed the reason why the code was written this way.

>  
>  	/* Setup AUX registers */
>  	I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
> -- 
> 1.7.11.7

-- 
Ville Syrjälä
Intel OTC