From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Souza, Jose" Subject: Re: [PATCH 8/9] drm/i915/psr: Set the right frames values Date: Fri, 30 Nov 2018 21:18:29 +0000 Message-ID: <7ebae6906aaf428b21a75005275125ef41be635b.camel@intel.com> References: <20181127003710.18618-1-jose.souza@intel.com> <20181127003710.18618-8-jose.souza@intel.com> <25f005c1045634118b5644055cc68848731f4c12.camel@intel.com> <3825c10f31fcdc3ed2011deb82f8975b2061d69f.camel@intel.com> <1817b0b52f8923efc2d3f3079b1d3816ce0518d8.camel@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0471508244==" Return-path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9491E6E679 for ; Fri, 30 Nov 2018 21:18:31 +0000 (UTC) In-Reply-To: <1817b0b52f8923efc2d3f3079b1d3816ce0518d8.camel@intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "intel-gfx@lists.freedesktop.org" , "Pandiyan, Dhinakaran" Cc: "Vivi, Rodrigo" List-Id: intel-gfx@lists.freedesktop.org --===============0471508244== Content-Language: en-US Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-ELLR7IohQNhjoYrOohDG" --=-ELLR7IohQNhjoYrOohDG Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=C3=A9 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. > > > >=20 > > > > 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. > > > >=20 > > > > Cc: Rodrigo Vivi > > > > Cc: Dhinakaran Pandiyan > > > > Signed-off-by: Jos=C3=A9 Roberto de Souza > > > > --- > > > > drivers/gpu/drm/i915/intel_psr.c | 12 +++++------- > > > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > >=20 > > > > 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 =3D &dev_priv->psr; > > > > u32 val; > > > > =20 > > > > - /* 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 =3D max(6, dev_priv- > > > > >vbt.psr.idle_frames); > > > > + val =3D EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr- > > > > >sink_sync_latency + > > > > 1); > > > > =20 > > > > - idle_frames =3D max(idle_frames, psr->sink_sync_latency + > > > > 1); > > > > - val =3D EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames); > > > > + /* Avoid deep sleep as much as possible to avoid PSR2 > > > > idle > > > > state */ > > > > + val |=3D EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15); > > >=20 > > > Avoid deep sleep as much as possible? Why? We get the best power > > > savings in deep sleep, why make it harder to achieve that? > >=20 > > 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. > >=20 > > 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. >=20 > 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. >=20 > 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 * 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. >=20 > 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. >=20 > -DK >=20 > > >=20 > > > > =20 > > > > /* 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) >=3D 10 || > > > > IS_GEMINILAKE(dev_priv)) > > > > val |=3D EDP_Y_COORDINATE_ENABLE; > > > > =20 > > > > - val |=3D EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr- > > > > >sink_sync_latency + > > > > 1); > > > > - > > > > if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >=3D 0 && > > > > dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <=3D 50) > > > > val |=3D EDP_PSR2_TP2_TIME_50us; --=-ELLR7IohQNhjoYrOohDG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEVNG051EijGa0MiaQVenbO/mOWkkFAlwBqSMACgkQVenbO/mO WknbPAf/bd4dMbsmIZdMV1tk/1Smql+ns/SZCLuy2ABAdGdlELsk3n8A2CZonaWw BiHe5oGlNwI/m3CXdQlrcLU31EsfXljxZK1FzCPG/RPmHF2Tx4dKCEfGz7oNbTw3 Sr0xslEo3DEK9cQrY4dUF7/J8zEpBa0mdNWJfQOQMGHLK4gXRYw+AyWLBIGhZ7rZ JT5F0/CISakcr8RmzvOSNlQW+8nYYsqQscHRII9qca0VWEtAjsog3hWjWPz6dLlC vCQeQ6Wb/GnhNfkebEz7rJA6DzfB6BhJmupcPzrT9pUPYrNc7FDZSPkSSxr+uTzY Fqi2OPlCcDTCLZTwMrEICw6lsRKsBw== =jKar -----END PGP SIGNATURE----- --=-ELLR7IohQNhjoYrOohDG-- --===============0471508244== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0471508244==--