| Message ID | 20190809183223.12031-4-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DC3CO Support for TGL | expand |
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@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Animesh Manna <animesh.manna@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Anshuman Gupta <anshuman.gupta@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). (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 >
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@intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Animesh Manna <animesh.manna@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Anshuman Gupta <anshuman.gupta@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 >>
On Tue, Aug 27, 2019 at 06:31:31PM +0530, Gupta, Anshuman wrote: > > > 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@intel.com> > > > Cc: Imre Deak <imre.deak@intel.com> > > > Cc: Animesh Manna <animesh.manna@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@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); dc_off_enabled can get stale this way until you acquire the lock. Probably easier to lookup_power_well() and then check its state with the lock held. > > 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(). In that case you could just call power_well->enable(), ->disable(), and in the ->disable() hook you'd enable the DC state accorindg to csr.max_dc_state? > > 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 > > >
On 8/27/2019 6:50 PM, Imre Deak wrote: > On Tue, Aug 27, 2019 at 06:31:31PM +0530, Gupta, Anshuman wrote: >> >> >> 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@intel.com> >>>> Cc: Imre Deak <imre.deak@intel.com> >>>> Cc: Animesh Manna <animesh.manna@intel.com> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@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); > > dc_off_enabled can get stale this way until you acquire the lock. > Probably easier to lookup_power_well() and then check its state with the > lock held. > >> >> 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(). > > In that case you could just call power_well->enable(), ->disable(), and > in the ->disable() hook you'd enable the DC state accorindg to > csr.max_dc_state? Thanks for suggestion, will incorporate suggesting approach. Thanks, Anshuman > >> >> 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 >>>>
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) +{ + 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; + + 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,
"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@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Animesh Manna <animesh.manna@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- .../drm/i915/display/intel_display_power.c | 69 ++++++++++++++++++- 1 file changed, 67 insertions(+), 2 deletions(-)