| Message ID | 20190913082339.1785-6-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DC3CO Support for TGL | expand |
On Fri, Sep 13, 2019 at 01:53:37PM +0530, Anshuman Gupta wrote: > Add dc3co helper functions to enable/disable psr2 deep sleep. > Adhere B.Specs by disallow DC3CO state before PSR2 exit. > Enable PSR2 exitline event and program the desired scanlines > to exit DC3CO in intel_psr_enable function at modeset path. > Disable the DC3CO exitline in order to maintian consistent > pipe config state in encoder disable path. > > v1: Moved calling of tgl_enable_psr2_transcoder_exitline() to > intel_psr_enable(). [Imre] > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Animesh Manna <animesh.manna@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> You missed some of the review comments for v7, see below. > --- > drivers/gpu/drm/i915/display/intel_psr.c | 51 ++++++++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_psr.h | 2 + > drivers/gpu/drm/i915/i915_drv.h | 1 + > 3 files changed, 54 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index b3c7eef53bf3..11d37f96ce71 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -534,6 +534,48 @@ transcoder_has_psr2(struct drm_i915_private *dev_priv, enum transcoder trans) > return trans == TRANSCODER_EDP; > } > > +static void psr2_program_idle_frames(struct drm_i915_private *dev_priv, > + u32 idle_frames) > +{ > + u32 val; > + > + idle_frames <<= EDP_PSR2_IDLE_FRAME_SHIFT; > + val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)); > + val &= ~EDP_PSR2_IDLE_FRAME_MASK; > + val |= idle_frames; > + I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val); > +} > + > +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv) > +{ > + int idle_frames = 0; No need for a variable for that. > + > + psr2_program_idle_frames(dev_priv, idle_frames); > +} > + > +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv) > +{ > + int idle_frames; > + > + /* > + * Let's use 6 as the minimum to cover all known cases including the > + * off-by-one issue that HW has in some cases. > + */ > + idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > + idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1); > + psr2_program_idle_frames(dev_priv, idle_frames); > +} > + > +static void tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv) > +{ > + if (!IS_TIGERLAKE(dev_priv)) > + return; Could we use crtc_state->dc3co_exitline instead (which we would set only whenever we want to enable DC3co)? > + > + cancel_delayed_work(&dev_priv->csr.idle_work); > + /* Before PSR2 exit disallow dc3co*/ > + tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6); > +} > + > static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > struct intel_crtc_state *crtc_state) > { > @@ -799,6 +841,10 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > WARN_ON(dev_priv->drrs.dp); > > + /* Enable PSR2 transcoder exit line */ > + if (crtc_state->has_psr2) > + tgl_enable_psr2_transcoder_exitline(crtc_state); This is too late to program the EXITLINE reg, since the transcoder is already enabled. > + > mutex_lock(&dev_priv->psr.lock); > > if (!psr_global_enabled(dev_priv->psr.debug)) { > @@ -829,6 +875,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv) > } > > if (dev_priv->psr.psr2_enabled) { > + tgl_disallow_dc3co_on_psr2_exit(dev_priv); So DC5/6 will be reenabled, but I can't see how PSR deep sleep gets reenabled. I think we should have a function that we call both from here and the idle thread that reenables DC5/6 and also PSR deep sleep. > val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)); > WARN_ON(!(val & EDP_PSR2_ENABLE)); > val &= ~EDP_PSR2_ENABLE; > @@ -895,6 +942,10 @@ void intel_psr_disable(struct intel_dp *intel_dp, > if (WARN_ON(!CAN_PSR(dev_priv))) > return; > > + /* Disable PSR2 transcoder exit line */ > + if (old_crtc_state->has_psr2) > + tgl_disable_psr2_transcoder_exitline(old_crtc_state); The transcoder is still active here so we can't program the EXITLINE reg at this point. Please add tgl_enable/disable_psr2_transcoder_exitline() in this patch where the functions are actually used, the review is difficult when I have to jump between the two patches. OTOH tgl_psr2_deep_sleep_enable/disable() is used in a follow-up patch, but you define them already here; those should be also moved where they are used for easier review. > + > mutex_lock(&dev_priv->psr.lock); > > intel_psr_disable_locked(intel_dp); > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h > index 46e4de8b8cd5..75a9862f36fd 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.h > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > @@ -35,5 +35,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp); > int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state, > u32 *out_value); > bool intel_psr_enabled(struct intel_dp *intel_dp); > +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv); > +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv); > > #endif /* __INTEL_PSR_H__ */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 68fb732c24c8..4521b9381db3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -494,6 +494,7 @@ struct i915_psr { > bool link_standby; > bool colorimetry_support; > bool psr2_enabled; > + bool psr2_deep_slp_disabled; > u8 sink_sync_latency; > ktime_t last_entry_attempt; > ktime_t last_exit; > -- > 2.21.0 >
On 2019-09-23 at 19:42:02 +0300, Imre Deak wrote: > On Fri, Sep 13, 2019 at 01:53:37PM +0530, Anshuman Gupta wrote: > > Add dc3co helper functions to enable/disable psr2 deep sleep. > > Adhere B.Specs by disallow DC3CO state before PSR2 exit. > > Enable PSR2 exitline event and program the desired scanlines > > to exit DC3CO in intel_psr_enable function at modeset path. > > Disable the DC3CO exitline in order to maintian consistent > > pipe config state in encoder disable path. > > > > v1: Moved calling of tgl_enable_psr2_transcoder_exitline() to > > intel_psr_enable(). [Imre] > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Animesh Manna <animesh.manna@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > You missed some of the review comments for v7, see below. Ah, somehow i missed this mail itself. https://patchwork.freedesktop.org/patch/329538/?series=64923&rev=8 I will address these comments. below there are some follow up comments. > > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 51 ++++++++++++++++++++++++ > > drivers/gpu/drm/i915/display/intel_psr.h | 2 + > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > 3 files changed, 54 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > index b3c7eef53bf3..11d37f96ce71 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -534,6 +534,48 @@ transcoder_has_psr2(struct drm_i915_private *dev_priv, enum transcoder trans) > > return trans == TRANSCODER_EDP; > > } > > > > +static void psr2_program_idle_frames(struct drm_i915_private *dev_priv, > > + u32 idle_frames) > > +{ > > + u32 val; > > + > > + idle_frames <<= EDP_PSR2_IDLE_FRAME_SHIFT; > > + val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)); > > + val &= ~EDP_PSR2_IDLE_FRAME_MASK; > > + val |= idle_frames; > > + I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val); > > +} > > + > > +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv) > > +{ > > + int idle_frames = 0; > > No need for a variable for that. > > > + > > + psr2_program_idle_frames(dev_priv, idle_frames); > > +} > > + > > +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv) > > +{ > > + int idle_frames; > > + > > + /* > > + * Let's use 6 as the minimum to cover all known cases including the > > + * off-by-one issue that HW has in some cases. > > + */ > > + idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > > + idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1); > > + psr2_program_idle_frames(dev_priv, idle_frames); > > +} > > + > > +static void tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv) > > +{ > > + if (!IS_TIGERLAKE(dev_priv)) > > + return; > > Could we use crtc_state->dc3co_exitline instead (which we would set only > whenever we want to enable DC3co)? > > > + > > + cancel_delayed_work(&dev_priv->csr.idle_work); > > + /* Before PSR2 exit disallow dc3co*/ > > + tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6); > > +} > > + > > static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > > struct intel_crtc_state *crtc_state) > > { > > @@ -799,6 +841,10 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > > > WARN_ON(dev_priv->drrs.dp); > > > > + /* Enable PSR2 transcoder exit line */ > > + if (crtc_state->has_psr2) > > + tgl_enable_psr2_transcoder_exitline(crtc_state); > > This is too late to program the EXITLINE reg, since the transcoder is > already enabled. intel_psr_enable/disable called by encoder enable/disable fucntion. AFAIR u seemed to had opinion call it from intel_psr_enable() https://patchwork.freedesktop.org/patch/323128/?series=64923&rev=3 How about calling these from encoder pre_enable functions ? or any other better place apart from haswell_crtc_enable/disable. > > > + > > mutex_lock(&dev_priv->psr.lock); > > > > if (!psr_global_enabled(dev_priv->psr.debug)) { > > @@ -829,6 +875,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv) > > } > > > > if (dev_priv->psr.psr2_enabled) { > > + tgl_disallow_dc3co_on_psr2_exit(dev_priv); > > So DC5/6 will be reenabled, but I can't see how PSR deep sleep gets > reenabled. I think we should have a function that we call both from here > and the idle thread that reenables DC5/6 and also PSR deep sleep. IMO we don't need to enable PSR2 deep sleep when PSR gets exit, no use of it. It will always get enabled by intel_psr_activate()->hsw_activate_psr2(). Thanks, Anshuman Gupta. > > > val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)); > > WARN_ON(!(val & EDP_PSR2_ENABLE)); > > val &= ~EDP_PSR2_ENABLE; > > @@ -895,6 +942,10 @@ void intel_psr_disable(struct intel_dp *intel_dp, > > if (WARN_ON(!CAN_PSR(dev_priv))) > > return; > > > > + /* Disable PSR2 transcoder exit line */ > > + if (old_crtc_state->has_psr2) > > + tgl_disable_psr2_transcoder_exitline(old_crtc_state); > > The transcoder is still active here so we can't program the EXITLINE reg > at this point. > > Please add tgl_enable/disable_psr2_transcoder_exitline() in this patch > where the functions are actually used, the review is difficult when I > have to jump between the two patches. > > OTOH tgl_psr2_deep_sleep_enable/disable() is used in a follow-up patch, > but you define them already here; those should be also moved where they > are used for easier review. > > > + > > mutex_lock(&dev_priv->psr.lock); > > > > intel_psr_disable_locked(intel_dp); > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h > > index 46e4de8b8cd5..75a9862f36fd 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.h > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > > @@ -35,5 +35,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp); > > int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state, > > u32 *out_value); > > bool intel_psr_enabled(struct intel_dp *intel_dp); > > +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv); > > +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv); > > > > #endif /* __INTEL_PSR_H__ */ > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 68fb732c24c8..4521b9381db3 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -494,6 +494,7 @@ struct i915_psr { > > bool link_standby; > > bool colorimetry_support; > > bool psr2_enabled; > > + bool psr2_deep_slp_disabled; > > u8 sink_sync_latency; > > ktime_t last_entry_attempt; > > ktime_t last_exit; > > -- > > 2.21.0 > >
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index b3c7eef53bf3..11d37f96ce71 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -534,6 +534,48 @@ transcoder_has_psr2(struct drm_i915_private *dev_priv, enum transcoder trans) return trans == TRANSCODER_EDP; } +static void psr2_program_idle_frames(struct drm_i915_private *dev_priv, + u32 idle_frames) +{ + u32 val; + + idle_frames <<= EDP_PSR2_IDLE_FRAME_SHIFT; + val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)); + val &= ~EDP_PSR2_IDLE_FRAME_MASK; + val |= idle_frames; + I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val); +} + +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv) +{ + int idle_frames = 0; + + psr2_program_idle_frames(dev_priv, idle_frames); +} + +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv) +{ + int idle_frames; + + /* + * Let's use 6 as the minimum to cover all known cases including the + * off-by-one issue that HW has in some cases. + */ + idle_frames = max(6, dev_priv->vbt.psr.idle_frames); + idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1); + psr2_program_idle_frames(dev_priv, idle_frames); +} + +static void tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv) +{ + if (!IS_TIGERLAKE(dev_priv)) + return; + + cancel_delayed_work(&dev_priv->csr.idle_work); + /* Before PSR2 exit disallow dc3co*/ + tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6); +} + static bool intel_psr2_config_valid(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state) { @@ -799,6 +841,10 @@ void intel_psr_enable(struct intel_dp *intel_dp, WARN_ON(dev_priv->drrs.dp); + /* Enable PSR2 transcoder exit line */ + if (crtc_state->has_psr2) + tgl_enable_psr2_transcoder_exitline(crtc_state); + mutex_lock(&dev_priv->psr.lock); if (!psr_global_enabled(dev_priv->psr.debug)) { @@ -829,6 +875,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv) } if (dev_priv->psr.psr2_enabled) { + tgl_disallow_dc3co_on_psr2_exit(dev_priv); val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)); WARN_ON(!(val & EDP_PSR2_ENABLE)); val &= ~EDP_PSR2_ENABLE; @@ -895,6 +942,10 @@ void intel_psr_disable(struct intel_dp *intel_dp, if (WARN_ON(!CAN_PSR(dev_priv))) return; + /* Disable PSR2 transcoder exit line */ + if (old_crtc_state->has_psr2) + tgl_disable_psr2_transcoder_exitline(old_crtc_state); + mutex_lock(&dev_priv->psr.lock); intel_psr_disable_locked(intel_dp); diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h index 46e4de8b8cd5..75a9862f36fd 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.h +++ b/drivers/gpu/drm/i915/display/intel_psr.h @@ -35,5 +35,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp); int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state, u32 *out_value); bool intel_psr_enabled(struct intel_dp *intel_dp); +void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv); +void tgl_psr2_deep_sleep_enable(struct drm_i915_private *dev_priv); #endif /* __INTEL_PSR_H__ */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 68fb732c24c8..4521b9381db3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -494,6 +494,7 @@ struct i915_psr { bool link_standby; bool colorimetry_support; bool psr2_enabled; + bool psr2_deep_slp_disabled; u8 sink_sync_latency; ktime_t last_entry_attempt; ktime_t last_exit;
Add dc3co helper functions to enable/disable psr2 deep sleep. Adhere B.Specs by disallow DC3CO state before PSR2 exit. Enable PSR2 exitline event and program the desired scanlines to exit DC3CO in intel_psr_enable function at modeset path. Disable the DC3CO exitline in order to maintian consistent pipe config state in encoder disable path. v1: Moved calling of tgl_enable_psr2_transcoder_exitline() to intel_psr_enable(). [Imre] Cc: Jani Nikula <jani.nikula@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Animesh Manna <animesh.manna@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 51 ++++++++++++++++++++++++ drivers/gpu/drm/i915/display/intel_psr.h | 2 + drivers/gpu/drm/i915/i915_drv.h | 1 + 3 files changed, 54 insertions(+)