| Message ID | 1426585215-8788-33-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
These are review comments for 1) http://lists.freedesktop.org/archives/intel-gfx/2015-March/062167.html 2) http://lists.freedesktop.org/archives/intel-gfx/2015-March/062168.html Couple of comments: 1) Defines for DC_STATE_EN* are coming up as part of http://lists.freedesktop.org/archives/intel-gfx/2015-April/063640.html. Need to rebase this patch on top of it then or vice-versa. 2) DC5 has to enabled back after disabling DC9 if PW2 is power gated.
On su, 2015-04-12 at 16:02 +0530, sagar.a.kamble@intel.com wrote: > These are review comments for > 1) http://lists.freedesktop.org/archives/intel-gfx/2015-March/062167.html > 2) http://lists.freedesktop.org/archives/intel-gfx/2015-March/062168.html It'd be better to have inlined review comments responding to the original email. > Couple of comments: > 1) Defines for DC_STATE_EN* are coming up as part of > http://lists.freedesktop.org/archives/intel-gfx/2015-April/063640.html. > Need to rebase this patch on top of it then or vice-versa. Yes, I can rebase this once Animesh's patchset gets merged. It's also a trivial conflict that can be easily resolved while merging, so it's not an issue imo. > 2) DC5 has to enabled back after disabling DC9 if PW2 is power gated. BXT DC5/runtime PM support will be added only later. At that point the enabling of DC5 should be done from bxt_resume_prepare() if the the DMC firmware is loaded. For now I'd just add the missing TODO comment about this to bxt_resume_prepare() as you suggested elsewhere. --Imre
On Mon, 2015-04-13 at 13:09 +0300, Imre Deak wrote: > On su, 2015-04-12 at 16:02 +0530, sagar.a.kamble@intel.com wrote: > > These are review comments for > > 1) http://lists.freedesktop.org/archives/intel-gfx/2015-March/062167.html > > 2) http://lists.freedesktop.org/archives/intel-gfx/2015-March/062168.html > > It'd be better to have inlined review comments responding to the > original email. Yes. Sorry for the inconvenience. My ML subscription was in digest mode. So replied using only message-id knowing from Deepak. Now I have switched to individual mails. > > > Couple of comments: > > 1) Defines for DC_STATE_EN* are coming up as part of > > http://lists.freedesktop.org/archives/intel-gfx/2015-April/063640.html. > > Need to rebase this patch on top of it then or vice-versa. > > Yes, I can rebase this once Animesh's patchset gets merged. It's also a > trivial conflict that can be easily resolved while merging, so it's not > an issue imo. > > > 2) DC5 has to enabled back after disabling DC9 if PW2 is power gated. > > BXT DC5/runtime PM support will be added only later. At that point the > enabling of DC5 should be done from bxt_resume_prepare() if the the DMC > firmware is loaded. For now I'd just add the missing TODO comment about > this to bxt_resume_prepare() as you suggested elsewhere. Thanks. Reviewed-by: Sagar Kamble <sagar.a.kamble at intel.com> > > --Imre >
On Tue, Mar 17, 2015 at 11:39:58AM +0200, Imre Deak wrote: > From: "A.Sunil Kamath" <sunil.kamath@intel.com> > > v2: Modified as per review comments from Imre > - Mention enabling instead of allowing in the debug trace and > remove unnecessary comments. > > v3: > - Rebase to latest. > - Move DC9-related functions from intel_display.c to intel_runtime_pm.c. > > v4: (imre) > - remove DC5 disabling, it's a nop at this point > - squashed in Suketu's "Assert the requirements to enter or exit DC9" > patch > - remove check for RUNTIME_PM from assert_can_enable_dc9, it's not a > dependency > > Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com> (v3) > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 5 +++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > drivers/gpu/drm/i915/intel_runtime_pm.c | 66 +++++++++++++++++++++++++++++++++ > 3 files changed, 73 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 95532b4..4c781cb 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6942,6 +6942,11 @@ enum bxt_phy { > #define BXT_DE_PLL_PLL_ENABLE (1 << 31) > #define BXT_DE_PLL_LOCK (1 << 30) > > +/* GEN9 DC */ > +#define DC_STATE_EN 0x45504 > +#define DC_STATE_EN_UPTO_DC5 (1<<0) > +#define DC_STATE_EN_DC9 (1<<3) > + > /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register, > * since on HSW we can't write to it using I915_WRITE. */ > #define D_COMP_HSW (MCHBAR_MIRROR_BASE_SNB + 0x5F0C) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 4bc2041..262314b 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1024,6 +1024,8 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv); > void bxt_init_cdclk(struct drm_device *dev); > void bxt_uninit_cdclk(struct drm_device *dev); > void bxt_ddi_phy_init(struct drm_device *dev); > +void bxt_enable_dc9(struct drm_i915_private *dev_priv); > +void bxt_disable_dc9(struct drm_i915_private *dev_priv); > void intel_dp_get_m_n(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config); > void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n); > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index ff5cce3..8fe2fde 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -351,6 +351,72 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv, > BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) | \ > BIT(POWER_DOMAIN_INIT)) > > +static void assert_can_enable_dc9(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev = dev_priv->dev; > + > + WARN(!IS_BROXTON(dev), "Platform doesn't support DC9.\n"); > + WARN((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC9), > + "DC9 already programmed to be enabled.\n"); > + WARN(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5, > + "DC5 still not disabled to enable DC9.\n"); > + WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on.\n"); > + WARN(intel_irqs_enabled(dev_priv), "Interrupts not disabled yet.\n"); > + > + /* > + * TODO: check for the following to verify the conditions to enter DC9 > + * state are satisfied: > + * 1] Check relevant display engine registers to verify if mode set > + * disable sequence was followed. > + * 2] Check if display uninitialize sequence is initialized. > + */ > +} > + > +static void assert_can_disable_dc9(struct drm_i915_private *dev_priv) > +{ > + WARN(intel_irqs_enabled(dev_priv), "Interrupts not disabled yet.\n"); > + WARN(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_DC9), > + "DC9 already programmed to be disabled.\n"); > + WARN(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5, > + "DC5 still not disabled.\n"); > + > + /* > + * TODO: check for the following to verify DC9 state was indeed > + * entered before programming to disable it: > + * 1] Check relevant display engine registers to verify if mode > + * set disable sequence was followed. > + * 2] Check if display uninitialize sequence is initialized. > + */ > +} > + > +void bxt_enable_dc9(struct drm_i915_private *dev_priv) > +{ > + uint32_t val; > + > + assert_can_enable_dc9(dev_priv); > + > + DRM_DEBUG_KMS("Enabling DC9\n"); > + > + val = I915_READ(DC_STATE_EN); > + val |= DC_STATE_EN_DC9; > + I915_WRITE(DC_STATE_EN, val); > + POSTING_READ(DC_STATE_EN); > +} > + > +void bxt_disable_dc9(struct drm_i915_private *dev_priv) > +{ > + uint32_t val; > + > + assert_can_disable_dc9(dev_priv); > + > + DRM_DEBUG_KMS("Disabling DC9\n"); > + > + val = I915_READ(DC_STATE_EN); > + val &= ~DC_STATE_EN_DC9; > + I915_WRITE(DC_STATE_EN, val); > + POSTING_READ(DC_STATE_EN); > +} Standard comment about patch splitting: Please don't add functions or structures (or new member fields and other variables) without using them in the same patch: That way the patch can't be understood fully when just linearly reading through patches and hence somewhat defeats the purpose of patch splitting. Instead I recommend to first wire up dummy functions to sketch the larger picture and then in the next patch fill in details. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 95532b4..4c781cb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6942,6 +6942,11 @@ enum bxt_phy { #define BXT_DE_PLL_PLL_ENABLE (1 << 31) #define BXT_DE_PLL_LOCK (1 << 30) +/* GEN9 DC */ +#define DC_STATE_EN 0x45504 +#define DC_STATE_EN_UPTO_DC5 (1<<0) +#define DC_STATE_EN_DC9 (1<<3) + /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register, * since on HSW we can't write to it using I915_WRITE. */ #define D_COMP_HSW (MCHBAR_MIRROR_BASE_SNB + 0x5F0C) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4bc2041..262314b 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1024,6 +1024,8 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv); void bxt_init_cdclk(struct drm_device *dev); void bxt_uninit_cdclk(struct drm_device *dev); void bxt_ddi_phy_init(struct drm_device *dev); +void bxt_enable_dc9(struct drm_i915_private *dev_priv); +void bxt_disable_dc9(struct drm_i915_private *dev_priv); void intel_dp_get_m_n(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config); void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n); diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index ff5cce3..8fe2fde 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -351,6 +351,72 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv, BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) | \ BIT(POWER_DOMAIN_INIT)) +static void assert_can_enable_dc9(struct drm_i915_private *dev_priv) +{ + struct drm_device *dev = dev_priv->dev; + + WARN(!IS_BROXTON(dev), "Platform doesn't support DC9.\n"); + WARN((I915_READ(DC_STATE_EN) & DC_STATE_EN_DC9), + "DC9 already programmed to be enabled.\n"); + WARN(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5, + "DC5 still not disabled to enable DC9.\n"); + WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on.\n"); + WARN(intel_irqs_enabled(dev_priv), "Interrupts not disabled yet.\n"); + + /* + * TODO: check for the following to verify the conditions to enter DC9 + * state are satisfied: + * 1] Check relevant display engine registers to verify if mode set + * disable sequence was followed. + * 2] Check if display uninitialize sequence is initialized. + */ +} + +static void assert_can_disable_dc9(struct drm_i915_private *dev_priv) +{ + WARN(intel_irqs_enabled(dev_priv), "Interrupts not disabled yet.\n"); + WARN(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_DC9), + "DC9 already programmed to be disabled.\n"); + WARN(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5, + "DC5 still not disabled.\n"); + + /* + * TODO: check for the following to verify DC9 state was indeed + * entered before programming to disable it: + * 1] Check relevant display engine registers to verify if mode + * set disable sequence was followed. + * 2] Check if display uninitialize sequence is initialized. + */ +} + +void bxt_enable_dc9(struct drm_i915_private *dev_priv) +{ + uint32_t val; + + assert_can_enable_dc9(dev_priv); + + DRM_DEBUG_KMS("Enabling DC9\n"); + + val = I915_READ(DC_STATE_EN); + val |= DC_STATE_EN_DC9; + I915_WRITE(DC_STATE_EN, val); + POSTING_READ(DC_STATE_EN); +} + +void bxt_disable_dc9(struct drm_i915_private *dev_priv) +{ + uint32_t val; + + assert_can_disable_dc9(dev_priv); + + DRM_DEBUG_KMS("Disabling DC9\n"); + + val = I915_READ(DC_STATE_EN); + val &= ~DC_STATE_EN_DC9; + I915_WRITE(DC_STATE_EN, val); + POSTING_READ(DC_STATE_EN); +} + static void skl_set_power_well(struct drm_i915_private *dev_priv, struct i915_power_well *power_well, bool enable) {