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@xxxxxxxxx> > > Cc: Imre Deak <imre.deak@xxxxxxxxx> > > Cc: Animesh Manna <animesh.manna@xxxxxxxxx> > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx> > > 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 > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
