[PATCH 00/18] Redesign of dmc firmware loading.

[PATCH 00/18] Redesign of dmc firmware loading.

[Animesh Manna]

Display microcontroller(DMC) used to save and restore display engine status
while entering into low power display states for gen9 platform.
Though skylake and broxton both are gen9 platform but dmc act diferently.
Skylake is solely dependednt on dmc for entering into low power
state - namely dc5 and dc6. Whereas broxton has low power state
dc5 and dc9 and dc9 does not need dmc.

As both gen9 platforms behaviour is different with respect to dmc,
so software design will be different and follow the below design
principles.

For skylake:
------------
If firmware loading is successful,
- Driver can goto D3 in suspend time.
- Will not disable power-well-1 as dmc will handle save/restore/shutdown.
If firmware loading is failed,
- Leak rpm which will block D3 in suspend.
- Will disable power-well-1 which will save some power.

For broxton:
------------
Irrespective of firmware loading succesful/failed driver should not
block D3 during suspend as dc9 (lowest possible state) is independent
of dmc and will not block disable call of power-well-1.

During debugging PC10 entry issue for skylake found below observation, 
- dmc will get confuse if driver already disable power-well-1 during
dc6 entry and will not work as expected.
- The above same applicable for cdclk pll as well.
- mmio read/write after dc6 trigger will cause display engine hang.

Based on Daniel's review comments and thiinking of above pointers
following patches is created.

Animesh Manna (10):
  drm/i915/bxt: Path added of dmc firmware ver1 for BXT
  drm/i915/bxt: Modified HAS_CSR, added support for BXT.
  drm/i915/bxt: Stepping info added for bxt.
  drm/i915/gen9: block disable call for pw1 if dmc firmware is present.
  drm/i915/gen9: csr_init after runtime pm enable
  drm/i915/gen9: Use flush_work to synchronize with dmc loader
  drm/i915/skl: Making DC6 entry is the last call in suspend flow.
  drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.
  drm/i915/skl: Removed csr firmware load in resume path.
  drm/i915/gen9: Removed byte swapping for csr firmware.

Daniel Vetter (8):
  drm/i915/gen9: move assert_csr_loaded into intel_rpm.c
  drm/i915/gen9: Remove csr.state, csr_lock and related code.
  drm/i915/gen9: Align line continuations in intel_csr.c.
  drm/i915/gen9: Simplify csr loading failure printing.
  drm/i915/gen9: extract parse_csr_fw.
  drm/i915/gen9: Don't try to load garbage dmc firmware on resume
  drm/i915/gen9: Use dev_priv in csr functions
  drm/i915: Use request_firmware and our own async work

 drivers/gpu/drm/i915/i915_dma.c         |  11 +-
 drivers/gpu/drm/i915/i915_drv.c         |  33 +----
 drivers/gpu/drm/i915/i915_drv.h         |  16 +--
 drivers/gpu/drm/i915/i915_reg.h         |  16 +++
 drivers/gpu/drm/i915/intel_csr.c        | 245 ++++++++++++--------------------
 drivers/gpu/drm/i915/intel_display.c    |  11 +-
 drivers/gpu/drm/i915/intel_drv.h        |  12 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c |  47 +++---
 8 files changed, 155 insertions(+), 236 deletions(-)

-- 
2.0.2

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

[PATCH 01/18] drm/i915/bxt: Path added of dmc firmware ver1 for BXT

[Animesh Manna]

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 6d8a7bf..1866426 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -42,8 +42,10 @@
  */
 
 #define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
+#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin"
 
 MODULE_FIRMWARE(I915_CSR_SKL);
+MODULE_FIRMWARE(I915_CSR_BXT);
 
 /*
 * SKL CSR registers for DC5 and DC6
@@ -417,6 +419,8 @@ void intel_csr_ucode_init(struct drm_device *dev)
 
 	if (IS_SKYLAKE(dev))
 		csr->fw_path = I915_CSR_SKL;
+	else if (IS_BROXTON(dev))
+		csr->fw_path = I915_CSR_BXT;
 	else {
 		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
 		intel_csr_load_status_set(dev_priv, FW_FAILED);
-- 
2.0.2

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

[PATCH 02/18] drm/i915/bxt: Modified HAS_CSR, added support for BXT.

[Animesh Manna]

Modified HAS_CSR macro defination which earlier only supported
for skl, now added support for BXT.

Cc: Vetter, Daniel <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b94ada9..a2eac8e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2542,7 +2542,7 @@ struct drm_i915_cmd_table {
 #define HAS_RC6(dev)		(INTEL_INFO(dev)->gen >= 6)
 #define HAS_RC6p(dev)		(INTEL_INFO(dev)->gen == 6 || IS_IVYBRIDGE(dev))
 
-#define HAS_CSR(dev)	(IS_SKYLAKE(dev))
+#define HAS_CSR(dev)	(IS_SKYLAKE(dev) || IS_BROXTON(dev))
 
 #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
 				    INTEL_INFO(dev)->gen >= 8)
-- 
2.0.2

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

[PATCH 03/18] drm/i915/bxt: Stepping info added for bxt.

[Animesh Manna]

Added stepping info in intel_csr.c which is required to extract
specific firmware from packaged dmc firmware.

Cc: Vetter, Daniel <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 1866426..f440299 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -183,11 +183,19 @@ static const struct stepping_info skl_stepping_info[] = {
 		{'G', '0'}, {'H', '0'}, {'I', '0'}
 };
 
+static struct stepping_info bxt_stepping_info[] = {
+		{'A', '0'}, {'A', '1'}, {'A', '2'},
+		{'B', '0'}, {'B', '1'}, {'B', '2'}
+};
+
 static char intel_get_stepping(struct drm_device *dev)
 {
 	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
 			ARRAY_SIZE(skl_stepping_info)))
 		return skl_stepping_info[dev->pdev->revision].stepping;
+	else if (IS_BROXTON(dev) && (dev->pdev->revision <
+			ARRAY_SIZE(bxt_stepping_info)))
+		return bxt_stepping_info[dev->pdev->revision].stepping;
 	else
 		return -ENODATA;
 }
@@ -197,6 +205,9 @@ static char intel_get_substepping(struct drm_device *dev)
 	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
 			ARRAY_SIZE(skl_stepping_info)))
 		return skl_stepping_info[dev->pdev->revision].substepping;
+	else if (IS_BROXTON(dev) && (dev->pdev->revision <
+			ARRAY_SIZE(bxt_stepping_info)))
+		return bxt_stepping_info[dev->pdev->revision].substepping;
 	else
 		return -ENODATA;
 }
-- 
2.0.2

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

[PATCH 04/18] drm/i915/gen9: block disable call for pw1 if dmc firmware is present.

[Animesh Manna]

Grabbing a runtime pm reference with intel_runtime_pm_get will only
prevent device D3. But dmc firmware is required even earlier (namely
for the skl power well 1). DMC is responsible to save the status of
power well 1 and shut off the power well when panel is self refresh
mode of display is completely off.

Another interesting criteria to work dmc as expected is pw1 to be
enabled by driver and dmc will shut it off in its execution
sequence. If already disabled by driver dmc will get confuse and
behave differently than expected found during pc10 entry issue
for skl.

So berfore we disable power-well 1, added check if dmc firmware is
present and driver will not disable power well 1, but for any reason
if firmware is not present of failed to load we can shut off the
power well 1 which will save some power.

As skl is currently fully dependent on dmc to go in lowest possible
power state (dc6) but the same is not applicable for bxt. Display
engine can enter into dc9 without dmc, hence unblocking disable call.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6393b76..e6156d5 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1141,8 +1141,10 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
 		WARN_ON(!power_well->count);
-
-		if (!--power_well->count && i915.disable_power_well) {
+		if (IS_SKYLAKE(dev_priv) && (power_well->data == SKL_DISP_PW_1) &&
+			(intel_csr_load_status_get(dev_priv) == FW_LOADED))
+			DRM_DEBUG_KMS("dmc will disable pw1");
+		else if (!--power_well->count && i915.disable_power_well) {
 			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
 			power_well->hw_enabled = false;
 			power_well->ops->disable(dev_priv, power_well);
-- 
2.0.2

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

[PATCH 05/18] drm/i915/gen9: csr_init after runtime pm enable

[Animesh Manna]

As skl is fully dependent on dmc to go to low power state (dc5/dc6)
which requires a trigger from rpm and to ensure the dmc firmware
is available for runtime pm support rpm-reference-count is used
by not releasing the rpm reference acquire when starting the
firmware loader work.

So moved the intel_csr_ucode_init call after runtime pm enable.

Since have introduced a async work in next patches for loading
firmware and flush_work() will be used while disabling pw2. So
there's no need for any additional synchronization between the
dmc loader and trigger for low power state.

Note that for bxt without dmc, display engine can go to lowest
possible state (dc9), so releasing the rpm reference.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         |  6 +++---
 drivers/gpu/drm/i915/intel_csr.c        | 11 ++++++-----
 drivers/gpu/drm/i915/intel_runtime_pm.c | 17 +++--------------
 3 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b1f9e55..cdd3fbd 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -877,9 +877,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_uncore_init(dev);
 
-	/* Load CSR Firmware for SKL */
-	intel_csr_ucode_init(dev);
-
 	ret = i915_gem_gtt_init(dev);
 	if (ret)
 		goto out_freecsr;
@@ -1025,6 +1022,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	intel_runtime_pm_enable(dev_priv);
 
+	/* Load CSR Firmware for SKL */
+	intel_csr_ucode_init(dev);
+
 	i915_audio_component_init(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index f440299..e759190 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -404,10 +404,13 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
 out:
-	if (fw_loaded)
+	if (fw_loaded || IS_BROXTON(dev))
 		intel_runtime_pm_put(dev_priv);
-	else
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
+
+	/*
+	 * We require the dmc firmware for runtime pm on skl - leak the rpm
+	 * reference in case this failed to disable rpm on.
+	 */
 
 	release_firmware(fw);
 }
@@ -477,8 +480,6 @@ void intel_csr_ucode_fini(struct drm_device *dev)
 
 void assert_csr_loaded(struct drm_i915_private *dev_priv)
 {
-	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
-	     "CSR is not loaded.\n");
 	WARN(!I915_READ(CSR_PROGRAM_BASE),
 				"CSR program storage start is NULL\n");
 	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index e6156d5..a9bb299 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -644,21 +644,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
-				enum csr_state state;
-				/* TODO: wait for a completion event or
-				 * similar here instead of busy
-				 * waiting using wait_for function.
-				 */
-				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
-						FW_UNINITIALIZED, 1000);
-				if (state != FW_LOADED)
-					DRM_ERROR("CSR firmware not ready (%d)\n",
-							state);
+				if (SKL_ENABLE_DC6(dev))
+					skl_enable_dc6(dev_priv);
 				else
-					if (SKL_ENABLE_DC6(dev))
-						skl_enable_dc6(dev_priv);
-					else
-						gen9_enable_dc5(dev_priv);
+					gen9_enable_dc5(dev_priv);
 			}
 		}
 	}
-- 
2.0.2

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

[PATCH 06/18] drm/i915/gen9: move assert_csr_loaded into intel_rpm.c

[Animesh Manna]

From: Daniel Vetter <daniel.vetter@intel.com>

Avoids non-static functions since all the callers are in intel_rpm.c.
Only thing we need for that is to move the register definitions into
i915_reg.h.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 16 ++++++++++++++++
 drivers/gpu/drm/i915/intel_csr.c        | 24 ------------------------
 drivers/gpu/drm/i915/intel_drv.h        |  1 -
 drivers/gpu/drm/i915/intel_runtime_pm.c |  8 ++++++++
 4 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8cf7756..18169a1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7367,6 +7367,22 @@ enum skl_disp_power_wells {
 #define  DC_STATE_DEBUG                  0x45520
 #define  DC_STATE_DEBUG_MASK_MEMORY_UP	(1<<1)
 
+/*
+* SKL CSR registers for DC5 and DC6
+*/
+#define CSR_PROGRAM_BASE		0x80000
+#define CSR_SSP_BASE_ADDR_GEN9		0x00002FC0
+#define CSR_HTP_ADDR_SKL		0x00500034
+#define CSR_SSP_BASE			0x8F074
+#define CSR_HTP_SKL			0x8F004
+#define CSR_LAST_WRITE			0x8F034
+#define CSR_LAST_WRITE_VALUE		0xc003b400
+/* MMIO address range for CSR program (0x80000 - 0x82FFF) */
+#define CSR_MAX_FW_SIZE			0x2FFF
+#define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
+#define CSR_MMIO_START_RANGE	0x80000
+#define CSR_MMIO_END_RANGE		0x8FFFF
+
 /* 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_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index e759190..d3e78aa 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -47,22 +47,6 @@
 MODULE_FIRMWARE(I915_CSR_SKL);
 MODULE_FIRMWARE(I915_CSR_BXT);
 
-/*
-* SKL CSR registers for DC5 and DC6
-*/
-#define CSR_PROGRAM_BASE		0x80000
-#define CSR_SSP_BASE_ADDR_GEN9		0x00002FC0
-#define CSR_HTP_ADDR_SKL		0x00500034
-#define CSR_SSP_BASE			0x8F074
-#define CSR_HTP_SKL			0x8F004
-#define CSR_LAST_WRITE			0x8F034
-#define CSR_LAST_WRITE_VALUE		0xc003b400
-/* MMIO address range for CSR program (0x80000 - 0x82FFF) */
-#define CSR_MAX_FW_SIZE			0x2FFF
-#define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
-#define CSR_MMIO_START_RANGE	0x80000
-#define CSR_MMIO_END_RANGE		0x8FFFF
-
 struct intel_css_header {
 	/* 0x09 for DMC */
 	uint32_t module_type;
@@ -477,11 +461,3 @@ void intel_csr_ucode_fini(struct drm_device *dev)
 	intel_csr_load_status_set(dev_priv, FW_FAILED);
 	kfree(dev_priv->csr.dmc_payload);
 }
-
-void assert_csr_loaded(struct drm_i915_private *dev_priv)
-{
-	WARN(!I915_READ(CSR_PROGRAM_BASE),
-				"CSR program storage start is NULL\n");
-	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
-	WARN(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n");
-}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 47cef0e..020e6b4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1154,7 +1154,6 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
 					enum csr_state state);
 void intel_csr_load_program(struct drm_device *dev);
 void intel_csr_ucode_fini(struct drm_device *dev);
-void assert_csr_loaded(struct drm_i915_private *dev_priv);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index a9bb299..1067aa2 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -439,6 +439,14 @@ static void gen9_set_dc_state_debugmask_memory_up(
 	}
 }
 
+static void assert_csr_loaded(struct drm_i915_private *dev_priv)
+{
+	WARN(!I915_READ(CSR_PROGRAM_BASE),
+				"CSR program storage start is NULL\n");
+	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
+	WARN(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n");
+}
+
 static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-- 
2.0.2

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

[PATCH 07/18] drm/i915/gen9: Remove csr.state, csr_lock and related code.

[Animesh Manna]

From: Daniel Vetter <daniel.vetter@intel.com>

This removes two anti-patterns:
- Locking shouldn't be used to synchronize with async work (of any
  form, whether callbacks, workers or other threads). This is what the
  mutex_lock/unlock seems to have been for in intel_csr_load_program.
  Instead ordering should be ensured with the generic
  wait_for_completion()/complete(). Or more specific functions
  provided by the core kernel like e.g.
  flush_work()/cancel_work_sync() in the case of synchronizing with a
  work item.

- Don't invent own completion like the following code did with the
  (already removed) wait_for(csr_load_status_get()) pattern - it's
  really hard to get these right when you want them to be _really_
  correct (and be fast) in all cases. Furthermore it's easier to read
  code using the well-known primitives than new ones using
  non-standard names.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c  |  1 -
 drivers/gpu/drm/i915/i915_drv.c  |  6 -----
 drivers/gpu/drm/i915/i915_drv.h  | 10 ---------
 drivers/gpu/drm/i915/intel_csr.c | 48 +---------------------------------------
 drivers/gpu/drm/i915/intel_drv.h |  3 ---
 5 files changed, 1 insertion(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index cdd3fbd..e8a503b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -831,7 +831,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->mmio_flip_lock);
 	mutex_init(&dev_priv->sb_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
-	mutex_init(&dev_priv->csr_lock);
 
 	intel_pm_setup(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0d6775a..6e7edce 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1017,12 +1017,6 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
-	/*
-	 * This is to ensure that CSR isn't identified as loaded before
-	 * CSR-loading program is called during runtime-resume.
-	 */
-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
-
 	skl_uninit_cdclk(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a2eac8e..f986cc2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -734,12 +734,6 @@ struct intel_uncore {
 #define for_each_fw_domain(domain__, dev_priv__, i__) \
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
-enum csr_state {
-	FW_UNINITIALIZED = 0,
-	FW_LOADED,
-	FW_FAILED
-};
-
 struct intel_csr {
 	const char *fw_path;
 	__be32 *dmc_payload;
@@ -747,7 +741,6 @@ struct intel_csr {
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
 	uint32_t mmiodata[8];
-	enum csr_state state;
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
@@ -1699,9 +1692,6 @@ struct drm_i915_private {
 
 	struct intel_csr csr;
 
-	/* Display CSR-related protection */
-	struct mutex csr_lock;
-
 	struct intel_gmbus gmbus[GMBUS_NUM_PINS];
 
 	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index d3e78aa..99e4d6a 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -197,40 +197,6 @@ static char intel_get_substepping(struct drm_device *dev)
 }
 
 /**
- * intel_csr_load_status_get() - to get firmware loading status.
- * @dev_priv: i915 device.
- *
- * This function helps to get the firmware loading status.
- *
- * Return: Firmware loading status.
- */
-enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
-{
-	enum csr_state state;
-
-	mutex_lock(&dev_priv->csr_lock);
-	state = dev_priv->csr.state;
-	mutex_unlock(&dev_priv->csr_lock);
-
-	return state;
-}
-
-/**
- * intel_csr_load_status_set() - help to set firmware loading status.
- * @dev_priv: i915 device.
- * @state: enumeration of firmware loading status.
- *
- * Set the firmware loading status.
- */
-void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
-			enum csr_state state)
-{
-	mutex_lock(&dev_priv->csr_lock);
-	dev_priv->csr.state = state;
-	mutex_unlock(&dev_priv->csr_lock);
-}
-
-/**
  * intel_csr_load_program() - write the firmware from memory to register.
  * @dev: drm device.
  *
@@ -249,7 +215,6 @@ void intel_csr_load_program(struct drm_device *dev)
 		return;
 	}
 
-	mutex_lock(&dev_priv->csr_lock);
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
@@ -259,9 +224,6 @@ void intel_csr_load_program(struct drm_device *dev)
 		I915_WRITE(dev_priv->csr.mmioaddr[i],
 			dev_priv->csr.mmiodata[i]);
 	}
-
-	dev_priv->csr.state = FW_LOADED;
-	mutex_unlock(&dev_priv->csr_lock);
 }
 
 static void finish_csr_load(const struct firmware *fw, void *context)
@@ -419,11 +381,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 		csr->fw_path = I915_CSR_SKL;
 	else if (IS_BROXTON(dev))
 		csr->fw_path = I915_CSR_BXT;
-	else {
-		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
-		return;
-	}
 
 	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
 
@@ -438,10 +395,8 @@ void intel_csr_ucode_init(struct drm_device *dev)
 				&dev_priv->dev->pdev->dev,
 				GFP_KERNEL, dev_priv,
 				finish_csr_load);
-	if (ret) {
+	if (ret)
 		i915_firmware_load_error_print(csr->fw_path, ret);
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
-	}
 }
 
 /**
@@ -458,6 +413,5 @@ void intel_csr_ucode_fini(struct drm_device *dev)
 	if (!HAS_CSR(dev))
 		return;
 
-	intel_csr_load_status_set(dev_priv, FW_FAILED);
 	kfree(dev_priv->csr.dmc_payload);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 020e6b4..46143f6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1149,9 +1149,6 @@ u32 skl_plane_ctl_rotation(unsigned int rotation);
 
 /* intel_csr.c */
 void intel_csr_ucode_init(struct drm_device *dev);
-enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv);
-void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
-					enum csr_state state);
 void intel_csr_load_program(struct drm_device *dev);
 void intel_csr_ucode_fini(struct drm_device *dev);
 
-- 
2.0.2

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

[PATCH 08/18] drm/i915/gen9: Align line continuations in intel_csr.c.

[Animesh Manna]

From: Daniel Vetter <daniel.vetter@intel.com>

Standard is to align continuations of parameter lists and if
conditions to the opening ( in i915 and drm code.

Apply this across the entire file since it was sticking out a bit too
much.

Also align register definitions while at it.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  2 +-
 drivers/gpu/drm/i915/intel_csr.c | 50 ++++++++++++++++++++--------------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 18169a1..ea4cb96 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7380,7 +7380,7 @@ enum skl_disp_power_wells {
 /* MMIO address range for CSR program (0x80000 - 0x82FFF) */
 #define CSR_MAX_FW_SIZE			0x2FFF
 #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
-#define CSR_MMIO_START_RANGE	0x80000
+#define CSR_MMIO_START_RANGE		0x80000
 #define CSR_MMIO_END_RANGE		0x8FFFF
 
 /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 99e4d6a..0560db4 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -162,23 +162,23 @@ struct stepping_info {
 };
 
 static const struct stepping_info skl_stepping_info[] = {
-		{'A', '0'}, {'B', '0'}, {'C', '0'},
-		{'D', '0'}, {'E', '0'}, {'F', '0'},
-		{'G', '0'}, {'H', '0'}, {'I', '0'}
+	{'A', '0'}, {'B', '0'}, {'C', '0'},
+	{'D', '0'}, {'E', '0'}, {'F', '0'},
+	{'G', '0'}, {'H', '0'}, {'I', '0'}
 };
 
 static struct stepping_info bxt_stepping_info[] = {
-		{'A', '0'}, {'A', '1'}, {'A', '2'},
-		{'B', '0'}, {'B', '1'}, {'B', '2'}
+	{'A', '0'}, {'A', '1'}, {'A', '2'},
+	{'B', '0'}, {'B', '1'}, {'B', '2'}
 };
 
 static char intel_get_stepping(struct drm_device *dev)
 {
 	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
-			ARRAY_SIZE(skl_stepping_info)))
+				ARRAY_SIZE(skl_stepping_info)))
 		return skl_stepping_info[dev->pdev->revision].stepping;
 	else if (IS_BROXTON(dev) && (dev->pdev->revision <
-			ARRAY_SIZE(bxt_stepping_info)))
+				ARRAY_SIZE(bxt_stepping_info)))
 		return bxt_stepping_info[dev->pdev->revision].stepping;
 	else
 		return -ENODATA;
@@ -187,10 +187,10 @@ static char intel_get_stepping(struct drm_device *dev)
 static char intel_get_substepping(struct drm_device *dev)
 {
 	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
-			ARRAY_SIZE(skl_stepping_info)))
+				ARRAY_SIZE(skl_stepping_info)))
 		return skl_stepping_info[dev->pdev->revision].substepping;
 	else if (IS_BROXTON(dev) && (dev->pdev->revision <
-			ARRAY_SIZE(bxt_stepping_info)))
+				ARRAY_SIZE(bxt_stepping_info)))
 		return bxt_stepping_info[dev->pdev->revision].substepping;
 	else
 		return -ENODATA;
@@ -218,11 +218,11 @@ void intel_csr_load_program(struct drm_device *dev)
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
-			(u32 __force)payload[i]);
+			   (u32 __force)payload[i]);
 
 	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
 		I915_WRITE(dev_priv->csr.mmioaddr[i],
-			dev_priv->csr.mmiodata[i]);
+			   dev_priv->csr.mmiodata[i]);
 	}
 }
 
@@ -254,20 +254,20 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	/* Extract CSS Header information*/
 	css_header = (struct intel_css_header *)fw->data;
 	if (sizeof(struct intel_css_header) !=
-		(css_header->header_len * 4)) {
+	    (css_header->header_len * 4)) {
 		DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
-			(css_header->header_len * 4));
+			  (css_header->header_len * 4));
 		goto out;
 	}
 	readcount += sizeof(struct intel_css_header);
 
 	/* Extract Package Header information*/
 	package_header = (struct intel_package_header *)
-					&fw->data[readcount];
+		&fw->data[readcount];
 	if (sizeof(struct intel_package_header) !=
-		(package_header->header_len * 4)) {
+	    (package_header->header_len * 4)) {
 		DRM_ERROR("Firmware has wrong package header length %u bytes\n",
-			(package_header->header_len * 4));
+			  (package_header->header_len * 4));
 		goto out;
 	}
 	readcount += sizeof(struct intel_package_header);
@@ -275,7 +275,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	/* Search for dmc_offset to find firware binary. */
 	for (i = 0; i < package_header->num_entries; i++) {
 		if (package_header->fw_info[i].substepping == '*' &&
-			stepping == package_header->fw_info[i].stepping) {
+		    stepping == package_header->fw_info[i].stepping) {
 			dmc_offset = package_header->fw_info[i].offset;
 			break;
 		} else if (stepping == package_header->fw_info[i].stepping &&
@@ -283,7 +283,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 			dmc_offset = package_header->fw_info[i].offset;
 			break;
 		} else if (package_header->fw_info[i].stepping == '*' &&
-			package_header->fw_info[i].substepping == '*')
+			   package_header->fw_info[i].substepping == '*')
 			dmc_offset = package_header->fw_info[i].offset;
 	}
 	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
@@ -296,7 +296,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	dmc_header = (struct intel_dmc_header *)&fw->data[readcount];
 	if (sizeof(struct intel_dmc_header) != (dmc_header->header_len)) {
 		DRM_ERROR("Firmware has wrong dmc header length %u bytes\n",
-				(dmc_header->header_len));
+			  (dmc_header->header_len));
 		goto out;
 	}
 	readcount += sizeof(struct intel_dmc_header);
@@ -304,15 +304,15 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	/* Cache the dmc header info. */
 	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
 		DRM_ERROR("Firmware has wrong mmio count %u\n",
-						dmc_header->mmio_count);
+			  dmc_header->mmio_count);
 		goto out;
 	}
 	csr->mmio_count = dmc_header->mmio_count;
 	for (i = 0; i < dmc_header->mmio_count; i++) {
 		if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE &&
-			dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {
+		    dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {
 			DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
-						dmc_header->mmioaddr[i]);
+				  dmc_header->mmioaddr[i]);
 			goto out;
 		}
 		csr->mmioaddr[i] = dmc_header->mmioaddr[i];
@@ -392,9 +392,9 @@ void intel_csr_ucode_init(struct drm_device *dev)
 
 	/* CSR supported for platform, load firmware */
 	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
-				&dev_priv->dev->pdev->dev,
-				GFP_KERNEL, dev_priv,
-				finish_csr_load);
+				      &dev_priv->dev->pdev->dev,
+				      GFP_KERNEL, dev_priv,
+				      finish_csr_load);
 	if (ret)
 		i915_firmware_load_error_print(csr->fw_path, ret);
 }
-- 
2.0.2

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

[PATCH 09/18] drm/i915/gen9: Simplify csr loading failure printing.

[Animesh Manna]

From: Daniel Vetter <daniel.vetter@intel.com>

If we really want to we can be more verbose here, but we really don't
need an entire function for this.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  | 20 --------------------
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_csr.c |  8 +++-----
 3 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6e7edce..44dd28f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -537,26 +537,6 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
 	return true;
 }
 
-void i915_firmware_load_error_print(const char *fw_path, int err)
-{
-	DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err);
-
-	/*
-	 * If the reason is not known assume -ENOENT since that's the most
-	 * usual failure mode.
-	 */
-	if (!err)
-		err = -ENOENT;
-
-	if (!(IS_BUILTIN(CONFIG_DRM_I915) && err == -ENOENT))
-		return;
-
-	DRM_ERROR(
-	  "The driver is built-in, so to load the firmware you need to\n"
-	  "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n"
-	  "in your initrd/initramfs image.\n");
-}
-
 static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f986cc2..94cf79a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2631,7 +2631,6 @@ extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
 extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
 extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
-void i915_firmware_load_error_print(const char *fw_path, int err);
 
 /* intel_hotplug.c */
 void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask);
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 0560db4..381d891 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -241,10 +241,8 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	__be32 *dmc_payload;
 	bool fw_loaded = false;
 
-	if (!fw) {
-		i915_firmware_load_error_print(csr->fw_path, 0);
+	if (!fw)
 		goto out;
-	}
 
 	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
 		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
@@ -352,6 +350,8 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 out:
 	if (fw_loaded || IS_BROXTON(dev))
 		intel_runtime_pm_put(dev_priv);
+	else
+		DRM_ERROR("Failed to load DMC firmware, disabling rpm\n");
 
 	/*
 	 * We require the dmc firmware for runtime pm on skl - leak the rpm
@@ -395,8 +395,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 				      &dev_priv->dev->pdev->dev,
 				      GFP_KERNEL, dev_priv,
 				      finish_csr_load);
-	if (ret)
-		i915_firmware_load_error_print(csr->fw_path, ret);
 }
 
 /**
-- 
2.0.2

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

[PATCH 10/18] drm/i915/gen9: extract parse_csr_fw.

[Animesh Manna]

From: Daniel Vetter <daniel.vetter@intel.com>

The loader function will get a bit more complicated soon, extract the
parsing code to make the control flow clearer. While doing that just
use dev_priv->csr.dmc_payload as the indicator for whether it all
suceeded or not.

Also restrict the forced big-edian casting to just one place.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c        | 58 +++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_runtime_pm.c |  2 +-
 2 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 381d891..0813ff2 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -207,7 +207,7 @@ static char intel_get_substepping(struct drm_device *dev)
 void intel_csr_load_program(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	__be32 *payload = dev_priv->csr.dmc_payload;
+	uint32_t *payload = dev_priv->csr.dmc_payload;
 	uint32_t i, fw_size;
 
 	if (!IS_GEN9(dev)) {
@@ -217,8 +217,7 @@ void intel_csr_load_program(struct drm_device *dev)
 
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
-		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
-			   (u32 __force)payload[i]);
+		I915_WRITE(CSR_PROGRAM_BASE + i * 4, payload[i]);
 
 	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
 		I915_WRITE(dev_priv->csr.mmioaddr[i],
@@ -226,9 +225,9 @@ void intel_csr_load_program(struct drm_device *dev)
 	}
 }
 
-static void finish_csr_load(const struct firmware *fw, void *context)
+static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
+			      const struct firmware *fw)
 {
-	struct drm_i915_private *dev_priv = context;
 	struct drm_device *dev = dev_priv->dev;
 	struct intel_css_header *css_header;
 	struct intel_package_header *package_header;
@@ -238,15 +237,11 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	char substepping = intel_get_substepping(dev);
 	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
 	uint32_t i;
-	__be32 *dmc_payload;
-	bool fw_loaded = false;
-
-	if (!fw)
-		goto out;
+	uint32_t *dmc_payload;
 
 	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
 		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
-		goto out;
+		return NULL;
 	}
 
 	/* Extract CSS Header information*/
@@ -255,7 +250,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	    (css_header->header_len * 4)) {
 		DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
 			  (css_header->header_len * 4));
-		goto out;
+		return NULL;
 	}
 	readcount += sizeof(struct intel_css_header);
 
@@ -266,7 +261,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	    (package_header->header_len * 4)) {
 		DRM_ERROR("Firmware has wrong package header length %u bytes\n",
 			  (package_header->header_len * 4));
-		goto out;
+		return NULL;
 	}
 	readcount += sizeof(struct intel_package_header);
 
@@ -286,7 +281,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	}
 	if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
 		DRM_ERROR("Firmware not supported for %c stepping\n", stepping);
-		goto out;
+		return NULL;
 	}
 	readcount += dmc_offset;
 
@@ -295,7 +290,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	if (sizeof(struct intel_dmc_header) != (dmc_header->header_len)) {
 		DRM_ERROR("Firmware has wrong dmc header length %u bytes\n",
 			  (dmc_header->header_len));
-		goto out;
+		return NULL;
 	}
 	readcount += sizeof(struct intel_dmc_header);
 
@@ -303,7 +298,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
 		DRM_ERROR("Firmware has wrong mmio count %u\n",
 			  dmc_header->mmio_count);
-		goto out;
+		return NULL;
 	}
 	csr->mmio_count = dmc_header->mmio_count;
 	for (i = 0; i < dmc_header->mmio_count; i++) {
@@ -311,7 +306,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 		    dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {
 			DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
 				  dmc_header->mmioaddr[i]);
-			goto out;
+			return NULL;
 		}
 		csr->mmioaddr[i] = dmc_header->mmioaddr[i];
 		csr->mmiodata[i] = dmc_header->mmiodata[i];
@@ -321,17 +316,16 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	nbytes = dmc_header->fw_size * 4;
 	if (nbytes > CSR_MAX_FW_SIZE) {
 		DRM_ERROR("CSR firmware too big (%u) bytes\n", nbytes);
-		goto out;
+		return NULL;
 	}
 	csr->dmc_fw_size = dmc_header->fw_size;
 
-	csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
-	if (!csr->dmc_payload) {
+	dmc_payload = kmalloc(nbytes, GFP_KERNEL);
+	if (!dmc_payload) {
 		DRM_ERROR("Memory allocation failed for dmc payload\n");
-		goto out;
+		return NULL;
 	}
 
-	dmc_payload = csr->dmc_payload;
 	for (i = 0; i < dmc_header->fw_size; i++) {
 		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
 		/*
@@ -339,16 +333,30 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 		 * little-endian format in the firmware image and programmed
 		 * as 32 bit big-endian format to memory.
 		 */
-		dmc_payload[i] = cpu_to_be32(*tmp);
+		dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
 	}
 
+	return dmc_payload;
+}
+
+static void finish_csr_load(const struct firmware *fw, void *context)
+{
+	struct drm_i915_private *dev_priv = context;
+	struct drm_device *dev = dev_priv->dev;
+
+	if (!fw)
+		goto out;
+
+	dev_priv->csr.dmc_payload = parse_csr_fw(dev_priv, fw);
+	if (!dev_priv->csr.dmc_payload)
+		goto out;
+
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
-	fw_loaded = true;
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
 out:
-	if (fw_loaded || IS_BROXTON(dev))
+	if (dev_priv->csr.dmc_payload || IS_BROXTON(dev))
 		intel_runtime_pm_put(dev_priv);
 	else
 		DRM_ERROR("Failed to load DMC firmware, disabling rpm\n");
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 1067aa2..5f1ae23 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1139,7 +1139,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
 	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
 		WARN_ON(!power_well->count);
 		if (IS_SKYLAKE(dev_priv) && (power_well->data == SKL_DISP_PW_1) &&
-			(intel_csr_load_status_get(dev_priv) == FW_LOADED))
+			dev_priv->csr.dmc_payload)
 			DRM_DEBUG_KMS("dmc will disable pw1");
 		else if (!--power_well->count && i915.disable_power_well) {
 			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
-- 
2.0.2

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

[PATCH 11/18] drm/i915/gen9: Don't try to load garbage dmc firmware on resume

[Animesh Manna]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

We need to make sure we don't put garbage into the hw if dmc firmware
loading failed mid-thru.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 2 +-
 drivers/gpu/drm/i915/intel_csr.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 94cf79a..1117300 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -736,7 +736,7 @@ struct intel_uncore {
 
 struct intel_csr {
 	const char *fw_path;
-	__be32 *dmc_payload;
+	uint32_t *dmc_payload;
 	uint32_t dmc_fw_size;
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 0813ff2..56381cf 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -215,6 +215,9 @@ void intel_csr_load_program(struct drm_device *dev)
 		return;
 	}
 
+	if (!dev_priv->csr.dmc_payload)
+		return;
+
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4, payload[i]);
-- 
2.0.2

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

[PATCH 12/18] drm/i915/gen9: Use dev_priv in csr functions

[Animesh Manna]

From: Daniel Vetter <daniel.vetter@intel.com>

As all csr firmware related opertion are not using any
any data structures of drm framework level, so better to
use dev_priv instead of dev. it's a new style! :)

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c  |  6 +++---
 drivers/gpu/drm/i915/i915_drv.c  |  4 +---
 drivers/gpu/drm/i915/intel_csr.c | 31 +++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h |  6 +++---
 4 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e8a503b..15b0642 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1022,7 +1022,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	intel_runtime_pm_enable(dev_priv);
 
 	/* Load CSR Firmware for SKL */
-	intel_csr_ucode_init(dev);
+	intel_csr_ucode_init(dev_priv);
 
 	i915_audio_component_init(dev_priv);
 
@@ -1052,7 +1052,7 @@ out_mtrrfree:
 out_gtt:
 	i915_global_gtt_cleanup(dev);
 out_freecsr:
-	intel_csr_ucode_fini(dev);
+	intel_csr_ucode_fini(dev_priv);
 	intel_uncore_fini(dev);
 	pci_iounmap(dev->pdev, dev_priv->regs);
 put_bridge:
@@ -1133,7 +1133,7 @@ int i915_driver_unload(struct drm_device *dev)
 	intel_fbc_cleanup_cfb(dev_priv);
 	i915_gem_cleanup_stolen(dev);
 
-	intel_csr_ucode_fini(dev);
+	intel_csr_ucode_fini(dev_priv);
 
 	intel_teardown_gmbus(dev);
 	intel_teardown_mchbar(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44dd28f..98343eb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1043,10 +1043,8 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
 
 static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
 	skl_init_cdclk(dev_priv);
-	intel_csr_load_program(dev);
+	intel_csr_load_program(dev_priv);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 56381cf..f42449b 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -198,19 +198,18 @@ static char intel_get_substepping(struct drm_device *dev)
 
 /**
  * intel_csr_load_program() - write the firmware from memory to register.
- * @dev: drm device.
+ * @dev_priv: i915 drm device.
  *
  * CSR firmware is read from a .bin file and kept in internal memory one time.
  * Everytime display comes back from low power state this function is called to
  * copy the firmware from internal memory to registers.
  */
-void intel_csr_load_program(struct drm_device *dev)
+void intel_csr_load_program(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t *payload = dev_priv->csr.dmc_payload;
 	uint32_t i, fw_size;
 
-	if (!IS_GEN9(dev)) {
+	if (!IS_GEN9(dev_priv)) {
 		DRM_ERROR("No CSR support available for this platform\n");
 		return;
 	}
@@ -345,7 +344,6 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 static void finish_csr_load(const struct firmware *fw, void *context)
 {
 	struct drm_i915_private *dev_priv = context;
-	struct drm_device *dev = dev_priv->dev;
 
 	if (!fw)
 		goto out;
@@ -355,11 +353,11 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 		goto out;
 
 	/* load csr program during system boot, as needed for DC states */
-	intel_csr_load_program(dev);
+	intel_csr_load_program(dev_priv);
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
 out:
-	if (dev_priv->csr.dmc_payload || IS_BROXTON(dev))
+	if (dev_priv->csr.dmc_payload || IS_BROXTON(dev_priv))
 		intel_runtime_pm_put(dev_priv);
 	else
 		DRM_ERROR("Failed to load DMC firmware, disabling rpm\n");
@@ -374,23 +372,22 @@ out:
 
 /**
  * intel_csr_ucode_init() - initialize the firmware loading.
- * @dev: drm device.
+ * @dev_priv: i915 drm device.
  *
  * This function is called at the time of loading the display driver to read
  * firmware from a .bin file and copied into a internal memory.
  */
-void intel_csr_ucode_init(struct drm_device *dev)
+void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_csr *csr = &dev_priv->csr;
 	int ret;
 
-	if (!HAS_CSR(dev))
+	if (!HAS_CSR(dev_priv))
 		return;
 
-	if (IS_SKYLAKE(dev))
+	if (IS_SKYLAKE(dev_priv))
 		csr->fw_path = I915_CSR_SKL;
-	else if (IS_BROXTON(dev))
+	else if (IS_BROXTON(dev_priv))
 		csr->fw_path = I915_CSR_BXT;
 
 	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
@@ -410,16 +407,14 @@ void intel_csr_ucode_init(struct drm_device *dev)
 
 /**
  * intel_csr_ucode_fini() - unload the CSR firmware.
- * @dev: drm device.
+ * @dev_priv: i915 drm device.
  *
  * Firmmware unloading includes freeing the internal momory and reset the
  * firmware loading status.
  */
-void intel_csr_ucode_fini(struct drm_device *dev)
+void intel_csr_ucode_fini(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (!HAS_CSR(dev))
+	if (!HAS_CSR(dev_priv))
 		return;
 
 	kfree(dev_priv->csr.dmc_payload);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46143f6..644286f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1148,9 +1148,9 @@ u32 skl_plane_ctl_tiling(uint64_t fb_modifier);
 u32 skl_plane_ctl_rotation(unsigned int rotation);
 
 /* intel_csr.c */
-void intel_csr_ucode_init(struct drm_device *dev);
-void intel_csr_load_program(struct drm_device *dev);
-void intel_csr_ucode_fini(struct drm_device *dev);
+void intel_csr_ucode_init(struct drm_i915_private *);
+void intel_csr_load_program(struct drm_i915_private *);
+void intel_csr_ucode_fini(struct drm_i915_private *);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
2.0.2

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

[PATCH 13/18] drm/i915: Use request_firmware and our own async work

[Animesh Manna]

From: Daniel Vetter <daniel.vetter@intel.com>

Two benefits:
- We can use FW_LOADER_USERSPACE_FALLBACK.
- We can use flush_work to synchronize with the oustanding worker,
  which is a notch more obvious what it does than having a special
  completion.

The next patch will properly synchronize against the async loader in
the resume and unload code.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_csr.c | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1117300..b45f6b4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -735,6 +735,7 @@ struct intel_uncore {
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
 struct intel_csr {
+	struct work_struct work;
 	const char *fw_path;
 	uint32_t *dmc_payload;
 	uint32_t dmc_fw_size;
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index f42449b..36386324 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -341,10 +341,16 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 	return dmc_payload;
 }
 
-static void finish_csr_load(const struct firmware *fw, void *context)
+static void csr_load_work_fn(struct work_struct *work)
 {
-	struct drm_i915_private *dev_priv = context;
+	struct drm_i915_private *dev_priv;
+	const struct firmware *fw;
+	int ret;
+
+	dev_priv = container_of(work, typeof(*dev_priv), csr.work);
 
+	ret = request_firmware(&fw, dev_priv->csr.fw_path,
+			       &dev_priv->dev->pdev->dev);
 	if (!fw)
 		goto out;
 
@@ -380,7 +386,8 @@ out:
 void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_csr *csr = &dev_priv->csr;
-	int ret;
+
+	INIT_WORK(&dev_priv->csr.work, csr_load_work_fn);
 
 	if (!HAS_CSR(dev_priv))
 		return;
@@ -390,19 +397,12 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
 	else if (IS_BROXTON(dev_priv))
 		csr->fw_path = I915_CSR_BXT;
 
-	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
-
 	/*
 	 * Obtain a runtime pm reference, until CSR is loaded,
 	 * to avoid entering runtime-suspend.
 	 */
 	intel_runtime_pm_get(dev_priv);
-
-	/* CSR supported for platform, load firmware */
-	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
-				      &dev_priv->dev->pdev->dev,
-				      GFP_KERNEL, dev_priv,
-				      finish_csr_load);
+	schedule_work(&dev_priv->csr.work);
 }
 
 /**
-- 
2.0.2

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

[PATCH 14/18] drm/i915/gen9: Use flush_work to synchronize with dmc loader

[Animesh Manna]

As power well 1 is superset of power well 2 and always pw2
will be disabled first and then pw1. On the other hand dmc
is responsible to save & restore back pw1 when display
engine goes and come back from low power state. Before
disabling pw1 dmc must be loaded, so adding flush_work()
while disabling pw2 which ensure that firmware will be
available before disabling pw1 in suspend flow.

Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         | 2 --
 drivers/gpu/drm/i915/intel_csr.c        | 2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 98343eb..ddf8a25 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -995,8 +995,6 @@ static int i915_pm_resume(struct device *dev)
 
 static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
-	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
-
 	skl_uninit_cdclk(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 36386324..1858f02 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -417,5 +417,7 @@ void intel_csr_ucode_fini(struct drm_i915_private *dev_priv)
 	if (!HAS_CSR(dev_priv))
 		return;
 
+	flush_work(&dev_priv->csr.work);
+
 	kfree(dev_priv->csr.dmc_payload);
 }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 5f1ae23..a5059e8 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -652,6 +652,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
+				flush_work(&dev_priv->csr.work);
 				if (SKL_ENABLE_DC6(dev))
 					skl_enable_dc6(dev_priv);
 				else
-- 
2.0.2

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

[PATCH 15/18] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Animesh Manna]

Mmio register access after dc6/dc5 entry is causing the
system hang, so enabling dc6 as the last call in suspend flow.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  6 ++++++
 drivers/gpu/drm/i915/intel_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ddf8a25..77b35fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -995,6 +995,9 @@ static int i915_pm_resume(struct device *dev)
 
 static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
+	if (dev_priv->csr.dmc_payload)
+		skl_enable_dc6(dev_priv);
+
 	skl_uninit_cdclk(dev_priv);
 
 	return 0;
@@ -1041,6 +1044,9 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
 
 static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
+	if (dev_priv->csr.dmc_payload)
+		skl_disable_dc6(dev_priv);
+
 	skl_init_cdclk(dev_priv);
 	intel_csr_load_program(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 644286f..0d13f50 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1117,6 +1117,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);
 void bxt_disable_dc9(struct drm_i915_private *dev_priv);
 void skl_init_cdclk(struct drm_i915_private *dev_priv);
 void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
+void skl_enable_dc6(struct drm_i915_private *dev_priv);
+void skl_disable_dc6(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 a5059e8..ddae00e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -540,7 +540,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
 		"DC6 already programmed to be disabled.\n");
 }
 
-static void skl_enable_dc6(struct drm_i915_private *dev_priv)
+void skl_enable_dc6(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
 
@@ -557,7 +557,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
 	POSTING_READ(DC_STATE_EN);
 }
 
-static void skl_disable_dc6(struct drm_i915_private *dev_priv)
+void skl_disable_dc6(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
 
@@ -618,10 +618,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 				!I915_READ(HSW_PWR_WELL_BIOS),
 				"Invalid for power well status to be enabled, unless done by the BIOS, \
 				when request is to disable!\n");
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
+			if (power_well->data == SKL_DISP_PW_2) {
+				if (GEN9_ENABLE_DC5(dev))
+					gen9_disable_dc5(dev_priv);
 				if (SKL_ENABLE_DC6(dev)) {
-					skl_disable_dc6(dev_priv);
 					/*
 					 * DDI buffer programming unnecessary during driver-load/resume
 					 * as it's already done during modeset initialization then.
@@ -629,8 +629,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 					 */
 					if (!dev_priv->power_domains.initializing)
 						intel_prepare_ddi(dev);
-				} else {
-					gen9_disable_dc5(dev_priv);
 				}
 			}
 			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
@@ -650,12 +648,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			POSTING_READ(HSW_PWR_WELL_DRIVER);
 			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
 
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
+			if (power_well->data == SKL_DISP_PW_2) {
 				flush_work(&dev_priv->csr.work);
-				if (SKL_ENABLE_DC6(dev))
-					skl_enable_dc6(dev_priv);
-				else
+				if (GEN9_ENABLE_DC5(dev))
 					gen9_enable_dc5(dev_priv);
 			}
 		}
-- 
2.0.2

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

[PATCH 16/18] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Animesh Manna]

While display engine entering into low power state no need to disable
cdclk pll as CSR firmware of dmc will take care. If pll is already
enabled firmware execution sequence will be blocked. This is one
of the criteria for dmc to work properly.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af0bcfe..ef2ef4d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
 	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
 		DRM_ERROR("DBuf power disable timeout\n");
 
-	/* disable DPLL0 */
-	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
-	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
-		DRM_ERROR("Couldn't disable DPLL0\n");
+	if (dev_priv->csr.dmc_payload) {
+		/* disable DPLL0 */
+		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
+					~LCPLL_PLL_ENABLE);
+		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
+			DRM_ERROR("Couldn't disable DPLL0\n");
+	}
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
 }
-- 
2.0.2

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

[PATCH 17/18] drm/i915/skl: Removed csr firmware load in resume path.

[Animesh Manna]

As csr firmware is taking care of loading the firmware,
so no need for driver to load again.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 77b35fd..f5e720b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1048,7 +1048,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 		skl_disable_dc6(dev_priv);
 
 	skl_init_cdclk(dev_priv);
-	intel_csr_load_program(dev_priv);
 
 	return 0;
 }
-- 
2.0.2

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

[PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

[Animesh Manna]

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 1858f02..50779e0 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -328,16 +328,7 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 		return NULL;
 	}
 
-	for (i = 0; i < dmc_header->fw_size; i++) {
-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
-		/*
-		 * The firmware payload is an array of 32 bit words stored in
-		 * little-endian format in the firmware image and programmed
-		 * as 32 bit big-endian format to memory.
-		 */
-		dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
-	}
-
+	memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);
 	return dmc_payload;
 }
 
-- 
2.0.2

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

Re: [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6864
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              295/295              294/295
SNB                                  315/315              315/315
IVB                                  342/342              342/342
BYT                 -2              285/285              283/285
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-rmfb-interruptible      PASS(1)      DMESG_WARN(1)
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/18] Redesign of dmc firmware loading.

[Sunil Kamath]

On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
> Display microcontroller(DMC) used to save and restore display engine status
> while entering into low power display states for gen9 platform.
> Though skylake and broxton both are gen9 platform but dmc act diferently.
> Skylake is solely dependednt on dmc for entering into low power
> state - namely dc5 and dc6. Whereas broxton has low power state
> dc5 and dc9 and dc9 does not need dmc.
>
> As both gen9 platforms behaviour is different with respect to dmc,
> so software design will be different and follow the below design
> principles.

Thanks Animesh for pushing these latest patches in sequence. [including 
all latest bug fix and firmware load design change request].
With this we can already discard my older BXT specific DMC patches.

Also below SKL patches are very important to be merged asap. As they fix 
some of the major issues to achieve PC10.[which was blocked because of 
DMC DC6].

Will start reviewing right away.


Though we have positive test results already, need a detailed review to 
ensure that both SKL/BXT everything taken care also compatible with all 
linux distros.

-Sunil

>
> For skylake:
> ------------
> If firmware loading is successful,
> - Driver can goto D3 in suspend time.
> - Will not disable power-well-1 as dmc will handle save/restore/shutdown.
> If firmware loading is failed,
> - Leak rpm which will block D3 in suspend.
> - Will disable power-well-1 which will save some power.
>
> For broxton:
> ------------
> Irrespective of firmware loading succesful/failed driver should not
> block D3 during suspend as dc9 (lowest possible state) is independent
> of dmc and will not block disable call of power-well-1.
>
> During debugging PC10 entry issue for skylake found below observation,
> - dmc will get confuse if driver already disable power-well-1 during
> dc6 entry and will not work as expected.
> - The above same applicable for cdclk pll as well.
> - mmio read/write after dc6 trigger will cause display engine hang.
>
> Based on Daniel's review comments and thiinking of above pointers
> following patches is created.
>
> Animesh Manna (10):
>    drm/i915/bxt: Path added of dmc firmware ver1 for BXT
>    drm/i915/bxt: Modified HAS_CSR, added support for BXT.
>    drm/i915/bxt: Stepping info added for bxt.
>    drm/i915/gen9: block disable call for pw1 if dmc firmware is present.
>    drm/i915/gen9: csr_init after runtime pm enable
>    drm/i915/gen9: Use flush_work to synchronize with dmc loader
>    drm/i915/skl: Making DC6 entry is the last call in suspend flow.
>    drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.
>    drm/i915/skl: Removed csr firmware load in resume path.
>    drm/i915/gen9: Removed byte swapping for csr firmware.
>
> Daniel Vetter (8):
>    drm/i915/gen9: move assert_csr_loaded into intel_rpm.c
>    drm/i915/gen9: Remove csr.state, csr_lock and related code.
>    drm/i915/gen9: Align line continuations in intel_csr.c.
>    drm/i915/gen9: Simplify csr loading failure printing.
>    drm/i915/gen9: extract parse_csr_fw.
>    drm/i915/gen9: Don't try to load garbage dmc firmware on resume
>    drm/i915/gen9: Use dev_priv in csr functions
>    drm/i915: Use request_firmware and our own async work
>
>   drivers/gpu/drm/i915/i915_dma.c         |  11 +-
>   drivers/gpu/drm/i915/i915_drv.c         |  33 +----
>   drivers/gpu/drm/i915/i915_drv.h         |  16 +--
>   drivers/gpu/drm/i915/i915_reg.h         |  16 +++
>   drivers/gpu/drm/i915/intel_csr.c        | 245 ++++++++++++--------------------
>   drivers/gpu/drm/i915/intel_display.c    |  11 +-
>   drivers/gpu/drm/i915/intel_drv.h        |  12 +-
>   drivers/gpu/drm/i915/intel_runtime_pm.c |  47 +++---
>   8 files changed, 155 insertions(+), 236 deletions(-)
>

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

Re: [PATCH 01/18] drm/i915/bxt: Path added of dmc firmware ver1 for BXT

[Sunil Kamath]

On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---

Please update the commit message header relevant details.
even one liner is sufficient.

Minor thing to take care.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>

- Sunil
>   drivers/gpu/drm/i915/intel_csr.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 6d8a7bf..1866426 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -42,8 +42,10 @@
>    */
>   
>   #define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
> +#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin"
>   
>   MODULE_FIRMWARE(I915_CSR_SKL);
> +MODULE_FIRMWARE(I915_CSR_BXT);
>   
>   /*
>   * SKL CSR registers for DC5 and DC6
> @@ -417,6 +419,8 @@ void intel_csr_ucode_init(struct drm_device *dev)
>   
>   	if (IS_SKYLAKE(dev))
>   		csr->fw_path = I915_CSR_SKL;
> +	else if (IS_BROXTON(dev))
> +		csr->fw_path = I915_CSR_BXT;
>   	else {
>   		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
>   		intel_csr_load_status_set(dev_priv, FW_FAILED);

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

Re: [PATCH 02/18] drm/i915/bxt: Modified HAS_CSR, added support for BXT.

[Sunil Kamath]

On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
> Modified HAS_CSR macro defination which earlier only supported
> for skl, now added support for BXT.
>
> Cc: Vetter, Daniel <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b94ada9..a2eac8e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2542,7 +2542,7 @@ struct drm_i915_cmd_table {
>   #define HAS_RC6(dev)		(INTEL_INFO(dev)->gen >= 6)
>   #define HAS_RC6p(dev)		(INTEL_INFO(dev)->gen == 6 || IS_IVYBRIDGE(dev))
>   
> -#define HAS_CSR(dev)	(IS_SKYLAKE(dev))
> +#define HAS_CSR(dev)	(IS_SKYLAKE(dev) || IS_BROXTON(dev))

with this change, we are adding DMC support for entire Gen9.
You can replace SKL and BXT specific check with Gen9 check alone.

Again minor change to take care.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>

-Sunil

>   
>   #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
>   				    INTEL_INFO(dev)->gen >= 8)

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

Re: [PATCH 03/18] drm/i915/bxt: Stepping info added for bxt.

[Sunil Kamath]

On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
> Added stepping info in intel_csr.c which is required to extract
> specific firmware from packaged dmc firmware.
>
> Cc: Vetter, Daniel <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_csr.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 1866426..f440299 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -183,11 +183,19 @@ static const struct stepping_info skl_stepping_info[] = {
>   		{'G', '0'}, {'H', '0'}, {'I', '0'}
>   };
>   
> +static struct stepping_info bxt_stepping_info[] = {
> +		{'A', '0'}, {'A', '1'}, {'A', '2'},
> +		{'B', '0'}, {'B', '1'}, {'B', '2'}
> +};
> +

I understand that these stepping structure is as per present limited info.

>   static char intel_get_stepping(struct drm_device *dev)
>   {
>   	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
>   			ARRAY_SIZE(skl_stepping_info)))
>   		return skl_stepping_info[dev->pdev->revision].stepping;
> +	else if (IS_BROXTON(dev) && (dev->pdev->revision <
> +			ARRAY_SIZE(bxt_stepping_info)))
> +		return bxt_stepping_info[dev->pdev->revision].stepping;
>   	else
>   		return -ENODATA;
>   }
> @@ -197,6 +205,9 @@ static char intel_get_substepping(struct drm_device *dev)
>   	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
>   			ARRAY_SIZE(skl_stepping_info)))
>   		return skl_stepping_info[dev->pdev->revision].substepping;
> +	else if (IS_BROXTON(dev) && (dev->pdev->revision <
> +			ARRAY_SIZE(bxt_stepping_info)))
> +		return bxt_stepping_info[dev->pdev->revision].substepping;
>   	else
>   		return -ENODATA;
>   }

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

Re: [PATCH 04/18] drm/i915/gen9: block disable call for pw1 if dmc firmware is present.

[Daniel Vetter]

On Sun, Jul 26, 2015 at 12:30:25AM +0530, Animesh Manna wrote:
> Grabbing a runtime pm reference with intel_runtime_pm_get will only
> prevent device D3. But dmc firmware is required even earlier (namely
> for the skl power well 1). DMC is responsible to save the status of
> power well 1 and shut off the power well when panel is self refresh
> mode of display is completely off.
> 
> Another interesting criteria to work dmc as expected is pw1 to be
> enabled by driver and dmc will shut it off in its execution
> sequence. If already disabled by driver dmc will get confuse and
> behave differently than expected found during pc10 entry issue
> for skl.
> 
> So berfore we disable power-well 1, added check if dmc firmware is
> present and driver will not disable power well 1, but for any reason
> if firmware is not present of failed to load we can shut off the
> power well 1 which will save some power.
> 
> As skl is currently fully dependent on dmc to go in lowest possible
> power state (dc6) but the same is not applicable for bxt. Display
> engine can enter into dc9 without dmc, hence unblocking disable call.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>

Please use the approach I've laid out in my original patch series with
"drm/i915: use correct power domain for csr loading" and "drm/i915: Only
allow rpm on gen9+ with dmc loaded". Your approach here requires a
flush_work in the runtime pm callbacks which can deadlock.

If you want to make dmc optional on bxt then you need to adjust the 2nd
patch a bit to no longer leak the runtime pm reference. Your approach here
is an inversion of control and that doesn't work well since control
inversions very easily lead to locking inversions.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6393b76..e6156d5 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1141,8 +1141,10 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  
>  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
>  		WARN_ON(!power_well->count);
> -
> -		if (!--power_well->count && i915.disable_power_well) {
> +		if (IS_SKYLAKE(dev_priv) && (power_well->data == SKL_DISP_PW_1) &&
> +			(intel_csr_load_status_get(dev_priv) == FW_LOADED))
> +			DRM_DEBUG_KMS("dmc will disable pw1");
> +		else if (!--power_well->count && i915.disable_power_well) {
>  			DRM_DEBUG_KMS("disabling %s\n", power_well->name);
>  			power_well->hw_enabled = false;
>  			power_well->ops->disable(dev_priv, power_well);
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: [PATCH 05/18] drm/i915/gen9: csr_init after runtime pm enable

[Daniel Vetter]

On Sun, Jul 26, 2015 at 12:30:26AM +0530, Animesh Manna wrote:
> As skl is fully dependent on dmc to go to low power state (dc5/dc6)
> which requires a trigger from rpm and to ensure the dmc firmware
> is available for runtime pm support rpm-reference-count is used
> by not releasing the rpm reference acquire when starting the
> firmware loader work.
> 
> So moved the intel_csr_ucode_init call after runtime pm enable.
> 
> Since have introduced a async work in next patches for loading
> firmware and flush_work() will be used while disabling pw2. So
> there's no need for any additional synchronization between the
> dmc loader and trigger for low power state.
> 
> Note that for bxt without dmc, display engine can go to lowest
> possible state (dc9), so releasing the rpm reference.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c         |  6 +++---
>  drivers/gpu/drm/i915/intel_csr.c        | 11 ++++++-----
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 17 +++--------------
>  3 files changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index b1f9e55..cdd3fbd 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -877,9 +877,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_uncore_init(dev);
>  
> -	/* Load CSR Firmware for SKL */
> -	intel_csr_ucode_init(dev);
> -
>  	ret = i915_gem_gtt_init(dev);
>  	if (ret)
>  		goto out_freecsr;
> @@ -1025,6 +1022,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	intel_runtime_pm_enable(dev_priv);
>  
> +	/* Load CSR Firmware for SKL */
> +	intel_csr_ucode_init(dev);
> +
>  	i915_audio_component_init(dev_priv);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index f440299..e759190 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -404,10 +404,13 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>  
>  	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
>  out:
> -	if (fw_loaded)
> +	if (fw_loaded || IS_BROXTON(dev))

Imo the IS_BXT should be separate patch.

>  		intel_runtime_pm_put(dev_priv);
> -	else
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
> +
> +	/*
> +	 * We require the dmc firmware for runtime pm on skl - leak the rpm
> +	 * reference in case this failed to disable rpm on.
> +	 */

Looks like part of my "drm/i915: Only allow rpm on gen9+ with dmc loaded"
patch snuck in here, should be split out again.
-Daniel

>  
>  	release_firmware(fw);
>  }
> @@ -477,8 +480,6 @@ void intel_csr_ucode_fini(struct drm_device *dev)
>  
>  void assert_csr_loaded(struct drm_i915_private *dev_priv)
>  {
> -	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
> -	     "CSR is not loaded.\n");
>  	WARN(!I915_READ(CSR_PROGRAM_BASE),
>  				"CSR program storage start is NULL\n");
>  	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index e6156d5..a9bb299 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -644,21 +644,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  
>  			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>  				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* TODO: wait for a completion event or
> -				 * similar here instead of busy
> -				 * waiting using wait_for function.
> -				 */
> -				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> -						FW_UNINITIALIZED, 1000);
> -				if (state != FW_LOADED)
> -					DRM_ERROR("CSR firmware not ready (%d)\n",
> -							state);
> +				if (SKL_ENABLE_DC6(dev))
> +					skl_enable_dc6(dev_priv);
>  				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +					gen9_enable_dc5(dev_priv);
>  			}
>  		}
>  	}
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: [PATCH 04/18] drm/i915/gen9: block disable call for pw1 if dmc firmware is present.

[Daniel Vetter]

On Mon, Jul 27, 2015 at 10:48:16AM +0200, Daniel Vetter wrote:
> On Sun, Jul 26, 2015 at 12:30:25AM +0530, Animesh Manna wrote:
> > Grabbing a runtime pm reference with intel_runtime_pm_get will only
> > prevent device D3. But dmc firmware is required even earlier (namely
> > for the skl power well 1). DMC is responsible to save the status of
> > power well 1 and shut off the power well when panel is self refresh
> > mode of display is completely off.
> > 
> > Another interesting criteria to work dmc as expected is pw1 to be
> > enabled by driver and dmc will shut it off in its execution
> > sequence. If already disabled by driver dmc will get confuse and
> > behave differently than expected found during pc10 entry issue
> > for skl.
> > 
> > So berfore we disable power-well 1, added check if dmc firmware is
> > present and driver will not disable power well 1, but for any reason
> > if firmware is not present of failed to load we can shut off the
> > power well 1 which will save some power.
> > 
> > As skl is currently fully dependent on dmc to go in lowest possible
> > power state (dc6) but the same is not applicable for bxt. Display
> > engine can enter into dc9 without dmc, hence unblocking disable call.
> > 
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Damien Lespiau <damien.lespiau@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Sunil Kamath <sunil.kamath@intel.com>
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> 
> Please use the approach I've laid out in my original patch series with
> "drm/i915: use correct power domain for csr loading" and "drm/i915: Only
> allow rpm on gen9+ with dmc loaded". Your approach here requires a
> flush_work in the runtime pm callbacks which can deadlock.
> 
> If you want to make dmc optional on bxt then you need to adjust the 2nd
> patch a bit to no longer leak the runtime pm reference. Your approach here
> is an inversion of control and that doesn't work well since control
> inversions very easily lead to locking inversions.

Summary of what we just discussed offline:

Ok I was confused here with the intel_csr_load_status_get() check. If we
apply the above to patch from me first then we don't need that check any
more, and the patch itself looks good as a bugfix for skl dmc firmware
assumptions.
-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: [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

[Nagaraju, Vathsala]

Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
It's   Vathsala Nagaraju <Vathsala.Nagaraju@intel.com>

Commit message: Removed byte swapping for csr firmware.

Commit message does not convey as to why the change was made.  "This change is needed as DMC firmware loading failed on skylake due  byte swap  done in the driver"


-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Animesh Manna
Sent: Sunday, July 26, 2015 12:31 AM
To: intel-gfx@lists.freedesktop.org
Cc: Vatsala Nagaraju
Subject: [Intel-gfx] [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 1858f02..50779e0 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -328,16 +328,7 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 		return NULL;
 	}
 
-	for (i = 0; i < dmc_header->fw_size; i++) {
-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
-		/*
-		 * The firmware payload is an array of 32 bit words stored in
-		 * little-endian format in the firmware image and programmed
-		 * as 32 bit big-endian format to memory.
-		 */
-		dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
-	}
-
+	memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);
 	return dmc_payload;
 }
 
-- 
2.0.2

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

Re: [PATCH 14/18] drm/i915/gen9: Use flush_work to synchronize with dmc loader

[Daniel Vetter]

On Sun, Jul 26, 2015 at 12:30:35AM +0530, Animesh Manna wrote:
> As power well 1 is superset of power well 2 and always pw2
> will be disabled first and then pw1. On the other hand dmc
> is responsible to save & restore back pw1 when display
> engine goes and come back from low power state. Before
> disabling pw1 dmc must be loaded, so adding flush_work()
> while disabling pw2 which ensure that firmware will be
> available before disabling pw1 in suspend flow.
> 
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 2 --
>  drivers/gpu/drm/i915/intel_csr.c        | 2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 1 +
>  3 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 98343eb..ddf8a25 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -995,8 +995,6 @@ static int i915_pm_resume(struct device *dev)
>  
>  static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  {
> -	/* Enabling DC6 is not a hard requirement to enter runtime D3 */

Don't we need a flush_work here to make sure dmc is ready before going
into suspend?

> -
>  	skl_uninit_cdclk(dev_priv);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 36386324..1858f02 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -417,5 +417,7 @@ void intel_csr_ucode_fini(struct drm_i915_private *dev_priv)
>  	if (!HAS_CSR(dev_priv))
>  		return;
>  
> +	flush_work(&dev_priv->csr.work);
> +
>  	kfree(dev_priv->csr.dmc_payload);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 5f1ae23..a5059e8 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -652,6 +652,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  
>  			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>  				power_well->data == SKL_DISP_PW_2) {
> +				flush_work(&dev_priv->csr.work);

This flush_work here has two problems:
- It's an inversion of control since this is low-level code blocking on
  high-level. I think this is here is safe but in general it's very easy
  to create deadlocks and hence this pattern is discouraged.
- dmc loading might take a while (especially on cros/android where i915 is
  built-in but fw needs to wait for the full system before it can be
  loaded). We do it asynchronously so that i915 init can proceed and we
  can enable outputs really early, but the modeset code also does power
  well get/put calls and might end up in here, resulting in a stall.

If we instead prevent execution of this low-level power-well code by
grabbing a power_domain reference then both problems are solved:
- No flush_work needed since we'll only get into this code when the last
  power domain references is dropped, which necessarily means dmc loader
  has completed.
- No blocking of modesets while dmc loading is still in progress since the
  get/put power_domain calls will simply be no-ops too.

Long story short: If you apply the two patches I mentioned in my reply to
patch 4 there shouldn't be a need for this flush_work here in
skl_set_power_well.

Thanks, Daniel

>  				if (SKL_ENABLE_DC6(dev))
>  					skl_enable_dc6(dev_priv);
>  				else
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: [PATCH 04/18] drm/i915/gen9: block disable call for pw1 if dmc firmware is present.

[Sunil Kamath]

On Tuesday 28 July 2015 01:27 PM, Daniel Vetter wrote:
> On Mon, Jul 27, 2015 at 10:48:16AM +0200, Daniel Vetter wrote:
>> On Sun, Jul 26, 2015 at 12:30:25AM +0530, Animesh Manna wrote:
>>> Grabbing a runtime pm reference with intel_runtime_pm_get will only
>>> prevent device D3. But dmc firmware is required even earlier (namely
>>> for the skl power well 1). DMC is responsible to save the status of
>>> power well 1 and shut off the power well when panel is self refresh
>>> mode of display is completely off.
>>>
>>> Another interesting criteria to work dmc as expected is pw1 to be
>>> enabled by driver and dmc will shut it off in its execution
>>> sequence. If already disabled by driver dmc will get confuse and
>>> behave differently than expected found during pc10 entry issue
>>> for skl.
>>>
>>> So berfore we disable power-well 1, added check if dmc firmware is
>>> present and driver will not disable power well 1, but for any reason
>>> if firmware is not present of failed to load we can shut off the
>>> power well 1 which will save some power.
>>>
>>> As skl is currently fully dependent on dmc to go in lowest possible
>>> power state (dc6) but the same is not applicable for bxt. Display
>>> engine can enter into dc9 without dmc, hence unblocking disable call.
>>>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> Please use the approach I've laid out in my original patch series with
>> "drm/i915: use correct power domain for csr loading" and "drm/i915: Only
>> allow rpm on gen9+ with dmc loaded". Your approach here requires a
>> flush_work in the runtime pm callbacks which can deadlock.
>>
>> If you want to make dmc optional on bxt then you need to adjust the 2nd
>> patch a bit to no longer leak the runtime pm reference. Your approach here
>> is an inversion of control and that doesn't work well since control
>> inversions very easily lead to locking inversions.
> Summary of what we just discussed offline:
>
> Ok I was confused here with the intel_csr_load_status_get() check. If we
> apply the above to patch from me first then we don't need that check any
> more, and the patch itself looks good as a bugfix for skl dmc firmware
> assumptions.
> -Daniel
Thanks alot Damien.

Animesh,
As discussed in same call, bug fix should go initial patches in this series.

also its good to add the check in skl_set_power_well function itself 
than intel_display_power_put

- Sunil

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

Re: [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

[Sunil Kamath]

On Tuesday 28 July 2015 01:38 PM, Nagaraju, Vathsala wrote:
> Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
> It's   Vathsala Nagaraju <Vathsala.Nagaraju@intel.com>
>
> Commit message: Removed byte swapping for csr firmware.
>
> Commit message does not convey as to why the change was made.  "This change is needed as DMC firmware loading failed on skylake due  byte swap  done in the driver"

yes. agree.

Same review comments as other patches. Bug fixing patches should go 
first in sequence and also commit message should contain relevant info 
on fix/patch.

This byte swapping is not needed at all.

Minimal things to take care.

- Sunil



>
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Animesh Manna
> Sent: Sunday, July 26, 2015 12:31 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vatsala Nagaraju
> Subject: [Intel-gfx] [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.
>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_csr.c | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 1858f02..50779e0 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -328,16 +328,7 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
>   		return NULL;
>   	}
>   
> -	for (i = 0; i < dmc_header->fw_size; i++) {
> -		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
> -		/*
> -		 * The firmware payload is an array of 32 bit words stored in
> -		 * little-endian format in the firmware image and programmed
> -		 * as 32 bit big-endian format to memory.
> -		 */
> -		dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
> -	}
> -
> +	memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);
>   	return dmc_payload;
>   }
>   

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

Re: [PATCH 17/18] drm/i915/skl: Removed csr firmware load in resume path.

[Sunil Kamath]

On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
> As csr firmware is taking care of loading the firmware,
> so no need for driver to load again.
>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 77b35fd..f5e720b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1048,7 +1048,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>   		skl_disable_dc6(dev_priv);
>   
>   	skl_init_cdclk(dev_priv);
> -	intel_csr_load_program(dev_priv);

The context save and restore program is reset on cold boot, warm reset, 
PCI function level reset, and hibernate/suspend.

Will it not impact?

- Sunil
>   
>   	return 0;
>   }

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

Re: [PATCH 17/18] drm/i915/skl: Removed csr firmware load in resume path.

[Sunil Kamath]

On Tuesday 28 July 2015 04:53 PM, Sunil Kamath wrote:
> On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
>> As csr firmware is taking care of loading the firmware,
>> so no need for driver to load again.
>>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 77b35fd..f5e720b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1048,7 +1048,6 @@ static int skl_resume_prepare(struct 
>> drm_i915_private *dev_priv)
>>           skl_disable_dc6(dev_priv);
>>         skl_init_cdclk(dev_priv);
>> -    intel_csr_load_program(dev_priv);
>
> The context save and restore program is reset on cold boot, warm 
> reset, PCI function level reset, and hibernate/suspend.
>
> Will it not impact?
>
> - Sunil

If the concern is about checking DC5/DC6 counters, isn't it good idea to 
add that as debug print before f/w reload?
Than to avoid completely reloading of entire firmware?

- Sunil
>>         return 0;
>>   }
>

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

Re: [PATCH 17/18] drm/i915/skl: Removed csr firmware load in resume path.

[Sunil Kamath]

On Tuesday 28 July 2015 04:53 PM, Sunil Kamath wrote:
> On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
>> As csr firmware is taking care of loading the firmware,
>> so no need for driver to load again.
>>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 77b35fd..f5e720b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1048,7 +1048,6 @@ static int skl_resume_prepare(struct 
>> drm_i915_private *dev_priv)
>>           skl_disable_dc6(dev_priv);
>>         skl_init_cdclk(dev_priv);
>> -    intel_csr_load_program(dev_priv);
>
> The context save and restore program is reset on cold boot, warm 
> reset, PCI function level reset, and hibernate/suspend.
>
> Will it not impact?
>
> - Sunil

If the intention is just to check the DC5/DC6 counters, why cant we just 
add debug prints before f/w reload? Than to just avoiding reloading fw 
itself.

- Sunil
>>         return 0;
>>   }
>

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

Re: [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

[Sunil Kamath]

On Tuesday 28 July 2015 04:39 PM, Sunil Kamath wrote:
> On Tuesday 28 July 2015 01:38 PM, Nagaraju, Vathsala wrote:
>> Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
>> It's   Vathsala Nagaraju <Vathsala.Nagaraju@intel.com>
>>
>> Commit message: Removed byte swapping for csr firmware.
>>
>> Commit message does not convey as to why the change was made. "This 
>> change is needed as DMC firmware loading failed on skylake due  byte 
>> swap  done in the driver"
>
> yes. agree.
>
> Same review comments as other patches. Bug fixing patches should go 
> first in sequence and also commit message should contain relevant info 
> on fix/patch.
>
> This byte swapping is not needed at all.
>
> Minimal things to take care.
>
> - Sunil
>
>
>
>>
>>
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
>> Behalf Of Animesh Manna
>> Sent: Sunday, July 26, 2015 12:31 AM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Vatsala Nagaraju
>> Subject: [Intel-gfx] [PATCH 18/18] drm/i915/gen9: Removed byte 
>> swapping for csr firmware.
>>
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>> Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_csr.c | 11 +----------
>>   1 file changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c 
>> b/drivers/gpu/drm/i915/intel_csr.c
>> index 1858f02..50779e0 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -328,16 +328,7 @@ static uint32_t *parse_csr_fw(struct 
>> drm_i915_private *dev_priv,
>>           return NULL;
>>       }
>>   -    for (i = 0; i < dmc_header->fw_size; i++) {
>> -        uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
>> -        /*
>> -         * The firmware payload is an array of 32 bit words stored in
>> -         * little-endian format in the firmware image and programmed
>> -         * as 32 bit big-endian format to memory.
>> -         */
>> -        dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
>> -    }
>> -
>> +    memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);

*4 missing for byte conversion here.
memcpy(dmc_payload, &fw->data[readcount], (dmc_header->fw_size)*4); ?
Still issue to be addressed here to confirm that fw is loaded fine.

- Sunil
>>       return dmc_payload;
>>   }
>

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

Re: [PATCH 17/18] drm/i915/skl: Removed csr firmware load in resume path.

[Sunil Kamath]

On Wednesday 29 July 2015 04:40 PM, Sunil Kamath wrote:
> On Tuesday 28 July 2015 04:53 PM, Sunil Kamath wrote:
>> On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
>>> As csr firmware is taking care of loading the firmware,
>>> so no need for driver to load again.
>>>
>>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Sunil Kamath <sunil.kamath@intel.com>
>>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index 77b35fd..f5e720b 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -1048,7 +1048,6 @@ static int skl_resume_prepare(struct 
>>> drm_i915_private *dev_priv)
>>>           skl_disable_dc6(dev_priv);
>>>         skl_init_cdclk(dev_priv);
>>> -    intel_csr_load_program(dev_priv);
>>
>> The context save and restore program is reset on cold boot, warm 
>> reset, PCI function level reset, and hibernate/suspend.
>>
>> Will it not impact?
>>
>> - Sunil
>
> If the intention is just to check the DC5/DC6 counters, why cant we 
> just add debug prints before f/w reload? Than to just avoiding 
> reloading fw itself.
>
> - Sunil

Dont hurry on this patch.
Need to close on the above opens.

- Sunil
>>>         return 0;
>>>   }
>>
>

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

Re: [PATCH 16/18] drm/i915/skl: Do not disable cdclk PLL if csr firmware is present.

[Sunil Kamath]

On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
> While display engine entering into low power state no need to disable
> cdclk pll as CSR firmware of dmc will take care. If pll is already
> enabled firmware execution sequence will be blocked. This is one
> of the criteria for dmc to work properly.
>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-bt: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfe..ef2ef4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5675,10 +5675,13 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
>   	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
>   		DRM_ERROR("DBuf power disable timeout\n");
>   
> -	/* disable DPLL0 */
> -	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> -	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> -		DRM_ERROR("Couldn't disable DPLL0\n");
> +	if (dev_priv->csr.dmc_payload) {
> +		/* disable DPLL0 */
> +		I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) &
> +					~LCPLL_PLL_ENABLE);
> +		if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> +			DRM_ERROR("Couldn't disable DPLL0\n");
> +	}
>   
>   	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>   }
This is a right bug fix.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>

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

Re: [PATCH 15/18] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Sunil Kamath]

On Sunday 26 July 2015 12:30 AM, Animesh Manna wrote:
> Mmio register access after dc6/dc5 entry is causing the
> system hang, so enabling dc6 as the last call in suspend flow.
>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Sunil Kamath <sunil.kamath@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c         |  6 ++++++
>   drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
>   3 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ddf8a25..77b35fd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -995,6 +995,9 @@ static int i915_pm_resume(struct device *dev)
>   
>   static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>   {
> +	if (dev_priv->csr.dmc_payload)
> +		skl_enable_dc6(dev_priv);
> +
>   	skl_uninit_cdclk(dev_priv);

This is really right change.
With this we will go back to our original design.
Deepest possible display state will be triggered by respective suspend 
complete.
for example in BXT we trigger DC9 from bxt_suspend_complete and works 
fine with rpm integrated.
Same case here for SKL for DC6 which uses DMC.

Right bug fix.

Reviewed-by: A.Sunil Kamath <sunil.kamath@intel.com>

>   
>   	return 0;
> @@ -1041,6 +1044,9 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
>   
>   static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>   {
> +	if (dev_priv->csr.dmc_payload)
> +		skl_disable_dc6(dev_priv);
> +
>   	skl_init_cdclk(dev_priv);
>   	intel_csr_load_program(dev_priv);
>   
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 644286f..0d13f50 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1117,6 +1117,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);
>   void bxt_disable_dc9(struct drm_i915_private *dev_priv);
>   void skl_init_cdclk(struct drm_i915_private *dev_priv);
>   void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> +void skl_enable_dc6(struct drm_i915_private *dev_priv);
> +void skl_disable_dc6(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 a5059e8..ddae00e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -540,7 +540,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
>   		"DC6 already programmed to be disabled.\n");
>   }
>   
> -static void skl_enable_dc6(struct drm_i915_private *dev_priv)
> +void skl_enable_dc6(struct drm_i915_private *dev_priv)
>   {
>   	uint32_t val;
>   
> @@ -557,7 +557,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
>   	POSTING_READ(DC_STATE_EN);
>   }
>   
> -static void skl_disable_dc6(struct drm_i915_private *dev_priv)
> +void skl_disable_dc6(struct drm_i915_private *dev_priv)
>   {
>   	uint32_t val;
>   
> @@ -618,10 +618,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   				!I915_READ(HSW_PWR_WELL_BIOS),
>   				"Invalid for power well status to be enabled, unless done by the BIOS, \
>   				when request is to disable!\n");
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
> +				if (GEN9_ENABLE_DC5(dev))
> +					gen9_disable_dc5(dev_priv);
>   				if (SKL_ENABLE_DC6(dev)) {
> -					skl_disable_dc6(dev_priv);
>   					/*
>   					 * DDI buffer programming unnecessary during driver-load/resume
>   					 * as it's already done during modeset initialization then.
> @@ -629,8 +629,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   					 */
>   					if (!dev_priv->power_domains.initializing)
>   						intel_prepare_ddi(dev);
> -				} else {
> -					gen9_disable_dc5(dev_priv);
>   				}
>   			}
>   			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> @@ -650,12 +648,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   			POSTING_READ(HSW_PWR_WELL_DRIVER);
>   			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>   
> -			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> -				power_well->data == SKL_DISP_PW_2) {
> +			if (power_well->data == SKL_DISP_PW_2) {
>   				flush_work(&dev_priv->csr.work);
> -				if (SKL_ENABLE_DC6(dev))
> -					skl_enable_dc6(dev_priv);
> -				else
> +				if (GEN9_ENABLE_DC5(dev))
>   					gen9_enable_dc5(dev_priv);
>   			}
>   		}

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

Re: [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

[Nagaraju, Vathsala]

memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);  
dmc_header->fw_size  is wrong.  
This will result in 0's  after 0x80734  location, results in system hang.

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Animesh Manna
Sent: Sunday, July 26, 2015 12:31 AM
To: intel-gfx@lists.freedesktop.org
Cc: Vatsala Nagaraju
Subject: [Intel-gfx] [PATCH 18/18] drm/i915/gen9: Removed byte swapping for csr firmware.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vatsala Nagaraju <vatsala.nagraju@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 1858f02..50779e0 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -328,16 +328,7 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 		return NULL;
 	}
 
-	for (i = 0; i < dmc_header->fw_size; i++) {
-		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
-		/*
-		 * The firmware payload is an array of 32 bit words stored in
-		 * little-endian format in the firmware image and programmed
-		 * as 32 bit big-endian format to memory.
-		 */
-		dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
-	}
-
+	memcpy(dmc_payload, &fw->data[readcount], dmc_header->fw_size);
 	return dmc_payload;
 }
 
-- 
2.0.2

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

Re: [PATCH 15/18] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

[Nagaraju, Vathsala]

Hang is due to patch 18 not mmio access.

-----Original Message-----
From: Manna, Animesh 
Sent: Sunday, July 26, 2015 12:31 AM
To: intel-gfx@lists.freedesktop.org
Cc: Manna, Animesh; Lespiau, Damien; Deak, Imre; Kamath, Sunil; Nagaraju, Vathsala; Bhardwaj, Rajneesh
Subject: [PATCH 15/18] drm/i915/skl: Making DC6 entry is the last call in suspend flow.

Mmio register access after dc6/dc5 entry is causing the system hang, so enabling dc6 as the last call in suspend flow.

Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Sunil Kamath <sunil.kamath@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  6 ++++++
 drivers/gpu/drm/i915/intel_drv.h        |  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 19 +++++++------------
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index ddf8a25..77b35fd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -995,6 +995,9 @@ static int i915_pm_resume(struct device *dev)
 
 static int skl_suspend_complete(struct drm_i915_private *dev_priv)  {
+	if (dev_priv->csr.dmc_payload)
+		skl_enable_dc6(dev_priv);
+
 	skl_uninit_cdclk(dev_priv);
 
 	return 0;
@@ -1041,6 +1044,9 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
 
 static int skl_resume_prepare(struct drm_i915_private *dev_priv)  {
+	if (dev_priv->csr.dmc_payload)
+		skl_disable_dc6(dev_priv);
+
 	skl_init_cdclk(dev_priv);
 	intel_csr_load_program(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 644286f..0d13f50 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1117,6 +1117,8 @@ void bxt_enable_dc9(struct drm_i915_private *dev_priv);  void bxt_disable_dc9(struct drm_i915_private *dev_priv);  void skl_init_cdclk(struct drm_i915_private *dev_priv);  void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
+void skl_enable_dc6(struct drm_i915_private *dev_priv); void 
+skl_disable_dc6(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 a5059e8..ddae00e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -540,7 +540,7 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
 		"DC6 already programmed to be disabled.\n");  }
 
-static void skl_enable_dc6(struct drm_i915_private *dev_priv)
+void skl_enable_dc6(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
 
@@ -557,7 +557,7 @@ static void skl_enable_dc6(struct drm_i915_private *dev_priv)
 	POSTING_READ(DC_STATE_EN);
 }
 
-static void skl_disable_dc6(struct drm_i915_private *dev_priv)
+void skl_disable_dc6(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
 
@@ -618,10 +618,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 				!I915_READ(HSW_PWR_WELL_BIOS),
 				"Invalid for power well status to be enabled, unless done by the BIOS, \
 				when request is to disable!\n");
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
+			if (power_well->data == SKL_DISP_PW_2) {
+				if (GEN9_ENABLE_DC5(dev))
+					gen9_disable_dc5(dev_priv);
 				if (SKL_ENABLE_DC6(dev)) {
-					skl_disable_dc6(dev_priv);
 					/*
 					 * DDI buffer programming unnecessary during driver-load/resume
 					 * as it's already done during modeset initialization then.
@@ -629,8 +629,6 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 					 */
 					if (!dev_priv->power_domains.initializing)
 						intel_prepare_ddi(dev);
-				} else {
-					gen9_disable_dc5(dev_priv);
 				}
 			}
 			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask); @@ -650,12 +648,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			POSTING_READ(HSW_PWR_WELL_DRIVER);
 			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
 
-			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
-				power_well->data == SKL_DISP_PW_2) {
+			if (power_well->data == SKL_DISP_PW_2) {
 				flush_work(&dev_priv->csr.work);
-				if (SKL_ENABLE_DC6(dev))
-					skl_enable_dc6(dev_priv);
-				else
+				if (GEN9_ENABLE_DC5(dev))
 					gen9_enable_dc5(dev_priv);
 			}
 		}
--
2.0.2

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