Re: [PATCH v8 6/7] drm/i915/tgl: switch between dc3co and dc5 based on display idleness

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux