| Message ID | 1391796588-2015-1-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Fri, Feb 7, 2014 at 5:14 PM, Ville Syrjälä <ville.syrjala@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@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. 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
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); /* Setup AUX registers */ I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
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(-)