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

[Rodrigo Vivi]

On Fri, Feb 7, 2014 at 5:14 PM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> 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 at linux.intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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.

To be honest, I think I agree with you, but apparently performance
counter inc improved on this way...

>
> 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.

 I'll do a carefull check and local tests and send new version fixed
or with good comments.

>
>>
>>       /* Setup AUX registers */
>>       I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
>> --
>> 1.7.11.7
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br