Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.

2015-04-14 Thread Daniel Vetter 
On Mon, Apr 13, 2015 at 04:51:37PM +0300, Imre Deak wrote:
 On ma, 2015-04-13 at 14:17 +0100, Damien Lespiau wrote:
  On Thu, Apr 02, 2015 at 06:58:22PM +0300, Imre Deak wrote:
   On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote:
From: A.Sunil Kamath sunil.kam...@intel.com

This patch just implements the basic enable and disable
functions of DC5 state which is needed for both SKL and BXT.
   
   Reviewed-by: Imre Deak imre.d...@intel.com
  
  For the record, this patch generates compilation warnings when applied
  on its own:
  
  drivers/gpu/drm/i915/intel_runtime_pm.c:368:13: warning: ‘gen9_enable_dc5’ 
  defined but not used [-Wunused-function]
   static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
   ^
  drivers/gpu/drm/i915/intel_runtime_pm.c:386:13: warning: ‘gen9_disable_dc5’ 
  defined but not used [-Wunused-function]
   static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
   ^
  
  Generally speaking, in a series, each step should compile without warning 
  and
  result in a working driver (for bisectability).
 
 Yes, agreed. Splitting the changes into patches could've been done in
 better a way. I also commented on a related topic of adding something in
 the patchset and removing it later. We'll try to pay more attention to
 this in the future.
 
 Animesh, if you resend this patchset anyway I think you could switch the
 order of 2/8 and 3/8 and add the calls to the above functions in this
 patch to get rid of the warnings. Also please make sure that things
 compile without a warning after each patch as Damien suggested.

Yeah generally I don't like when patches add functions and structures and
don't use them - in the end it's fairly hard to review something if you
don't know how it's getting called used, which means you can't read the
patches linearly. And the point of splitting them is to give reviewers
small digestable chunks instead of the full thing.

Imo it's better to split things the other way round, i.e. wire up stub
functions at first, then fill them out. Instead of the first patch adding
the implementation and the 2nd one wiring things up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.

2015-04-13 Thread Imre Deak 
On ma, 2015-04-13 at 14:17 +0100, Damien Lespiau wrote:
 On Thu, Apr 02, 2015 at 06:58:22PM +0300, Imre Deak wrote:
  On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote:
   From: A.Sunil Kamath sunil.kam...@intel.com
   
   This patch just implements the basic enable and disable
   functions of DC5 state which is needed for both SKL and BXT.
  
  Reviewed-by: Imre Deak imre.d...@intel.com
 
 For the record, this patch generates compilation warnings when applied
 on its own:
 
 drivers/gpu/drm/i915/intel_runtime_pm.c:368:13: warning: ‘gen9_enable_dc5’ 
 defined but not used [-Wunused-function]
  static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
  ^
 drivers/gpu/drm/i915/intel_runtime_pm.c:386:13: warning: ‘gen9_disable_dc5’ 
 defined but not used [-Wunused-function]
  static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
  ^
 
 Generally speaking, in a series, each step should compile without warning and
 result in a working driver (for bisectability).

Yes, agreed. Splitting the changes into patches could've been done in
better a way. I also commented on a related topic of adding something in
the patchset and removing it later. We'll try to pay more attention to
this in the future.

Animesh, if you resend this patchset anyway I think you could switch the
order of 2/8 and 3/8 and add the calls to the above functions in this
patch to get rid of the warnings. Also please make sure that things
compile without a warning after each patch as Damien suggested.

--Imre


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.

2015-04-13 Thread Damien Lespiau 
On Thu, Apr 02, 2015 at 06:58:22PM +0300, Imre Deak wrote:
 On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote:
  From: A.Sunil Kamath sunil.kam...@intel.com
  
  This patch just implements the basic enable and disable
  functions of DC5 state which is needed for both SKL and BXT.
 
 Reviewed-by: Imre Deak imre.d...@intel.com

For the record, this patch generates compilation warnings when applied
on its own:

drivers/gpu/drm/i915/intel_runtime_pm.c:368:13: warning: ‘gen9_enable_dc5’ 
defined but not used [-Wunused-function]
 static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
 ^
drivers/gpu/drm/i915/intel_runtime_pm.c:386:13: warning: ‘gen9_disable_dc5’ 
defined but not used [-Wunused-function]
 static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
 ^

Generally speaking, in a series, each step should compile without warning and
result in a working driver (for bisectability).

-- 
Damien
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.

2015-04-02 Thread Imre Deak 
On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote:
 From: A.Sunil Kamath sunil.kam...@intel.com
 
 This patch just implements the basic enable and disable
 functions of DC5 state which is needed for both SKL and BXT.
 
 Its important to load respective CSR program before calling
 enable, which anyways will happen as CSR program is executed
 during boot.
 
 DC5 is a power saving state where hardware dynamically disables
 power well 1 and the CDCLK PLL and saves the associated registers.
 
 DC5 can be entered when software allows it, power well 2 is
 disabled, and hardware detects that all pipes are disabled
 or pipe A is enabled with PSR active.
 
 Its better to configure display engine to have power well 2 disabled before
 getting into DC5 enable function. Hence rpm framework will have to
 ensure to check status of power well 2 before calling gen9_enable_dc5.
 
 Rather dc5 entry criteria should be decided based on power well 2 status.
 If disabled, then call gen9_enable_dc5.
 
 v2: Replace HAS_ with IS_ check as per Daniel's review comments
 
 v3: Cleared the bits dc5/dc6 enable of DC_STATE_EN register
 before setting them as per Satheesh's review comments.
 
 v4: call POSTING_READ for every write to a register to ensure that
 its written immediately.
 
 v5: Modified as per review comments from Imre.
 - Squashed register definitions into this patch.
 - Finetuned comments and functions.
 
 v6:
 Avoid redundant writes in gen9_set_dc_state_debugmask_memory_up function.
 
 v7:
 1] Rebase to latest.
 2] Move all runtime PM functions defined in intel_display.c to
intel_runtime_pm.c.
 
 v8: Rebased to drm-intel-nightly. (Animesh)
 
 Issue: VIZ-2819
 Signed-off-by: A.Sunil Kamath sunil.kam...@intel.com
 Signed-off-by: Damien Lespiau damien.lesp...@intel.com
 Signed-off-by: Animesh Manna animesh.ma...@intel.com

Reviewed-by: Imre Deak imre.d...@intel.com

 ---
  drivers/gpu/drm/i915/i915_reg.h | 11 
  drivers/gpu/drm/i915/intel_runtime_pm.c | 47 
 +
  2 files changed, 58 insertions(+)
 
 diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
 index 77faa2b..d064e95 100644
 --- a/drivers/gpu/drm/i915/i915_reg.h
 +++ b/drivers/gpu/drm/i915/i915_reg.h
 @@ -6794,6 +6794,17 @@ enum skl_disp_power_wells {
  #define CSR_MMIO_END_RANGE   0x8
  #define CSR_MAX_MMIO_COUNT   8
  
 +/*
 +* SKL DC
 +*/
 +#define  DC_STATE_EN 0x45504
 +#define  DC_STATE_EN_UPTO_DC5(10)
 +#define  DC_STATE_EN_UPTO_DC6(20)
 +#define  DC_STATE_EN_UPTO_DC5_DC6_MASK   0x3
 +
 +#define  DC_STATE_DEBUG  0x45520
 +#define  DC_STATE_DEBUG_MASK_MEMORY_UP   (11)
 +
  /* 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_runtime_pm.c 
 b/drivers/gpu/drm/i915/intel_runtime_pm.c
 index ce00e69..bc6cee9 100644
 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
 +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
 @@ -319,6 +319,53 @@ static void hsw_set_power_well(struct drm_i915_private 
 *dev_priv,
   SKL_DISPLAY_MISC_IO_POWER_DOMAINS)) |   \
   BIT(POWER_DOMAIN_INIT))
  
 +static void gen9_set_dc_state_debugmask_memory_up(
 + struct drm_i915_private *dev_priv)
 +{
 + uint32_t val;
 +
 + /* The below bit doesn't need to be cleared ever afterwards */
 + val = I915_READ(DC_STATE_DEBUG);
 + if (!(val  DC_STATE_DEBUG_MASK_MEMORY_UP)) {
 + val |= DC_STATE_DEBUG_MASK_MEMORY_UP;
 + I915_WRITE(DC_STATE_DEBUG, val);
 + POSTING_READ(DC_STATE_DEBUG);
 + }
 +}
 +
 +static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
 +{
 + struct drm_device *dev = dev_priv-dev;
 + uint32_t val;
 +
 + WARN_ON(!IS_GEN9(dev));
 +
 + DRM_DEBUG_KMS(Enabling DC5\n);
 +
 + gen9_set_dc_state_debugmask_memory_up(dev_priv);
 +
 + val = I915_READ(DC_STATE_EN);
 + val = ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
 + val |= DC_STATE_EN_UPTO_DC5;
 + I915_WRITE(DC_STATE_EN, val);
 + POSTING_READ(DC_STATE_EN);
 +}
 +
 +static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
 +{
 + struct drm_device *dev = dev_priv-dev;
 + uint32_t val;
 +
 + WARN_ON(!IS_GEN9(dev));
 +
 + DRM_DEBUG_KMS(Disabling DC5\n);
 +
 + val = I915_READ(DC_STATE_EN);
 + val = ~DC_STATE_EN_UPTO_DC5;
 + 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)
  {


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.

2015-04-01 Thread Animesh Manna 
From: A.Sunil Kamath sunil.kam...@intel.com

This patch just implements the basic enable and disable
functions of DC5 state which is needed for both SKL and BXT.

Its important to load respective CSR program before calling
enable, which anyways will happen as CSR program is executed
during boot.

DC5 is a power saving state where hardware dynamically disables
power well 1 and the CDCLK PLL and saves the associated registers.

DC5 can be entered when software allows it, power well 2 is
disabled, and hardware detects that all pipes are disabled
or pipe A is enabled with PSR active.

Its better to configure display engine to have power well 2 disabled before
getting into DC5 enable function. Hence rpm framework will have to
ensure to check status of power well 2 before calling gen9_enable_dc5.

Rather dc5 entry criteria should be decided based on power well 2 status.
If disabled, then call gen9_enable_dc5.

v2: Replace HAS_ with IS_ check as per Daniel's review comments

v3: Cleared the bits dc5/dc6 enable of DC_STATE_EN register
before setting them as per Satheesh's review comments.

v4: call POSTING_READ for every write to a register to ensure that
its written immediately.

v5: Modified as per review comments from Imre.
- Squashed register definitions into this patch.
- Finetuned comments and functions.

v6:
Avoid redundant writes in gen9_set_dc_state_debugmask_memory_up function.

v7:
1] Rebase to latest.
2] Move all runtime PM functions defined in intel_display.c to
   intel_runtime_pm.c.

v8: Rebased to drm-intel-nightly. (Animesh)

Issue: VIZ-2819
Signed-off-by: A.Sunil Kamath sunil.kam...@intel.com
Signed-off-by: Damien Lespiau damien.lesp...@intel.com
Signed-off-by: Animesh Manna animesh.ma...@intel.com
---
 drivers/gpu/drm/i915/i915_reg.h | 11 
 drivers/gpu/drm/i915/intel_runtime_pm.c | 47 +
 2 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 77faa2b..d064e95 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6794,6 +6794,17 @@ enum skl_disp_power_wells {
 #define CSR_MMIO_END_RANGE 0x8
 #define CSR_MAX_MMIO_COUNT 8
 
+/*
+* SKL DC
+*/
+#define  DC_STATE_EN   0x45504
+#define  DC_STATE_EN_UPTO_DC5  (10)
+#define  DC_STATE_EN_UPTO_DC6  (20)
+#define  DC_STATE_EN_UPTO_DC5_DC6_MASK   0x3
+
+#define  DC_STATE_DEBUG  0x45520
+#define  DC_STATE_DEBUG_MASK_MEMORY_UP (11)
+
 /* 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_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ce00e69..bc6cee9 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -319,6 +319,53 @@ static void hsw_set_power_well(struct drm_i915_private 
*dev_priv,
SKL_DISPLAY_MISC_IO_POWER_DOMAINS)) |   \
BIT(POWER_DOMAIN_INIT))
 
+static void gen9_set_dc_state_debugmask_memory_up(
+   struct drm_i915_private *dev_priv)
+{
+   uint32_t val;
+
+   /* The below bit doesn't need to be cleared ever afterwards */
+   val = I915_READ(DC_STATE_DEBUG);
+   if (!(val  DC_STATE_DEBUG_MASK_MEMORY_UP)) {
+   val |= DC_STATE_DEBUG_MASK_MEMORY_UP;
+   I915_WRITE(DC_STATE_DEBUG, val);
+   POSTING_READ(DC_STATE_DEBUG);
+   }
+}
+
+static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
+{
+   struct drm_device *dev = dev_priv-dev;
+   uint32_t val;
+
+   WARN_ON(!IS_GEN9(dev));
+
+   DRM_DEBUG_KMS(Enabling DC5\n);
+
+   gen9_set_dc_state_debugmask_memory_up(dev_priv);
+
+   val = I915_READ(DC_STATE_EN);
+   val = ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
+   val |= DC_STATE_EN_UPTO_DC5;
+   I915_WRITE(DC_STATE_EN, val);
+   POSTING_READ(DC_STATE_EN);
+}
+
+static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
+{
+   struct drm_device *dev = dev_priv-dev;
+   uint32_t val;
+
+   WARN_ON(!IS_GEN9(dev));
+
+   DRM_DEBUG_KMS(Disabling DC5\n);
+
+   val = I915_READ(DC_STATE_EN);
+   val = ~DC_STATE_EN_UPTO_DC5;
+   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)
 {
-- 
2.0.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.

2015-04-01 Thread Animesh Manna 
From: A.Sunil Kamath sunil.kam...@intel.com

This patch just implements the basic enable and disable
functions of DC5 state which is needed for both SKL and BXT.

Its important to load respective CSR program before calling
enable, which anyways will happen as CSR program is executed
during boot.

DC5 is a power saving state where hardware dynamically disables
power well 1 and the CDCLK PLL and saves the associated registers.

DC5 can be entered when software allows it, power well 2 is
disabled, and hardware detects that all pipes are disabled
or pipe A is enabled with PSR active.

Its better to configure display engine to have power well 2 disabled before
getting into DC5 enable function. Hence rpm framework will have to
ensure to check status of power well 2 before calling gen9_enable_dc5.

Rather dc5 entry criteria should be decided based on power well 2 status.
If disabled, then call gen9_enable_dc5.

v2: Replace HAS_ with IS_ check as per Daniel's review comments

v3: Cleared the bits dc5/dc6 enable of DC_STATE_EN register
before setting them as per Satheesh's review comments.

v4: call POSTING_READ for every write to a register to ensure that
its written immediately.

v5: Modified as per review comments from Imre.
- Squashed register definitions into this patch.
- Finetuned comments and functions.

v6:
Avoid redundant writes in gen9_set_dc_state_debugmask_memory_up function.

v7:
1] Rebase to latest.
2] Move all runtime PM functions defined in intel_display.c to
   intel_runtime_pm.c.

v8: Rebased to drm-intel-nightly. (Animesh)

Issue: VIZ-2819
Signed-off-by: A.Sunil Kamath sunil.kam...@intel.com
Signed-off-by: Damien Lespiau damien.lesp...@intel.com
Signed-off-by: Animesh Manna animesh.ma...@intel.com
---
 drivers/gpu/drm/i915/i915_reg.h | 11 
 drivers/gpu/drm/i915/intel_runtime_pm.c | 47 +
 2 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 77faa2b..d064e95 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6794,6 +6794,17 @@ enum skl_disp_power_wells {
 #define CSR_MMIO_END_RANGE 0x8
 #define CSR_MAX_MMIO_COUNT 8
 
+/*
+* SKL DC
+*/
+#define  DC_STATE_EN   0x45504
+#define  DC_STATE_EN_UPTO_DC5  (10)
+#define  DC_STATE_EN_UPTO_DC6  (20)
+#define  DC_STATE_EN_UPTO_DC5_DC6_MASK   0x3
+
+#define  DC_STATE_DEBUG  0x45520
+#define  DC_STATE_DEBUG_MASK_MEMORY_UP (11)
+
 /* 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_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ce00e69..bc6cee9 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -319,6 +319,53 @@ static void hsw_set_power_well(struct drm_i915_private 
*dev_priv,
SKL_DISPLAY_MISC_IO_POWER_DOMAINS)) |   \
BIT(POWER_DOMAIN_INIT))
 
+static void gen9_set_dc_state_debugmask_memory_up(
+   struct drm_i915_private *dev_priv)
+{
+   uint32_t val;
+
+   /* The below bit doesn't need to be cleared ever afterwards */
+   val = I915_READ(DC_STATE_DEBUG);
+   if (!(val  DC_STATE_DEBUG_MASK_MEMORY_UP)) {
+   val |= DC_STATE_DEBUG_MASK_MEMORY_UP;
+   I915_WRITE(DC_STATE_DEBUG, val);
+   POSTING_READ(DC_STATE_DEBUG);
+   }
+}
+
+static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
+{
+   struct drm_device *dev = dev_priv-dev;
+   uint32_t val;
+
+   WARN_ON(!IS_GEN9(dev));
+
+   DRM_DEBUG_KMS(Enabling DC5\n);
+
+   gen9_set_dc_state_debugmask_memory_up(dev_priv);
+
+   val = I915_READ(DC_STATE_EN);
+   val = ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
+   val |= DC_STATE_EN_UPTO_DC5;
+   I915_WRITE(DC_STATE_EN, val);
+   POSTING_READ(DC_STATE_EN);
+}
+
+static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
+{
+   struct drm_device *dev = dev_priv-dev;
+   uint32_t val;
+
+   WARN_ON(!IS_GEN9(dev));
+
+   DRM_DEBUG_KMS(Disabling DC5\n);
+
+   val = I915_READ(DC_STATE_EN);
+   val = ~DC_STATE_EN_UPTO_DC5;
+   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)
 {
-- 
2.0.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx