On Thu, Sep 26, 2019 at 08:26:20PM +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: Inited the busy_frontbuffer_bits, 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]
>
> 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 | 84 +++++++++++++++++++
> .../drm/i915/display/intel_display_power.h | 4 +
> .../gpu/drm/i915/display/intel_frontbuffer.c | 1 +
> drivers/gpu/drm/i915/display/intel_psr.c | 34 ++++++++
> drivers/gpu/drm/i915/display/intel_psr.h | 2 +
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> 6 files changed, 126 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 84e4cfd95b43..00bb82e6a18f 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);
> @@ -797,6 +798,9 @@ void tgl_clear_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> val = I915_READ(EXITLINE(cstate->cpu_transcoder));
> val &= ~(EXITLINE_MASK | 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);
Let's move this to intel_psr_disable_locked(), for symmetry with the
cancel_work in intel_psr_exit().
> }
>
> void tgl_set_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> @@ -816,6 +820,19 @@ void tgl_set_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
> }
>
> +static u32 intel_get_frame_time_us(const struct intel_crtc_state *cstate)
> +{
> + u32 frametime_us;
> +
> + if (!cstate || !cstate->base.active)
> + return 0;
> +
> + frametime_us = DIV_ROUND_UP(1000*1000,
> + drm_mode_vrefresh(&cstate->base.adjusted_mode));
Just return the value directly, and leave spaces around operators (see
Documentation/process/coding-style.rst 3.1). Let's move the helper where
it'll be used (intel_ddi.c).
> +
> + return frametime_us;
> +}
> +
> /*
> * DC3CO requires to enable exitline and program DC3CO requires
> * exit scanlines to TRANS_EXITLINE register, which should only
> @@ -872,6 +889,72 @@ void tgl_dc3co_exitline_get_config(struct intel_crtc_state *crtc_state)
> crtc_state->dc3co_exitline = val & EXITLINE_MASK;
> }
>
> +/*
> + * 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;
> +
> + if (!CAN_PSR(dev_priv))
> + return;
> +
> + if (origin != ORIGIN_FLIP)
> + return;
> +
> + mutex_lock(&dev_priv->psr.lock);
> +
> + if (!dev_priv->psr.dc3co_exitline)
> + goto unlock;
> +
> + crtc = intel_get_crtc_for_pipe(dev_priv, dev_priv->psr.pipe);
> + cstate = to_intel_crtc_state(crtc->base.state);
Using crtc->state is verboten in general (except for the atomic code
that uses it as a starting point of a new commit). So let's compute the
frame time too in the compute hook along with exitline and store it in
dev->psr.
> +
> + 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.
> + */
We only want to act on the flip event if it happened on psr.pipe. So
if (!(frontbuffer_bits &
INTEL_FRONTBUFFER_ALL_MASK(dev_priv->psr.pipe)))
goto unlock;
> + tgl_psr2_deep_sleep_disable(dev_priv);
> + 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);
> +}
> +
> +void tgl_enable_psr2_deep_sleep_dc6(struct drm_i915_private *dev_priv)
> +{
> + tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
> + tgl_psr2_deep_sleep_enable(dev_priv);
> +}
> +
> +static 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);
> +
> + mutex_lock(&dev_priv->psr.lock);
> + /* If delayed work is pending, it is not idle */
> + if (delayed_work_pending(&dev_priv->csr.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
> allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv)
> {
> @@ -4241,6 +4324,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);
Let's move csr.idle_work to psr.idle_work and all the above
funcs/changes you added now in intel_display_power.c to intel_psr.c, as
all of it is psr specific.
>
> /*
> * 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 4e76f93bbee5..0ebdeab26d8e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -10,6 +10,7 @@
> #include "intel_runtime_pm.h"
> #include "intel_sprite.h"
> #include "i915_reg.h"
> +#include "intel_frontbuffer.h"
>
> struct drm_i915_private;
> struct intel_encoder;
> @@ -266,6 +267,9 @@ void tgl_dc3co_exitline_compute_config(struct intel_encoder *encoder,
> void tgl_dc3co_exitline_get_config(struct intel_crtc_state *crtc_state);
> void tgl_clear_psr2_transcoder_exitline(const struct intel_crtc_state *state);
> void tgl_set_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_enable_psr2_deep_sleep_dc6(struct drm_i915_private *dev_priv);
>
> 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);
Let's call the above from intel_psr_flush() instead of having to export
it.
> 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 bf0b741d3243..5faaf35ba4ff 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -534,10 +534,44 @@ 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)
> +{
> + psr2_program_idle_frames(dev_priv, 0);
> +}
> +
> +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 (!dev_priv->psr.dc3co_exitline)
> return;
> +
> + cancel_delayed_work(&dev_priv->csr.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,
> 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 c3aba078262f..ed3dd4757f72 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -340,6 +340,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
