* [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" @ 2016-09-08 0:42 Rodrigo Vivi 2016-09-08 1:53 ` ✗ Fi.CI.BAT: warning for " Patchwork 2016-09-08 8:01 ` [PATCH] " Ville Syrjälä 0 siblings, 2 replies; 5+ messages in thread From: Rodrigo Vivi @ 2016-09-08 0:42 UTC (permalink / raw) To: intel-gfx; +Cc: Jani Nikula, Daniel Vetter, Dominik Brodowski, Rodrigo Vivi 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@dominikbrodowski.net> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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 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); 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 5+ messages in thread
* ✗ Fi.CI.BAT: warning for Revert "drm/i915/psr: Make idle_frames sensible again" 2016-09-08 0:42 [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" Rodrigo Vivi @ 2016-09-08 1:53 ` Patchwork 2016-09-08 8:01 ` [PATCH] " Ville Syrjälä 1 sibling, 0 replies; 5+ messages in thread From: Patchwork @ 2016-09-08 1:53 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx == Series Details == Series: Revert "drm/i915/psr: Make idle_frames sensible again" URL : https://patchwork.freedesktop.org/series/12148/ State : warning == Summary == Series 12148v1 Revert "drm/i915/psr: Make idle_frames sensible again" http://patchwork.freedesktop.org/api/1.0/series/12148/revisions/1/mbox/ Test gem_exec_gttfill: Subgroup basic: pass -> SKIP (fi-snb-2600) fi-bdw-5557u total:252 pass:233 dwarn:2 dfail:1 fail:1 skip:15 fi-bsw-n3050 total:252 pass:203 dwarn:1 dfail:1 fail:1 skip:46 fi-hsw-4770k total:252 pass:226 dwarn:2 dfail:1 fail:1 skip:22 fi-hsw-4770r total:252 pass:222 dwarn:2 dfail:1 fail:1 skip:26 fi-ilk-650 total:252 pass:179 dwarn:2 dfail:1 fail:3 skip:67 fi-ivb-3520m total:252 pass:217 dwarn:2 dfail:1 fail:1 skip:31 fi-ivb-3770 total:252 pass:217 dwarn:2 dfail:1 fail:1 skip:31 fi-skl-6260u total:252 pass:234 dwarn:2 dfail:1 fail:1 skip:14 fi-skl-6700k total:252 pass:219 dwarn:3 dfail:1 fail:1 skip:28 fi-snb-2520m total:252 pass:204 dwarn:1 dfail:1 fail:1 skip:45 fi-snb-2600 total:252 pass:202 dwarn:2 dfail:1 fail:1 skip:46 fi-byt-n2820 failed to collect. IGT log at Patchwork_2489/fi-byt-n2820/igt.log Results at /archive/results/CI_IGT_test/Patchwork_2489/ a6f2d1b98141ef44ea5d6c480c7fb2ce821189d2 drm-intel-nightly: 2016y-09m-07d-23h-08m-05s UTC integration manifest c04448b Revert "drm/i915/psr: Make idle_frames sensible again" _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" 2016-09-08 0:42 [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" Rodrigo Vivi 2016-09-08 1:53 ` ✗ Fi.CI.BAT: warning for " Patchwork @ 2016-09-08 8:01 ` Ville Syrjälä 2016-09-08 8:53 ` Jani Nikula 1 sibling, 1 reply; 5+ messages in thread From: Ville Syrjälä @ 2016-09-08 8:01 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, Dominik Brodowski 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@dominikbrodowski.net> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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 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;'? > 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@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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" 2016-09-08 8:01 ` [PATCH] " Ville Syrjälä @ 2016-09-08 8:53 ` Jani Nikula 2016-09-13 9:05 ` Jani Nikula 0 siblings, 1 reply; 5+ messages in thread From: Jani Nikula @ 2016-09-08 8:53 UTC (permalink / raw) To: Ville Syrjälä, Rodrigo Vivi Cc: Daniel Vetter, intel-gfx, Dominik Brodowski On Thu, 08 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@dominikbrodowski.net> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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 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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" 2016-09-08 8:53 ` Jani Nikula @ 2016-09-13 9:05 ` Jani Nikula 0 siblings, 0 replies; 5+ messages in thread From: Jani Nikula @ 2016-09-13 9:05 UTC (permalink / raw) To: Ville Syrjälä, Rodrigo Vivi Cc: Daniel Vetter, intel-gfx, Dominik Brodowski On Thu, 08 Sep 2016, Jani Nikula <jani.nikula@intel.com> wrote: > On Thu, 08 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@dominikbrodowski.net> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> Cc: Daniel Vetter <daniel.vetter@intel.com> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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 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@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-13 9:05 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-08 0:42 [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" Rodrigo Vivi 2016-09-08 1:53 ` ✗ Fi.CI.BAT: warning for " Patchwork 2016-09-08 8:01 ` [PATCH] " Ville Syrjälä 2016-09-08 8:53 ` Jani Nikula 2016-09-13 9:05 ` Jani Nikula
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.