On Fri, 2018-11-30 at 13:18 -0800, Souza, Jose wrote: > On Fri, 2018-11-30 at 11:35 -0800, Dhinakaran Pandiyan wrote: > > 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? > > I don't have power data, just stimations of how many frames it would > state in each state. > > > > > 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. > > 2 flips per second is too low, lets take a 4 flips per seconds(user > will be typing faster than 2 keys per second): > > # Base > Modeset at 60hz > 9 frames to activate PSR2 > > # With 1 frame to enter deep sleep > 36 frames of PSR2 idle > 4 frames of sleep > 20 frames of deep sleep > + tranning at every PSR2 exit > > # With 15 frames to enter deep sleep > 9 frames of PSR2 idle > 4 frames of sleep reading memory > 47 frames of sleep without read memory We are comparing apples to oranges, I have no idea how much power this SU sleep state actually saves in comparison to deep sleep. Without any numbers, I'm not sold on the optimization you are proposing. -DK > * It would not enter in deep sleep so in the next second it would be > 4 frames of sleep reading memory > 56 frames of sleep without read memory > > And when screen is realy idle like a savings screen it would still go > to deep sleep. > > > > > 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. > > Other problem of leaving the number of frames to enter deep sleep to > minumum is that it would almost never do selective update it would be > like a PSR1. > > > > > > -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
