On Thu, 08 Sep 2016, Jani Nikula <jani.nikula@xxxxxxxxx> wrote: > 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. Pushed. BR, Jani. > > 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
