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.nik...@intel.com>
> > > Cc: Imre Deak <imre.d...@intel.com>
> > > Cc: Animesh Manna <animesh.ma...@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> > > Signed-off-by: Anshuman Gupta <anshuman.gu...@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
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
