On 2019-09-11 at 11:21:42 +0300, Imre Deak wrote:
> On Mon, Sep 09, 2019 at 09:49:17PM +0530, Anshuman Gupta wrote:
> > On 2019-09-08 at 19:44:35 +0300, Imre Deak wrote:
> > > On Sat, Sep 07, 2019 at 10:44:39PM +0530, Anshuman Gupta wrote:
> > Hi Imre,
> > Thanks for reviewing the pacthes i will rework the patches.
> > There are few comments from my side which will help to rework.
> > > > Add max_dc_state and tgl_set_target_dc_state() API
> > > > in order to enable DC3CO state with existing DC states.
> > > > max_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.
> > > >
> > > > 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 | 111 ++++++++++++++----
> > > > .../drm/i915/display/intel_display_power.h | 3 +
> > > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > > 3 files changed, 95 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > index 496fa1b53ffb..83b10f61ee42 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > > @@ -772,6 +772,29 @@ static void gen9_set_dc_state(struct
> > > > drm_i915_private *dev_priv, u32 state)
> > > > dev_priv->csr.dc_state = val & mask;
> > > > }
> > > >
> > > > +static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
> > >
> > > Should be tgl_enable_dc3co(), to match the rest of DC state helpers.
> > >
> > > > +{
> > > > + if (!dev_priv->psr.sink_psr2_support)
> > > > + return;
> > >
> > > PSR knows when to enable DC3co, so no need to double-check that here.
> > >
> > > > +
> > > > + if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO)
> > >
> > > This check is out-of-place wrt. the same checks for other DC states.
> > >
> > > > + 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);
> > > > +}
> > > > +
> > > > static void bxt_enable_dc9(struct drm_i915_private *dev_priv)
> > > > {
> > > > assert_can_enable_dc9(dev_priv);
> > > > @@ -939,7 +962,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,24 +979,32 @@ static void gen9_disable_dc_states(struct
> > > > drm_i915_private *dev_priv)
> > > > {
> > > > struct intel_cdclk_state cdclk_state = {};
> > > >
> > > > - gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> > > > + tgl_disallow_dc3co(dev_priv);
> > > > + } else {
> > >
> > > With an early return you can avoid the extra diff and make reviewing
> > > easier.
> > >
> > > > + gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > >
> > > > - dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > > > - /* Can't read out voltage_level so can't use
> > > > intel_cdclk_changed() */
> > > > - WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
> > > > &cdclk_state));
> > > > + dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > > > + /*
> > > > + * Can't read out voltage_level so can't use
> > > > + * intel_cdclk_changed()
> > > > + */
> > > > + WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw,
> > > > + &cdclk_state));
> > > >
> > > > - gen9_assert_dbuf_enabled(dev_priv);
> > > > + gen9_assert_dbuf_enabled(dev_priv);
> > > >
> > > > - if (IS_GEN9_LP(dev_priv))
> > > > - bxt_verify_ddi_phy_power_wells(dev_priv);
> > > > + if (IS_GEN9_LP(dev_priv))
> > > > + bxt_verify_ddi_phy_power_wells(dev_priv);
> > > >
> > > > - if (INTEL_GEN(dev_priv) >= 11)
> > > > - /*
> > > > - * DMC retains HW context only for port A, the other
> > > > combo
> > > > - * PHY's HW context for port B is lost after DC
> > > > transitions,
> > > > - * so we need to restore it manually.
> > > > - */
> > > > - intel_combo_phy_init(dev_priv);
> > > > + if (INTEL_GEN(dev_priv) >= 11)
> > > > + /*
> > > > + * DMC retains HW context only for port A, the
> > > > other
> > > > + * combo PHY's HW context for port B is lost
> > > > after
> > > > + * DC transitions, so we need to restore it
> > > > manually.
> > > > + */
> > > > + intel_combo_phy_init(dev_priv);
> > > > + }
> > > > }
> > > >
> > > > static void gen9_dc_off_power_well_enable(struct drm_i915_private
> > > > *dev_priv,
> > > > @@ -987,10 +1019,48 @@ static void
> > > > gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> > > > if (!dev_priv->csr.dmc_payload)
> > > > return;
> > > >
> > > > - if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
> > > > - skl_enable_dc6(dev_priv);
> > > > - else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > > > - gen9_enable_dc5(dev_priv);
> > > > + if (dev_priv->csr.max_dc_state & DC_STATE_EN_DC3CO) {
> > >
> > > target_dc_state would be a better name and it shold be an exact state
> > > instead of a mask.
> > >
> > > > + tgl_allow_dc3co(dev_priv);
> > > > + } else if (dev_priv->csr.max_dc_state &
> > > > DC_STATE_EN_UPTO_DC5_DC6_MASK) {
> > > > + if (dev_priv->csr.allowed_dc_mask &
> > > > DC_STATE_EN_UPTO_DC6)
> > >
> > > We should make these checks more uniform, by checking here only
> > > target_dc_state.
> > If i understand correctly this comment is about to have a uniform
> > condition target_dc_state in order to decide which DC state we
> > interested. Then we may require to check allowed_dc_mask accordingly in
> > tgl_set_target_dc_state(),
>
> Yes, we could check here only target_dc_state, setting that one only to
> values permitted by allowed_dc_mask.
For older platform setting target_dc_state value permitted by allowed_dc_mask
i am planning to have a allowed_dc_mask_to_target_dc_state() func, which
will be called from intel_power_domains_init, and initilizes target_dc_state.
for tgl onwards tgl_set_target_dc_state() will do it.
Hope it will be ok ?
>
> > but that would not be fitting for
> > older platforms.
> > What is your input on this? please correct me if i am wrong here.
> > >
> > > > + skl_enable_dc6(dev_priv);
> > > > + else if (dev_priv->csr.allowed_dc_mask &
> > > > DC_STATE_EN_UPTO_DC5)
> > > > + gen9_enable_dc5(dev_priv);
> > > > + }
> > > > +}
> > > > +
> > > > +void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32
> > > > state,
> > > > + bool psr2_deep_sleep)
> > >
> > > psr2_deep_sleep is a PSR internal stuff, while PSR is using the power
> > > domains framework, so let's keep the deep sleep programming in the PSR
> > > code.
> > I wanted not to enable/disable psr2 deep sleep for each flip flush request.
> > Though PSR2 deep sleep is PSR internal stuff but it is H/W has tied with
> > DC3CO.
> > That was intention to keep psr2_deep sleep enable/diable here (Doing in
> > Patch 6).
> > In order to switch between DC3CO and DC5 we need to call
> > tgl_psr2_deep_sleep_disable/enable() from outside of PSR code.
> > please correct me if i am wrong here.
>
> I think it still belongs to the PSR code with other steps needed for
> DC3co setup, like the idle frame programming. PSR deep sleep could also
> be enabled/disabled independently of the DC state we enable (for
> instance enable/disable deep sleep when both DC3co and DC5 is
> disabled). It also needs the PSR lock, which again is internal to the
> PSR code.
>
> > Thanks ,
> > Anshuman Gupta.
> > >
> > > > +{
> > > > + 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, TGL_DISP_DC_OFF);
> > > > +
> > > > + if (!power_well)
> > > > + goto unlock;
> > > > +
> > > > + dc_off_enabled = power_well->desc->ops->is_enabled(dev_priv,
> > > > + power_well);
> > > > + if (state == dev_priv->csr.max_dc_state)
> > > > + goto unlock;
> > > > +
> > > > + 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.max_dc_state = state;
> > > > + power_well->desc->ops->disable(dev_priv, power_well);
> > > > + goto unlock;
> > > > + }
> > > > + dev_priv->csr.max_dc_state = state;
> > > > +
> > > > +unlock:
> > > > + mutex_unlock(&power_domains->lock);
> > > > }
> > > >
> > > > static void i9xx_power_well_sync_hw_noop(struct drm_i915_private
> > > > *dev_priv,
> > > > @@ -3610,7 +3680,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 = TGL_DISP_DC_OFF,
> > >
> > > Let's assign this ID on all platforms for consistency.
> > IMO that should be done in different patch, as tgl_set_target_dc_state() is
> > only consumer of this id ?
> > What is your opinion on this ?
> > >
> > > > },
> > > > {
> > > > .name = "power well 2",
> > > > @@ -4039,6 +4109,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);
> > > >
> > > > + dev_priv->csr.max_dc_state = DC_STATE_EN_UPTO_DC5_DC6_MASK;
> > > > 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..69ebde992342 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,
> > > > + TGL_DISP_DC_OFF,
> > > > };
> > > >
> > > > #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> > > > @@ -256,6 +257,8 @@ 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,
> > > > + bool psr2_deep_sleep);
> > > >
> > > > 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 db7480831e52..999da5d2da0b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -336,6 +336,7 @@ struct intel_csr {
> > > > i915_reg_t mmioaddr[20];
> > > > u32 mmiodata[20];
> > > > u32 dc_state;
> > > > + u32 max_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
