| Message ID | 1463590036-17824-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 18, 2016 at 06:47:12PM +0200, Daniel Vetter wrote: > This reverts > > commit dfaf37baa07513d2c37afff79978807d2d10221a > Author: Rodrigo Vivi <rodrigo.vivi@intel.com> > Date: Mon Dec 7 14:45:20 2015 -0800 > > drm/i915: Fix idle_frames counter. > > and > > commit 97173eaf5f33b1e85efdb06d593d333480b60bf3 > Author: Rodrigo Vivi <rodrigo.vivi@intel.com> > Date: Tue Jul 7 16:28:55 2015 -0700 > > drm/i915: PSR: Increase idle_frames > > and implements > > commit d44b4dcbd1b44737462b77971d216d21a9413341 > Author: Rodrigo Vivi <rodrigo.vivi@intel.com> > Date: Fri Nov 14 08:52:31 2014 -0800 > > drm/i915: HSW/BDW PSR Set idle_frames = VBT + 1 > > without the hack to use 2 idle frames when VBT says 1. We keep the + 1 > just for safety, although I haven't really figured out why that one > exists. > > It's nonsense. idle_frames = number of frames where the screen is > entirely idle before we think about entering PSR. > > idle_patter = part of link training, and we probably totally butchered > link training because we told the hw to entirely skip it. No wonder > PSR occasionally just fell over. > > I suspect the reason we've increased idle frames is that it makes PSR > entry slightly less likely, and more likely to happen in a quite > system, which probably increased the changes the panel came back up > without link training. The proper fix is to implement link training > for PSR. Quite the mess there. At the least this makes things look a bit more sane. FWIW Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Sonika Jindal <sonika.jindal@intel.com> > Cc: Durgadoss R <durgadoss.r@intel.com> > Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > 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 a788d1e9589b..0295d8dd483f 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -272,14 +272,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) > struct drm_i915_private *dev_priv = dev->dev_private; > > uint32_t max_sleep_time = 0x1f; > - /* > - * 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. > + /* 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. > */ > - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > + uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1; > uint32_t val = EDP_PSR_ENABLE; > > val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > -- > 2.8.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, May 18, 2016 at 10:46 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, May 18, 2016 at 06:47:12PM +0200, Daniel Vetter wrote: >> This reverts >> >> commit dfaf37baa07513d2c37afff79978807d2d10221a >> Author: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Date: Mon Dec 7 14:45:20 2015 -0800 >> >> drm/i915: Fix idle_frames counter. >> >> and >> >> commit 97173eaf5f33b1e85efdb06d593d333480b60bf3 >> Author: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Date: Tue Jul 7 16:28:55 2015 -0700 >> >> drm/i915: PSR: Increase idle_frames >> >> and implements >> >> commit d44b4dcbd1b44737462b77971d216d21a9413341 >> Author: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Date: Fri Nov 14 08:52:31 2014 -0800 >> >> drm/i915: HSW/BDW PSR Set idle_frames = VBT + 1 >> >> without the hack to use 2 idle frames when VBT says 1. We keep the + 1 >> just for safety, although I haven't really figured out why that one >> exists. >> >> It's nonsense. idle_frames = number of frames where the screen is >> entirely idle before we think about entering PSR. >> >> idle_patter = part of link training, and we probably totally butchered >> link training because we told the hw to entirely skip it. No wonder >> PSR occasionally just fell over. >> >> I suspect the reason we've increased idle frames is that it makes PSR >> entry slightly less likely, and more likely to happen in a quite >> system, which probably increased the changes the panel came back up >> without link training. The proper fix is to implement link training >> for PSR. > > Quite the mess there. At the least this makes things look a bit more > sane. > > FWIW > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Sonika Jindal <sonika.jindal@intel.com> >> Cc: Durgadoss R <durgadoss.r@intel.com> >> Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> --- >> 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 a788d1e9589b..0295d8dd483f 100644 >> --- a/drivers/gpu/drm/i915/intel_psr.c >> +++ b/drivers/gpu/drm/i915/intel_psr.c >> @@ -272,14 +272,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> uint32_t max_sleep_time = 0x1f; >> - /* >> - * 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. >> + /* 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. >> */ >> - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); >> + uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1; Hm, I believe this is dangerous. First because this comment above explain that in the past we found cases where panel demands 4 idles but VBT was incorrectly unset. Also because at some point HW guys found out they had a bug on the idle frames calculation so they were off by one, so setting it to one would mean in some cases HW would use 0 and start entering PSR when it shouldn't yet. But yeap, I know at least nowadays our sw tracking helper could help in this case I believed we should stat on the safest side here... >> uint32_t val = EDP_PSR_ENABLE; >> >> val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; >> -- >> 2.8.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index a788d1e9589b..0295d8dd483f 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -272,14 +272,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) struct drm_i915_private *dev_priv = dev->dev_private; uint32_t max_sleep_time = 0x1f; - /* - * 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. + /* 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. */ - uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); + uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1; uint32_t val = EDP_PSR_ENABLE; val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
This reverts commit dfaf37baa07513d2c37afff79978807d2d10221a Author: Rodrigo Vivi <rodrigo.vivi@intel.com> Date: Mon Dec 7 14:45:20 2015 -0800 drm/i915: Fix idle_frames counter. and commit 97173eaf5f33b1e85efdb06d593d333480b60bf3 Author: Rodrigo Vivi <rodrigo.vivi@intel.com> Date: Tue Jul 7 16:28:55 2015 -0700 drm/i915: PSR: Increase idle_frames and implements commit d44b4dcbd1b44737462b77971d216d21a9413341 Author: Rodrigo Vivi <rodrigo.vivi@intel.com> Date: Fri Nov 14 08:52:31 2014 -0800 drm/i915: HSW/BDW PSR Set idle_frames = VBT + 1 without the hack to use 2 idle frames when VBT says 1. We keep the + 1 just for safety, although I haven't really figured out why that one exists. It's nonsense. idle_frames = number of frames where the screen is entirely idle before we think about entering PSR. idle_patter = part of link training, and we probably totally butchered link training because we told the hw to entirely skip it. No wonder PSR occasionally just fell over. I suspect the reason we've increased idle frames is that it makes PSR entry slightly less likely, and more likely to happen in a quite system, which probably increased the changes the panel came back up without link training. The proper fix is to implement link training for PSR. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Sonika Jindal <sonika.jindal@intel.com> Cc: Durgadoss R <durgadoss.r@intel.com> Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)