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.
> ---
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
