From: Imre Deak <imre.deak@intel.com>
To: Anshuman Gupta <anshuman.gupta@intel.com>
Cc: jani.nikula@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v7 3/7] drm/i915/tgl: Enable DC3CO state in "DC Off" power well
Date: Wed, 11 Sep 2019 11:21:42 +0300 [thread overview]
Message-ID: <20190911082142.GA943@ideak-desk.fi.intel.com> (raw)
In-Reply-To: <20190909161917.GA18105@intel.com>
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 <jani.nikula@intel.com>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Animesh Manna <animesh.manna@intel.com>
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > > ---
> > > .../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.
> 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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-09-11 8:22 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-07 17:14 [PATCH v7 0/7] DC3CO Support for TGL Anshuman Gupta
2019-09-07 17:14 ` [PATCH v7 1/7] drm/i915/tgl: Add DC3CO required register and bits Anshuman Gupta
2019-09-11 9:08 ` Animesh Manna
2019-09-07 17:14 ` [PATCH v7 2/7] drm/i915/tgl: Add DC3CO mask to allowed_dc_mask and gen9_dc_mask Anshuman Gupta
2019-09-11 9:12 ` Animesh Manna
2019-09-07 17:14 ` [PATCH v7 3/7] drm/i915/tgl: Enable DC3CO state in "DC Off" power well Anshuman Gupta
2019-09-08 16:44 ` Imre Deak
2019-09-09 16:19 ` Anshuman Gupta
2019-09-11 8:21 ` Imre Deak [this message]
2019-09-11 12:42 ` Anshuman Gupta
2019-09-07 17:14 ` [PATCH v7 4/7] drm/i915/tgl: Do modeset to enable and configure DC3CO exitline Anshuman Gupta
2019-09-08 17:10 ` Imre Deak
2019-09-07 17:14 ` [PATCH v7 5/7] drm/i915/tgl: DC3CO PSR2 helper Anshuman Gupta
2019-09-08 17:20 ` Imre Deak
2019-09-07 17:14 ` [PATCH v7 6/7] drm/i915/tgl: switch between dc3co and dc5 based on display idleness Anshuman Gupta
2019-09-08 17:55 ` Imre Deak
2019-09-10 9:56 ` Anshuman Gupta
2019-09-11 8:50 ` Imre Deak
2019-09-11 9:57 ` Anshuman Gupta
2019-09-07 17:14 ` [PATCH v7 7/7] drm/i915/tgl: Add DC3CO counter in i915_dmc_info Anshuman Gupta
2019-09-07 17:34 ` ✗ Fi.CI.CHECKPATCH: warning for DC3CO Support for TGL (rev7) Patchwork
2019-09-07 17:37 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-09-07 17:58 ` ✓ Fi.CI.BAT: success " Patchwork
2019-09-07 19:03 ` ✗ Fi.CI.IGT: failure " Patchwork
2019-09-08 5:46 ` ✗ Fi.CI.CHECKPATCH: warning for DC3CO Support for TGL (rev8) Patchwork
2019-09-08 5:49 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-09-08 6:10 ` ✓ Fi.CI.BAT: success " Patchwork
2019-09-08 7:19 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190911082142.GA943@ideak-desk.fi.intel.com \
--to=imre.deak@intel.com \
--cc=anshuman.gupta@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.