[PATCH v3 00/13] drm/i915: Redesign dmc firmware loading.

[PATCH v3 00/13] drm/i915: Redesign dmc firmware loading.

[Imre Deak]

This is a rebased version of [1], addressing the review comments. It
depends on Mika's FW version blacklisting/capture patchset [2].

I have added my Reviewed-by to those patches that I only rebased or
updated the commit message, patches 1,4,12 have changes from me, so
someone else would need to review those.

I tested this on BXT and SKL, on SKL-Y with additional patches from
Patrik and Paulo I could reach PC10 state.

[1]
http://lists.freedesktop.org/archives/intel-gfx/2015-August/072921.html

[2]
http://lists.freedesktop.org/archives/intel-gfx/2015-October/078898.html

Animesh Manna (3):
  drm/i915/gen9: csr_init after runtime pm enable
  drm/i915/gen9: Use flush_work to synchronize with dmc loader
  drm/i915/skl: Removed assert for csr-fw-loading check during disabling
    dc6

Daniel Vetter (9):
  drm/i915: use correct power domain for csr loading
  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: Don't try to load garbage dmc firmware on resume
  drm/i915/gen9: Use dev_priv in csr functions
  drm/i915/gen9: extract parse_csr_fw
  drm/i915: Use request_firmware and our own async work

Imre Deak (1):
  drm/i915/gen9: flush DMC fw loading work during system suspend

 drivers/gpu/drm/i915/i915_dma.c         |  10 +-
 drivers/gpu/drm/i915/i915_drv.c         |  40 ++-----
 drivers/gpu/drm/i915/i915_drv.h         |  12 +--
 drivers/gpu/drm/i915/i915_gpu_error.c   |   2 +-
 drivers/gpu/drm/i915/intel_csr.c        | 182 ++++++++++++--------------------
 drivers/gpu/drm/i915/intel_drv.h        |  10 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c |  26 ++---
 7 files changed, 95 insertions(+), 187 deletions(-)

-- 
2.1.4

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

[PATCH v3 01/13] drm/i915/gen9: csr_init after runtime pm enable

[Imre Deak]

From: Animesh Manna <animesh.manna@intel.com>

Skl is fully dependent on dmc for going to low power state (dc5/dc6).
This requires a trigger from rpm. To ensure the dmc firmware
is available for runtime pm support rpm-reference-count is used
by not releasing the rpm reference if firmware loading is
not completed.

So moved the intel_csr_ucode_init call after runtime pm enable.

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>
[imre: moved the call right after power domain init to avoid race with
 the console modesetting]
---
 drivers/gpu/drm/i915/i915_dma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2336af9..5d68942 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -398,6 +398,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_power_domains_init_hw(dev_priv);
 
+	intel_csr_ucode_init(dev);
+
 	ret = intel_irq_install(dev_priv);
 	if (ret)
 		goto cleanup_gem_stolen;
@@ -942,9 +944,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;
-- 
2.1.4

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

[PATCH v3 02/13] drm/i915: use correct power domain for csr loading

[Imre Deak]

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

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 2).

Hence we need to grab a rpm reference higher up in the hierarchy. For
simplicity just grab the _INIT display power well. That's a bit too
much, but since the firmware loading task should completely fairly
quickly this won't be a real problem really.

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>
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index bd305da..2c9bf3f 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -408,7 +408,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 
 out:
 	if (fw_loaded) {
-		intel_runtime_pm_put(dev_priv);
+		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
 		DRM_INFO("Finished loading %s (v%u.%u)\n",
 			 dev_priv->csr.fw_path,
@@ -455,7 +455,7 @@ void intel_csr_ucode_init(struct drm_device *dev)
 	 * Obtain a runtime pm reference, until CSR is loaded,
 	 * to avoid entering runtime-suspend.
 	 */
-	intel_runtime_pm_get(dev_priv);
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
 	/* CSR supported for platform, load firmware */
 	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
-- 
2.1.4

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

[PATCH v3 03/13] drm/i915/gen9: move assert_csr_loaded into intel_rpm.c

[Imre Deak]

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

Avoids non-static functions since all the callers are in intel_rpm.c.

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>
[imre: removed note about reg definitions from commit message, since
 it's not relevant any more]
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c        | 10 ----------
 drivers/gpu/drm/i915/intel_drv.h        |  1 -
 drivers/gpu/drm/i915/intel_runtime_pm.c |  8 ++++++++
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 2c9bf3f..cd6fb58d 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -485,13 +485,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_ONCE(intel_csr_load_status_get(dev_priv) != FW_LOADED,
-		  "CSR is not loaded.\n");
-	WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
-		  "CSR program storage start is NULL\n");
-	WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
-	WARN_ONCE(!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 1a3bbdc..3d1c744 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1211,7 +1211,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 e50cc88..92746e1 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -458,6 +458,14 @@ static void gen9_set_dc_state_debugmask_memory_up(
 	}
 }
 
+void assert_csr_loaded(struct drm_i915_private *dev_priv)
+{
+	WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
+		  "CSR program storage start is NULL\n");
+	WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
+	WARN_ONCE(!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.1.4

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

[PATCH v3 04/13] drm/i915/gen9: Remove csr.state, csr_lock and related code.

[Imre Deak]

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.

Before enabling/disabling DC6 check if the firmware is loaded
successfully. This is guaranteed during runtime s/r, since otherwise we
don't enable RPM, but not during system s/r.

Note that it's still unclear whether we need to enable/disable DC6
during system s/r, until that's clarified, keep the current behavior and
enable/disable DC6.

Also after this patch there is a race during system s/r where the
firmware may not be loaded yet, that's addressed in an upcoming patch.

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>
[imre: added code and note about checking if the firmware loaded ok,
 before enabling/disabling it]
---
 drivers/gpu/drm/i915/i915_dma.c         |  1 -
 drivers/gpu/drm/i915/i915_drv.c         | 11 ++------
 drivers/gpu/drm/i915/i915_drv.h         | 10 -------
 drivers/gpu/drm/i915/i915_gpu_error.c   |  2 +-
 drivers/gpu/drm/i915/intel_csr.c        | 46 +--------------------------------
 drivers/gpu/drm/i915/intel_drv.h        |  3 ---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ++----------
 7 files changed, 6 insertions(+), 84 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5d68942..621914c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -897,7 +897,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);
 	mutex_init(&dev_priv->av_mutex);
 
 	intel_pm_setup(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 121c539..7efc798 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1050,18 +1050,11 @@ static int i915_pm_resume(struct device *dev)
 
 static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
-	enum csr_state state;
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
 	skl_uninit_cdclk(dev_priv);
 
-	/* 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)
+	if (dev_priv->csr.dmc_payload)
 		skl_enable_dc6(dev_priv);
 
 	return 0;
@@ -1110,7 +1103,7 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
-	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
+	if (dev_priv->csr.dmc_payload)
 		skl_disable_dc6(dev_priv);
 
 	skl_init_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0bee438..0aeac06 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -738,12 +738,6 @@ struct intel_uncore {
 #define CSR_VERSION_MAJOR(version)	((version) >> 16)
 #define CSR_VERSION_MINOR(version)	((version) & 0xffff)
 
-enum csr_state {
-	FW_UNINITIALIZED = 0,
-	FW_LOADED,
-	FW_FAILED
-};
-
 struct intel_csr {
 	const char *fw_path;
 	uint32_t *dmc_payload;
@@ -752,7 +746,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) \
@@ -1724,9 +1717,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/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index aaf4a1f..25dc4f1 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -370,7 +370,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	if (HAS_CSR(dev)) {
 		struct intel_csr *csr = &dev_priv->csr;
 
-		err_printf(m, "DMC load state: %d\n", csr->state);
+		err_printf(m, "DMC loaded: %s\n", yesno(csr->dmc_payload));
 		err_printf(m, "DMC fw version: %d.%d\n",
 			   CSR_VERSION_MAJOR(csr->version),
 			   CSR_VERSION_MINOR(csr->version));
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index cd6fb58d..eddad62 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -203,40 +203,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.
  *
@@ -264,7 +230,6 @@ void intel_csr_load_program(struct drm_device *dev)
 	if (I915_READ(CSR_PROGRAM(0)))
 		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(i), payload[i]);
@@ -273,9 +238,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)
@@ -415,8 +377,6 @@ out:
 			 CSR_VERSION_MAJOR(csr->version),
 			 CSR_VERSION_MINOR(csr->version));
 	} else {
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
-
 		i915_firmware_load_error_print(csr->fw_path, 0);
 	}
 
@@ -445,7 +405,6 @@ void intel_csr_ucode_init(struct drm_device *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;
 	}
 
@@ -462,10 +421,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);
-	}
 }
 
 /**
@@ -482,6 +439,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 3d1c744..2e48b3f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1206,9 +1206,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);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 92746e1..30b5801 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -664,8 +664,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 	} else {
 		if (enable_requested) {
 			if (IS_SKYLAKE(dev) &&
-				(power_well->data == SKL_DISP_PW_1) &&
-				(intel_csr_load_status_get(dev_priv) == FW_LOADED))
+				(power_well->data == SKL_DISP_PW_1))
 				DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
 			else {
 				I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
@@ -674,20 +673,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			}
 
 			if (GEN9_ENABLE_DC5(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_DEBUG("CSR firmware not ready (%d)\n",
-							state);
-				else
+				power_well->data == SKL_DISP_PW_2)
 					gen9_enable_dc5(dev_priv);
-			}
 		}
 	}
 
-- 
2.1.4

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

[PATCH v3 05/13] drm/i915/gen9: Align line continuations in intel_csr.c.

[Imre Deak]

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.

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>
[imre: removed note about reg definitions from the commit message, it's
 not relevant any more]
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index eddad62..489ecfe 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -236,7 +236,7 @@ void intel_csr_load_program(struct drm_device *dev)
 
 	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]);
 	}
 }
 
@@ -266,9 +266,9 @@ 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;
 	}
 
@@ -291,11 +291,11 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 
 	/* 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);
@@ -303,7 +303,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 &&
@@ -311,7 +311,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) {
@@ -324,7 +324,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);
@@ -332,15 +332,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];
@@ -418,9 +418,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.1.4

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

[PATCH v3 06/13] drm/i915/gen9: Simplify csr loading failure printing.

[Imre Deak]

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>
Reviewed-by: Imre Deak <imre.deak@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 |  6 ++++--
 3 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7efc798..366f832 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -567,26 +567,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 0aeac06..9918b61 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2701,7 +2701,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 489ecfe..1f02fe0 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -377,7 +377,7 @@ out:
 			 CSR_VERSION_MAJOR(csr->version),
 			 CSR_VERSION_MINOR(csr->version));
 	} else {
-		i915_firmware_load_error_print(csr->fw_path, 0);
+		DRM_ERROR("Failed to load DMC firmware, disabling rpm\n");
 	}
 
 	release_firmware(fw);
@@ -421,8 +421,10 @@ 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);
+		DRM_ERROR("Failed to load DMC firmware, disabling rpm (%d)\n",
+			  ret);
 }
 
 /**
-- 
2.1.4

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

[PATCH v3 07/13] drm/i915/gen9: Don't try to load garbage dmc firmware on resume

[Imre Deak]

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

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>
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 1f02fe0..bbf6b8d 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -227,7 +227,7 @@ void intel_csr_load_program(struct drm_device *dev)
 	 * Unfortunately the ACPI subsystem doesn't yet give us a way to
 	 * differentiate this, hence figure it out with this hack.
 	 */
-	if (I915_READ(CSR_PROGRAM(0)))
+	if ((!dev_priv->csr.dmc_payload) || I915_READ(CSR_PROGRAM(0)))
 		return;
 
 	fw_size = dev_priv->csr.dmc_fw_size;
-- 
2.1.4

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

[PATCH v3 08/13] drm/i915/gen9: Use dev_priv in csr functions

[Imre Deak]

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>
Reviewed-by: Imre Deak <imre.deak@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 | 26 +++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h |  6 +++---
 4 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 621914c..eddcbfd 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -398,7 +398,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_power_domains_init_hw(dev_priv);
 
-	intel_csr_ucode_init(dev);
+	intel_csr_ucode_init(dev_priv);
 
 	ret = intel_irq_install(dev_priv);
 	if (ret)
@@ -1116,7 +1116,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:
@@ -1199,7 +1199,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 366f832..eccdb41 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1081,13 +1081,11 @@ 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;
-
 	if (dev_priv->csr.dmc_payload)
 		skl_disable_dc6(dev_priv);
 
 	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 bbf6b8d..d45920f 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -204,19 +204,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;
 	u32 *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;
 	}
@@ -365,7 +364,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	memcpy(dmc_payload, &fw->data[readcount], nbytes);
 
 	/* load csr program during system boot, as needed for DC states */
-	intel_csr_load_program(dev);
+	intel_csr_load_program(dev_priv);
 	fw_loaded = true;
 
 out:
@@ -385,21 +384,20 @@ 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_priv))
 		csr->fw_path = I915_CSR_BXT;
@@ -429,16 +427,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 2e48b3f..a2006b7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1205,9 +1205,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.1.4

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

[PATCH v3 09/13] drm/i915/gen9: extract parse_csr_fw

[Imre Deak]

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.

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>
[imre: remove note about BE cast from commit message, it's not relevant
 any more]
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 51 +++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index d45920f..ce0c47f 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -239,9 +239,9 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv)
 	}
 }
 
-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;
@@ -252,14 +252,13 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
 	uint32_t i;
 	uint32_t *dmc_payload;
-	bool fw_loaded = false;
 
 	if (!fw)
-		goto out;
+		return NULL;
 
 	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
 		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
-		goto out;
+		return NULL;
 	}
 
 	/* Extract CSS Header information*/
@@ -268,7 +267,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;
 	}
 
 	csr->version = css_header->version;
@@ -283,7 +282,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 			 CSR_VERSION_MINOR(csr->version),
 			 SKL_REQUIRED_FW_MAJOR,
 			 SKL_REQUIRED_FW_MINOR);
-		goto out;
+		return NULL;
 	}
 
 	readcount += sizeof(struct intel_css_header);
@@ -295,7 +294,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);
 
@@ -315,7 +314,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;
 
@@ -324,7 +323,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);
 
@@ -332,7 +331,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++) {
@@ -340,7 +339,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];
@@ -350,25 +349,39 @@ 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;
 	memcpy(dmc_payload, &fw->data[readcount], nbytes);
 
+	return dmc_payload;
+}
+
+static void finish_csr_load(const struct firmware *fw, void *context)
+{
+	struct drm_i915_private *dev_priv = context;
+	struct intel_csr *csr = &dev_priv->csr;
+
+	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_priv);
-	fw_loaded = true;
 
 out:
-	if (fw_loaded) {
+	if (dev_priv->csr.dmc_payload) {
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
 		DRM_INFO("Finished loading %s (v%u.%u)\n",
-- 
2.1.4

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

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

[Imre Deak]

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>
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_csr.c | 27 +++++++++++++--------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9918b61..ffd6068 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -739,6 +739,7 @@ struct intel_uncore {
 #define CSR_VERSION_MINOR(version)	((version) & 0xffff)
 
 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 ce0c47f..148c137 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -364,11 +364,18 @@ 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 intel_csr *csr = &dev_priv->csr;
+	struct drm_i915_private *dev_priv;
+	struct intel_csr *csr;
+	const struct firmware *fw;
+	int ret;
 
+	dev_priv = container_of(work, typeof(*dev_priv), csr.work);
+	csr = &dev_priv->csr;
+
+	ret = request_firmware(&fw, dev_priv->csr.fw_path,
+			       &dev_priv->dev->pdev->dev);
 	if (!fw)
 		goto out;
 
@@ -376,7 +383,6 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	if (!dev_priv->csr.dmc_payload)
 		goto out;
 
-
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev_priv);
 
@@ -405,7 +411,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;
@@ -427,15 +434,7 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
 	 */
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
-	/* 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);
-
-	if (ret)
-		DRM_ERROR("Failed to load DMC firmware, disabling rpm (%d)\n",
-			  ret);
+	schedule_work(&dev_priv->csr.work);
 }
 
 /**
-- 
2.1.4

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

[PATCH v3 11/13] drm/i915/gen9: Use flush_work to synchronize with dmc loader

[Imre Deak]

From: Animesh Manna <animesh.manna@intel.com>

During driver unload to ensure we dont have any pending task,
flush_work added to complete firmware loading task.

v1: Initial version.

v2: As per review comments from Daniel,
Removed flush_work from skl_set_power_well. As we have taken
power well refernece and rpm count during firmware loading
by using display_power_domain_get/put - this will always
ensure rpm will be blocked if firmware is not loaded.

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>
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  | 2 --
 drivers/gpu/drm/i915/intel_csr.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index eccdb41..96650ed 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1030,8 +1030,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);
 
 	if (dev_priv->csr.dmc_payload)
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 148c137..ecb7c70 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -449,5 +449,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);
 }
-- 
2.1.4

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

[PATCH v3 12/13] drm/i915/gen9: flush DMC fw loading work during system suspend

[Imre Deak]

Currently during system s/r we enable/disable DC6, so before we do so
make sure that the firmware loading is complete.

Note that whether we need to enable DC6 for S3/S4 is still open.  At
least the firmware program is lost during S3 and we need to reprogram it
after resuming. Until this is clarified we keep the current behavior and
enable/disable DC6.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 96650ed..6de2287 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -656,6 +656,9 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_display_set_init_power(dev_priv, false);
 
+	if (HAS_CSR(dev_priv))
+		flush_work(&dev_priv->csr.work);
+
 	return 0;
 }
 
-- 
2.1.4

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

[PATCH v3 13/13] drm/i915/skl: Removed assert for csr-fw-loading check during disabling dc6

[Imre Deak]

From: Animesh Manna <animesh.manna@intel.com>

As during disabling dc6 no need to check for csr firmware
loading status, so removed the assert call (Requested by Damien).

Cc: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
Reviewed-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 30b5801..b77143e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -554,7 +554,6 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
 	if (dev_priv->power_domains.initializing)
 		return;
 
-	assert_csr_loaded(dev_priv);
 	WARN_ONCE(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
 		  "DC6 already programmed to be disabled.\n");
 }
-- 
2.1.4

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

Re: [PATCH v3 00/13] drm/i915: Redesign dmc firmware loading.

[Jani Nikula]

On Wed, 28 Oct 2015, Imre Deak <imre.deak@intel.com> wrote:
> This is a rebased version of [1], addressing the review comments. It
> depends on Mika's FW version blacklisting/capture patchset [2].
>
> I have added my Reviewed-by to those patches that I only rebased or
> updated the commit message, patches 1,4,12 have changes from me, so
> someone else would need to review those.

You still need to add your Signed-off-by at the end when you're posting
someone else's patches, whether you modified them or not.

BR,
Jani.

>
> I tested this on BXT and SKL, on SKL-Y with additional patches from
> Patrik and Paulo I could reach PC10 state.
>
> [1]
> http://lists.freedesktop.org/archives/intel-gfx/2015-August/072921.html
>
> [2]
> http://lists.freedesktop.org/archives/intel-gfx/2015-October/078898.html
>
> Animesh Manna (3):
>   drm/i915/gen9: csr_init after runtime pm enable
>   drm/i915/gen9: Use flush_work to synchronize with dmc loader
>   drm/i915/skl: Removed assert for csr-fw-loading check during disabling
>     dc6
>
> Daniel Vetter (9):
>   drm/i915: use correct power domain for csr loading
>   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: Don't try to load garbage dmc firmware on resume
>   drm/i915/gen9: Use dev_priv in csr functions
>   drm/i915/gen9: extract parse_csr_fw
>   drm/i915: Use request_firmware and our own async work
>
> Imre Deak (1):
>   drm/i915/gen9: flush DMC fw loading work during system suspend
>
>  drivers/gpu/drm/i915/i915_dma.c         |  10 +-
>  drivers/gpu/drm/i915/i915_drv.c         |  40 ++-----
>  drivers/gpu/drm/i915/i915_drv.h         |  12 +--
>  drivers/gpu/drm/i915/i915_gpu_error.c   |   2 +-
>  drivers/gpu/drm/i915/intel_csr.c        | 182 ++++++++++++--------------------
>  drivers/gpu/drm/i915/intel_drv.h        |  10 +-
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  26 ++---
>  7 files changed, 95 insertions(+), 187 deletions(-)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 04/13] drm/i915/gen9: Remove csr.state, csr_lock and related code.

[Animesh Manna]

On 10/29/2015 3:28 AM, Imre Deak wrote:
> 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.
>
> Before enabling/disabling DC6 check if the firmware is loaded
> successfully. This is guaranteed during runtime s/r, since otherwise we
> don't enable RPM, but not during system s/r.
Yes as DC5 and DC6 display low power state is coupled with dmc firmware 
so it is a safe
to have a check before enable/disable dc5/dc6.
>
> Note that it's still unclear whether we need to enable/disable DC6
> during system s/r, until that's clarified, keep the current behavior and
> enable/disable DC6.
>
> Also after this patch there is a race during system s/r where the
> firmware may not be loaded yet, that's addressed in an upcoming patch.
>
> 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>
> [imre: added code and note about checking if the firmware loaded ok,
>   before enabling/disabling it]
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_dma.c         |  1 -
>   drivers/gpu/drm/i915/i915_drv.c         | 11 ++------
>   drivers/gpu/drm/i915/i915_drv.h         | 10 -------
>   drivers/gpu/drm/i915/i915_gpu_error.c   |  2 +-
>   drivers/gpu/drm/i915/intel_csr.c        | 46 +--------------------------------
>   drivers/gpu/drm/i915/intel_drv.h        |  3 ---
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ++----------
>   7 files changed, 6 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5d68942..621914c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -897,7 +897,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);
>   	mutex_init(&dev_priv->av_mutex);
>   
>   	intel_pm_setup(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 121c539..7efc798 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1050,18 +1050,11 @@ static int i915_pm_resume(struct device *dev)
>   
>   static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>   {
> -	enum csr_state state;
>   	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>   
>   	skl_uninit_cdclk(dev_priv);
>   
> -	/* 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)
> +	if (dev_priv->csr.dmc_payload)
>   		skl_enable_dc6(dev_priv);
>   
>   	return 0;
> @@ -1110,7 +1103,7 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>   {
>   	struct drm_device *dev = dev_priv->dev;
>   
> -	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
> +	if (dev_priv->csr.dmc_payload)
>   		skl_disable_dc6(dev_priv);
>   
>   	skl_init_cdclk(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0bee438..0aeac06 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -738,12 +738,6 @@ struct intel_uncore {
>   #define CSR_VERSION_MAJOR(version)	((version) >> 16)
>   #define CSR_VERSION_MINOR(version)	((version) & 0xffff)
>   
> -enum csr_state {
> -	FW_UNINITIALIZED = 0,
> -	FW_LOADED,
> -	FW_FAILED
> -};
> -
>   struct intel_csr {
>   	const char *fw_path;
>   	uint32_t *dmc_payload;
> @@ -752,7 +746,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) \
> @@ -1724,9 +1717,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/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index aaf4a1f..25dc4f1 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -370,7 +370,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>   	if (HAS_CSR(dev)) {
>   		struct intel_csr *csr = &dev_priv->csr;
>   
> -		err_printf(m, "DMC load state: %d\n", csr->state);
> +		err_printf(m, "DMC loaded: %s\n", yesno(csr->dmc_payload));
>   		err_printf(m, "DMC fw version: %d.%d\n",
>   			   CSR_VERSION_MAJOR(csr->version),
>   			   CSR_VERSION_MINOR(csr->version));
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index cd6fb58d..eddad62 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -203,40 +203,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.
>    *
> @@ -264,7 +230,6 @@ void intel_csr_load_program(struct drm_device *dev)
>   	if (I915_READ(CSR_PROGRAM(0)))
>   		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(i), payload[i]);
> @@ -273,9 +238,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)
> @@ -415,8 +377,6 @@ out:
>   			 CSR_VERSION_MAJOR(csr->version),
>   			 CSR_VERSION_MINOR(csr->version));
>   	} else {
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
> -
>   		i915_firmware_load_error_print(csr->fw_path, 0);
>   	}
>   
> @@ -445,7 +405,6 @@ void intel_csr_ucode_init(struct drm_device *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;
>   	}
>   
> @@ -462,10 +421,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);
> -	}
>   }
>   
>   /**
> @@ -482,6 +439,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 3d1c744..2e48b3f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1206,9 +1206,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);
>   
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 92746e1..30b5801 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -664,8 +664,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   	} else {
>   		if (enable_requested) {
>   			if (IS_SKYLAKE(dev) &&
> -				(power_well->data == SKL_DISP_PW_1) &&
> -				(intel_csr_load_status_get(dev_priv) == FW_LOADED))
> +				(power_well->data == SKL_DISP_PW_1))
>   				DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
>   			else {
>   				I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> @@ -674,20 +673,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   			}
>   
>   			if (GEN9_ENABLE_DC5(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_DEBUG("CSR firmware not ready (%d)\n",
> -							state);
> -				else
> +				power_well->data == SKL_DISP_PW_2)
>   					gen9_enable_dc5(dev_priv);
> -			}
>   		}
>   	}
>   

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

Re: [PATCH v3 12/13] drm/i915/gen9: flush DMC fw loading work during system suspend

[Animesh Manna]

On 10/29/2015 3:29 AM, Imre Deak wrote:
> Currently during system s/r we enable/disable DC6, so before we do so
> make sure that the firmware loading is complete.
>
> Note that whether we need to enable DC6 for S3/S4 is still open.  At
> least the firmware program is lost during S3 and we need to reprogram it
> after resuming. Until this is clarified we keep the current behavior and
> enable/disable DC6.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 96650ed..6de2287 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -656,6 +656,9 @@ static int i915_drm_suspend(struct drm_device *dev)
>   
>   	intel_display_set_init_power(dev_priv, false);
>   
> +	if (HAS_CSR(dev_priv))
> +		flush_work(&dev_priv->csr.work);
> +
>   	return 0;
>   }
>   

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

Re: [PATCH v3 01/13] drm/i915/gen9: csr_init after runtime pm enable

[Sunil Kamath]

On Thursday 29 October 2015 03:28 AM, Imre Deak wrote:
> From: Animesh Manna <animesh.manna@intel.com>
>
> Skl is fully dependent on dmc for going to low power state (dc5/dc6).
> This requires a trigger from rpm. To ensure the dmc firmware
> is available for runtime pm support rpm-reference-count is used
> by not releasing the rpm reference if firmware loading is
> not completed.
>
> So moved the intel_csr_ucode_init call after runtime pm enable.
>
> 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>
> [imre: moved the call right after power domain init to avoid race with
>   the console modesetting]
> ---
>   drivers/gpu/drm/i915/i915_dma.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 2336af9..5d68942 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -398,6 +398,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>   
>   	intel_power_domains_init_hw(dev_priv);
>   
> +	intel_csr_ucode_init(dev);
> +

Its really unclear why this call to be done here.
We need to call just after intel_runtime_pm_enable.
With this change it will do much in advance.

Can you please add some more details about "how adding change after 
iniet_runtime_pm_enable() will create race condition for console 
modesetting?"

- Sunil


>   	ret = intel_irq_install(dev_priv);
>   	if (ret)
>   		goto cleanup_gem_stolen;
> @@ -942,9 +944,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;

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

Re: [PATCH v3 00/13] drm/i915: Redesign dmc firmware loading.

[Imre Deak]

On to, 2015-10-29 at 10:08 +0200, Jani Nikula wrote:
> On Wed, 28 Oct 2015, Imre Deak <imre.deak@intel.com> wrote:
> > This is a rebased version of [1], addressing the review comments. It
> > depends on Mika's FW version blacklisting/capture patchset [2].
> >
> > I have added my Reviewed-by to those patches that I only rebased or
> > updated the commit message, patches 1,4,12 have changes from me, so
> > someone else would need to review those.
> 
> You still need to add your Signed-off-by at the end when you're posting
> someone else's patches, whether you modified them or not.

Thanks for the note I missed those, I didn't really think about this. So
for all the patches in this series where it's missing:
Signed-off-by: Imre Deak <imre.deak@intel.com>

> 
> BR,
> Jani.
> 
> >
> > I tested this on BXT and SKL, on SKL-Y with additional patches from
> > Patrik and Paulo I could reach PC10 state.
> >
> > [1]
> > http://lists.freedesktop.org/archives/intel-gfx/2015-August/072921.html
> >
> > [2]
> > http://lists.freedesktop.org/archives/intel-gfx/2015-October/078898.html
> >
> > Animesh Manna (3):
> >   drm/i915/gen9: csr_init after runtime pm enable
> >   drm/i915/gen9: Use flush_work to synchronize with dmc loader
> >   drm/i915/skl: Removed assert for csr-fw-loading check during disabling
> >     dc6
> >
> > Daniel Vetter (9):
> >   drm/i915: use correct power domain for csr loading
> >   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: Don't try to load garbage dmc firmware on resume
> >   drm/i915/gen9: Use dev_priv in csr functions
> >   drm/i915/gen9: extract parse_csr_fw
> >   drm/i915: Use request_firmware and our own async work
> >
> > Imre Deak (1):
> >   drm/i915/gen9: flush DMC fw loading work during system suspend
> >
> >  drivers/gpu/drm/i915/i915_dma.c         |  10 +-
> >  drivers/gpu/drm/i915/i915_drv.c         |  40 ++-----
> >  drivers/gpu/drm/i915/i915_drv.h         |  12 +--
> >  drivers/gpu/drm/i915/i915_gpu_error.c   |   2 +-
> >  drivers/gpu/drm/i915/intel_csr.c        | 182 ++++++++++++--------------------
> >  drivers/gpu/drm/i915/intel_drv.h        |  10 +-
> >  drivers/gpu/drm/i915/intel_runtime_pm.c |  26 ++---
> >  7 files changed, 95 insertions(+), 187 deletions(-)
> 


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

Re: [PATCH v3 01/13] drm/i915/gen9: csr_init after runtime pm enable

[Imre Deak]

On to, 2015-10-29 at 15:48 +0530, Sunil Kamath wrote:
> On Thursday 29 October 2015 03:28 AM, Imre Deak wrote:
> > From: Animesh Manna <animesh.manna@intel.com>
> >
> > Skl is fully dependent on dmc for going to low power state (dc5/dc6).
> > This requires a trigger from rpm. To ensure the dmc firmware
> > is available for runtime pm support rpm-reference-count is used
> > by not releasing the rpm reference if firmware loading is
> > not completed.
> >
> > So moved the intel_csr_ucode_init call after runtime pm enable.
> >
> > 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>
> > [imre: moved the call right after power domain init to avoid race with
> >   the console modesetting]
> > ---
> >   drivers/gpu/drm/i915/i915_dma.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 2336af9..5d68942 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -398,6 +398,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >   
> >   	intel_power_domains_init_hw(dev_priv);
> >   
> > +	intel_csr_ucode_init(dev);
> > +
> 
> Its really unclear why this call to be done here.
> We need to call just after intel_runtime_pm_enable.
> With this change it will do much in advance.

In general the earlier we start the firmware loading work the better,
since we can start saving power the earliest this way.

> Can you please add some more details about "how adding change after 
> iniet_runtime_pm_enable() will create race condition for console 
> modesetting?"

We want to avoid to call into either the power well code or the runtime
s/r handlers. This is why we take a ref on the INIT power domain in
intel_csr_ucode_init() which should prevent both. Now if we do this only
after the point where a power well reference can drop to zero, there is
a chance that we call into the power well code before we took the above
INIT power domain ref.

If you look at i915_load_modeset_init(), it schedules
intel_fbdev_initial_config(), which may end up doing a modeset for the
console, leading up to intel_modeset_setup_hw_state() and
intel_display_set_init_power(false). Depending on the the active outputs
this may drop the last reference on some domains and call into the power
well code.

Due to similar reasons we could drop the last runtime PM reference and
call the runtime s/r handlers before the firmware is loaded if we took
the INIT power domain ref only after calling intel_runtime_pm_enable().

--Imre

> 
> - Sunil
> 
> 
> >   	ret = intel_irq_install(dev_priv);
> >   	if (ret)
> >   		goto cleanup_gem_stolen;
> > @@ -942,9 +944,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;
> 


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

Re: [PATCH v3 01/13] drm/i915/gen9: csr_init after runtime pm enable

[Sunil Kamath]

[-- Attachment #1.1: Type: text/plain, Size: 4061 bytes --]

On Thursday 29 October 2015 07:25 PM, Imre Deak wrote:
> On to, 2015-10-29 at 15:48 +0530, Sunil Kamath wrote:
>> On Thursday 29 October 2015 03:28 AM, Imre Deak wrote:
>>> From: Animesh Manna <animesh.manna@intel.com>
>>>
>>> Skl is fully dependent on dmc for going to low power state (dc5/dc6).
>>> This requires a trigger from rpm. To ensure the dmc firmware
>>> is available for runtime pm support rpm-reference-count is used
>>> by not releasing the rpm reference if firmware loading is
>>> not completed.
>>>
>>> So moved the intel_csr_ucode_init call after runtime pm enable.
>>>
>>> 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>
>>> [imre: moved the call right after power domain init to avoid race with
>>>    the console modesetting]
>>> ---
>>>    drivers/gpu/drm/i915/i915_dma.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>>> index 2336af9..5d68942 100644
>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>> @@ -398,6 +398,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>>>    
>>>    	intel_power_domains_init_hw(dev_priv);
>>>    
>>> +	intel_csr_ucode_init(dev);
>>> +
>> Its really unclear why this call to be done here.
>> We need to call just after intel_runtime_pm_enable.
>> With this change it will do much in advance.
> In general the earlier we start the firmware loading work the better,
> since we can start saving power the earliest this way.
>
>> Can you please add some more details about "how adding change after
>> iniet_runtime_pm_enable() will create race condition for console
>> modesetting?"
> We want to avoid to call into either the power well code or the runtime
> s/r handlers. This is why we take a ref on the INIT power domain in
> intel_csr_ucode_init() which should prevent both. Now if we do this only
> after the point where a power well reference can drop to zero, there is
> a chance that we call into the power well code before we took the above
> INIT power domain ref.
>
> If you look at i915_load_modeset_init(), it schedules
> intel_fbdev_initial_config(), which may end up doing a modeset for the
> console, leading up to intel_modeset_setup_hw_state() and
> intel_display_set_init_power(false). Depending on the the active outputs
> this may drop the last reference on some domains and call into the power
> well code.
>
> Due to similar reasons we could drop the last runtime PM reference and
> call the runtime s/r handlers before the firmware is loaded if we took
> the INIT power domain ref only after calling intel_runtime_pm_enable().
>
> --Imre
>
>> - Sunil
>>

Concern that we had was:

In intel_csr_ucode_init() we are calling 
intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); which intern call 
intel_runtime_pm_get. That means rpm will not be enabled before this 
code flow.

Now it’s clear that that's not a problem. As we do RPM gets even 
earlier. It will only drop an RPM reference, that in effect will make it 
possible to enter RPM. There are places taking an RPM ref already 
earlier like in intel_power_domains_init_hw() and RPM references are 
taken even before our driver's probe runtime is called by the pci core. 
So that's completely normal.

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

>>>    	ret = intel_irq_install(dev_priv);
>>>    	if (ret)
>>>    		goto cleanup_gem_stolen;
>>> @@ -942,9 +944,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;
>


[-- Attachment #1.2: Type: text/html, Size: 5618 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v3 00/13] drm/i915: Redesign dmc firmware loading.

[Daniel Stone]

Hi,

On 28 October 2015 at 21:58, Imre Deak <imre.deak@intel.com> wrote:
> This is a rebased version of [1], addressing the review comments. It
> depends on Mika's FW version blacklisting/capture patchset [2].
>
> I have added my Reviewed-by to those patches that I only rebased or
> updated the commit message, patches 1,4,12 have changes from me, so
> someone else would need to review those.
>
> I tested this on BXT and SKL, on SKL-Y with additional patches from
> Patrik and Paulo I could reach PC10 state.

Tested-by: Daniel Stone <daniels@collabora.com> # SKL

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

Re: [PATCH v3 03/13] drm/i915/gen9: move assert_csr_loaded into intel_rpm.c

[Imre Deak]

On ke, 2015-10-28 at 23:58 +0200, Imre Deak wrote:
> From: Daniel Vetter <daniel.vetter@intel.com>
> 
> Avoids non-static functions since all the callers are in intel_rpm.c.
> 
> 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>
> [imre: removed note about reg definitions from commit message, since
>  it's not relevant any more]
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_csr.c        | 10 ----------
>  drivers/gpu/drm/i915/intel_drv.h        |  1 -
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  8 ++++++++
>  3 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c
> b/drivers/gpu/drm/i915/intel_csr.c
> index 2c9bf3f..cd6fb58d 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -485,13 +485,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_ONCE(intel_csr_load_status_get(dev_priv) != FW_LOADED,
> -		  "CSR is not loaded.\n");
> -	WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
> -		  "CSR program storage start is NULL\n");
> -	WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not
> fine\n");
> -	WARN_ONCE(!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 1a3bbdc..3d1c744 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1211,7 +1211,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 e50cc88..92746e1 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -458,6 +458,14 @@ static void
> gen9_set_dc_state_debugmask_memory_up(
>  	}
>  }
>  
> +void assert_csr_loaded(struct drm_i915_private *dev_priv)

I missed it during review, but this is static. With that added my R-b
still holds.

> +{
> +	WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
> +		  "CSR program storage start is NULL\n");
> +	WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not
> fine\n");
> +	WARN_ONCE(!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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 03/13] drm/i915/gen9: move assert_csr_loaded into intel_rpm.c

[Jani Nikula]

On Thu, 12 Nov 2015, Imre Deak <imre.deak@intel.com> wrote:
> On ke, 2015-10-28 at 23:58 +0200, Imre Deak wrote:
>> From: Daniel Vetter <daniel.vetter@intel.com>
>> 
>> Avoids non-static functions since all the callers are in intel_rpm.c.
>> 
>> 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>
>> [imre: removed note about reg definitions from commit message, since
>>  it's not relevant any more]
>> Reviewed-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_csr.c        | 10 ----------
>>  drivers/gpu/drm/i915/intel_drv.h        |  1 -
>>  drivers/gpu/drm/i915/intel_runtime_pm.c |  8 ++++++++
>>  3 files changed, 8 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c
>> b/drivers/gpu/drm/i915/intel_csr.c
>> index 2c9bf3f..cd6fb58d 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -485,13 +485,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_ONCE(intel_csr_load_status_get(dev_priv) != FW_LOADED,
>> -		  "CSR is not loaded.\n");
>> -	WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
>> -		  "CSR program storage start is NULL\n");
>> -	WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not
>> fine\n");
>> -	WARN_ONCE(!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 1a3bbdc..3d1c744 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1211,7 +1211,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 e50cc88..92746e1 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -458,6 +458,14 @@ static void
>> gen9_set_dc_state_debugmask_memory_up(
>>  	}
>>  }
>>  
>> +void assert_csr_loaded(struct drm_i915_private *dev_priv)
>
> I missed it during review, but this is static. With that added my R-b
> still holds.

Pushed up to and including this patch, with the static added. The next
one doesn't apply cleanly.

BR,
Jani.


>
>> +{
>> +	WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
>> +		  "CSR program storage start is NULL\n");
>> +	WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not
>> fine\n");
>> +	WARN_ONCE(!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;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 03/13] drm/i915/gen9: move assert_csr_loaded into intel_rpm.c

[Imre Deak]

On to, 2015-11-12 at 16:48 +0200, Jani Nikula wrote:
> On Thu, 12 Nov 2015, Imre Deak <imre.deak@intel.com> wrote:
> > On ke, 2015-10-28 at 23:58 +0200, Imre Deak wrote:
> > > From: Daniel Vetter <daniel.vetter@intel.com>
> > > 
> > > Avoids non-static functions since all the callers are in
> > > intel_rpm.c.
> > > 
> > > 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>
> > > [imre: removed note about reg definitions from commit message,
> > > since
> > >  it's not relevant any more]
> > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_csr.c        | 10 ----------
> > >  drivers/gpu/drm/i915/intel_drv.h        |  1 -
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  8 ++++++++
> > >  3 files changed, 8 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_csr.c
> > > b/drivers/gpu/drm/i915/intel_csr.c
> > > index 2c9bf3f..cd6fb58d 100644
> > > --- a/drivers/gpu/drm/i915/intel_csr.c
> > > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > > @@ -485,13 +485,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_ONCE(intel_csr_load_status_get(dev_priv) !=
> > > FW_LOADED,
> > > -		  "CSR is not loaded.\n");
> > > -	WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
> > > -		  "CSR program storage start is NULL\n");
> > > -	WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not
> > > fine\n");
> > > -	WARN_ONCE(!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 1a3bbdc..3d1c744 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1211,7 +1211,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 e50cc88..92746e1 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -458,6 +458,14 @@ static void
> > > gen9_set_dc_state_debugmask_memory_up(
> > >  	}
> > >  }
> > >  
> > > +void assert_csr_loaded(struct drm_i915_private *dev_priv)
> > 
> > I missed it during review, but this is static. With that added my R
> > -b
> > still holds.
> 
> Pushed up to and including this patch, with the static added. The
> next
> one doesn't apply cleanly.

Ok, there were two minor conflicts with Mika's firmware version patchse
t that got merged meanwhile. I resend those two patches as v4.

--Imre

> 
> BR,
> Jani.
> 
> 
> > 
> > > +{
> > > +	WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
> > > +		  "CSR program storage start is NULL\n");
> > > +	WARN_ONCE(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not
> > > fine\n");
> > > +	WARN_ONCE(!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;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 04/13] drm/i915/gen9: Remove csr.state, csr_lock and related code.

[Imre Deak]

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.

Before enabling/disabling DC6 check if the firmware is loaded
successfully. This is guaranteed during runtime s/r, since otherwise we
don't enable RPM, but not during system s/r.

Note that it's still unclear whether we need to enable/disable DC6
during system s/r, until that's clarified, keep the current behavior and
enable/disable DC6.

Also after this patch there is a race during system s/r where the
firmware may not be loaded yet, that's addressed in an upcoming patch.

v2-v3:
- unchanged
v4:
- rebased on latest drm-intel-nightly

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>
[imre: added code and note about checking if the firmware loaded ok,
 before enabling/disabling it]
Reviewed-by: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         |  1 -
 drivers/gpu/drm/i915/i915_drv.c         | 11 ++------
 drivers/gpu/drm/i915/i915_drv.h         | 10 -------
 drivers/gpu/drm/i915/intel_csr.c        | 46 +--------------------------------
 drivers/gpu/drm/i915/intel_drv.h        |  3 ---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ++----------
 6 files changed, 5 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index cbed87a..2326189 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -892,7 +892,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);
 	mutex_init(&dev_priv->av_mutex);
 
 	intel_pm_setup(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9f55209..aa34fcb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1087,18 +1087,11 @@ static int i915_pm_resume(struct device *dev)
 
 static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
-	enum csr_state state;
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
 	skl_uninit_cdclk(dev_priv);
 
-	/* 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)
+	if (dev_priv->csr.dmc_payload)
 		skl_enable_dc6(dev_priv);
 
 	return 0;
@@ -1147,7 +1140,7 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
-	if (intel_csr_load_status_get(dev_priv) == FW_LOADED)
+	if (dev_priv->csr.dmc_payload)
 		skl_disable_dc6(dev_priv);
 
 	skl_init_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d5cf30b..389b8dd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -738,12 +738,6 @@ struct intel_uncore {
 #define CSR_VERSION_MAJOR(version)	((version) >> 16)
 #define CSR_VERSION_MINOR(version)	((version) & 0xffff)
 
-enum csr_state {
-	FW_UNINITIALIZED = 0,
-	FW_LOADED,
-	FW_FAILED
-};
-
 struct intel_csr {
 	const char *fw_path;
 	uint32_t *dmc_payload;
@@ -752,7 +746,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) \
@@ -1709,9 +1702,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 b29dd23..11efa13 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -199,40 +199,6 @@ static const struct stepping_info *intel_get_stepping_info(struct drm_device *de
 }
 
 /**
- * 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.
  *
@@ -260,7 +226,6 @@ void intel_csr_load_program(struct drm_device *dev)
 	if (I915_READ(CSR_PROGRAM(0)))
 		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(i), payload[i]);
@@ -269,9 +234,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)
@@ -412,8 +374,6 @@ out:
 			 CSR_VERSION_MAJOR(csr->version),
 			 CSR_VERSION_MINOR(csr->version));
 	} else {
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
-
 		i915_firmware_load_error_print(csr->fw_path, 0);
 	}
 
@@ -442,7 +402,6 @@ void intel_csr_ucode_init(struct drm_device *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;
 	}
 
@@ -459,10 +418,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);
-	}
 }
 
 /**
@@ -479,6 +436,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 57c419d..5f42511 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1221,9 +1221,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);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6585ca7..0df1aa6 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -664,8 +664,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 	} else {
 		if (enable_requested) {
 			if (IS_SKYLAKE(dev) &&
-				(power_well->data == SKL_DISP_PW_1) &&
-				(intel_csr_load_status_get(dev_priv) == FW_LOADED))
+				(power_well->data == SKL_DISP_PW_1))
 				DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n");
 			else {
 				I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
@@ -674,20 +673,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			}
 
 			if (GEN9_ENABLE_DC5(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_DEBUG("CSR firmware not ready (%d)\n",
-							state);
-				else
+				power_well->data == SKL_DISP_PW_2)
 					gen9_enable_dc5(dev_priv);
-			}
 		}
 	}
 
-- 
2.5.0

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

[PATCH v4 09/13] drm/i915/gen9: extract parse_csr_fw

[Imre Deak]

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.

v2-v3:
- unchanged
v4:
- rebased on top of latest drm-intel-nightly

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>
[imre: remove note about BE cast from commit message, it's not relevant
 any more]
Reviewed-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 51 +++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index d14d11c..496f54b 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -235,9 +235,9 @@ void intel_csr_load_program(struct drm_i915_private *dev_priv)
 	}
 }
 
-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;
@@ -248,14 +248,13 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
 	uint32_t i;
 	uint32_t *dmc_payload;
-	bool fw_loaded = false;
 
 	if (!fw)
-		goto out;
+		return NULL;
 
 	if (!stepping_info) {
 		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
-		goto out;
+		return NULL;
 	}
 
 	stepping = stepping_info->stepping;
@@ -267,7 +266,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;
 	}
 
 	csr->version = css_header->version;
@@ -280,7 +279,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 			 CSR_VERSION_MINOR(csr->version),
 			 CSR_VERSION_MAJOR(SKL_CSR_VERSION_REQUIRED),
 			 CSR_VERSION_MINOR(SKL_CSR_VERSION_REQUIRED));
-		goto out;
+		return NULL;
 	}
 
 	readcount += sizeof(struct intel_css_header);
@@ -292,7 +291,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);
 
@@ -312,7 +311,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;
 
@@ -321,7 +320,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);
 
@@ -329,7 +328,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++) {
@@ -337,7 +336,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];
@@ -347,25 +346,39 @@ 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;
 	memcpy(dmc_payload, &fw->data[readcount], nbytes);
 
+	return dmc_payload;
+}
+
+static void finish_csr_load(const struct firmware *fw, void *context)
+{
+	struct drm_i915_private *dev_priv = context;
+	struct intel_csr *csr = &dev_priv->csr;
+
+	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_priv);
-	fw_loaded = true;
 
 out:
-	if (fw_loaded) {
+	if (dev_priv->csr.dmc_payload) {
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
 		DRM_INFO("Finished loading %s (v%u.%u)\n",
-- 
2.5.0

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

Re: [PATCH v3 00/13] drm/i915: Redesign dmc firmware loading.

[Jani Nikula]

On Wed, 28 Oct 2015, Imre Deak <imre.deak@intel.com> wrote:
> This is a rebased version of [1], addressing the review comments. It
> depends on Mika's FW version blacklisting/capture patchset [2].

Entire series pushed to drm-intel-next-queued, thanks for everyone
involved.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx