On Fri, Sep 13, 2019 at 01:53:35PM +0530, Anshuman Gupta wrote:
> Add target_dc_state and tgl_set_target_dc_state() API
> in order to enable DC3CO state with existing DC states.
> target_dc_state will enable/disable the desired DC state in
> DC_STATE_EN reg when "DC Off" power well gets disable/enable.
>
> v2: commit log improvement.
> v3: Used intel_wait_for_register to wait for DC3CO exit. [Imre]
> Used gen9_set_dc_state() to allow/disallow DC3CO. [Imre]
> Moved transcoder psr2 exit line enablement from tgl_allow_dc3co()
> to a appropriate place haswell_crtc_enable(). [Imre]
> Changed the DC3CO power well enabled call back logic as
> recommended in review comments. [Imre]
> v4: Used wait_for_us() instead of intel_wait_for_reg(). [Imre (IRC)]
> v5: using udelay() instead of waiting for DC3CO exit status.
> v6: Fixed minor unwanted change.
> v7: Removed DC3CO powerwell and POWER_DOMAIN_VIDEO.
> v8: Uniform checks by using only target_dc_state instead of allowed_dc_mask
> in "DC off" power well callback. [Imre]
> Adding "DC off" power well id to older platforms. [Imre]
> Removed psr2_deep_sleep flag from tgl_set_target_dc_state. [Imre]
>
> Cc: Jani Nikula <[email protected]>
> Cc: Imre Deak <[email protected]>
> Cc: Animesh Manna <[email protected]>
> Signed-off-by: Anshuman Gupta <[email protected]>
> ---
> .../drm/i915/display/intel_display_power.c | 102 ++++++++++++++++--
> .../drm/i915/display/intel_display_power.h | 2 +
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> 3 files changed, 96 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 24cd9320ad4c..7965e07257a0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -772,6 +772,36 @@ static void gen9_set_dc_state(struct drm_i915_private
> *dev_priv, u32 state)
> dev_priv->csr.dc_state = val & mask;
> }
>
> +static void
> +allowed_dc_mask_to_target_dc_state(struct drm_i915_private *dev_priv)
> +{
> + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> + dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC6;
> + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> + dev_priv->csr.target_dc_state = DC_STATE_EN_UPTO_DC5;
> +}
> +
> +static void tgl_enable_dc3co(struct drm_i915_private *dev_priv)
> +{
> + DRM_DEBUG_KMS("Enabling DC3CO\n");
> + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO);
> +}
> +
> +static void tgl_disable_dc3co(struct drm_i915_private *dev_priv)
> +{
> + u32 val;
> +
> + DRM_DEBUG_KMS("Disabling DC3CO\n");
> + val = I915_READ(DC_STATE_EN);
> + val &= ~DC_STATE_DC3CO_STATUS;
> + I915_WRITE(DC_STATE_EN, val);
> + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> + /*
> + * Delay of 200us DC3CO Exit time B.Spec 49196
> + */
> + udelay(200);
> +}
> +
> static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
> {
> assert_can_enable_dc9(dev_priv);
> @@ -939,7 +969,8 @@ static void bxt_verify_ddi_phy_power_wells(struct
> drm_i915_private *dev_priv)
> static bool gen9_dc_off_power_well_enabled(struct drm_i915_private *dev_priv,
> struct i915_power_well *power_well)
> {
> - return (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0;
> + return ((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC3CO) == 0 &&
> + (I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5_DC6_MASK) == 0);
> }
>
> static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
> @@ -955,6 +986,11 @@ static void gen9_disable_dc_states(struct
> drm_i915_private *dev_priv)
> {
> struct intel_cdclk_state cdclk_state = {};
>
> + if (dev_priv->csr.target_dc_state == DC_STATE_EN_DC3CO) {
> + tgl_disable_dc3co(dev_priv);
> + return;
> + }
> +
> gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>
> dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> @@ -987,12 +1023,59 @@ static void gen9_dc_off_power_well_disable(struct
> drm_i915_private *dev_priv,
> if (!dev_priv->csr.dmc_payload)
> return;
>
Using a switch here would be clearer.
> - if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> + if (dev_priv->csr.target_dc_state == DC_STATE_EN_DC3CO)
> + tgl_enable_dc3co(dev_priv);
> + else if (dev_priv->csr.target_dc_state == DC_STATE_EN_UPTO_DC6)
> skl_enable_dc6(dev_priv);
> - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> + else if (dev_priv->csr.target_dc_state == DC_STATE_EN_UPTO_DC5)
> gen9_enable_dc5(dev_priv);
> }
>
> +void tgl_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 (!power_well)
WARN_ON() on the above condition, since it shouldn't happen?
> + goto unlock;
> +
> + if (state == dev_priv->csr.target_dc_state)
> + goto unlock;
> +
> + /*
> + * Compute the DC5/6 state wrt to the permisisble allowed dc mask.
> + * DC3CO allowed mask has already been checked in tgl_dc3co_flush
> + * It doesn't need to check here again.
It would be better not to special case DC3co here, check it too against
allowed_dc_mask and bail out afterwards if the adjusted state equals
target_dc_state.
> + */
> + if (state != DC_STATE_EN_DC3CO) {
> + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> + state = DC_STATE_EN_UPTO_DC6;
> + else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> + state = DC_STATE_EN_UPTO_DC5;
> + }
> +
> + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> + power_well);
> + if (!dc_off_enabled) {
> + /*
> + * Need to disable the DC off power well to
> + * effect target DC state.
> + */
> + power_well->desc->ops->enable(dev_priv, power_well);
> + dev_priv->csr.target_dc_state = state;
> + power_well->desc->ops->disable(dev_priv, power_well);
> + goto unlock;
> + }
> + dev_priv->csr.target_dc_state = state;
Would be clearer by:
if (dc_off_was_disabled)
power_well->enable();
target_dc_state = state;
if (dc_off_was_disabled)
power_Well->disable();
> +
> +unlock:
> + mutex_unlock(&power_domains->lock);
> +}
> +
> static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> struct i915_power_well *power_well)
> {
> @@ -2938,7 +3021,7 @@ static const struct i915_power_well_desc
> skl_power_wells[] = {
> .name = "DC off",
> .domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
> .ops = &gen9_dc_off_power_well_ops,
> - .id = DISP_PW_ID_NONE,
> + .id = SKL_DISP_DC_OFF,
> },
> {
> .name = "power well 2",
> @@ -3020,7 +3103,7 @@ static const struct i915_power_well_desc
> bxt_power_wells[] = {
> .name = "DC off",
> .domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
> .ops = &gen9_dc_off_power_well_ops,
> - .id = DISP_PW_ID_NONE,
> + .id = SKL_DISP_DC_OFF,
> },
> {
> .name = "power well 2",
> @@ -3080,7 +3163,7 @@ static const struct i915_power_well_desc
> glk_power_wells[] = {
> .name = "DC off",
> .domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS,
> .ops = &gen9_dc_off_power_well_ops,
> - .id = DISP_PW_ID_NONE,
> + .id = SKL_DISP_DC_OFF,
> },
> {
> .name = "power well 2",
> @@ -3249,7 +3332,7 @@ static const struct i915_power_well_desc
> cnl_power_wells[] = {
> .name = "DC off",
> .domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS,
> .ops = &gen9_dc_off_power_well_ops,
> - .id = DISP_PW_ID_NONE,
> + .id = SKL_DISP_DC_OFF,
> },
> {
> .name = "power well 2",
> @@ -3377,7 +3460,7 @@ static const struct i915_power_well_desc
> icl_power_wells[] = {
> .name = "DC off",
> .domains = ICL_DISPLAY_DC_OFF_POWER_DOMAINS,
> .ops = &gen9_dc_off_power_well_ops,
> - .id = DISP_PW_ID_NONE,
> + .id = SKL_DISP_DC_OFF,
> },
> {
> .name = "power well 2",
> @@ -3610,7 +3693,7 @@ static const struct i915_power_well_desc
> tgl_power_wells[] = {
> .name = "DC off",
> .domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
> .ops = &gen9_dc_off_power_well_ops,
> - .id = DISP_PW_ID_NONE,
> + .id = SKL_DISP_DC_OFF,
> },
> {
> .name = "power well 2",
> @@ -4043,6 +4126,7 @@ int intel_power_domains_init(struct drm_i915_private
> *dev_priv)
> dev_priv->csr.allowed_dc_mask =
> get_allowed_dc_mask(dev_priv, i915_modparams.enable_dc);
>
> + allowed_dc_mask_to_target_dc_state(dev_priv);
> BUILD_BUG_ON(POWER_DOMAIN_NUM > 64);
>
> mutex_init(&power_domains->lock);
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h
> b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 737b5def7fc6..13fc705799fd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -100,6 +100,7 @@ enum i915_power_well_id {
> SKL_DISP_PW_MISC_IO,
> SKL_DISP_PW_1,
> SKL_DISP_PW_2,
> + SKL_DISP_DC_OFF,
> };
>
> #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> @@ -256,6 +257,7 @@ 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 tgl_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/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 84fb0245cf62..68fb732c24c8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -337,6 +337,7 @@ struct intel_csr {
> i915_reg_t mmioaddr[20];
> u32 mmiodata[20];
> u32 dc_state;
> + u32 target_dc_state;
> u32 allowed_dc_mask;
> intel_wakeref_t wakeref;
> };
> --
> 2.21.0
>
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
