On Mon, Sep 30, 2019 at 11:11:36PM +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]
> v6: Used dc3co_exitline check instead of TGL and dc3co allowed_dc_mask
> checks, used delayed_work_pending with the psr lock and removed the
> psr2_deep_slp_disabled flag. [Imre]
> v7: Code refactoring moved the most of functional code to inte_psr.c [Imre]
> Using frontbuffer_bits on psr.pipe check instead of
> busy_frontbuffer_bits. [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 | 45 ++++++++
> .../drm/i915/display/intel_display_power.h | 2 +
> drivers/gpu/drm/i915/display/intel_psr.c | 109 +++++++++++++++++-
> drivers/gpu/drm/i915/i915_drv.h | 2 +
> 4 files changed, 157 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 67ba92dd8366..9fddebfda169 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -886,6 +886,51 @@ lookup_power_well(struct drm_i915_private *dev_priv,
> return &dev_priv->power_domains.power_wells[0];
> }
>
> +/**
> + * intel_display_power_set_target_dc_state - Set target dc state.
> + * @dev_priv: i915 device
> + * @state: state which needs to be set as target_dc_state.
> + *
> + * This function set the "DC off" power well target_dc_state,
> + * based upon this target_dc_stste, "DC off" power well will
> + * enable desired DC state.
> + */
> +void intel_display_power_set_target_dc_state(struct drm_i915_private *dev_priv,
> + u32 state)
> +{
> + struct i915_power_well *power_well;
> + bool dc_off_enabled;
> + struct i915_power_domains *power_domains = &dev_priv->power_domains;
> +
> + mutex_lock(&power_domains->lock);
> + power_well = lookup_power_well(dev_priv, SKL_DISP_DC_OFF);
> +
> + if (WARN_ON(!power_well))
> + goto unlock;
> +
> + state = sanitize_target_dc_state(dev_priv, state);
> +
> + if (state == dev_priv->csr.target_dc_state)
> + goto unlock;
> +
> + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> + power_well);
> + /*
> + * If DC off power well is disabled, need to enable and disable the
> + * DC off power well to effect target DC state.
> + */
> + if (!dc_off_enabled)
> + power_well->desc->ops->enable(dev_priv, power_well);
> +
> + dev_priv->csr.target_dc_state = state;
> +
> + if (!dc_off_enabled)
> + power_well->desc->ops->disable(dev_priv, power_well);
> +
> +unlock:
> + mutex_unlock(&power_domains->lock);
> +}
> +
> static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
> {
> bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 7d72faf474b2..1da04f3e0fb3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -257,6 +257,8 @@ void intel_display_power_suspend_late(struct drm_i915_private *i915);
> void intel_display_power_resume_early(struct drm_i915_private *i915);
> void intel_display_power_suspend(struct drm_i915_private *i915);
> void intel_display_power_resume(struct drm_i915_private *i915);
> +void intel_display_power_set_target_dc_state(struct drm_i915_private *dev_priv,
> + u32 state);
>
> const char *
> intel_display_power_domain_str(enum intel_display_power_domain domain);
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index b3c7eef53bf3..6a6f1031d29b 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -534,6 +534,68 @@ 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);
> +}
Some minor issues, could be also addressed as a follow-up:
> +
> +static void tgl_psr2_deep_sleep_disable(struct drm_i915_private *dev_priv)
> +{
This is better named tgl_psr2_enable_dc3co(), moving the
intel_display_power_set_target_dc_state(dev_priv, DC_STATE_EN_DC3CO);
call to this function too.
> + psr2_program_idle_frames(dev_priv, 0);
> +}
> +
> +static 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_enable_psr2_deep_sleep_dc6(struct drm_i915_private *dev_priv)
> +{
This is better named tgl_psr2_disable_dc3co().
> + intel_display_power_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> + tgl_psr2_deep_sleep_enable(dev_priv);
tgl_psr2_deep_sleep_enable() could be inlined here, since there is no
other user for it. That would make the function more symmetric with
tgl_psr2_enable_dc3co().
> +}
> +
> +static void tgl_dc5_idle_thread(struct work_struct *work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(work, typeof(*dev_priv), psr.idle_work.work);
> +
> + mutex_lock(&dev_priv->psr.lock);
> + /* If delayed work is pending, it is not idle */
> + if (delayed_work_pending(&dev_priv->psr.idle_work))
> + goto unlock;
> +
> + DRM_DEBUG_KMS("DC5/6 idle thread\n");
> + tgl_enable_psr2_deep_sleep_dc6(dev_priv);
> +unlock:
> + mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +static void tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv)
> +{
> + if (!dev_priv->psr.dc3co_exitline)
> + return;
> +
> + cancel_delayed_work(&dev_priv->psr.idle_work);
> + /* Before PSR2 exit disallow dc3co*/
> + tgl_enable_psr2_deep_sleep_dc6(dev_priv);
> +}
> +
> static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> struct intel_crtc_state *crtc_state)
> {
> @@ -746,6 +808,7 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
> dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
> dev_priv->psr.busy_frontbuffer_bits = 0;
> dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
> + dev_priv->psr.dc3co_exitline = crtc_state->dc3co_exitline;
> dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
>
> /*
> @@ -829,6 +892,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;
> @@ -901,6 +965,7 @@ void intel_psr_disable(struct intel_dp *intel_dp,
>
> mutex_unlock(&dev_priv->psr.lock);
> cancel_work_sync(&dev_priv->psr.work);
> + cancel_delayed_work_sync(&dev_priv->psr.idle_work);
> }
>
> static void psr_force_hw_tracking_exit(struct drm_i915_private *dev_priv)
> @@ -1208,6 +1273,45 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> mutex_unlock(&dev_priv->psr.lock);
> }
>
> +/*
> + * When we will be completely rely on PSR2 S/W tracking in future,
> + * intel_psr_flush() will invalidate and flush the PSR for ORIGIN_FLIP
> + * event also therefore tgl_dc3co_flush() require to be changed
> + * accrodingly in future.
> + */
> +static void
> +tgl_dc3co_flush(struct drm_i915_private *dev_priv,
> + unsigned int frontbuffer_bits, enum fb_op_origin origin)
> +{
> + u32 delay;
> +
> + mutex_lock(&dev_priv->psr.lock);
> +
> + if (!dev_priv->psr.dc3co_exitline)
> + goto unlock;
> +
> + if (!dev_priv->psr.psr2_enabled || !dev_priv->psr.active)
> + goto unlock;
> +
> + /*
> + * At every frontbuffer flush flip event modified delay of delayed work,
> + * when delayed work schedules that means display has been idle.
> + */
> + if (!(frontbuffer_bits &
> + INTEL_FRONTBUFFER_ALL_MASK(dev_priv->psr.pipe)))
> + goto unlock;
> +
> + tgl_psr2_deep_sleep_disable(dev_priv);
> + intel_display_power_set_target_dc_state(dev_priv, DC_STATE_EN_DC3CO);
> + /* DC5/DC6 required idle frames = 6 */
> + delay = 6 * dev_priv->psr.dc3co_frame_time_us;
> + mod_delayed_work(system_wq, &dev_priv->psr.idle_work,
> + usecs_to_jiffies(delay));
> +
> +unlock:
> + mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> /**
> * intel_psr_flush - Flush PSR
> * @dev_priv: i915 device
> @@ -1227,8 +1331,10 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> if (!CAN_PSR(dev_priv))
> return;
>
> - if (origin == ORIGIN_FLIP)
> + if (origin == ORIGIN_FLIP) {
> + tgl_dc3co_flush(dev_priv, frontbuffer_bits, origin);
> return;
> + }
>
> mutex_lock(&dev_priv->psr.lock);
> if (!dev_priv->psr.enabled) {
> @@ -1284,6 +1390,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
>
> INIT_WORK(&dev_priv->psr.work, intel_psr_work);
> + INIT_DELAYED_WORK(&dev_priv->psr.idle_work, tgl_dc5_idle_thread);
> mutex_init(&dev_priv->psr.lock);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7b2318c5c7a0..980af06a0607 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -502,6 +502,8 @@ struct i915_psr {
> bool sink_not_reliable;
> bool irq_aux_error;
> u16 su_x_granularity;
> + u32 dc3co_exitline;
> + struct delayed_work idle_work;
> };
>
> #define QUIRK_LVDS_SSC_DISABLE (1<<1)
> --
> 2.21.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
