On Thu, 08 Sep 2016, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Wed, Sep 07, 2016 at 05:42:31PM -0700, Rodrigo Vivi wrote: >> This reverts commit 1c80c25fb622973dd135878e98d172be20859049. >> >> There are panels that needs 4 idle frames before entering PSR, >> but VBT is unproperly set. >> >> Also lately it was identified that idle frame count calculated at HW >> can be off by 1, what makes the minimum of 2, at least. >> >> Without the current vbt+1 we are with the risk of having HW calculating >> 0 idle frames and entering PSR when it shouldn't. Regardless the lack >> of link training. >> >> Cc: Dominik Brodowski <linux@xxxxxxxxxxxxxxxxxxxx> >> Cc: Jani Nikula <jani.nikula@xxxxxxxxx> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/intel_psr.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c >> index 59a21c9..108ba1e 100644 >> --- a/drivers/gpu/drm/i915/intel_psr.c >> +++ b/drivers/gpu/drm/i915/intel_psr.c >> @@ -255,14 +255,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) >> struct drm_i915_private *dev_priv = to_i915(dev); >> >> uint32_t max_sleep_time = 0x1f; >> - /* Lately it was identified that depending on panel idle frame count >> - * calculated at HW can be off by 1. So let's use what came >> - * from VBT + 1. >> - * There are also other cases where panel demands at least 4 >> - * but VBT is not being set. To cover these 2 cases lets use >> - * at least 5 when VBT isn't set to be on the safest side. >> + /* >> + * Let's respect VBT in case VBT asks a higher idle_frame value. >> + * Let's use 6 as the minimum to cover all known cases including >> + * the off-by-one issue that HW has in some cases. Also there are >> + * cases where sink should be able to train >> + * with the 5 or 6 idle patterns. >> */ >> - uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1; >> + uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > I don't really understand this logic compared to the explanations. > > Shouldn't we do something like 'idle_frames = max(4, psr.idle_frames) + 1;'? We're at rc5, smells like revert, not trial and error. Side note, looking at the VBT spec on tp1, tp2/tp3 wakeup times, there seems to be some confusion about what the values mean. Has there perhaps been a change in the spec? What's the VBT version where the problems happen? BR, Jani. > >> uint32_t val = EDP_PSR_ENABLE; >> >> val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
