| Message ID | 20181127003710.18618-8-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] drm/i915: Disable PSR in Apple panels | expand |
On Mon, Nov 26, 2018 at 04:37:09PM -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@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > 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); Too many changes in a single patch that I couldn't understand why we are removing the minimal of 6 that was our safe net. > + 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); > > /* 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; > -- > 2.19.2 >
On Thu, 2018-11-29 at 15:10 -0800, Rodrigo Vivi wrote: > On Mon, Nov 26, 2018 at 04:37:09PM -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@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > 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); > > Too many changes in a single patch that I couldn't understand why we > are removing the minimal of 6 that was our safe net. The vbt idle_frames should not be used for PSR2 as PSR2 just wait for X frames(idle or not) to activate PSR2 so just rely in sink sync_latency should be enough but I can bring it back for safety. > > > + 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); > > > > /* 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; > > -- > > 2.19.2 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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); /* 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;
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@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)