On Thu, 2018-11-29 at 17:00 -0800, Souza, Jose wrote: > On Thu, 2018-11-29 at 15:33 -0800, Dhinakaran Pandiyan wrote: > > On Mon, 2018-11-26 at 16:37 -0800, José Roberto de Souza wrote: > > > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the > > > number > > > of > > > frames that it should wait to enter PSR, what is wrong. > > > Here it is setting this field with the highest value to avoid > > > PSR2 > > > exits frequently, as when HW exit deep sleep it needs to go to > > > idle > > > state causing a PSR exit for then waiting a few frames before > > > activate PSR2 again. > > > This will result in more power saving as the sleep state also > > > provide > > > some power savings by doing selective updates instead of full > > > screen > > > updates. > > > > > > About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of > > > frames > > > (not idle frames) that PSR2 hardware will wait to activate PSR2, > > > so > > > lets keep using the sink sync latency. > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 12 +++++------- > > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index ba7bbe3f8df2..6fd793fec5e9 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct > > > intel_dp > > > *intel_dp) > > > struct i915_psr *psr = &dev_priv->psr; > > > u32 val; > > > > > > - /* Let's use 6 as the minimum to cover all known cases > > > including the > > > - * off-by-one issue that HW has in some cases. > > > + /* sink_sync_latency of 8 means source has to wait for more > > > than 8 > > > + * frames, we'll go with 9 frames for now > > > */ > > > - int idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > > + val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency + > > > 1); > > > > > > - idle_frames = max(idle_frames, psr->sink_sync_latency + 1); > > > - val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames); > > > + /* Avoid deep sleep as much as possible to avoid PSR2 idle > > > state */ > > > + val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15); > > > > Avoid deep sleep as much as possible? Why? We get the best power > > savings in deep sleep, why make it harder to achieve that? > > As said in commit message a small frame count to enter in deep sleep > will cause frequent PSR exits and when HW comes back from deep sleep > it > needs to go to idle state. So it will need to wait for > EDP_PSR2_FRAMES_BEFORE_ACTIVATE() frames before activate PSR again. > > A regular productivity tools(Office and email) user would benefit > from > that as the mouse cursor blinking would make PSR2 go from deep sleep > to > idle state and stay in idle as long as cursor is blinking. With 15 > frames user will stay most of the time in PSR2 sleep state that > already > provide some power savings. Do you have any numbers to justify that not entering deep sleep (just doing SU) is better than entering deep sleep and exiting? Even with a blinking cursor at 2 flips/second, there is enough time to wait for 9 idle frames (max currently), enter deep sleep and exit(~2 frames) between flips. Why not leave EDP_PSR2_FRAMES_BEFORE_ACTIVATE as it is and reduce EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP to the minimum? But then again, I'd like to see some numbers if it's possible. -DK > > > > > > > > > > > /* FIXME: selective update is probably totally broken because > > > it doesn't > > > * mesh at all with our frontbuffer tracking. And the hw alone > > > isn't > > > @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct intel_dp > > > *intel_dp) > > > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) > > > val |= EDP_Y_COORDINATE_ENABLE; > > > > > > - val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency + > > > 1); > > > - > > > if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 && > > > dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50) > > > val |= EDP_PSR2_TP2_TIME_50us; _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
