On Fri, Sep 13, 2019 at 01:53:38PM +0530, Anshuman Gupta wrote:
> DC3CO is useful power state, when DMC detects PSR2 idle frame
> while an active video playback, playing 30fps video on 60hz panel
> is the classic example of this use case.
>
> B.Specs:49196 has a restriction to enable DC3CO only for Video Playback.
> It will be worthy to enable DC3CO after completion of each pageflip
> and switch back to DC5 when display is idle because driver doesn't
> differentiate between video playback and a normal pageflip.
> We will use Frontbuffer flush call tgl_dc3co_flush() to enable DC3CO
> state only for ORIGIN_FLIP flush call, because DC3CO state has primarily
> targeted for VPB use case. We are not interested here for frontbuffer
> invalidates calls because that triggers PSR2 exit, which will
> explicitly disable DC3CO.
>
> DC5 and DC6 saves more power, but can't be entered during video
> playback because there are not enough idle frames in a row to meet
> most PSR2 panel deep sleep entry requirement typically 4 frames.
> As PSR2 existing implementation is using minimum 6 idle frames for
> deep sleep, it is safer to enable DC5/6 after 6 idle frames
> (By scheduling a delayed work of 6 idle frames, once DC3CO has been
> enabled after a pageflip).
>
> After manually waiting for 6 idle frames DC5/6 will be enabled and
> PSR2 deep sleep idle frames will be restored to 6 idle frames, at this
> point DMC will triggers DC5/6 once PSR2 enters to deep sleep after
> 6 idle frames.
> In future when we will enable S/W PSR2 tracking, we can change the
> PSR2 required deep sleep idle frames to 1 so DMC can trigger the
> DC5/6 immediately after S/W manual waiting of 6 idle frames get
> complete.
>
> v2: calculated s/w state to switch over dc3co when there is an
> update. [Imre]
> Used cancel_delayed_work_sync() in order to avoid any race
> with already scheduled delayed work. [Imre]
> v3: Cancel_delayed_work_sync() may blocked the commit work.
> hence dropping it, dc5_idle_thread() checks the valid wakeref before
> putting the reference count, which avoids any chances of dropping
> a zero wakeref. [Imre (IRC)]
> v4: Used frontbuffer flush mechanism. [Imre]
> v5: Used psr.pipe to extract frontbuffer busy bits. [Imre]
> Used cancel_delayed_work_sync() in encoder disable path. [Imre]
> Used mod_delayed_work() instead of cancelling and scheduling a
> delayed work. [Imre]
> Used psr.lock in tgl_dc5_idle_thread() to enable psr2 deep
> sleep. [Imre]
> Removed DC5_REQ_IDLE_FRAMES macro. [Imre]
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Animesh Manna <animesh.manna@xxxxxxxxx>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> ---
> .../drm/i915/display/intel_display_power.c | 97 +++++++++++++++++++
> .../drm/i915/display/intel_display_power.h | 4 +
> .../gpu/drm/i915/display/intel_frontbuffer.c | 1 +
> drivers/gpu/drm/i915/display/intel_psr.c | 1 +
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> 5 files changed, 104 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 42eabcdecf00..a605047cb28a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -20,6 +20,7 @@
> #include "intel_sideband.h"
> #include "intel_tc.h"
> #include "intel_pm.h"
> +#include "intel_psr.h"
>
> bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> enum i915_power_well_id power_well_id);
> @@ -773,6 +774,27 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
> dev_priv->csr.dc_state = val & mask;
> }
>
> +static u32 intel_get_frame_time_us(const struct intel_crtc_state *cstate)
> +{
> + u32 pixel_rate, crtc_htotal, crtc_vtotal;
> + u32 frametime_us;
> +
> + if (!cstate || !cstate->base.active)
> + return 0;
> +
> + pixel_rate = cstate->pixel_rate;
> +
> + if (WARN_ON(pixel_rate == 0))
> + return 0;
> +
> + crtc_htotal = cstate->base.adjusted_mode.crtc_htotal;
> + crtc_vtotal = cstate->base.adjusted_mode.crtc_vtotal;
> + frametime_us = DIV_ROUND_UP(crtc_htotal * crtc_vtotal * 1000ULL,
> + pixel_rate);
> +
> + return frametime_us;
> +}
> +
> void tgl_disable_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> {
> struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> @@ -784,6 +806,9 @@ void tgl_disable_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> val = I915_READ(EXITLINE(cstate->cpu_transcoder));
> val &= ~EXITLINE_ENABLE;
> I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
> +
> + /* As psr2 encoder has disabled, cancel the dc5 idle delayed work */
> + cancel_delayed_work_sync(&dev_priv->csr.idle_work);
> }
>
> void tgl_enable_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> @@ -817,6 +842,60 @@ void tgl_enable_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
> }
>
> +/*
> + * When we will enable manual PSR2 S/W tracking in future
> + * we will implement this entire DC3CO flush logic in
> + * intel_psr_flush().
> + */
> +void tgl_dc3co_flush(struct drm_i915_private *dev_priv,
> + unsigned int frontbuffer_bits, enum fb_op_origin origin)
> +{
> + struct intel_crtc_state *cstate;
> + struct intel_crtc *crtc;
> + u32 delay;
> + unsigned int busy_frontbuffer_bits;
> +
> + if (!IS_TIGERLAKE(dev_priv))
> + return;
> +
> + if (!(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO))
> + return;
Could we use crtc_state->dc3co_exitline instead of the two checks above?
> +
> + if (origin != ORIGIN_FLIP)
> + return;
> +
> + mutex_lock(&dev_priv->psr.lock);
> + crtc = intel_get_crtc_for_pipe(dev_priv, dev_priv->psr.pipe);
> + cstate = to_intel_crtc_state(crtc->base.state);
> +
> + frontbuffer_bits &=
> + INTEL_FRONTBUFFER_ALL_MASK(dev_priv->psr.pipe);
> +
> + busy_frontbuffer_bits &= ~frontbuffer_bits;
This is still wrong, busy_frontbuffer_bits is not inited.
> +
> + if (!dev_priv->psr.psr2_enabled || !dev_priv->psr.active)
> + goto unlock;
> +
> + /*
> + * At every flip frontbuffer flush modified delay of delayed work,
> + * when delayed schedules that means display has been idle.
> + */
> + if (!busy_frontbuffer_bits) {
> + if (!dev_priv->psr.psr2_deep_slp_disabled) {
I think reenabling PSR deep sleep if it was already enabled is not a
problem, we don't need a flag just to avoid that.
> + tgl_psr2_deep_sleep_disable(dev_priv);
> + dev_priv->psr.psr2_deep_slp_disabled = true;
> + }
> + tgl_set_target_dc_state(dev_priv, DC_STATE_EN_DC3CO);
> + /* DC5/DC6 required idle frames = 6 */
> + delay = 6 * intel_get_frame_time_us(cstate);
> + mod_delayed_work(system_wq, &dev_priv->csr.idle_work,
> + usecs_to_jiffies(delay));
> + }
> +
> +unlock:
> + mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> static bool tgl_dc3co_is_edp_connected(struct intel_crtc_state *crtc_state)
> {
> struct drm_atomic_state *state = crtc_state->base.state;
> @@ -876,6 +955,23 @@ void tgl_dc3co_crtc_get_config(struct intel_crtc_state *crtc_state)
> crtc_state->has_dc3co_exitline = true;
> }
>
> +void tgl_dc5_idle_thread(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, typeof(*dev_priv), csr.idle_work.work);
> +
> + /* If delayed work is pending, it is not idle */
> + if (delayed_work_pending(&dev_priv->csr.idle_work))
> + return;
The above should be checked with the psr.lock held to avoid a race.
> +
> + DRM_DEBUG_KMS("DC5/6 idle thread\n");
> + mutex_lock(&dev_priv->psr.lock);
> + tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> + tgl_psr2_deep_sleep_enable(dev_priv);
> + dev_priv->psr.psr2_deep_slp_disabled = false;
> + mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> static void
> allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv)
> {
> @@ -4237,6 +4333,7 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>
> INIT_DELAYED_WORK(&power_domains->async_put_work,
> intel_display_power_put_async_work);
> + INIT_DELAYED_WORK(&dev_priv->csr.idle_work, tgl_dc5_idle_thread);
>
> /*
> * The enabling order will be from lower to higher indexed wells,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index bf68f26a5a37..87725a23fead 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -9,6 +9,7 @@
> #include "intel_display.h"
> #include "intel_runtime_pm.h"
> #include "i915_reg.h"
> +#include "intel_frontbuffer.h"
>
> struct drm_i915_private;
> struct intel_encoder;
> @@ -265,6 +266,9 @@ void tgl_dc3co_crtc_compute_config(struct drm_i915_private *dev_priv,
> void tgl_dc3co_crtc_get_config(struct intel_crtc_state *crtc_state);
> void tgl_disable_psr2_transcoder_exitline(const struct intel_crtc_state *state);
> void tgl_enable_psr2_transcoder_exitline(const struct intel_crtc_state *state);
> +void tgl_dc3co_flush(struct drm_i915_private *dev_priv,
> + unsigned int frontbuffer_bits, enum fb_op_origin origin);
> +void tgl_dc5_idle_thread(struct work_struct *work);
>
> const char *
> intel_display_power_domain_str(enum intel_display_power_domain domain);
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> index fc40dc1fdbcc..c3b10f6e4382 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> @@ -90,6 +90,7 @@ static void frontbuffer_flush(struct drm_i915_private *i915,
> might_sleep();
> intel_edp_drrs_flush(i915, frontbuffer_bits);
> intel_psr_flush(i915, frontbuffer_bits, origin);
> + tgl_dc3co_flush(i915, frontbuffer_bits, origin);
> intel_fbc_flush(i915, frontbuffer_bits, origin);
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 11d37f96ce71..552eaf2bd16f 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -574,6 +574,7 @@ static void tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv)
> 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);
> + dev_priv->psr.psr2_deep_slp_disabled = false;
> }
>
> static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4521b9381db3..25591c128c59 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -339,6 +339,7 @@ struct intel_csr {
> u32 dc_state;
> u32 target_dc_state;
> u32 allowed_dc_mask;
> + struct delayed_work idle_work;
> intel_wakeref_t wakeref;
> };
>
> --
> 2.21.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
