[Intel-gfx] [PATCH v5 3/9] drm/i915/tgl: Add power well to enable DC3CO state
Gupta, Anshuman
anshuman.gupta at intel.com
Tue Aug 27 13:01:31 UTC 2019
On 8/13/2019 8:16 PM, Imre Deak wrote:
> On Sat, Aug 10, 2019 at 12:02:17AM +0530, Anshuman Gupta wrote:
>> "DC3CO Off" power well inherits its power domains from
>> "DC Off" power well, these power domains will disallow
>> DC3CO when any external displays are connected and at
>> time of modeset and aux programming.
>> Renaming "DC Off" power well to "DC5 Off" power well.
>>
>> 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.
>>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Imre Deak <imre.deak at intel.com>
>> Cc: Animesh Manna <animesh.manna at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
>> ---
>> .../drm/i915/display/intel_display_power.c | 69 ++++++++++++++++++-
>> 1 file changed, 67 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>> index e2ef202aeeef..c9e92d48cdab 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> @@ -791,7 +791,26 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
>> dev_priv->csr.dc_state = val & mask;
>> }
>>
>> -static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
>> +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
>> +{
>> + gen9_set_dc_state(dev_priv, DC_STATE_EN_DC3CO);
>> +}
>> +
>> +static void tgl_disallow_dc3co(struct drm_i915_private *dev_priv)
>> +{
>> + u32 val;
>> +
>> + 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);
>> +}
>> +
>> +void bxt_enable_dc9(struct drm_i915_private *dev_priv)
>> {
>> assert_can_enable_dc9(dev_priv);
>>
>> @@ -1007,6 +1026,33 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>> gen9_enable_dc5(dev_priv);
>> }
>>
>> +static void tgl_dc3co_power_well_enable(struct drm_i915_private *dev_priv,
>> + struct i915_power_well *power_well)
>
> Should be called dc3co_off power well.
>
>> +{
>> + tgl_disallow_dc3co(dev_priv);
>> +}
>> +
>> +static void tgl_dc3co_power_well_disable(struct drm_i915_private *dev_priv,
>> + struct i915_power_well *power_well)
>> +{
>> + if (!dev_priv->psr.sink_psr2_support)
>> + return;
>
> We could end up enabling DC3CO while PSR2 is disabled after disabling
> a PSR2 capable output, which is against the spec.
>
> I'm thinking now that we should have a single dc_off power well and a
> new interface setting the max allowed DC state (DC3CO, DC5/6).
Hi Imre,
Could you please comment if below part of new API code
set_max_dc_state() is ok? with respect to our DC3CO design sync.
static void set_target_dc_state(struct drm_i915_private *dev_priv)
{
gen9_dc_off_power_well_disable(dev_priv, NULL);
}
void set_max_dc_state(struct drm_i915_private *dev_priv, u32 state)
{
bool dc_off_enabled;
struct i915_power_domains *power_domains =
&dev_priv->power_domains;
/* need to define an id for DC off power well "TGL_DISP_DC_OFF"*/
dc_off_enabled = intel_display_power_well_is_enabled(dev_priv,
TGL_DISP_DC_OFF);
mutex_lock(&power_domains->lock);
if (dc_off_enabled) {
dev_priv->csr.max_dc_state = state;
} else {
dev_priv->csr.max_dc_state = state;
set_target_dc_state(dev_priv);
}
mutex_unlock(&power_domains->lock);
}
gen9_dc_off_power_well_disable will enable max_dc_state to DC_STATE_EN
register accordingly. There can be only issue according to me here when
"DC off" power well is disable (this will happen when we want to set
max_dc_state to dc5 in dc5_idle_thread). In that case we need to
manually call set_target_dc_state()->gen9_dc_off_power_well_disable().
Thanks,
Anshuman Gupta.
>
> (Right now I think there is also a missing re-enabling of DC3CO when
> disabling DC5/6).
>
>> +
>> + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)
>> + tgl_allow_dc3co(dev_priv);
>> +}
>> +
>> +static bool tgl_dc3co_power_well_enabled(struct drm_i915_private *dev_priv,
>> + struct i915_power_well *power_well)
>> +{
>> + /*
>> + * Checking alone DC_STATE_EN is not enough as DC5 power well also
>> + * allow/disallow DC3CO to make sure both are not enabled at same time
>> + */
>> + 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 i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
>> struct i915_power_well *power_well)
>> {
>> @@ -2611,6 +2657,12 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>> BIT_ULL(POWER_DOMAIN_TRANSCODER_VDSC_PW2) | \
>> BIT_ULL(POWER_DOMAIN_INIT))
>>
>> +#define TGL_DISPLAY_DC3CO_OFF_POWER_DOMAINS ( \
>> + TGL_PW_2_POWER_DOMAINS | \
>> + BIT_ULL(POWER_DOMAIN_MODESET) | \
>> + BIT_ULL(POWER_DOMAIN_AUX_A) | \
>> + BIT_ULL(POWER_DOMAIN_INIT))
>> +
>> #define TGL_DISPLAY_DC_OFF_POWER_DOMAINS ( \
>> TGL_PW_2_POWER_DOMAINS | \
>> BIT_ULL(POWER_DOMAIN_MODESET) | \
>> @@ -2715,6 +2767,13 @@ static const struct i915_power_well_ops gen9_dc_off_power_well_ops = {
>> .is_enabled = gen9_dc_off_power_well_enabled,
>> };
>>
>> +static const struct i915_power_well_ops tgl_dc3co_power_well_ops = {
>> + .sync_hw = i9xx_power_well_sync_hw_noop,
>> + .enable = tgl_dc3co_power_well_enable,
>> + .disable = tgl_dc3co_power_well_disable,
>> + .is_enabled = tgl_dc3co_power_well_enabled,
>> +};
>> +
>> static const struct i915_power_well_ops bxt_dpio_cmn_power_well_ops = {
>> .sync_hw = i9xx_power_well_sync_hw_noop,
>> .enable = bxt_dpio_cmn_power_well_enable,
>> @@ -3626,11 +3685,17 @@ static const struct i915_power_well_desc tgl_power_wells[] = {
>> },
>> },
>> {
>> - .name = "DC off",
>> + .name = "DC5 off",
>> .domains = TGL_DISPLAY_DC_OFF_POWER_DOMAINS,
>> .ops = &gen9_dc_off_power_well_ops,
>> .id = DISP_PW_ID_NONE,
>> },
>> + {
>> + .name = "DC3CO off",
>> + .domains = TGL_DISPLAY_DC3CO_OFF_POWER_DOMAINS,
>> + .ops = &tgl_dc3co_power_well_ops,
>> + .id = DISP_PW_ID_NONE,
>> + },
>> {
>> .name = "power well 2",
>> .domains = TGL_PW_2_POWER_DOMAINS,
>> --
>> 2.21.0
>>
More information about the Intel-gfx
mailing list