[PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Animesh Manna]

Before enabling dc5/dc6, used wait for completion instead of busy waiting.

v1:
- Based on review comment from Daniel replaced mutex and related
implementation with completion. In current patch not used power
domain lock, don't want to block runtime power management if dmc
firmware failed to load. Will analyzing further and possibly send
as a incremental patch.
- Based on review comment from Damien, warning for firmware loading
failure is removed.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         |  1 -
 drivers/gpu/drm/i915/i915_drv.c         |  2 +-
 drivers/gpu/drm/i915/i915_drv.h         | 12 ++-----
 drivers/gpu/drm/i915/intel_csr.c        | 63 +++------------------------------
 drivers/gpu/drm/i915/intel_drv.h        |  3 --
 drivers/gpu/drm/i915/intel_runtime_pm.c | 16 +++------
 6 files changed, 12 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 78e6ae8..1f5c86c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -816,7 +816,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->mmio_flip_lock);
 	mutex_init(&dev_priv->dpio_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
-	mutex_init(&dev_priv->csr.csr_lock);
 
 	intel_pm_setup(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6bb6c47..c102268 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1034,7 +1034,7 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 	 * This is to ensure that CSR isn't identified as loaded before
 	 * CSR-loading program is called during runtime-resume.
 	 */
-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
+	reinit_completion(&dev_priv->csr.is_loaded);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 43011d7..423afa9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -50,6 +50,7 @@
 #include <linux/intel-iommu.h>
 #include <linux/kref.h>
 #include <linux/pm_qos.h>
+#include <linux/completion.h>
 
 /* General customization:
  */
@@ -669,23 +670,14 @@ struct intel_uncore {
 #define for_each_fw_domain(domain__, dev_priv__, i__) \
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
-enum csr_state {
-	FW_UNINITIALIZED = 0,
-	FW_LOADED,
-	FW_FAILED
-};
-
 struct intel_csr {
-	/* CSR protection, used to protect firmware loading status: csr_state */
-	struct mutex csr_lock;
-
+	struct completion is_loaded;
 	const char *fw_path;
 	__be32 *dmc_payload;
 	uint32_t dmc_fw_size;
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
 	uint32_t mmiodata[8];
-	enum csr_state state;
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index d1da2db..fec2bc5 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -32,13 +32,6 @@
  * onwards to drive newly added DMC (Display microcontroller) in display
  * engine to save and restore the state of display engine when it enter into
  * low-power state and comes back to normal.
- *
- * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
- * FW_LOADED, FW_FAILED.
- *
- * Once the firmware is written into the registers status will be moved from
- * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
- * be moved to FW_FAILED.
  */
 
 #define I915_CSR_SKL "i915/skl_dmc_ver4.bin"
@@ -200,40 +193,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.csr_lock);
-	state = dev_priv->csr.state;
-	mutex_unlock(&dev_priv->csr.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.csr_lock);
-	dev_priv->csr.state = state;
-	mutex_unlock(&dev_priv->csr.csr_lock);
-}
-
-/**
  * intel_csr_load_program() - write the firmware from memory to register.
  * @dev: drm device.
  *
@@ -252,7 +211,6 @@ void intel_csr_load_program(struct drm_device *dev)
 		return;
 	}
 
-	mutex_lock(&dev_priv->csr.csr_lock);
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
@@ -262,9 +220,7 @@ 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.csr_lock);
+	complete(&dev_priv->csr.is_loaded);
 }
 
 static void finish_csr_load(const struct firmware *fw, void *context)
@@ -280,7 +236,6 @@ 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;
 	__be32 *dmc_payload;
-	bool fw_loaded = false;
 
 	if (!fw) {
 		i915_firmware_load_error_print(csr->fw_path, 0);
@@ -387,14 +342,9 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
-	fw_loaded = true;
 
 out:
-	if (fw_loaded)
-		intel_runtime_pm_put(dev_priv);
-	else
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
-
+	intel_runtime_pm_put(dev_priv);
 	release_firmware(fw);
 }
 
@@ -418,7 +368,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 		csr->fw_path = I915_CSR_SKL;
 	else {
 		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
 		return;
 	}
 
@@ -428,15 +377,15 @@ void intel_csr_ucode_init(struct drm_device *dev)
 	 */
 	intel_runtime_pm_get(dev_priv);
 
+	init_completion(&csr->is_loaded);
+
 	/* 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) {
+	if (ret)
 		i915_firmware_load_error_print(csr->fw_path, ret);
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
-	}
 }
 
 /**
@@ -453,13 +402,11 @@ 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);
 }
 
 void assert_csr_loaded(struct drm_i915_private *dev_priv)
 {
-	WARN((intel_csr_load_status_get(dev_priv) != FW_LOADED), "CSR is not loaded.\n");
 	WARN(!I915_READ(CSR_PROGRAM_BASE),
 				"CSR program storage start is NULL\n");
 	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ea20edb..e022c98 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1155,9 +1155,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);
 void assert_csr_loaded(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b393db7..5a830d3 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -525,7 +525,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(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
 		"DC6 already programmed to be disabled.\n");
 }
@@ -642,21 +641,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
-				enum csr_state state;
-				/* TODO: wait for a completion event or
-				 * similar here instead of busy
-				 * waiting using wait_for function.
-				 */
-				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
-						FW_UNINITIALIZED, 1000);
-				if (state != FW_LOADED)
-					DRM_ERROR("CSR firmware not ready (%d)\n",
-							state);
-				else
+				if (wait_for_completion_timeout(
+					&dev_priv->csr.is_loaded, 100)) {
 					if (SKL_ENABLE_DC6(dev))
 						skl_enable_dc6(dev_priv);
 					else
 						gen9_enable_dc5(dev_priv);
+				} else
+					DRM_ERROR("CSR firmware not ready\n");
 			}
 		}
 	}
-- 
2.0.2

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

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Daniel Vetter]

On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna wrote:
> Before enabling dc5/dc6, used wait for completion instead of busy waiting.
> 
> v1:
> - Based on review comment from Daniel replaced mutex and related
> implementation with completion. In current patch not used power
> domain lock, don't want to block runtime power management if dmc
> firmware failed to load. Will analyzing further and possibly send
> as a incremental patch.
> - Based on review comment from Damien, warning for firmware loading
> failure is removed.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>

Sorry if this cross with my comments, but upon further digging into this
code we don't even need a completion at all. As long as we prevent the
power well from getting disabled by holding a reference for it before we
launch the firmware loader it'll all be fine. And the async work can then
just drop that reference when everything is set up.

Yes this means runtime D3 requires the firmware to be loaded, but that's
imo not a problem.

Also doing it this way is more in-line with how we do all the other async
setup. But there's another serious issue with this design here, see below.


> ---
>  drivers/gpu/drm/i915/i915_dma.c         |  1 -
>  drivers/gpu/drm/i915/i915_drv.c         |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h         | 12 ++-----
>  drivers/gpu/drm/i915/intel_csr.c        | 63 +++------------------------------
>  drivers/gpu/drm/i915/intel_drv.h        |  3 --
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 16 +++------
>  6 files changed, 12 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 78e6ae8..1f5c86c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -816,7 +816,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	spin_lock_init(&dev_priv->mmio_flip_lock);
>  	mutex_init(&dev_priv->dpio_lock);
>  	mutex_init(&dev_priv->modeset_restore_lock);
> -	mutex_init(&dev_priv->csr.csr_lock);
>  
>  	intel_pm_setup(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6bb6c47..c102268 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1034,7 +1034,7 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  	 * This is to ensure that CSR isn't identified as loaded before
>  	 * CSR-loading program is called during runtime-resume.
>  	 */
> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> +	reinit_completion(&dev_priv->csr.is_loaded);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 43011d7..423afa9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -50,6 +50,7 @@
>  #include <linux/intel-iommu.h>
>  #include <linux/kref.h>
>  #include <linux/pm_qos.h>
> +#include <linux/completion.h>
>  
>  /* General customization:
>   */
> @@ -669,23 +670,14 @@ struct intel_uncore {
>  #define for_each_fw_domain(domain__, dev_priv__, i__) \
>  	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
>  
> -enum csr_state {
> -	FW_UNINITIALIZED = 0,
> -	FW_LOADED,
> -	FW_FAILED
> -};
> -
>  struct intel_csr {
> -	/* CSR protection, used to protect firmware loading status: csr_state */
> -	struct mutex csr_lock;
> -
> +	struct completion is_loaded;
>  	const char *fw_path;
>  	__be32 *dmc_payload;
>  	uint32_t dmc_fw_size;
>  	uint32_t mmio_count;
>  	uint32_t mmioaddr[8];
>  	uint32_t mmiodata[8];
> -	enum csr_state state;
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index d1da2db..fec2bc5 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -32,13 +32,6 @@
>   * onwards to drive newly added DMC (Display microcontroller) in display
>   * engine to save and restore the state of display engine when it enter into
>   * low-power state and comes back to normal.
> - *
> - * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
> - * FW_LOADED, FW_FAILED.
> - *
> - * Once the firmware is written into the registers status will be moved from
> - * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
> - * be moved to FW_FAILED.
>   */
>  
>  #define I915_CSR_SKL "i915/skl_dmc_ver4.bin"
> @@ -200,40 +193,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.csr_lock);
> -	state = dev_priv->csr.state;
> -	mutex_unlock(&dev_priv->csr.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.csr_lock);
> -	dev_priv->csr.state = state;
> -	mutex_unlock(&dev_priv->csr.csr_lock);
> -}
> -
> -/**
>   * intel_csr_load_program() - write the firmware from memory to register.
>   * @dev: drm device.
>   *
> @@ -252,7 +211,6 @@ void intel_csr_load_program(struct drm_device *dev)
>  		return;
>  	}
>  
> -	mutex_lock(&dev_priv->csr.csr_lock);
>  	fw_size = dev_priv->csr.dmc_fw_size;
>  	for (i = 0; i < fw_size; i++)
>  		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> @@ -262,9 +220,7 @@ 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.csr_lock);
> +	complete(&dev_priv->csr.is_loaded);
>  }
>  
>  static void finish_csr_load(const struct firmware *fw, void *context)
> @@ -280,7 +236,6 @@ 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;
>  	__be32 *dmc_payload;
> -	bool fw_loaded = false;
>  
>  	if (!fw) {
>  		i915_firmware_load_error_print(csr->fw_path, 0);
> @@ -387,14 +342,9 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>  
>  	/* load csr program during system boot, as needed for DC states */
>  	intel_csr_load_program(dev);
> -	fw_loaded = true;
>  
>  out:
> -	if (fw_loaded)
> -		intel_runtime_pm_put(dev_priv);
> -	else
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
> -
> +	intel_runtime_pm_put(dev_priv);
>  	release_firmware(fw);
>  }
>  
> @@ -418,7 +368,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  		csr->fw_path = I915_CSR_SKL;
>  	else {
>  		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
>  		return;
>  	}
>  
> @@ -428,15 +377,15 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  	 */
>  	intel_runtime_pm_get(dev_priv);
>  
> +	init_completion(&csr->is_loaded);
> +
>  	/* 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) {
> +	if (ret)
>  		i915_firmware_load_error_print(csr->fw_path, ret);
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
> -	}
>  }
>  
>  /**
> @@ -453,13 +402,11 @@ 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);
>  }
>  
>  void assert_csr_loaded(struct drm_i915_private *dev_priv)
>  {
> -	WARN((intel_csr_load_status_get(dev_priv) != FW_LOADED), "CSR is not loaded.\n");
>  	WARN(!I915_READ(CSR_PROGRAM_BASE),
>  				"CSR program storage start is NULL\n");
>  	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ea20edb..e022c98 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1155,9 +1155,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);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b393db7..5a830d3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -525,7 +525,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(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
>  		"DC6 already programmed to be disabled.\n");
>  }
> @@ -642,21 +641,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  
>  			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>  				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* TODO: wait for a completion event or
> -				 * similar here instead of busy
> -				 * waiting using wait_for function.
> -				 */
> -				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> -						FW_UNINITIALIZED, 1000);
> -				if (state != FW_LOADED)
> -					DRM_ERROR("CSR firmware not ready (%d)\n",
> -							state);
> -				else
> +				if (wait_for_completion_timeout(
> +					&dev_priv->csr.is_loaded, 100)) {

The problem is with this design that this wait here isn't allowed to ever
time out, since if it does the book-keeping gets out of sync. And for a
bunch of reasons (delays, deadlocks) we can't have that wait here really
at all, much less one that never times out.

Chandra has already hit this on a system where the firmware just isn't
available.

Thanks, Daniel

>  					if (SKL_ENABLE_DC6(dev))
>  						skl_enable_dc6(dev_priv);
>  					else
>  						gen9_enable_dc5(dev_priv);
> +				} else
> +					DRM_ERROR("CSR firmware not ready\n");
>  			}
>  		}
>  	}
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Animesh Manna]

On 5/21/2015 5:41 PM, Daniel Vetter wrote:
> On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna wrote:
>> Before enabling dc5/dc6, used wait for completion instead of busy waiting.
>>
>> v1:
>> - Based on review comment from Daniel replaced mutex and related
>> implementation with completion. In current patch not used power
>> domain lock, don't want to block runtime power management if dmc
>> firmware failed to load. Will analyzing further and possibly send
>> as a incremental patch.
>> - Based on review comment from Damien, warning for firmware loading
>> failure is removed.
>>
>> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> Sorry if this cross with my comments, but upon further digging into this
> code we don't even need a completion at all. As long as we prevent the
> power well from getting disabled by holding a reference for it before we
> launch the firmware loader it'll all be fine. And the async work can then
> just drop that reference when everything is set up.
>
> Yes this means runtime D3 requires the firmware to be loaded, but that's
> imo not a problem.
>
> Also doing it this way is more in-line with how we do all the other async
> setup. But there's another serious issue with this design here, see below.
>
Ok, previously I was thinking more on how to pass the firmware loading status
before enabling low power states (dc5/dc6). Now while thinking about power
management framework I have a fundamental doubt - if got request to disable a power-well
and firmware loading failed for some reason, should we disable the power-well(option1) or
keep it enable(option2). Both the cases will not trigger for dc5/dc6.

I understood from your comment to follow option2, I will change the design accordingly
as the current implementation based on option1.

To implement option2, I can see there is one mutex present for a power domain, do not have
mutex for each power-well, which need to be added and can be used as you mentioned above.

If we want to follow option1, I can think a better way of managing firmware loading.
As firmware is required when display engine goes to low power state, so we can only parse
the packaged firmware during driver initialization. Firmware loading (writing to registers)
can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6.

Planning to implement option2 and will send for review, please correct me if I understood wrongly.

>> ---
>>   drivers/gpu/drm/i915/i915_dma.c         |  1 -
>>   drivers/gpu/drm/i915/i915_drv.c         |  2 +-
>>   drivers/gpu/drm/i915/i915_drv.h         | 12 ++-----
>>   drivers/gpu/drm/i915/intel_csr.c        | 63 +++------------------------------
>>   drivers/gpu/drm/i915/intel_drv.h        |  3 --
>>   drivers/gpu/drm/i915/intel_runtime_pm.c | 16 +++------
>>   6 files changed, 12 insertions(+), 85 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 78e6ae8..1f5c86c 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -816,7 +816,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>   	spin_lock_init(&dev_priv->mmio_flip_lock);
>>   	mutex_init(&dev_priv->dpio_lock);
>>   	mutex_init(&dev_priv->modeset_restore_lock);
>> -	mutex_init(&dev_priv->csr.csr_lock);
>>   
>>   	intel_pm_setup(dev);
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 6bb6c47..c102268 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1034,7 +1034,7 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>>   	 * This is to ensure that CSR isn't identified as loaded before
>>   	 * CSR-loading program is called during runtime-resume.
>>   	 */
>> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
>> +	reinit_completion(&dev_priv->csr.is_loaded);
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 43011d7..423afa9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -50,6 +50,7 @@
>>   #include <linux/intel-iommu.h>
>>   #include <linux/kref.h>
>>   #include <linux/pm_qos.h>
>> +#include <linux/completion.h>
>>   
>>   /* General customization:
>>    */
>> @@ -669,23 +670,14 @@ struct intel_uncore {
>>   #define for_each_fw_domain(domain__, dev_priv__, i__) \
>>   	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
>>   
>> -enum csr_state {
>> -	FW_UNINITIALIZED = 0,
>> -	FW_LOADED,
>> -	FW_FAILED
>> -};
>> -
>>   struct intel_csr {
>> -	/* CSR protection, used to protect firmware loading status: csr_state */
>> -	struct mutex csr_lock;
>> -
>> +	struct completion is_loaded;
>>   	const char *fw_path;
>>   	__be32 *dmc_payload;
>>   	uint32_t dmc_fw_size;
>>   	uint32_t mmio_count;
>>   	uint32_t mmioaddr[8];
>>   	uint32_t mmiodata[8];
>> -	enum csr_state state;
>>   };
>>   
>>   #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
>> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>> index d1da2db..fec2bc5 100644
>> --- a/drivers/gpu/drm/i915/intel_csr.c
>> +++ b/drivers/gpu/drm/i915/intel_csr.c
>> @@ -32,13 +32,6 @@
>>    * onwards to drive newly added DMC (Display microcontroller) in display
>>    * engine to save and restore the state of display engine when it enter into
>>    * low-power state and comes back to normal.
>> - *
>> - * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
>> - * FW_LOADED, FW_FAILED.
>> - *
>> - * Once the firmware is written into the registers status will be moved from
>> - * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
>> - * be moved to FW_FAILED.
>>    */
>>   
>>   #define I915_CSR_SKL "i915/skl_dmc_ver4.bin"
>> @@ -200,40 +193,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.csr_lock);
>> -	state = dev_priv->csr.state;
>> -	mutex_unlock(&dev_priv->csr.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.csr_lock);
>> -	dev_priv->csr.state = state;
>> -	mutex_unlock(&dev_priv->csr.csr_lock);
>> -}
>> -
>> -/**
>>    * intel_csr_load_program() - write the firmware from memory to register.
>>    * @dev: drm device.
>>    *
>> @@ -252,7 +211,6 @@ void intel_csr_load_program(struct drm_device *dev)
>>   		return;
>>   	}
>>   
>> -	mutex_lock(&dev_priv->csr.csr_lock);
>>   	fw_size = dev_priv->csr.dmc_fw_size;
>>   	for (i = 0; i < fw_size; i++)
>>   		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
>> @@ -262,9 +220,7 @@ 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.csr_lock);
>> +	complete(&dev_priv->csr.is_loaded);
>>   }
>>   
>>   static void finish_csr_load(const struct firmware *fw, void *context)
>> @@ -280,7 +236,6 @@ 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;
>>   	__be32 *dmc_payload;
>> -	bool fw_loaded = false;
>>   
>>   	if (!fw) {
>>   		i915_firmware_load_error_print(csr->fw_path, 0);
>> @@ -387,14 +342,9 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>>   
>>   	/* load csr program during system boot, as needed for DC states */
>>   	intel_csr_load_program(dev);
>> -	fw_loaded = true;
>>   
>>   out:
>> -	if (fw_loaded)
>> -		intel_runtime_pm_put(dev_priv);
>> -	else
>> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
>> -
>> +	intel_runtime_pm_put(dev_priv);
>>   	release_firmware(fw);
>>   }
>>   
>> @@ -418,7 +368,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
>>   		csr->fw_path = I915_CSR_SKL;
>>   	else {
>>   		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
>> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
>>   		return;
>>   	}
>>   
>> @@ -428,15 +377,15 @@ void intel_csr_ucode_init(struct drm_device *dev)
>>   	 */
>>   	intel_runtime_pm_get(dev_priv);
>>   
>> +	init_completion(&csr->is_loaded);
>> +
>>   	/* 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) {
>> +	if (ret)
>>   		i915_firmware_load_error_print(csr->fw_path, ret);
>> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
>> -	}
>>   }
>>   
>>   /**
>> @@ -453,13 +402,11 @@ 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);
>>   }
>>   
>>   void assert_csr_loaded(struct drm_i915_private *dev_priv)
>>   {
>> -	WARN((intel_csr_load_status_get(dev_priv) != FW_LOADED), "CSR is not loaded.\n");
>>   	WARN(!I915_READ(CSR_PROGRAM_BASE),
>>   				"CSR program storage start is NULL\n");
>>   	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index ea20edb..e022c98 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1155,9 +1155,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);
>>   void assert_csr_loaded(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index b393db7..5a830d3 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -525,7 +525,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(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
>>   		"DC6 already programmed to be disabled.\n");
>>   }
>> @@ -642,21 +641,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>>   
>>   			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>>   				power_well->data == SKL_DISP_PW_2) {
>> -				enum csr_state state;
>> -				/* TODO: wait for a completion event or
>> -				 * similar here instead of busy
>> -				 * waiting using wait_for function.
>> -				 */
>> -				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
>> -						FW_UNINITIALIZED, 1000);
>> -				if (state != FW_LOADED)
>> -					DRM_ERROR("CSR firmware not ready (%d)\n",
>> -							state);
>> -				else
>> +				if (wait_for_completion_timeout(
>> +					&dev_priv->csr.is_loaded, 100)) {
> The problem is with this design that this wait here isn't allowed to ever
> time out, since if it does the book-keeping gets out of sync. And for a
> bunch of reasons (delays, deadlocks) we can't have that wait here really
> at all, much less one that never times out.
>
> Chandra has already hit this on a system where the firmware just isn't
> available.
>
> Thanks, Daniel
>
>>   					if (SKL_ENABLE_DC6(dev))
>>   						skl_enable_dc6(dev_priv);
>>   					else
>>   						gen9_enable_dc5(dev_priv);
>> +				} else
>> +					DRM_ERROR("CSR firmware not ready\n");
>>   			}
>>   		}
>>   	}
>> -- 
>> 2.0.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Daniel Vetter]

On Thu, May 21, 2015 at 10:35:07PM +0530, Animesh Manna wrote:
> 
> 
> On 5/21/2015 5:41 PM, Daniel Vetter wrote:
> >On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna wrote:
> >>Before enabling dc5/dc6, used wait for completion instead of busy waiting.
> >>
> >>v1:
> >>- Based on review comment from Daniel replaced mutex and related
> >>implementation with completion. In current patch not used power
> >>domain lock, don't want to block runtime power management if dmc
> >>firmware failed to load. Will analyzing further and possibly send
> >>as a incremental patch.
> >>- Based on review comment from Damien, warning for firmware loading
> >>failure is removed.
> >>
> >>Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >Sorry if this cross with my comments, but upon further digging into this
> >code we don't even need a completion at all. As long as we prevent the
> >power well from getting disabled by holding a reference for it before we
> >launch the firmware loader it'll all be fine. And the async work can then
> >just drop that reference when everything is set up.
> >
> >Yes this means runtime D3 requires the firmware to be loaded, but that's
> >imo not a problem.
> >
> >Also doing it this way is more in-line with how we do all the other async
> >setup. But there's another serious issue with this design here, see below.
> >
> Ok, previously I was thinking more on how to pass the firmware loading status
> before enabling low power states (dc5/dc6). Now while thinking about power
> management framework I have a fundamental doubt - if got request to disable a power-well
> and firmware loading failed for some reason, should we disable the power-well(option1) or
> keep it enable(option2). Both the cases will not trigger for dc5/dc6.
> 
> I understood from your comment to follow option2, I will change the design accordingly
> as the current implementation based on option1.
> 
> To implement option2, I can see there is one mutex present for a power domain, do not have
> mutex for each power-well, which need to be added and can be used as you mentioned above.
> 
> If we want to follow option1, I can think a better way of managing firmware loading.
> As firmware is required when display engine goes to low power state, so we can only parse
> the packaged firmware during driver initialization. Firmware loading (writing to registers)
> can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6.
> 
> Planning to implement option2 and will send for review, please correct me if I understood wrongly.

power wells are reference counted, which means the only thing you need to
do is grab such a reference. Then the corresponding power well won't ever
get disabled until the async firmware loader has done it's job and
released the power well reference again.

See e.g. how the async rps setup grabs a runtime_pm reference (works the
same for power wells, they simply nest within the runtime pm stuff). So
for option2 no fiddling with mutexes or power well internals needed at all
(well the wait_for can be removed afterwards ofc). And we don't need any
other mutex, the csr_mutex and crs.state can be removed afterwards too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Sagar Arun Kamble]

On Thu, 2015-05-21 at 23:29 +0200, Daniel Vetter wrote:
> On Thu, May 21, 2015 at 10:35:07PM +0530, Animesh Manna wrote:
> > 
> > 
> > On 5/21/2015 5:41 PM, Daniel Vetter wrote:
> > >On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna wrote:
> > >>Before enabling dc5/dc6, used wait for completion instead of busy waiting.
> > >>
> > >>v1:
> > >>- Based on review comment from Daniel replaced mutex and related
> > >>implementation with completion. In current patch not used power
> > >>domain lock, don't want to block runtime power management if dmc
> > >>firmware failed to load. Will analyzing further and possibly send
> > >>as a incremental patch.
> > >>- Based on review comment from Damien, warning for firmware loading
> > >>failure is removed.
> > >>
> > >>Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > >Sorry if this cross with my comments, but upon further digging into this
> > >code we don't even need a completion at all. As long as we prevent the
> > >power well from getting disabled by holding a reference for it before we
> > >launch the firmware loader it'll all be fine. And the async work can then
> > >just drop that reference when everything is set up.
> > >
> > >Yes this means runtime D3 requires the firmware to be loaded, but that's
> > >imo not a problem.
> > >
> > >Also doing it this way is more in-line with how we do all the other async
> > >setup. But there's another serious issue with this design here, see below.
> > >
> > Ok, previously I was thinking more on how to pass the firmware loading status
> > before enabling low power states (dc5/dc6). Now while thinking about power
> > management framework I have a fundamental doubt - if got request to disable a power-well
> > and firmware loading failed for some reason, should we disable the power-well(option1) or
> > keep it enable(option2). Both the cases will not trigger for dc5/dc6.
> > 
> > I understood from your comment to follow option2, I will change the design accordingly
> > as the current implementation based on option1.
> > 
> > To implement option2, I can see there is one mutex present for a power domain, do not have
> > mutex for each power-well, which need to be added and can be used as you mentioned above.
> > 
> > If we want to follow option1, I can think a better way of managing firmware loading.
> > As firmware is required when display engine goes to low power state, so we can only parse
> > the packaged firmware during driver initialization. Firmware loading (writing to registers)
> > can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6.
> > 
> > Planning to implement option2 and will send for review, please correct me if I understood wrongly.
> 
> power wells are reference counted, which means the only thing you need to
> do is grab such a reference. Then the corresponding power well won't ever
> get disabled until the async firmware loader has done it's job and
> released the power well reference again.
> 
> See e.g. how the async rps setup grabs a runtime_pm reference (works the
> same for power wells, they simply nest within the runtime pm stuff). So
> for option2 no fiddling with mutexes or power well internals needed at all
> (well the wait_for can be removed afterwards ofc). And we don't need any
> other mutex, the csr_mutex and crs.state can be removed afterwards too.
> -Daniel

Hi Daniel,

We already are grabbing RPM reference before start of DMC FW load and
release post load completion.

DC5/6 can happen without Runtime PM as well. So we need to wait for CSR
FW load for some time once we disable PW2.

Having completion instead of csr.lock+csr.state is  correct thing to do.

Thanks,
Sagar


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

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Dave Gordon]

On 04/06/15 06:59, Sagar Arun Kamble wrote:
> 
> Hi Daniel,
> 
> We already are grabbing RPM reference before start of DMC FW load and
> release post load completion.
> 
> DC5/6 can happen without Runtime PM as well. So we need to wait for CSR
> FW load for some time once we disable PW2.
> 
> Having completion instead of csr.lock+csr.state is  correct thing to do.
> 
> Thanks,
> Sagar
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Hi guys,

we have a new unified loader for GuC and HuC, which we intend should be
usable for any other microcontrollers that might be added in future. So
it would be good to consider using it for the DMC/CSR/etc.

The most-recently posted version was Alex's on 2015-04-29, but we're
just about to re-post an updated patchset for command submission via the
GuC, which will include the both unified firmware loading module and the
GuC-specific part of the loader.

Please have a look at these patches and let me know whether there's
anything that would help with using the unified code for your uC(s).

http://lists.freedesktop.org/archives/intel-gfx/2015-April/065575.html
http://lists.freedesktop.org/archives/intel-gfx/2015-April/065583.html

One thing that may not be obvious from the posted patches is that the
uC-specific code can override the way the f/w image is saved e.g. if the
loaded file image contains sections that are only needed once, they need
not be saved; more generally, if the way that the data in the image file
is organised doesn't match the way the data is used, the loader can
shuffle the content around for the convenience of the final consumer.
(We needed that at one stage, as the GuC firmware format didn't match
the constraints of the GuC DMA engine; but the format has been fixed
since, so the data-shuffler is not part of the public patch sequence).

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

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Daniel Vetter]

On Thu, Jun 04, 2015 at 11:29:35AM +0530, Sagar Arun Kamble wrote:
> On Thu, 2015-05-21 at 23:29 +0200, Daniel Vetter wrote:
> > On Thu, May 21, 2015 at 10:35:07PM +0530, Animesh Manna wrote:
> > > 
> > > 
> > > On 5/21/2015 5:41 PM, Daniel Vetter wrote:
> > > >On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna wrote:
> > > >>Before enabling dc5/dc6, used wait for completion instead of busy waiting.
> > > >>
> > > >>v1:
> > > >>- Based on review comment from Daniel replaced mutex and related
> > > >>implementation with completion. In current patch not used power
> > > >>domain lock, don't want to block runtime power management if dmc
> > > >>firmware failed to load. Will analyzing further and possibly send
> > > >>as a incremental patch.
> > > >>- Based on review comment from Damien, warning for firmware loading
> > > >>failure is removed.
> > > >>
> > > >>Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > >Sorry if this cross with my comments, but upon further digging into this
> > > >code we don't even need a completion at all. As long as we prevent the
> > > >power well from getting disabled by holding a reference for it before we
> > > >launch the firmware loader it'll all be fine. And the async work can then
> > > >just drop that reference when everything is set up.
> > > >
> > > >Yes this means runtime D3 requires the firmware to be loaded, but that's
> > > >imo not a problem.
> > > >
> > > >Also doing it this way is more in-line with how we do all the other async
> > > >setup. But there's another serious issue with this design here, see below.
> > > >
> > > Ok, previously I was thinking more on how to pass the firmware loading status
> > > before enabling low power states (dc5/dc6). Now while thinking about power
> > > management framework I have a fundamental doubt - if got request to disable a power-well
> > > and firmware loading failed for some reason, should we disable the power-well(option1) or
> > > keep it enable(option2). Both the cases will not trigger for dc5/dc6.
> > > 
> > > I understood from your comment to follow option2, I will change the design accordingly
> > > as the current implementation based on option1.
> > > 
> > > To implement option2, I can see there is one mutex present for a power domain, do not have
> > > mutex for each power-well, which need to be added and can be used as you mentioned above.
> > > 
> > > If we want to follow option1, I can think a better way of managing firmware loading.
> > > As firmware is required when display engine goes to low power state, so we can only parse
> > > the packaged firmware during driver initialization. Firmware loading (writing to registers)
> > > can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6.
> > > 
> > > Planning to implement option2 and will send for review, please correct me if I understood wrongly.
> > 
> > power wells are reference counted, which means the only thing you need to
> > do is grab such a reference. Then the corresponding power well won't ever
> > get disabled until the async firmware loader has done it's job and
> > released the power well reference again.
> > 
> > See e.g. how the async rps setup grabs a runtime_pm reference (works the
> > same for power wells, they simply nest within the runtime pm stuff). So
> > for option2 no fiddling with mutexes or power well internals needed at all
> > (well the wait_for can be removed afterwards ofc). And we don't need any
> > other mutex, the csr_mutex and crs.state can be removed afterwards too.
> > -Daniel
> 
> Hi Daniel,
> 
> We already are grabbing RPM reference before start of DMC FW load and
> release post load completion.
> 
> DC5/6 can happen without Runtime PM as well. So we need to wait for CSR
> FW load for some time once we disable PW2.

If dc5/6 can happen despite you holding a runtime pm reference, then
you're holding the wrong runtime pm reference. They nest, hence just walk
up the chain of power domains until you have one that does prevent dc5/6.

I guess there's a confusion between runtime pm as the overall concept
(which I'm talking about here, implemented in intel_rpm.c) and the linux
runtime pm api which is only used for the very last level of runtime pm in
i915 to control entry/exit to D3. On top of that we stack the power wells
i915 specific runtime pm api and the corresponding busyness tracking on
the render side.

> Having completion instead of csr.lock+csr.state is  correct thing to do.

I hope the above explanation helps. I can follow up with a sketch patch
too if you want.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Daniel Vetter]

On Thu, Jun 04, 2015 at 03:36:32PM +0100, Dave Gordon wrote:
> On 04/06/15 06:59, Sagar Arun Kamble wrote:
> > 
> > Hi Daniel,
> > 
> > We already are grabbing RPM reference before start of DMC FW load and
> > release post load completion.
> > 
> > DC5/6 can happen without Runtime PM as well. So we need to wait for CSR
> > FW load for some time once we disable PW2.
> > 
> > Having completion instead of csr.lock+csr.state is  correct thing to do.
> > 
> > Thanks,
> > Sagar
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> Hi guys,
> 
> we have a new unified loader for GuC and HuC, which we intend should be
> usable for any other microcontrollers that might be added in future. So
> it would be good to consider using it for the DMC/CSR/etc.
> 
> The most-recently posted version was Alex's on 2015-04-29, but we're
> just about to re-post an updated patchset for command submission via the
> GuC, which will include the both unified firmware loading module and the
> GuC-specific part of the loader.
> 
> Please have a look at these patches and let me know whether there's
> anything that would help with using the unified code for your uC(s).
> 
> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065575.html
> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065583.html
> 
> One thing that may not be obvious from the posted patches is that the
> uC-specific code can override the way the f/w image is saved e.g. if the
> loaded file image contains sections that are only needed once, they need
> not be saved; more generally, if the way that the data in the image file
> is organised doesn't match the way the data is used, the loader can
> shuffle the content around for the convenience of the final consumer.
> (We needed that at one stage, as the GuC firmware format didn't match
> the constraints of the GuC DMA engine; but the format has been fixed
> since, so the data-shuffler is not part of the public patch sequence).

Hm just looked at the generic firmware loader and not convinced this is a
good idea at all. As this discussion here shows synchronization here is a
tricky business. And in general driver load ordering is full of peril.
Trying to force a common abstraction for something rather trivial (we have
completions for the most basic case already) will only result in pains
trying to work around the not-quite-fitting infrastructure.

In my experience trying to extract common code at all costs is harmful
way too often.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Dave Gordon]

On 15/06/15 11:07, Daniel Vetter wrote:
> On Thu, Jun 04, 2015 at 03:36:32PM +0100, Dave Gordon wrote:
>> On 04/06/15 06:59, Sagar Arun Kamble wrote:
>>>
>>> Hi Daniel,
>>>
>>> We already are grabbing RPM reference before start of DMC FW load and
>>> release post load completion.
>>>
>>> DC5/6 can happen without Runtime PM as well. So we need to wait for CSR
>>> FW load for some time once we disable PW2.
>>>
>>> Having completion instead of csr.lock+csr.state is  correct thing to do.
>>>
>>> Thanks,
>>> Sagar
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> Hi guys,
>>
>> we have a new unified loader for GuC and HuC, which we intend should be
>> usable for any other microcontrollers that might be added in future. So
>> it would be good to consider using it for the DMC/CSR/etc.
>>
>> The most-recently posted version was Alex's on 2015-04-29, but we're
>> just about to re-post an updated patchset for command submission via the
>> GuC, which will include the both unified firmware loading module and the
>> GuC-specific part of the loader.
>>
>> Please have a look at these patches and let me know whether there's
>> anything that would help with using the unified code for your uC(s).
>>
>> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065575.html
>> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065583.html
>>
>> One thing that may not be obvious from the posted patches is that the
>> uC-specific code can override the way the f/w image is saved e.g. if the
>> loaded file image contains sections that are only needed once, they need
>> not be saved; more generally, if the way that the data in the image file
>> is organised doesn't match the way the data is used, the loader can
>> shuffle the content around for the convenience of the final consumer.
>> (We needed that at one stage, as the GuC firmware format didn't match
>> the constraints of the GuC DMA engine; but the format has been fixed
>> since, so the data-shuffler is not part of the public patch sequence).
> 
> Hm just looked at the generic firmware loader and not convinced this is a
> good idea at all. As this discussion here shows synchronization here is a
> tricky business. And in general driver load ordering is full of peril.
> Trying to force a common abstraction for something rather trivial (we have
> completions for the most basic case already) will only result in pains
> trying to work around the not-quite-fitting infrastructure.
> 
> In my experience trying to extract common code at all costs is harmful
> way too often.
> -Daniel

That version is now obsolete; the new version of the generic loader code
is in the GuC submission series that I just posted.

Thanks,
.Dave.

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

Re: [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading

[Daniel Vetter]

Quick summary of the tasks around dmc loader that we talked about in
our mtg just now.

- replace runtime_pm_get/put with display_power_get/put to make sure
we prevent entering dc5/6 code while the firmware loading is in
progress. Then remove the wait code in the skl power well code and
replace it with a simple status check (maybe add a new dmc_present
boolean) to decide which path to take. This way we only ever run that
code when firmware loading is guaranteed to have either completed or
failed, which means no bugs with tricky fallback/upgrade code at
runtime.

- replace request_firmware_nowait with the synchronous
request_firmware, but run from our own async work item. Use flush_work
instead of csr_load_status_set in suspend and driver unload code to
explicitly synchronize with that async work. Note that this replacment
also has the upside that request_firmware properly supports the
(optional, modern linux distros disable it) upstream userspace
firmware loader fallback.

The goal is to replace the hand-rolled synchronization with
established primitives (flush_work and our power well get/put code)
and completely remove the csr_load_status_set call and the csr_lock
mutex.

Thanks, Daniel

On Thu, May 21, 2015 at 12:19 PM, Animesh Manna <animesh.manna@intel.com> wrote:
> Before enabling dc5/dc6, used wait for completion instead of busy waiting.
>
> v1:
> - Based on review comment from Daniel replaced mutex and related
> implementation with completion. In current patch not used power
> domain lock, don't want to block runtime power management if dmc
> firmware failed to load. Will analyzing further and possibly send
> as a incremental patch.
> - Based on review comment from Damien, warning for firmware loading
> failure is removed.
>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c         |  1 -
>  drivers/gpu/drm/i915/i915_drv.c         |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h         | 12 ++-----
>  drivers/gpu/drm/i915/intel_csr.c        | 63 +++------------------------------
>  drivers/gpu/drm/i915/intel_drv.h        |  3 --
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 16 +++------
>  6 files changed, 12 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 78e6ae8..1f5c86c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -816,7 +816,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>         spin_lock_init(&dev_priv->mmio_flip_lock);
>         mutex_init(&dev_priv->dpio_lock);
>         mutex_init(&dev_priv->modeset_restore_lock);
> -       mutex_init(&dev_priv->csr.csr_lock);
>
>         intel_pm_setup(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6bb6c47..c102268 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1034,7 +1034,7 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>          * This is to ensure that CSR isn't identified as loaded before
>          * CSR-loading program is called during runtime-resume.
>          */
> -       intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> +       reinit_completion(&dev_priv->csr.is_loaded);
>
>         return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 43011d7..423afa9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -50,6 +50,7 @@
>  #include <linux/intel-iommu.h>
>  #include <linux/kref.h>
>  #include <linux/pm_qos.h>
> +#include <linux/completion.h>
>
>  /* General customization:
>   */
> @@ -669,23 +670,14 @@ struct intel_uncore {
>  #define for_each_fw_domain(domain__, dev_priv__, i__) \
>         for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
>
> -enum csr_state {
> -       FW_UNINITIALIZED = 0,
> -       FW_LOADED,
> -       FW_FAILED
> -};
> -
>  struct intel_csr {
> -       /* CSR protection, used to protect firmware loading status: csr_state */
> -       struct mutex csr_lock;
> -
> +       struct completion is_loaded;
>         const char *fw_path;
>         __be32 *dmc_payload;
>         uint32_t dmc_fw_size;
>         uint32_t mmio_count;
>         uint32_t mmioaddr[8];
>         uint32_t mmiodata[8];
> -       enum csr_state state;
>  };
>
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index d1da2db..fec2bc5 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -32,13 +32,6 @@
>   * onwards to drive newly added DMC (Display microcontroller) in display
>   * engine to save and restore the state of display engine when it enter into
>   * low-power state and comes back to normal.
> - *
> - * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
> - * FW_LOADED, FW_FAILED.
> - *
> - * Once the firmware is written into the registers status will be moved from
> - * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
> - * be moved to FW_FAILED.
>   */
>
>  #define I915_CSR_SKL "i915/skl_dmc_ver4.bin"
> @@ -200,40 +193,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.csr_lock);
> -       state = dev_priv->csr.state;
> -       mutex_unlock(&dev_priv->csr.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.csr_lock);
> -       dev_priv->csr.state = state;
> -       mutex_unlock(&dev_priv->csr.csr_lock);
> -}
> -
> -/**
>   * intel_csr_load_program() - write the firmware from memory to register.
>   * @dev: drm device.
>   *
> @@ -252,7 +211,6 @@ void intel_csr_load_program(struct drm_device *dev)
>                 return;
>         }
>
> -       mutex_lock(&dev_priv->csr.csr_lock);
>         fw_size = dev_priv->csr.dmc_fw_size;
>         for (i = 0; i < fw_size; i++)
>                 I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> @@ -262,9 +220,7 @@ 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.csr_lock);
> +       complete(&dev_priv->csr.is_loaded);
>  }
>
>  static void finish_csr_load(const struct firmware *fw, void *context)
> @@ -280,7 +236,6 @@ 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;
>         __be32 *dmc_payload;
> -       bool fw_loaded = false;
>
>         if (!fw) {
>                 i915_firmware_load_error_print(csr->fw_path, 0);
> @@ -387,14 +342,9 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>
>         /* load csr program during system boot, as needed for DC states */
>         intel_csr_load_program(dev);
> -       fw_loaded = true;
>
>  out:
> -       if (fw_loaded)
> -               intel_runtime_pm_put(dev_priv);
> -       else
> -               intel_csr_load_status_set(dev_priv, FW_FAILED);
> -
> +       intel_runtime_pm_put(dev_priv);
>         release_firmware(fw);
>  }
>
> @@ -418,7 +368,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
>                 csr->fw_path = I915_CSR_SKL;
>         else {
>                 DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
> -               intel_csr_load_status_set(dev_priv, FW_FAILED);
>                 return;
>         }
>
> @@ -428,15 +377,15 @@ void intel_csr_ucode_init(struct drm_device *dev)
>          */
>         intel_runtime_pm_get(dev_priv);
>
> +       init_completion(&csr->is_loaded);
> +
>         /* 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) {
> +       if (ret)
>                 i915_firmware_load_error_print(csr->fw_path, ret);
> -               intel_csr_load_status_set(dev_priv, FW_FAILED);
> -       }
>  }
>
>  /**
> @@ -453,13 +402,11 @@ 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);
>  }
>
>  void assert_csr_loaded(struct drm_i915_private *dev_priv)
>  {
> -       WARN((intel_csr_load_status_get(dev_priv) != FW_LOADED), "CSR is not loaded.\n");
>         WARN(!I915_READ(CSR_PROGRAM_BASE),
>                                 "CSR program storage start is NULL\n");
>         WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ea20edb..e022c98 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1155,9 +1155,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);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b393db7..5a830d3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -525,7 +525,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(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
>                 "DC6 already programmed to be disabled.\n");
>  }
> @@ -642,21 +641,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>
>                         if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>                                 power_well->data == SKL_DISP_PW_2) {
> -                               enum csr_state state;
> -                               /* TODO: wait for a completion event or
> -                                * similar here instead of busy
> -                                * waiting using wait_for function.
> -                                */
> -                               wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> -                                               FW_UNINITIALIZED, 1000);
> -                               if (state != FW_LOADED)
> -                                       DRM_ERROR("CSR firmware not ready (%d)\n",
> -                                                       state);
> -                               else
> +                               if (wait_for_completion_timeout(
> +                                       &dev_priv->csr.is_loaded, 100)) {
>                                         if (SKL_ENABLE_DC6(dev))
>                                                 skl_enable_dc6(dev_priv);
>                                         else
>                                                 gen9_enable_dc5(dev_priv);
> +                               } else
> +                                       DRM_ERROR("CSR firmware not ready\n");
>                         }
>                 }
>         }
> --
> 2.0.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Resign firmware loading for dmc

[Animesh Manna]

v1: Based on review comments from Daniel following changes are done.
- More focus is given for better synchronization.
- Replaced async firmware loading by using request_firmawre() call.
- Prevented entering in dc5/dc6 while firmware loading in process.
Now register programming for dc5/dc6 always will happen followed
by firmware loading.
- Removed the csr-lock and csr-state which was used before.
- Added a async work which is responsible for both loading the
firmware and register programming for dc5/dc6.
- Added flush_work() to explicitly synchronize the async work
during suspend and driver unloading.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         |   1 -
 drivers/gpu/drm/i915/i915_drv.c         |  10 +--
 drivers/gpu/drm/i915/i915_drv.h         |  13 ++-
 drivers/gpu/drm/i915/intel_csr.c        | 141 +++++++++++---------------------
 drivers/gpu/drm/i915/intel_drv.h        |   5 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c |  63 +++++++-------
 6 files changed, 92 insertions(+), 141 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..1ebf0e1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -820,7 +820,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->mmio_flip_lock);
 	mutex_init(&dev_priv->sb_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
-	mutex_init(&dev_priv->csr_lock);
 
 	intel_pm_setup(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e44dc0d..6049ce3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1019,11 +1019,8 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
-	/*
-	 * This is to ensure that CSR isn't identified as loaded before
-	 * CSR-loading program is called during runtime-resume.
-	 */
-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
+	dev_priv->csr.dmc_present = false;
+	flush_work(&dev_priv->csr.csr_work);
 
 	skl_uninit_cdclk(dev_priv);
 
@@ -1071,10 +1068,7 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
 
 static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
 	skl_init_cdclk(dev_priv);
-	intel_csr_load_program(dev);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1dbd957..73509f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -737,20 +737,16 @@ struct intel_uncore {
 #define for_each_fw_domain(domain__, dev_priv__, i__) \
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
-enum csr_state {
-	FW_UNINITIALIZED = 0,
-	FW_LOADED,
-	FW_FAILED
-};
-
 struct intel_csr {
+	bool dc_state_req;
+	struct work_struct csr_work;
 	const char *fw_path;
 	__be32 *dmc_payload;
 	uint32_t dmc_fw_size;
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
 	uint32_t mmiodata[8];
-	enum csr_state state;
+	bool dmc_present;
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
@@ -2631,6 +2627,9 @@ 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_csr.c */
+void intel_csr_setdc_work_fn(struct work_struct *__work);
+
 /* intel_hotplug.c */
 void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask);
 void intel_hpd_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 6d8a7bf..44f05a8 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -32,13 +32,6 @@
  * onwards to drive newly added DMC (Display microcontroller) in display
  * engine to save and restore the state of display engine when it enter into
  * low-power state and comes back to normal.
- *
- * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
- * FW_LOADED, FW_FAILED.
- *
- * Once the firmware is written into the registers status will be moved from
- * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
- * be moved to FW_FAILED.
  */
 
 #define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
@@ -199,48 +192,6 @@ static char intel_get_substepping(struct drm_device *dev)
 		return -ENODATA;
 }
 
-/**
- * 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.
- *
- * 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)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -252,7 +203,6 @@ void intel_csr_load_program(struct drm_device *dev)
 		return;
 	}
 
-	mutex_lock(&dev_priv->csr_lock);
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
@@ -262,14 +212,11 @@ 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)
+static void finish_csr_load(const struct firmware *fw,
+				struct drm_i915_private *dev_priv)
 {
-	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;
@@ -280,16 +227,15 @@ 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;
 	__be32 *dmc_payload;
-	bool fw_loaded = false;
 
 	if (!fw) {
 		i915_firmware_load_error_print(csr->fw_path, 0);
-		goto out;
+		return;
 	}
 
 	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
 		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
-		goto out;
+		return;
 	}
 
 	/* Extract CSS Header information*/
@@ -298,7 +244,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;
 	}
 	readcount += sizeof(struct intel_css_header);
 
@@ -309,7 +255,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;
 	}
 	readcount += sizeof(struct intel_package_header);
 
@@ -329,7 +275,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;
 	}
 	readcount += dmc_offset;
 
@@ -338,7 +284,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;
 	}
 	readcount += sizeof(struct intel_dmc_header);
 
@@ -346,7 +292,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;
 	}
 	csr->mmio_count = dmc_header->mmio_count;
 	for (i = 0; i < dmc_header->mmio_count; i++) {
@@ -354,7 +300,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;
 		}
 		csr->mmioaddr[i] = dmc_header->mmioaddr[i];
 		csr->mmiodata[i] = dmc_header->mmiodata[i];
@@ -364,14 +310,14 @@ 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;
 	}
 	csr->dmc_fw_size = dmc_header->fw_size;
 
 	csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
 	if (!csr->dmc_payload) {
 		DRM_ERROR("Memory allocation failed for dmc payload\n");
-		goto out;
+		return;
 	}
 
 	dmc_payload = csr->dmc_payload;
@@ -387,16 +333,42 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
-	fw_loaded = true;
+	dev_priv->csr.dmc_present = true;
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
-out:
-	if (fw_loaded)
-		intel_runtime_pm_put(dev_priv);
-	else
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
+}
 
-	release_firmware(fw);
+/**
+ * intel_display_load_csr() - write the firmware from memory to register.
+ * @dev: 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 to registers.
+ */
+
+void intel_display_load_csr(struct drm_i915_private *dev_priv)
+{
+	struct intel_csr *csr = &dev_priv->csr;
+	const struct firmware *fw;
+	int ret;
+
+	if (dev_priv->csr.dmc_present)
+		intel_csr_load_program(dev_priv->dev);
+	else {
+		/* CSR supported for platform, load firmware */
+		ret = request_firmware(&fw, csr->fw_path,
+				&dev_priv->dev->pdev->dev);
+
+		DRM_DEBUG_KMS("Loading %d\n", ret);
+
+		if (ret) {
+			i915_firmware_load_error_print(csr->fw_path, ret);
+			return;
+		}
+		finish_csr_load(fw, dev_priv);
+		release_firmware(fw);
+	}
 }
 
 /**
@@ -410,7 +382,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_csr *csr = &dev_priv->csr;
-	int ret;
 
 	if (!HAS_CSR(dev))
 		return;
@@ -419,27 +390,12 @@ void intel_csr_ucode_init(struct drm_device *dev)
 		csr->fw_path = I915_CSR_SKL;
 	else {
 		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
 		return;
 	}
 
 	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
 
-	/*
-	 * Obtain a runtime pm reference, until CSR is loaded,
-	 * to avoid entering runtime-suspend.
-	 */
-	intel_runtime_pm_get(dev_priv);
-
-	/* CSR supported for platform, load firmware */
-	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
-				&dev_priv->dev->pdev->dev,
-				GFP_KERNEL, dev_priv,
-				finish_csr_load);
-	if (ret) {
-		i915_firmware_load_error_print(csr->fw_path, ret);
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
-	}
+	INIT_WORK(&csr->csr_work, intel_csr_setdc_work_fn);
 }
 
 /**
@@ -456,14 +412,13 @@ void intel_csr_ucode_fini(struct drm_device *dev)
 	if (!HAS_CSR(dev))
 		return;
 
-	intel_csr_load_status_set(dev_priv, FW_FAILED);
+	flush_work(&dev_priv->csr.csr_work);
+
 	kfree(dev_priv->csr.dmc_payload);
 }
 
 void assert_csr_loaded(struct drm_i915_private *dev_priv)
 {
-	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
-	     "CSR is not loaded.\n");
 	WARN(!I915_READ(CSR_PROGRAM_BASE),
 				"CSR program storage start is NULL\n");
 	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3f0a890..b427407 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1163,10 +1163,7 @@ 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_display_load_csr(struct drm_i915_private *dev_priv);
 void intel_csr_ucode_fini(struct drm_device *dev);
 void assert_csr_loaded(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 1a45385..61c018d 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -527,7 +527,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(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
 		"DC6 already programmed to be disabled.\n");
 }
@@ -563,6 +562,37 @@ static void skl_disable_dc6(struct drm_i915_private *dev_priv)
 	POSTING_READ(DC_STATE_EN);
 }
 
+void intel_csr_setdc_work_fn(struct work_struct *__work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(__work, struct drm_i915_private, csr.csr_work);
+	struct intel_csr *csr = &dev_priv->csr;
+
+	if (csr->dc_state_req) {
+		intel_display_load_csr(dev_priv);
+
+		if (IS_SKYLAKE(dev_priv->dev))
+			skl_enable_dc6(dev_priv);
+		else
+			gen9_enable_dc5(dev_priv);
+	} else {
+		if (IS_SKYLAKE(dev_priv->dev)) {
+			skl_disable_dc6(dev_priv);
+			/*
+			 * DDI buffer programming unnecessary during
+			 * driver-load/resume as it's already done during
+			 * modeset initialization then. It's also invalid
+			 * here as encoder list is still uninitialized.
+			 */
+			if (!dev_priv->power_domains.initializing)
+				intel_prepare_ddi(dev_priv->dev);
+		} else {
+				gen9_disable_dc5(dev_priv);
+		}
+	}
+}
+
+
 static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			struct i915_power_well *power_well, bool enable)
 {
@@ -612,18 +642,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 				when request is to disable!\n");
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
-				if (SKL_ENABLE_DC6(dev)) {
-					skl_disable_dc6(dev_priv);
-					/*
-					 * DDI buffer programming unnecessary during driver-load/resume
-					 * as it's already done during modeset initialization then.
-					 * It's also invalid here as encoder list is still uninitialized.
-					 */
-					if (!dev_priv->power_domains.initializing)
-						intel_prepare_ddi(dev);
-				} else {
-					gen9_disable_dc5(dev_priv);
-				}
+				dev_priv->csr.dc_state_req = false;
+				schedule_work(&dev_priv->csr.csr_work);
 			}
 			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
 		}
@@ -644,21 +664,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
-				enum csr_state state;
-				/* TODO: wait for a completion event or
-				 * similar here instead of busy
-				 * waiting using wait_for function.
-				 */
-				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
-						FW_UNINITIALIZED, 1000);
-				if (state != FW_LOADED)
-					DRM_ERROR("CSR firmware not ready (%d)\n",
-							state);
-				else
-					if (SKL_ENABLE_DC6(dev))
-						skl_enable_dc6(dev_priv);
-					else
-						gen9_enable_dc5(dev_priv);
+				dev_priv->csr.dc_state_req = true;
+				schedule_work(&dev_priv->csr.csr_work);
 			}
 		}
 	}
-- 
2.0.2

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

Re: [PATCH] drm/i915: Resign firmware loading for dmc

[Daniel Vetter]

On Tue, Jul 07, 2015 at 06:10:12PM +0530, Animesh Manna wrote:
> v1: Based on review comments from Daniel following changes are done.
> - More focus is given for better synchronization.
> - Replaced async firmware loading by using request_firmawre() call.
> - Prevented entering in dc5/dc6 while firmware loading in process.
> Now register programming for dc5/dc6 always will happen followed
> by firmware loading.
> - Removed the csr-lock and csr-state which was used before.
> - Added a async work which is responsible for both loading the
> firmware and register programming for dc5/dc6.
> - Added flush_work() to explicitly synchronize the async work
> during suspend and driver unloading.

This should all be separate patches. Please split them up.

Thanks, Daniel

> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c         |   1 -
>  drivers/gpu/drm/i915/i915_drv.c         |  10 +--
>  drivers/gpu/drm/i915/i915_drv.h         |  13 ++-
>  drivers/gpu/drm/i915/intel_csr.c        | 141 +++++++++++---------------------
>  drivers/gpu/drm/i915/intel_drv.h        |   5 +-
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  63 +++++++-------
>  6 files changed, 92 insertions(+), 141 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index c5349fa..1ebf0e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -820,7 +820,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	spin_lock_init(&dev_priv->mmio_flip_lock);
>  	mutex_init(&dev_priv->sb_lock);
>  	mutex_init(&dev_priv->modeset_restore_lock);
> -	mutex_init(&dev_priv->csr_lock);
>  
>  	intel_pm_setup(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e44dc0d..6049ce3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1019,11 +1019,8 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  {
>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>  
> -	/*
> -	 * This is to ensure that CSR isn't identified as loaded before
> -	 * CSR-loading program is called during runtime-resume.
> -	 */
> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> +	dev_priv->csr.dmc_present = false;
> +	flush_work(&dev_priv->csr.csr_work);
>  
>  	skl_uninit_cdclk(dev_priv);
>  
> @@ -1071,10 +1068,7 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
>  
>  static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> -
>  	skl_init_cdclk(dev_priv);
> -	intel_csr_load_program(dev);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1dbd957..73509f7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -737,20 +737,16 @@ struct intel_uncore {
>  #define for_each_fw_domain(domain__, dev_priv__, i__) \
>  	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
>  
> -enum csr_state {
> -	FW_UNINITIALIZED = 0,
> -	FW_LOADED,
> -	FW_FAILED
> -};
> -
>  struct intel_csr {
> +	bool dc_state_req;
> +	struct work_struct csr_work;
>  	const char *fw_path;
>  	__be32 *dmc_payload;
>  	uint32_t dmc_fw_size;
>  	uint32_t mmio_count;
>  	uint32_t mmioaddr[8];
>  	uint32_t mmiodata[8];
> -	enum csr_state state;
> +	bool dmc_present;
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> @@ -2631,6 +2627,9 @@ 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_csr.c */
> +void intel_csr_setdc_work_fn(struct work_struct *__work);
> +
>  /* intel_hotplug.c */
>  void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask);
>  void intel_hpd_init(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 6d8a7bf..44f05a8 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -32,13 +32,6 @@
>   * onwards to drive newly added DMC (Display microcontroller) in display
>   * engine to save and restore the state of display engine when it enter into
>   * low-power state and comes back to normal.
> - *
> - * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
> - * FW_LOADED, FW_FAILED.
> - *
> - * Once the firmware is written into the registers status will be moved from
> - * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
> - * be moved to FW_FAILED.
>   */
>  
>  #define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
> @@ -199,48 +192,6 @@ static char intel_get_substepping(struct drm_device *dev)
>  		return -ENODATA;
>  }
>  
> -/**
> - * 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.
> - *
> - * 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)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -252,7 +203,6 @@ void intel_csr_load_program(struct drm_device *dev)
>  		return;
>  	}
>  
> -	mutex_lock(&dev_priv->csr_lock);
>  	fw_size = dev_priv->csr.dmc_fw_size;
>  	for (i = 0; i < fw_size; i++)
>  		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> @@ -262,14 +212,11 @@ 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)
> +static void finish_csr_load(const struct firmware *fw,
> +				struct drm_i915_private *dev_priv)
>  {
> -	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;
> @@ -280,16 +227,15 @@ 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;
>  	__be32 *dmc_payload;
> -	bool fw_loaded = false;
>  
>  	if (!fw) {
>  		i915_firmware_load_error_print(csr->fw_path, 0);
> -		goto out;
> +		return;
>  	}
>  
>  	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
>  		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
> -		goto out;
> +		return;
>  	}
>  
>  	/* Extract CSS Header information*/
> @@ -298,7 +244,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;
>  	}
>  	readcount += sizeof(struct intel_css_header);
>  
> @@ -309,7 +255,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;
>  	}
>  	readcount += sizeof(struct intel_package_header);
>  
> @@ -329,7 +275,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;
>  	}
>  	readcount += dmc_offset;
>  
> @@ -338,7 +284,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;
>  	}
>  	readcount += sizeof(struct intel_dmc_header);
>  
> @@ -346,7 +292,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;
>  	}
>  	csr->mmio_count = dmc_header->mmio_count;
>  	for (i = 0; i < dmc_header->mmio_count; i++) {
> @@ -354,7 +300,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;
>  		}
>  		csr->mmioaddr[i] = dmc_header->mmioaddr[i];
>  		csr->mmiodata[i] = dmc_header->mmiodata[i];
> @@ -364,14 +310,14 @@ 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;
>  	}
>  	csr->dmc_fw_size = dmc_header->fw_size;
>  
>  	csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
>  	if (!csr->dmc_payload) {
>  		DRM_ERROR("Memory allocation failed for dmc payload\n");
> -		goto out;
> +		return;
>  	}
>  
>  	dmc_payload = csr->dmc_payload;
> @@ -387,16 +333,42 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>  
>  	/* load csr program during system boot, as needed for DC states */
>  	intel_csr_load_program(dev);
> -	fw_loaded = true;
> +	dev_priv->csr.dmc_present = true;
>  
>  	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
> -out:
> -	if (fw_loaded)
> -		intel_runtime_pm_put(dev_priv);
> -	else
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
> +}
>  
> -	release_firmware(fw);
> +/**
> + * intel_display_load_csr() - write the firmware from memory to register.
> + * @dev: 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 to registers.
> + */
> +
> +void intel_display_load_csr(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_csr *csr = &dev_priv->csr;
> +	const struct firmware *fw;
> +	int ret;
> +
> +	if (dev_priv->csr.dmc_present)
> +		intel_csr_load_program(dev_priv->dev);
> +	else {
> +		/* CSR supported for platform, load firmware */
> +		ret = request_firmware(&fw, csr->fw_path,
> +				&dev_priv->dev->pdev->dev);
> +
> +		DRM_DEBUG_KMS("Loading %d\n", ret);
> +
> +		if (ret) {
> +			i915_firmware_load_error_print(csr->fw_path, ret);
> +			return;
> +		}
> +		finish_csr_load(fw, dev_priv);
> +		release_firmware(fw);
> +	}
>  }
>  
>  /**
> @@ -410,7 +382,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_csr *csr = &dev_priv->csr;
> -	int ret;
>  
>  	if (!HAS_CSR(dev))
>  		return;
> @@ -419,27 +390,12 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  		csr->fw_path = I915_CSR_SKL;
>  	else {
>  		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
>  		return;
>  	}
>  
>  	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
>  
> -	/*
> -	 * Obtain a runtime pm reference, until CSR is loaded,
> -	 * to avoid entering runtime-suspend.
> -	 */
> -	intel_runtime_pm_get(dev_priv);
> -
> -	/* CSR supported for platform, load firmware */
> -	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
> -				&dev_priv->dev->pdev->dev,
> -				GFP_KERNEL, dev_priv,
> -				finish_csr_load);
> -	if (ret) {
> -		i915_firmware_load_error_print(csr->fw_path, ret);
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
> -	}
> +	INIT_WORK(&csr->csr_work, intel_csr_setdc_work_fn);
>  }
>  
>  /**
> @@ -456,14 +412,13 @@ void intel_csr_ucode_fini(struct drm_device *dev)
>  	if (!HAS_CSR(dev))
>  		return;
>  
> -	intel_csr_load_status_set(dev_priv, FW_FAILED);
> +	flush_work(&dev_priv->csr.csr_work);
> +
>  	kfree(dev_priv->csr.dmc_payload);
>  }
>  
>  void assert_csr_loaded(struct drm_i915_private *dev_priv)
>  {
> -	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
> -	     "CSR is not loaded.\n");
>  	WARN(!I915_READ(CSR_PROGRAM_BASE),
>  				"CSR program storage start is NULL\n");
>  	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3f0a890..b427407 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1163,10 +1163,7 @@ 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_display_load_csr(struct drm_i915_private *dev_priv);
>  void intel_csr_ucode_fini(struct drm_device *dev);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 1a45385..61c018d 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -527,7 +527,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(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
>  		"DC6 already programmed to be disabled.\n");
>  }
> @@ -563,6 +562,37 @@ static void skl_disable_dc6(struct drm_i915_private *dev_priv)
>  	POSTING_READ(DC_STATE_EN);
>  }
>  
> +void intel_csr_setdc_work_fn(struct work_struct *__work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(__work, struct drm_i915_private, csr.csr_work);
> +	struct intel_csr *csr = &dev_priv->csr;
> +
> +	if (csr->dc_state_req) {
> +		intel_display_load_csr(dev_priv);
> +
> +		if (IS_SKYLAKE(dev_priv->dev))
> +			skl_enable_dc6(dev_priv);
> +		else
> +			gen9_enable_dc5(dev_priv);
> +	} else {
> +		if (IS_SKYLAKE(dev_priv->dev)) {
> +			skl_disable_dc6(dev_priv);
> +			/*
> +			 * DDI buffer programming unnecessary during
> +			 * driver-load/resume as it's already done during
> +			 * modeset initialization then. It's also invalid
> +			 * here as encoder list is still uninitialized.
> +			 */
> +			if (!dev_priv->power_domains.initializing)
> +				intel_prepare_ddi(dev_priv->dev);
> +		} else {
> +				gen9_disable_dc5(dev_priv);
> +		}
> +	}
> +}
> +
> +
>  static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  			struct i915_power_well *power_well, bool enable)
>  {
> @@ -612,18 +642,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  				when request is to disable!\n");
>  			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>  				power_well->data == SKL_DISP_PW_2) {
> -				if (SKL_ENABLE_DC6(dev)) {
> -					skl_disable_dc6(dev_priv);
> -					/*
> -					 * DDI buffer programming unnecessary during driver-load/resume
> -					 * as it's already done during modeset initialization then.
> -					 * It's also invalid here as encoder list is still uninitialized.
> -					 */
> -					if (!dev_priv->power_domains.initializing)
> -						intel_prepare_ddi(dev);
> -				} else {
> -					gen9_disable_dc5(dev_priv);
> -				}
> +				dev_priv->csr.dc_state_req = false;
> +				schedule_work(&dev_priv->csr.csr_work);
>  			}
>  			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>  		}
> @@ -644,21 +664,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  
>  			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>  				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* TODO: wait for a completion event or
> -				 * similar here instead of busy
> -				 * waiting using wait_for function.
> -				 */
> -				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> -						FW_UNINITIALIZED, 1000);
> -				if (state != FW_LOADED)
> -					DRM_ERROR("CSR firmware not ready (%d)\n",
> -							state);
> -				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +				dev_priv->csr.dc_state_req = true;
> +				schedule_work(&dev_priv->csr.csr_work);
>  			}
>  		}
>  	}
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Resign firmware loading for dmc

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6744
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -2              287/287              285/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 0/6] Redesign the dmc firmware loading.

[Animesh Manna]

v1: Based on review comments from Daniel following changes are done.
- More focus is given for better synchronization.
- Replaced async firmware loading by using request_firmawre() call.
- Prevented entering in dc5/dc6 while firmware loading in process.
Now register programming for dc5/dc6 always will happen followed
by firmware loading.
- Removed the csr-lock and csr-state which was used before.
- Added a async work which is responsible for both loading the
firmware and register programming for dc5/dc6.
- Added flush_work() to explicitly synchronize the async work
during suspend and driver unloading.
- Corrected the sanity check for mmio address of csr (Requested by Imre).
- Removed assert call of csr during disabling dc6 (Requested by Damien).


Animesh Manna (6):
  drm/i915/gen9: Removed csr-lock and csr-state
  drm/i915/gen9: Added a async work for fw-loading and dc5/dc6
    programming
  drm/i915/gen9: Replaced request_firmware_nowait() by
    request_firmware().
  drm/i915/gen9: Added dmc_present flag to check firmware loading
    status.
  drm/i915/skl: Removed assert for csr-fw-loading during disabling dc6.
  drm/i915/gen9: Corrected the sanity check of mmio address range for
    csr.

 drivers/gpu/drm/i915/i915_dma.c         |   1 -
 drivers/gpu/drm/i915/i915_drv.c         |  10 +--
 drivers/gpu/drm/i915/i915_drv.h         |  16 ++--
 drivers/gpu/drm/i915/intel_csr.c        | 144 +++++++++++---------------------
 drivers/gpu/drm/i915/intel_drv.h        |   5 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c |  62 +++++++-------
 6 files changed, 92 insertions(+), 146 deletions(-)

-- 
2.0.2

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

[PATCH 1/6] drm/i915/gen9: Removed csr-lock and csr-state

[Animesh Manna]

v1: As per review comments from Daniel, removed the csr-lock
and csr-state which was used before in dmc firmware loading.
Planning to have a single async task which will responsible
for firmware loading and register programming for dc5/dc6,
so removed csr-lock and csr-state from intel_csr structure.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c         |  1 -
 drivers/gpu/drm/i915/i915_drv.c         |  6 ----
 drivers/gpu/drm/i915/i915_drv.h         | 10 -------
 drivers/gpu/drm/i915/intel_csr.c        | 52 ---------------------------------
 drivers/gpu/drm/i915/intel_drv.h        |  3 --
 drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ++---------
 6 files changed, 3 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..1ebf0e1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -820,7 +820,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->mmio_flip_lock);
 	mutex_init(&dev_priv->sb_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
-	mutex_init(&dev_priv->csr_lock);
 
 	intel_pm_setup(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e44dc0d..4d8d2d8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1019,12 +1019,6 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
-	/*
-	 * This is to ensure that CSR isn't identified as loaded before
-	 * CSR-loading program is called during runtime-resume.
-	 */
-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
-
 	skl_uninit_cdclk(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1dbd957..b3a0fd6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -737,12 +737,6 @@ struct intel_uncore {
 #define for_each_fw_domain(domain__, dev_priv__, i__) \
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
-enum csr_state {
-	FW_UNINITIALIZED = 0,
-	FW_LOADED,
-	FW_FAILED
-};
-
 struct intel_csr {
 	const char *fw_path;
 	__be32 *dmc_payload;
@@ -750,7 +744,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) \
@@ -1689,9 +1682,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 6d8a7bf..62fd1b0 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -32,13 +32,6 @@
  * onwards to drive newly added DMC (Display microcontroller) in display
  * engine to save and restore the state of display engine when it enter into
  * low-power state and comes back to normal.
- *
- * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
- * FW_LOADED, FW_FAILED.
- *
- * Once the firmware is written into the registers status will be moved from
- * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
- * be moved to FW_FAILED.
  */
 
 #define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
@@ -200,40 +193,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.
  *
@@ -252,7 +211,6 @@ void intel_csr_load_program(struct drm_device *dev)
 		return;
 	}
 
-	mutex_lock(&dev_priv->csr_lock);
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
@@ -262,9 +220,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)
@@ -393,8 +348,6 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 out:
 	if (fw_loaded)
 		intel_runtime_pm_put(dev_priv);
-	else
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
 
 	release_firmware(fw);
 }
@@ -419,7 +372,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 		csr->fw_path = I915_CSR_SKL;
 	else {
 		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
 		return;
 	}
 
@@ -438,7 +390,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 				finish_csr_load);
 	if (ret) {
 		i915_firmware_load_error_print(csr->fw_path, ret);
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
 	}
 }
 
@@ -456,14 +407,11 @@ 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);
 }
 
 void assert_csr_loaded(struct drm_i915_private *dev_priv)
 {
-	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
-	     "CSR is not loaded.\n");
 	WARN(!I915_READ(CSR_PROGRAM_BASE),
 				"CSR program storage start is NULL\n");
 	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3f0a890..f437a90 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1163,9 +1163,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);
 void assert_csr_loaded(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 1a45385..0b81d6f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -644,21 +644,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
-				enum csr_state state;
-				/* TODO: wait for a completion event or
-				 * similar here instead of busy
-				 * waiting using wait_for function.
-				 */
-				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
-						FW_UNINITIALIZED, 1000);
-				if (state != FW_LOADED)
-					DRM_ERROR("CSR firmware not ready (%d)\n",
-							state);
+				if (SKL_ENABLE_DC6(dev))
+					skl_enable_dc6(dev_priv);
 				else
-					if (SKL_ENABLE_DC6(dev))
-						skl_enable_dc6(dev_priv);
-					else
-						gen9_enable_dc5(dev_priv);
+					gen9_enable_dc5(dev_priv);
 			}
 		}
 	}
-- 
2.0.2

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

[PATCH 2/6] drm/i915/gen9: Added a async work for fw-loading and dc5/dc6 programming

[Animesh Manna]

v1: As per review comments from Daniel, prevented entering in
dc5/dc6 while firmware loading in process. Now register programming
for dc5/dc6 always will happen followed by firmware loading. Added
a async work which is responsible for both loading the firmware
and register programming for dc5/dc6.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  1 +
 drivers/gpu/drm/i915/i915_drv.h         |  5 ++++
 drivers/gpu/drm/i915/intel_csr.c        |  3 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 51 ++++++++++++++++++++++-----------
 4 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4d8d2d8..217efcb 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1019,6 +1019,7 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
+	flush_work(&dev_priv->csr.csr_work);
 	skl_uninit_cdclk(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b3a0fd6..5ae45bd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -738,6 +738,8 @@ struct intel_uncore {
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
 struct intel_csr {
+	bool dc_state_req;
+	struct work_struct csr_work;
 	const char *fw_path;
 	__be32 *dmc_payload;
 	uint32_t dmc_fw_size;
@@ -2621,6 +2623,9 @@ 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_csr.c */
+void intel_csr_setdc_work_fn(struct work_struct *__work);
+
 /* intel_hotplug.c */
 void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask);
 void intel_hpd_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 62fd1b0..f3b4a17 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -391,6 +391,7 @@ void intel_csr_ucode_init(struct drm_device *dev)
 	if (ret) {
 		i915_firmware_load_error_print(csr->fw_path, ret);
 	}
+	INIT_WORK(&csr->csr_work, intel_csr_setdc_work_fn);
 }
 
 /**
@@ -407,6 +408,8 @@ void intel_csr_ucode_fini(struct drm_device *dev)
 	if (!HAS_CSR(dev))
 		return;
 
+	flush_work(&dev_priv->csr.csr_work);
+
 	kfree(dev_priv->csr.dmc_payload);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 0b81d6f..f261558 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -563,6 +563,37 @@ static void skl_disable_dc6(struct drm_i915_private *dev_priv)
 	POSTING_READ(DC_STATE_EN);
 }
 
+void intel_csr_setdc_work_fn(struct work_struct *__work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(__work, struct drm_i915_private, csr.csr_work);
+	struct intel_csr *csr = &dev_priv->csr;
+
+	if (csr->dc_state_req) {
+
+		/* TODO: Load the dmc firmware. */
+
+		if (IS_SKYLAKE(dev_priv->dev))
+			skl_enable_dc6(dev_priv);
+		else
+			gen9_enable_dc5(dev_priv);
+	} else {
+		if (IS_SKYLAKE(dev_priv->dev)) {
+			skl_disable_dc6(dev_priv);
+			/*
+			 * DDI buffer programming unnecessary during
+			 * driver-load/resume as it's already done during
+			 * modeset initialization then. It's also invalid
+			 * here as encoder list is still uninitialized.
+			 */
+			if (!dev_priv->power_domains.initializing)
+				intel_prepare_ddi(dev_priv->dev);
+		} else {
+				gen9_disable_dc5(dev_priv);
+		}
+	}
+}
+
 static void skl_set_power_well(struct drm_i915_private *dev_priv,
 			struct i915_power_well *power_well, bool enable)
 {
@@ -612,18 +643,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 				when request is to disable!\n");
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
-				if (SKL_ENABLE_DC6(dev)) {
-					skl_disable_dc6(dev_priv);
-					/*
-					 * DDI buffer programming unnecessary during driver-load/resume
-					 * as it's already done during modeset initialization then.
-					 * It's also invalid here as encoder list is still uninitialized.
-					 */
-					if (!dev_priv->power_domains.initializing)
-						intel_prepare_ddi(dev);
-				} else {
-					gen9_disable_dc5(dev_priv);
-				}
+				dev_priv->csr.dc_state_req = false;
+				schedule_work(&dev_priv->csr.csr_work);
 			}
 			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
 		}
@@ -644,10 +665,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
-				if (SKL_ENABLE_DC6(dev))
-					skl_enable_dc6(dev_priv);
-				else
-					gen9_enable_dc5(dev_priv);
+				dev_priv->csr.dc_state_req = true;
+				schedule_work(&dev_priv->csr.csr_work);
 			}
 		}
 	}
-- 
2.0.2

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

[PATCH 3/6] drm/i915/gen9: Replaced request_firmware_nowait() by request_firmware().

[Animesh Manna]

v1: As per review comments from Daniel, replaced async firmware
loading with request_firmware() which will load the dmc firmware and
once firmware is loaded, dc5/dc6 register programming can be done
in the same thread.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  3 --
 drivers/gpu/drm/i915/intel_csr.c        | 79 ++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h        |  2 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +-
 4 files changed, 41 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 217efcb..18aaaf6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1066,10 +1066,7 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
 
 static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
 	skl_init_cdclk(dev_priv);
-	intel_csr_load_program(dev);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index f3b4a17..8e9395f 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -192,14 +192,6 @@ static char intel_get_substepping(struct drm_device *dev)
 		return -ENODATA;
 }
 
-/**
- * intel_csr_load_program() - write the firmware from memory to register.
- * @dev: 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)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -222,9 +214,9 @@ void intel_csr_load_program(struct drm_device *dev)
 	}
 }
 
-static void finish_csr_load(const struct firmware *fw, void *context)
+static void finish_csr_load(const struct firmware *fw,
+				struct drm_i915_private *dev_priv)
 {
-	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;
@@ -235,16 +227,15 @@ 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;
 	__be32 *dmc_payload;
-	bool fw_loaded = false;
 
 	if (!fw) {
 		i915_firmware_load_error_print(csr->fw_path, 0);
-		goto out;
+		return;
 	}
 
 	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
 		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
-		goto out;
+		return;
 	}
 
 	/* Extract CSS Header information*/
@@ -253,7 +244,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;
 	}
 	readcount += sizeof(struct intel_css_header);
 
@@ -264,7 +255,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;
 	}
 	readcount += sizeof(struct intel_package_header);
 
@@ -284,7 +275,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;
 	}
 	readcount += dmc_offset;
 
@@ -293,7 +284,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;
 	}
 	readcount += sizeof(struct intel_dmc_header);
 
@@ -301,7 +292,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;
 	}
 	csr->mmio_count = dmc_header->mmio_count;
 	for (i = 0; i < dmc_header->mmio_count; i++) {
@@ -309,7 +300,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;
 		}
 		csr->mmioaddr[i] = dmc_header->mmioaddr[i];
 		csr->mmiodata[i] = dmc_header->mmiodata[i];
@@ -319,14 +310,14 @@ 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;
 	}
 	csr->dmc_fw_size = dmc_header->fw_size;
 
 	csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
 	if (!csr->dmc_payload) {
 		DRM_ERROR("Memory allocation failed for dmc payload\n");
-		goto out;
+		return;
 	}
 
 	dmc_payload = csr->dmc_payload;
@@ -342,13 +333,36 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
-	fw_loaded = true;
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
-out:
-	if (fw_loaded)
-		intel_runtime_pm_put(dev_priv);
+}
 
+/**
+ * intel_display_load_csr() - write the firmware from memory to register.
+ * @dev: 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 to registers.
+ */
+
+void intel_display_load_csr(struct drm_i915_private *dev_priv)
+{
+	struct intel_csr *csr = &dev_priv->csr;
+	const struct firmware *fw;
+	int ret;
+
+	/* CSR supported for platform, load firmware */
+	ret = request_firmware(&fw, csr->fw_path,
+			&dev_priv->dev->pdev->dev);
+
+	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
+
+	if (ret) {
+		i915_firmware_load_error_print(csr->fw_path, ret);
+		return;
+	}
+	finish_csr_load(fw, dev_priv);
 	release_firmware(fw);
 }
 
@@ -363,7 +377,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_csr *csr = &dev_priv->csr;
-	int ret;
 
 	if (!HAS_CSR(dev))
 		return;
@@ -377,20 +390,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 
 	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
 
-	/*
-	 * Obtain a runtime pm reference, until CSR is loaded,
-	 * to avoid entering runtime-suspend.
-	 */
-	intel_runtime_pm_get(dev_priv);
-
-	/* CSR supported for platform, load firmware */
-	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
-				&dev_priv->dev->pdev->dev,
-				GFP_KERNEL, dev_priv,
-				finish_csr_load);
-	if (ret) {
-		i915_firmware_load_error_print(csr->fw_path, ret);
-	}
 	INIT_WORK(&csr->csr_work, intel_csr_setdc_work_fn);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f437a90..b427407 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1163,7 +1163,7 @@ 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_display_load_csr(struct drm_i915_private *dev_priv);
 void intel_csr_ucode_fini(struct drm_device *dev);
 void assert_csr_loaded(struct drm_i915_private *dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index f261558..01e046e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -570,8 +570,7 @@ void intel_csr_setdc_work_fn(struct work_struct *__work)
 	struct intel_csr *csr = &dev_priv->csr;
 
 	if (csr->dc_state_req) {
-
-		/* TODO: Load the dmc firmware. */
+		intel_display_load_csr(dev_priv);
 
 		if (IS_SKYLAKE(dev_priv->dev))
 			skl_enable_dc6(dev_priv);
-- 
2.0.2

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

[PATCH 4/6] drm/i915/gen9: Added dmc_present flag to check firmware loading status.

[Animesh Manna]

Firmware loading can be optimized by setting the dmc_present flag
for the first time and later internallly stored firmware data
can be used.

Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_csr.c | 24 +++++++++++++++---------
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5ae45bd..4870666 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -746,6 +746,7 @@ struct intel_csr {
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
 	uint32_t mmiodata[8];
+	bool dmc_present;
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 8e9395f..d600640 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -333,6 +333,7 @@ static void finish_csr_load(const struct firmware *fw,
 
 	/* load csr program during system boot, as needed for DC states */
 	intel_csr_load_program(dev);
+	dev_priv->csr.dmc_present = true;
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
 }
@@ -352,18 +353,22 @@ void intel_display_load_csr(struct drm_i915_private *dev_priv)
 	const struct firmware *fw;
 	int ret;
 
-	/* CSR supported for platform, load firmware */
-	ret = request_firmware(&fw, csr->fw_path,
-			&dev_priv->dev->pdev->dev);
+	if (dev_priv->csr.dmc_present)
+		intel_csr_load_program(dev_priv->dev);
+	else {
+		/* CSR supported for platform, load firmware */
+		ret = request_firmware(&fw, csr->fw_path,
+				&dev_priv->dev->pdev->dev);
 
-	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
+		DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
 
-	if (ret) {
-		i915_firmware_load_error_print(csr->fw_path, ret);
-		return;
+		if (ret) {
+			i915_firmware_load_error_print(csr->fw_path, ret);
+			return;
+		}
+		finish_csr_load(fw, dev_priv);
+		release_firmware(fw);
 	}
-	finish_csr_load(fw, dev_priv);
-	release_firmware(fw);
 }
 
 /**
@@ -408,6 +413,7 @@ void intel_csr_ucode_fini(struct drm_device *dev)
 		return;
 
 	flush_work(&dev_priv->csr.csr_work);
+	dev_priv->csr.dmc_present = false;
 
 	kfree(dev_priv->csr.dmc_payload);
 }
-- 
2.0.2

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

[PATCH 5/6] drm/i915/skl: Removed assert for csr-fw-loading during disabling dc6.

[Animesh Manna]

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

Signed-off-by: Animesh Manna <animesh.manna@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 01e046e..ba29bfc 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -527,7 +527,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(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
 		"DC6 already programmed to be disabled.\n");
 }
-- 
2.0.2

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

[PATCH 6/6] drm/i915/gen9: Corrected the sanity check of mmio address range for csr.

[Animesh Manna]

Signed-off-by: Animesh Manna <animesh.manna@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 d600640..f515d54 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -296,7 +296,7 @@ static void finish_csr_load(const struct firmware *fw,
 	}
 	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 &&
+		if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE ||
 			dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {
 			DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
 						dmc_header->mmioaddr[i]);
-- 
2.0.2

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

Re: [PATCH 6/6] drm/i915/gen9: Corrected the sanity check of mmio address range for csr.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6752
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -2              287/287              285/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/gen9: Corrected the sanity check of mmio address range for csr.

[Daniel Vetter]

On Wed, Jul 08, 2015 at 07:54:47PM +0530, Animesh Manna wrote:
> Signed-off-by: Animesh Manna <animesh.manna@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 d600640..f515d54 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -296,7 +296,7 @@ static void finish_csr_load(const struct firmware *fw,
>  	}
>  	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 &&
> +		if (dmc_header->mmioaddr[i] < CSR_MMIO_START_RANGE ||
>  			dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {

Please also fix the alignement of the 2nd line, it should align with the
opening ( like this
	if (aa &&
	    bb) {
	    /* more code */
	}

Cheers, Daniel
>  			DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
>  						dmc_header->mmioaddr[i]);
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915/gen9: Removed csr-lock and csr-state

[Daniel Vetter]

On Wed, Jul 08, 2015 at 07:54:42PM +0530, Animesh Manna wrote:
> v1: As per review comments from Daniel, removed the csr-lock
> and csr-state which was used before in dmc firmware loading.
> Planning to have a single async task which will responsible
> for firmware loading and register programming for dc5/dc6,
> so removed csr-lock and csr-state from intel_csr structure.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>

This patch split-up breaks correctness of the code since here you remove
the existing dmc loading coordination code before the new code is in
place. This should be done the other way round when splitting patches,
i.e. first add the new code (or replace parts of the existing code as you
go), then remove the old one.

Patches for existing features should never degrade functionality.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_dma.c         |  1 -
>  drivers/gpu/drm/i915/i915_drv.c         |  6 ----
>  drivers/gpu/drm/i915/i915_drv.h         | 10 -------
>  drivers/gpu/drm/i915/intel_csr.c        | 52 ---------------------------------
>  drivers/gpu/drm/i915/intel_drv.h        |  3 --
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ++---------
>  6 files changed, 3 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index c5349fa..1ebf0e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -820,7 +820,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	spin_lock_init(&dev_priv->mmio_flip_lock);
>  	mutex_init(&dev_priv->sb_lock);
>  	mutex_init(&dev_priv->modeset_restore_lock);
> -	mutex_init(&dev_priv->csr_lock);
>  
>  	intel_pm_setup(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e44dc0d..4d8d2d8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1019,12 +1019,6 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
>  {
>  	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
>  
> -	/*
> -	 * This is to ensure that CSR isn't identified as loaded before
> -	 * CSR-loading program is called during runtime-resume.
> -	 */
> -	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> -
>  	skl_uninit_cdclk(dev_priv);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1dbd957..b3a0fd6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -737,12 +737,6 @@ struct intel_uncore {
>  #define for_each_fw_domain(domain__, dev_priv__, i__) \
>  	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
>  
> -enum csr_state {
> -	FW_UNINITIALIZED = 0,
> -	FW_LOADED,
> -	FW_FAILED
> -};
> -
>  struct intel_csr {
>  	const char *fw_path;
>  	__be32 *dmc_payload;
> @@ -750,7 +744,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) \
> @@ -1689,9 +1682,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 6d8a7bf..62fd1b0 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -32,13 +32,6 @@
>   * onwards to drive newly added DMC (Display microcontroller) in display
>   * engine to save and restore the state of display engine when it enter into
>   * low-power state and comes back to normal.
> - *
> - * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
> - * FW_LOADED, FW_FAILED.
> - *
> - * Once the firmware is written into the registers status will be moved from
> - * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
> - * be moved to FW_FAILED.
>   */
>  
>  #define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
> @@ -200,40 +193,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.
>   *
> @@ -252,7 +211,6 @@ void intel_csr_load_program(struct drm_device *dev)
>  		return;
>  	}
>  
> -	mutex_lock(&dev_priv->csr_lock);
>  	fw_size = dev_priv->csr.dmc_fw_size;
>  	for (i = 0; i < fw_size; i++)
>  		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> @@ -262,9 +220,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)
> @@ -393,8 +348,6 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>  out:
>  	if (fw_loaded)
>  		intel_runtime_pm_put(dev_priv);
> -	else
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
>  
>  	release_firmware(fw);
>  }
> @@ -419,7 +372,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  		csr->fw_path = I915_CSR_SKL;
>  	else {
>  		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
>  		return;
>  	}
>  
> @@ -438,7 +390,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  				finish_csr_load);
>  	if (ret) {
>  		i915_firmware_load_error_print(csr->fw_path, ret);
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
>  	}
>  }
>  
> @@ -456,14 +407,11 @@ 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);
>  }
>  
>  void assert_csr_loaded(struct drm_i915_private *dev_priv)
>  {
> -	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
> -	     "CSR is not loaded.\n");
>  	WARN(!I915_READ(CSR_PROGRAM_BASE),
>  				"CSR program storage start is NULL\n");
>  	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3f0a890..f437a90 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1163,9 +1163,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);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 1a45385..0b81d6f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -644,21 +644,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  
>  			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>  				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* TODO: wait for a completion event or
> -				 * similar here instead of busy
> -				 * waiting using wait_for function.
> -				 */
> -				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> -						FW_UNINITIALIZED, 1000);
> -				if (state != FW_LOADED)
> -					DRM_ERROR("CSR firmware not ready (%d)\n",
> -							state);
> +				if (SKL_ENABLE_DC6(dev))
> +					skl_enable_dc6(dev_priv);
>  				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +					gen9_enable_dc5(dev_priv);
>  			}
>  		}
>  	}
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915/gen9: Added dmc_present flag to check firmware loading status.

[Daniel Vetter]

On Wed, Jul 08, 2015 at 07:54:45PM +0530, Animesh Manna wrote:
> Firmware loading can be optimized by setting the dmc_present flag
> for the first time and later internallly stored firmware data
> can be used.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_csr.c | 24 +++++++++++++++---------
>  2 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5ae45bd..4870666 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -746,6 +746,7 @@ struct intel_csr {
>  	uint32_t mmio_count;
>  	uint32_t mmioaddr[8];
>  	uint32_t mmiodata[8];
> +	bool dmc_present;

You can instead look at dev_priv->csr.dmc_payload, no need to add an
additional variable.
-Daniel

>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 8e9395f..d600640 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -333,6 +333,7 @@ static void finish_csr_load(const struct firmware *fw,
>  
>  	/* load csr program during system boot, as needed for DC states */
>  	intel_csr_load_program(dev);
> +	dev_priv->csr.dmc_present = true;
>  
>  	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
>  }
> @@ -352,18 +353,22 @@ void intel_display_load_csr(struct drm_i915_private *dev_priv)
>  	const struct firmware *fw;
>  	int ret;
>  
> -	/* CSR supported for platform, load firmware */
> -	ret = request_firmware(&fw, csr->fw_path,
> -			&dev_priv->dev->pdev->dev);
> +	if (dev_priv->csr.dmc_present)
> +		intel_csr_load_program(dev_priv->dev);
> +	else {
> +		/* CSR supported for platform, load firmware */
> +		ret = request_firmware(&fw, csr->fw_path,
> +				&dev_priv->dev->pdev->dev);
>  
> -	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
> +		DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
>  
> -	if (ret) {
> -		i915_firmware_load_error_print(csr->fw_path, ret);
> -		return;
> +		if (ret) {
> +			i915_firmware_load_error_print(csr->fw_path, ret);
> +			return;
> +		}
> +		finish_csr_load(fw, dev_priv);
> +		release_firmware(fw);
>  	}
> -	finish_csr_load(fw, dev_priv);
> -	release_firmware(fw);
>  }
>  
>  /**
> @@ -408,6 +413,7 @@ void intel_csr_ucode_fini(struct drm_device *dev)
>  		return;
>  
>  	flush_work(&dev_priv->csr.csr_work);
> +	dev_priv->csr.dmc_present = false;
>  
>  	kfree(dev_priv->csr.dmc_payload);
>  }
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915/gen9: Replaced request_firmware_nowait() by request_firmware().

[Daniel Vetter]

On Wed, Jul 08, 2015 at 07:54:44PM +0530, Animesh Manna wrote:
> v1: As per review comments from Daniel, replaced async firmware
> loading with request_firmware() which will load the dmc firmware and
> once firmware is loaded, dc5/dc6 register programming can be done
> in the same thread.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>

Ok this isn't quite what I had in mind for the new firmware loader flow.
I'm working on a demonstration patch series with lots of detailed comments
in the commit message to explain my idea in detail. It won't be tested
(since I don't have a skl here) but should serve as a guideline for this
refactoring.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  3 --
>  drivers/gpu/drm/i915/intel_csr.c        | 79 ++++++++++++++++-----------------
>  drivers/gpu/drm/i915/intel_drv.h        |  2 +-
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +-
>  4 files changed, 41 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 217efcb..18aaaf6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1066,10 +1066,7 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
>  
>  static int skl_resume_prepare(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_device *dev = dev_priv->dev;
> -
>  	skl_init_cdclk(dev_priv);
> -	intel_csr_load_program(dev);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index f3b4a17..8e9395f 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -192,14 +192,6 @@ static char intel_get_substepping(struct drm_device *dev)
>  		return -ENODATA;
>  }
>  
> -/**
> - * intel_csr_load_program() - write the firmware from memory to register.
> - * @dev: 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)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -222,9 +214,9 @@ void intel_csr_load_program(struct drm_device *dev)
>  	}
>  }
>  
> -static void finish_csr_load(const struct firmware *fw, void *context)
> +static void finish_csr_load(const struct firmware *fw,
> +				struct drm_i915_private *dev_priv)
>  {
> -	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;
> @@ -235,16 +227,15 @@ 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;
>  	__be32 *dmc_payload;
> -	bool fw_loaded = false;
>  
>  	if (!fw) {
>  		i915_firmware_load_error_print(csr->fw_path, 0);
> -		goto out;
> +		return;
>  	}
>  
>  	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
>  		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
> -		goto out;
> +		return;
>  	}
>  
>  	/* Extract CSS Header information*/
> @@ -253,7 +244,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;
>  	}
>  	readcount += sizeof(struct intel_css_header);
>  
> @@ -264,7 +255,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;
>  	}
>  	readcount += sizeof(struct intel_package_header);
>  
> @@ -284,7 +275,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;
>  	}
>  	readcount += dmc_offset;
>  
> @@ -293,7 +284,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;
>  	}
>  	readcount += sizeof(struct intel_dmc_header);
>  
> @@ -301,7 +292,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;
>  	}
>  	csr->mmio_count = dmc_header->mmio_count;
>  	for (i = 0; i < dmc_header->mmio_count; i++) {
> @@ -309,7 +300,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;
>  		}
>  		csr->mmioaddr[i] = dmc_header->mmioaddr[i];
>  		csr->mmiodata[i] = dmc_header->mmiodata[i];
> @@ -319,14 +310,14 @@ 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;
>  	}
>  	csr->dmc_fw_size = dmc_header->fw_size;
>  
>  	csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
>  	if (!csr->dmc_payload) {
>  		DRM_ERROR("Memory allocation failed for dmc payload\n");
> -		goto out;
> +		return;
>  	}
>  
>  	dmc_payload = csr->dmc_payload;
> @@ -342,13 +333,36 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>  
>  	/* load csr program during system boot, as needed for DC states */
>  	intel_csr_load_program(dev);
> -	fw_loaded = true;
>  
>  	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
> -out:
> -	if (fw_loaded)
> -		intel_runtime_pm_put(dev_priv);
> +}
>  
> +/**
> + * intel_display_load_csr() - write the firmware from memory to register.
> + * @dev: 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 to registers.
> + */
> +
> +void intel_display_load_csr(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_csr *csr = &dev_priv->csr;
> +	const struct firmware *fw;
> +	int ret;
> +
> +	/* CSR supported for platform, load firmware */
> +	ret = request_firmware(&fw, csr->fw_path,
> +			&dev_priv->dev->pdev->dev);
> +
> +	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
> +
> +	if (ret) {
> +		i915_firmware_load_error_print(csr->fw_path, ret);
> +		return;
> +	}
> +	finish_csr_load(fw, dev_priv);
>  	release_firmware(fw);
>  }
>  
> @@ -363,7 +377,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_csr *csr = &dev_priv->csr;
> -	int ret;
>  
>  	if (!HAS_CSR(dev))
>  		return;
> @@ -377,20 +390,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  
>  	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
>  
> -	/*
> -	 * Obtain a runtime pm reference, until CSR is loaded,
> -	 * to avoid entering runtime-suspend.
> -	 */
> -	intel_runtime_pm_get(dev_priv);
> -
> -	/* CSR supported for platform, load firmware */
> -	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
> -				&dev_priv->dev->pdev->dev,
> -				GFP_KERNEL, dev_priv,
> -				finish_csr_load);
> -	if (ret) {
> -		i915_firmware_load_error_print(csr->fw_path, ret);
> -	}
>  	INIT_WORK(&csr->csr_work, intel_csr_setdc_work_fn);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f437a90..b427407 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1163,7 +1163,7 @@ 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_display_load_csr(struct drm_i915_private *dev_priv);
>  void intel_csr_ucode_fini(struct drm_device *dev);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index f261558..01e046e 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -570,8 +570,7 @@ void intel_csr_setdc_work_fn(struct work_struct *__work)
>  	struct intel_csr *csr = &dev_priv->csr;
>  
>  	if (csr->dc_state_req) {
> -
> -		/* TODO: Load the dmc firmware. */
> +		intel_display_load_csr(dev_priv);
>  
>  		if (IS_SKYLAKE(dev_priv->dev))
>  			skl_enable_dc6(dev_priv);
> -- 
> 2.0.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/12] drm/i915: use correct power domain for csr loading

[Daniel Vetter]

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: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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 6d8a7bf06dfc..16cd9dae1c1b 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
 out:
 	if (fw_loaded)
-		intel_runtime_pm_put(dev_priv);
+		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 	else
 		intel_csr_load_status_set(dev_priv, FW_FAILED);
 
@@ -429,7 +429,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 02/12] drm/i915: Only allow rpm on gen9+ with dmc loaded

[Daniel Vetter]

Instead of trying to deal with this complexity we'll simply require
that the dmc firmware is available for runtime pm support. We do that
by not releasing the rpm reference we acquire when starting the
firmware loader work. Note that since we hold a rpm reference (and rpm
get/put is synchronized with its own locking already) there's no need
for any additional synchronization between the dmc loader and the rpm
entry/exit code.

Hence we can remove all dmc_load_status_get calls, they don't do
anything any more.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c        |  9 +++++----
 drivers/gpu/drm/i915/intel_runtime_pm.c | 17 +++--------------
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 16cd9dae1c1b..03d83892cdb0 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -393,8 +393,11 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 out:
 	if (fw_loaded)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
-	else
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
+
+	/*
+	 * We require the dmc firmware for runtime pm on gen9+ - leak the rpm
+	 * reference in case this failed to disable rpm on.
+	 */
 
 	release_firmware(fw);
 }
@@ -462,8 +465,6 @@ void intel_csr_ucode_fini(struct drm_device *dev)
 
 void assert_csr_loaded(struct drm_i915_private *dev_priv)
 {
-	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
-	     "CSR is not loaded.\n");
 	WARN(!I915_READ(CSR_PROGRAM_BASE),
 				"CSR program storage start is NULL\n");
 	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 2628b21ff2c0..ed8c0cee738f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -644,21 +644,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
 
 			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
 				power_well->data == SKL_DISP_PW_2) {
-				enum csr_state state;
-				/* TODO: wait for a completion event or
-				 * similar here instead of busy
-				 * waiting using wait_for function.
-				 */
-				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
-						FW_UNINITIALIZED, 1000);
-				if (state != FW_LOADED)
-					DRM_ERROR("CSR firmware not ready (%d)\n",
-							state);
+				if (SKL_ENABLE_DC6(dev))
+					skl_enable_dc6(dev_priv);
 				else
-					if (SKL_ENABLE_DC6(dev))
-						skl_enable_dc6(dev_priv);
-					else
-						gen9_enable_dc5(dev_priv);
+					gen9_enable_dc5(dev_priv);
 			}
 		}
 	}
-- 
2.1.4

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

[PATCH 03/12] drm/i915: move assert_csr_loaded into intel_rpm.c

[Daniel Vetter]

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

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h         | 16 ++++++++++++++++
 drivers/gpu/drm/i915/intel_csr.c        | 24 ------------------------
 drivers/gpu/drm/i915/intel_drv.h        |  1 -
 drivers/gpu/drm/i915/intel_runtime_pm.c |  8 ++++++++
 4 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2a29bccb63ef..46af8c784d29 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7361,6 +7361,22 @@ enum skl_disp_power_wells {
 #define  DC_STATE_DEBUG                  0x45520
 #define  DC_STATE_DEBUG_MASK_MEMORY_UP	(1<<1)
 
+/*
+* SKL CSR registers for DC5 and DC6
+*/
+#define CSR_PROGRAM_BASE		0x80000
+#define CSR_SSP_BASE_ADDR_GEN9		0x00002FC0
+#define CSR_HTP_ADDR_SKL		0x00500034
+#define CSR_SSP_BASE			0x8F074
+#define CSR_HTP_SKL			0x8F004
+#define CSR_LAST_WRITE			0x8F034
+#define CSR_LAST_WRITE_VALUE		0xc003b400
+/* MMIO address range for CSR program (0x80000 - 0x82FFF) */
+#define CSR_MAX_FW_SIZE			0x2FFF
+#define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
+#define CSR_MMIO_START_RANGE	0x80000
+#define CSR_MMIO_END_RANGE		0x8FFFF
+
 /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
  * since on HSW we can't write to it using I915_WRITE. */
 #define D_COMP_HSW			(MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 03d83892cdb0..6362653d19af 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -45,22 +45,6 @@
 
 MODULE_FIRMWARE(I915_CSR_SKL);
 
-/*
-* SKL CSR registers for DC5 and DC6
-*/
-#define CSR_PROGRAM_BASE		0x80000
-#define CSR_SSP_BASE_ADDR_GEN9		0x00002FC0
-#define CSR_HTP_ADDR_SKL		0x00500034
-#define CSR_SSP_BASE			0x8F074
-#define CSR_HTP_SKL			0x8F004
-#define CSR_LAST_WRITE			0x8F034
-#define CSR_LAST_WRITE_VALUE		0xc003b400
-/* MMIO address range for CSR program (0x80000 - 0x82FFF) */
-#define CSR_MAX_FW_SIZE			0x2FFF
-#define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
-#define CSR_MMIO_START_RANGE	0x80000
-#define CSR_MMIO_END_RANGE		0x8FFFF
-
 struct intel_css_header {
 	/* 0x09 for DMC */
 	uint32_t module_type;
@@ -462,11 +446,3 @@ void intel_csr_ucode_fini(struct drm_device *dev)
 	intel_csr_load_status_set(dev_priv, FW_FAILED);
 	kfree(dev_priv->csr.dmc_payload);
 }
-
-void assert_csr_loaded(struct drm_i915_private *dev_priv)
-{
-	WARN(!I915_READ(CSR_PROGRAM_BASE),
-				"CSR program storage start is NULL\n");
-	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
-	WARN(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n");
-}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index beeb4d326cbe..ab585e734f99 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1168,7 +1168,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 ed8c0cee738f..33cc3375f87f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -439,6 +439,14 @@ static void gen9_set_dc_state_debugmask_memory_up(
 	}
 }
 
+static void assert_csr_loaded(struct drm_i915_private *dev_priv)
+{
+	WARN(!I915_READ(CSR_PROGRAM_BASE),
+				"CSR program storage start is NULL\n");
+	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
+	WARN(!I915_READ(CSR_HTP_SKL), "CSR HTP Not fine\n");
+}
+
 static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
-- 
2.1.4

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

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

[Daniel Vetter]

They don't serve a purpose any more after the preceding patches.

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 your own completion like this code did with the
  (already removed) wait_for(csr_load_status_get()) pattern - it's
  really hard to get these right when you want them to be _really_
  correct (and be fast) in all cases. Furthermore it's easier to read
  code using the well-known primitives than new ones using
  non-standard names.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c  |  1 -
 drivers/gpu/drm/i915/i915_drv.c  |  6 -----
 drivers/gpu/drm/i915/i915_drv.h  | 10 ---------
 drivers/gpu/drm/i915/intel_csr.c | 48 +---------------------------------------
 drivers/gpu/drm/i915/intel_drv.h |  3 ---
 5 files changed, 1 insertion(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 6d3a7466a430..1d1a3bb51bf2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -795,7 +795,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->mmio_flip_lock);
 	mutex_init(&dev_priv->sb_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
-	mutex_init(&dev_priv->csr_lock);
 
 	intel_pm_setup(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b49697a9225f..bcc991f4cb3f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1019,12 +1019,6 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
 	/* Enabling DC6 is not a hard requirement to enter runtime D3 */
 
-	/*
-	 * This is to ensure that CSR isn't identified as loaded before
-	 * CSR-loading program is called during runtime-resume.
-	 */
-	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
-
 	skl_uninit_cdclk(dev_priv);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 52d07fbd9cc8..9eaefd7ab82e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -734,12 +734,6 @@ struct intel_uncore {
 #define for_each_fw_domain(domain__, dev_priv__, i__) \
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
-enum csr_state {
-	FW_UNINITIALIZED = 0,
-	FW_LOADED,
-	FW_FAILED
-};
-
 struct intel_csr {
 	const char *fw_path;
 	__be32 *dmc_payload;
@@ -747,7 +741,6 @@ struct intel_csr {
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
 	uint32_t mmiodata[8];
-	enum csr_state state;
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
@@ -1698,9 +1691,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 6362653d19af..a0fbce046ef0 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -184,40 +184,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.
  *
@@ -236,7 +202,6 @@ void intel_csr_load_program(struct drm_device *dev)
 		return;
 	}
 
-	mutex_lock(&dev_priv->csr_lock);
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
@@ -246,9 +211,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)
@@ -404,11 +366,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 
 	if (IS_SKYLAKE(dev))
 		csr->fw_path = I915_CSR_SKL;
-	else {
-		DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
-		intel_csr_load_status_set(dev_priv, FW_FAILED);
-		return;
-	}
 
 	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
 
@@ -423,10 +380,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);
-	}
 }
 
 /**
@@ -443,6 +398,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 ab585e734f99..094a223238eb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1163,9 +1163,6 @@ u32 skl_plane_ctl_rotation(unsigned int rotation);
 
 /* intel_csr.c */
 void intel_csr_ucode_init(struct drm_device *dev);
-enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv);
-void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
-					enum csr_state state);
 void intel_csr_load_program(struct drm_device *dev);
 void intel_csr_ucode_fini(struct drm_device *dev);
 
-- 
2.1.4

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

[PATCH 05/12] drm/i915: Align line continuations in intel_csr.c

[Daniel Vetter]

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

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

Also align register definitions while at it.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  2 +-
 drivers/gpu/drm/i915/intel_csr.c | 44 ++++++++++++++++++++--------------------
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 46af8c784d29..15b78e40d177 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7374,7 +7374,7 @@ enum skl_disp_power_wells {
 /* MMIO address range for CSR program (0x80000 - 0x82FFF) */
 #define CSR_MAX_FW_SIZE			0x2FFF
 #define CSR_DEFAULT_FW_OFFSET		0xFFFFFFFF
-#define CSR_MMIO_START_RANGE	0x80000
+#define CSR_MMIO_START_RANGE		0x80000
 #define CSR_MMIO_END_RANGE		0x8FFFF
 
 /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index a0fbce046ef0..916c183f5fa4 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -160,15 +160,15 @@ struct stepping_info {
 };
 
 static const struct stepping_info skl_stepping_info[] = {
-		{'A', '0'}, {'B', '0'}, {'C', '0'},
-		{'D', '0'}, {'E', '0'}, {'F', '0'},
-		{'G', '0'}, {'H', '0'}, {'I', '0'}
+	{'A', '0'}, {'B', '0'}, {'C', '0'},
+	{'D', '0'}, {'E', '0'}, {'F', '0'},
+	{'G', '0'}, {'H', '0'}, {'I', '0'}
 };
 
 static char intel_get_stepping(struct drm_device *dev)
 {
 	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
-			ARRAY_SIZE(skl_stepping_info)))
+				ARRAY_SIZE(skl_stepping_info)))
 		return skl_stepping_info[dev->pdev->revision].stepping;
 	else
 		return -ENODATA;
@@ -177,7 +177,7 @@ static char intel_get_stepping(struct drm_device *dev)
 static char intel_get_substepping(struct drm_device *dev)
 {
 	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
-			ARRAY_SIZE(skl_stepping_info)))
+				ARRAY_SIZE(skl_stepping_info)))
 		return skl_stepping_info[dev->pdev->revision].substepping;
 	else
 		return -ENODATA;
@@ -205,11 +205,11 @@ void intel_csr_load_program(struct drm_device *dev)
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
-			(u32 __force)payload[i]);
+			   (u32 __force)payload[i]);
 
 	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
 		I915_WRITE(dev_priv->csr.mmioaddr[i],
-			dev_priv->csr.mmiodata[i]);
+			   dev_priv->csr.mmiodata[i]);
 	}
 }
 
@@ -241,20 +241,20 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	/* Extract CSS Header information*/
 	css_header = (struct intel_css_header *)fw->data;
 	if (sizeof(struct intel_css_header) !=
-		(css_header->header_len * 4)) {
+	    (css_header->header_len * 4)) {
 		DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
-			(css_header->header_len * 4));
+			  (css_header->header_len * 4));
 		goto out;
 	}
 	readcount += sizeof(struct intel_css_header);
 
 	/* Extract Package Header information*/
 	package_header = (struct intel_package_header *)
-					&fw->data[readcount];
+		&fw->data[readcount];
 	if (sizeof(struct intel_package_header) !=
-		(package_header->header_len * 4)) {
+	    (package_header->header_len * 4)) {
 		DRM_ERROR("Firmware has wrong package header length %u bytes\n",
-			(package_header->header_len * 4));
+			  (package_header->header_len * 4));
 		goto out;
 	}
 	readcount += sizeof(struct intel_package_header);
@@ -262,15 +262,15 @@ 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 &&
-			substepping == package_header->fw_info[i].substepping) {
+			   substepping == package_header->fw_info[i].substepping) {
 			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) {
@@ -283,7 +283,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);
@@ -291,15 +291,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];
@@ -377,9 +377,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 06/12] drm/i915: Simplify csr loading failure printing

[Daniel Vetter]

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

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  | 20 --------------------
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_csr.c |  8 +++-----
 3 files changed, 3 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index bcc991f4cb3f..adc3502068d1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -539,26 +539,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 9eaefd7ab82e..750c859a67c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2628,7 +2628,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 916c183f5fa4..59e2d98d7082 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -228,10 +228,8 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	__be32 *dmc_payload;
 	bool fw_loaded = false;
 
-	if (!fw) {
-		i915_firmware_load_error_print(csr->fw_path, 0);
+	if (!fw)
 		goto out;
-	}
 
 	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
 		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
@@ -339,6 +337,8 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 out:
 	if (fw_loaded)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+	else
+		DRM_ERROR("Failed to load DMC firmware, disabling runtime pm\n");
 
 	/*
 	 * We require the dmc firmware for runtime pm on gen9+ - leak the rpm
@@ -380,8 +380,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
 				      &dev_priv->dev->pdev->dev,
 				      GFP_KERNEL, dev_priv,
 				      finish_csr_load);
-	if (ret)
-		i915_firmware_load_error_print(csr->fw_path, ret);
 }
 
 /**
-- 
2.1.4

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

[PATCH 07/12] drm/i915/csr: extract parse_csr_fw

[Daniel Vetter]

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

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

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 59 ++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 59e2d98d7082..7ace2cc83269 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -194,7 +194,7 @@ static char intel_get_substepping(struct drm_device *dev)
 void intel_csr_load_program(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	__be32 *payload = dev_priv->csr.dmc_payload;
+	uint32_t *payload = dev_priv->csr.dmc_payload;
 	uint32_t i, fw_size;
 
 	if (!IS_GEN9(dev)) {
@@ -204,8 +204,7 @@ void intel_csr_load_program(struct drm_device *dev)
 
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
-		I915_WRITE(CSR_PROGRAM_BASE + i * 4,
-			   (u32 __force)payload[i]);
+		I915_WRITE(CSR_PROGRAM_BASE + i * 4, payload[i]);
 
 	for (i = 0; i < dev_priv->csr.mmio_count; i++) {
 		I915_WRITE(dev_priv->csr.mmioaddr[i],
@@ -213,9 +212,9 @@ void intel_csr_load_program(struct drm_device *dev)
 	}
 }
 
-static void finish_csr_load(const struct firmware *fw, void *context)
+static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
+			      const struct firmware *fw)
 {
-	struct drm_i915_private *dev_priv = context;
 	struct drm_device *dev = dev_priv->dev;
 	struct intel_css_header *css_header;
 	struct intel_package_header *package_header;
@@ -225,15 +224,11 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	char substepping = intel_get_substepping(dev);
 	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
 	uint32_t i;
-	__be32 *dmc_payload;
-	bool fw_loaded = false;
-
-	if (!fw)
-		goto out;
+	uint32_t *dmc_payload;
 
 	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
 		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
-		goto out;
+		return NULL;
 	}
 
 	/* Extract CSS Header information*/
@@ -242,7 +237,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	    (css_header->header_len * 4)) {
 		DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
 			  (css_header->header_len * 4));
-		goto out;
+		return NULL;
 	}
 	readcount += sizeof(struct intel_css_header);
 
@@ -253,7 +248,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);
 
@@ -273,7 +268,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;
 
@@ -282,7 +277,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);
 
@@ -290,7 +285,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++) {
@@ -298,7 +293,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];
@@ -308,17 +303,16 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	nbytes = dmc_header->fw_size * 4;
 	if (nbytes > CSR_MAX_FW_SIZE) {
 		DRM_ERROR("CSR firmware too big (%u) bytes\n", nbytes);
-		goto out;
+		return NULL;
 	}
 	csr->dmc_fw_size = dmc_header->fw_size;
 
-	csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
-	if (!csr->dmc_payload) {
+	dmc_payload = kmalloc(nbytes, GFP_KERNEL);
+	if (!dmc_payload) {
 		DRM_ERROR("Memory allocation failed for dmc payload\n");
-		goto out;
+		return NULL;
 	}
 
-	dmc_payload = csr->dmc_payload;
 	for (i = 0; i < dmc_header->fw_size; i++) {
 		uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
 		/*
@@ -326,16 +320,29 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 		 * little-endian format in the firmware image and programmed
 		 * as 32 bit big-endian format to memory.
 		 */
-		dmc_payload[i] = cpu_to_be32(*tmp);
+		dmc_payload[i] = (uint32_t __force) cpu_to_be32(*tmp);
 	}
 
-	/* load csr program during system boot, as needed for DC states */
+	return dmc_payload;
+}
+
+static void finish_csr_load(const struct firmware *fw, void *context)
+{
+	struct drm_i915_private *dev_priv = context;
+	struct drm_device *dev = dev_priv->dev;
+
+	if (!fw)
+		goto out;
+
+	dev_priv->csr.dmc_payload = parse_csr_fw(dev_priv, fw);
+	if (!dev_priv->csr.dmc_payload)
+		goto out;
+
 	intel_csr_load_program(dev);
-	fw_loaded = true;
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
 out:
-	if (fw_loaded)
+	if (dev_priv->csr.dmc_payload)
 		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 	else
 		DRM_ERROR("Failed to load DMC firmware, disabling runtime pm\n");
-- 
2.1.4

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

[PATCH 08/12] drm/i915: Don't try to load garbage dmc firmware on resume

[Daniel Vetter]

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

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 2 +-
 drivers/gpu/drm/i915/intel_csr.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 750c859a67c7..1145a354c385 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -736,7 +736,7 @@ struct intel_uncore {
 
 struct intel_csr {
 	const char *fw_path;
-	__be32 *dmc_payload;
+	uint32_t *dmc_payload;
 	uint32_t dmc_fw_size;
 	uint32_t mmio_count;
 	uint32_t mmioaddr[8];
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 7ace2cc83269..b87d5dfc04ce 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -202,6 +202,9 @@ void intel_csr_load_program(struct drm_device *dev)
 		return;
 	}
 
+	if (!dev_priv->csr.dmc_payload)
+		return;
+
 	fw_size = dev_priv->csr.dmc_fw_size;
 	for (i = 0; i < fw_size; i++)
 		I915_WRITE(CSR_PROGRAM_BASE + i * 4, payload[i]);
-- 
2.1.4

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

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

[Daniel Vetter]

It's the new style!

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@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 | 27 +++++++++++----------------
 drivers/gpu/drm/i915/intel_drv.h |  6 +++---
 4 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 1d1a3bb51bf2..3cb5afef64c3 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -841,7 +841,7 @@ 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);
+	intel_csr_ucode_init(dev_priv);
 
 	ret = i915_gem_gtt_init(dev);
 	if (ret)
@@ -1016,7 +1016,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:
@@ -1097,7 +1097,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 adc3502068d1..7dfe9136c86f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1045,10 +1045,8 @@ static int bxt_resume_prepare(struct drm_i915_private *dev_priv)
 
 static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
-	struct drm_device *dev = dev_priv->dev;
-
 	skl_init_cdclk(dev_priv);
-	intel_csr_load_program(dev);
+	intel_csr_load_program(dev_priv);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index b87d5dfc04ce..d46f8110e72a 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -185,19 +185,18 @@ static char intel_get_substepping(struct drm_device *dev)
 
 /**
  * intel_csr_load_program() - write the firmware from memory to register.
- * @dev: drm device.
+ * @dev_priv: i915 drm device.
  *
  * CSR firmware is read from a .bin file and kept in internal memory one time.
  * Everytime display comes back from low power state this function is called to
  * copy the firmware from internal memory to registers.
  */
-void intel_csr_load_program(struct drm_device *dev)
+void intel_csr_load_program(struct drm_i915_private *dev_priv)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t *payload = dev_priv->csr.dmc_payload;
 	uint32_t i, fw_size;
 
-	if (!IS_GEN9(dev)) {
+	if (!IS_GEN9(dev_priv)) {
 		DRM_ERROR("No CSR support available for this platform\n");
 		return;
 	}
@@ -332,7 +331,6 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 static void finish_csr_load(const struct firmware *fw, void *context)
 {
 	struct drm_i915_private *dev_priv = context;
-	struct drm_device *dev = dev_priv->dev;
 
 	if (!fw)
 		goto out;
@@ -341,7 +339,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
 	if (!dev_priv->csr.dmc_payload)
 		goto out;
 
-	intel_csr_load_program(dev);
+	intel_csr_load_program(dev_priv);
 
 	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
 out:
@@ -360,21 +358,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;
 
 	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
@@ -394,16 +391,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 094a223238eb..b22bded5b439 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1162,9 +1162,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 10/12] drm/i915: Use request_firmware and our own async work

[Daniel Vetter]

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: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_csr.c | 21 +++++++++++----------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1145a354c385..5160af0901ce 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -735,6 +735,7 @@ struct intel_uncore {
 	for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
 
 struct intel_csr {
+	struct work_struct work;
 	const char *fw_path;
 	uint32_t *dmc_payload;
 	uint32_t dmc_fw_size;
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index d46f8110e72a..58ff765dc9b5 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -328,10 +328,16 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 	return dmc_payload;
 }
 
-static void finish_csr_load(const struct firmware *fw, void *context)
+static void csr_load_work_fn(struct work_struct *work)
 {
-	struct drm_i915_private *dev_priv = context;
+	struct drm_i915_private *dev_priv;
+	const struct firmware *fw;
+	int ret;
+
+	dev_priv = container_of(work, typeof(*dev_priv), csr.work);
 
+	ret = request_firmware(&fw, dev_priv->csr.fw_path,
+			       &dev_priv->dev->pdev->dev);
 	if (!fw)
 		goto out;
 
@@ -366,7 +372,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;
@@ -374,19 +381,13 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
 	if (IS_SKYLAKE(dev_priv))
 		csr->fw_path = I915_CSR_SKL;
 
-	DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
-
 	/*
 	 * Obtain a runtime pm reference, until CSR is loaded,
 	 * to avoid entering runtime-suspend.
 	 */
 	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);
+	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 11/12] drm/i915: Use flush_work to synchronize with dmc loader

[Daniel Vetter]

I have just placed them at the exact spot the old code had a
set_state. This probably needs to be moved earlier into the suspend
sequence, but I didn't audit that.

FIXME:
- Do this audit and move the flush_work to the right places.
- For consistency and since we have the work already it would be nice
  to also load the dmc asynchronously on resume. We can reuse the same
  loader function by simply skipping the fw loading&parsing when
  dev_priv->csr.dmc_payload is set already.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  | 2 +-
 drivers/gpu/drm/i915/intel_csr.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7dfe9136c86f..2764f3520c2c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -997,7 +997,7 @@ 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 */
+	flush_work(&dev_priv->csr.work);
 
 	skl_uninit_cdclk(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 58ff765dc9b5..350b36f1237b 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -402,5 +402,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 12/12] drm/i915/csr: Simplify stepping computation

[Daniel Vetter]

No need to be overly fancy here.

Cc: Animesh Manna <animesh.manna@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_csr.c | 38 +++-----------------------------------
 1 file changed, 3 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index 350b36f1237b..6aadd3c7aa22 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -154,35 +154,6 @@ struct intel_dmc_header {
 	uint32_t reserved1[2];
 } __packed;
 
-struct stepping_info {
-	char stepping;
-	char substepping;
-};
-
-static const struct stepping_info skl_stepping_info[] = {
-	{'A', '0'}, {'B', '0'}, {'C', '0'},
-	{'D', '0'}, {'E', '0'}, {'F', '0'},
-	{'G', '0'}, {'H', '0'}, {'I', '0'}
-};
-
-static char intel_get_stepping(struct drm_device *dev)
-{
-	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
-				ARRAY_SIZE(skl_stepping_info)))
-		return skl_stepping_info[dev->pdev->revision].stepping;
-	else
-		return -ENODATA;
-}
-
-static char intel_get_substepping(struct drm_device *dev)
-{
-	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
-				ARRAY_SIZE(skl_stepping_info)))
-		return skl_stepping_info[dev->pdev->revision].substepping;
-	else
-		return -ENODATA;
-}
-
 /**
  * intel_csr_load_program() - write the firmware from memory to register.
  * @dev_priv: i915 drm device.
@@ -222,16 +193,13 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
 	struct intel_package_header *package_header;
 	struct intel_dmc_header *dmc_header;
 	struct intel_csr *csr = &dev_priv->csr;
-	char stepping = intel_get_stepping(dev);
-	char substepping = intel_get_substepping(dev);
+	char stepping, substepping;
 	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
 	uint32_t i;
 	uint32_t *dmc_payload;
 
-	if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
-		DRM_ERROR("Unknown stepping info, firmware loading failed\n");
-		return NULL;
-	}
+	substepping = '0';
+	stepping = 'A' + dev->pdev->revision;
 
 	/* Extract CSS Header information*/
 	css_header = (struct intel_css_header *)fw->data;
-- 
2.1.4

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

Re: [PATCH 01/12] drm/i915: use correct power domain for csr loading

[Animesh Manna]

On 7/10/2015 1:34 AM, Daniel Vetter wrote:
> Grabbing a runtime pm reference with intel_runtime_pm_get will only
> prevent device D3. But dmc firmware is required even earlier (namely
> for the skl power well 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: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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 6d8a7bf06dfc..16cd9dae1c1b 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
>   out:
>   	if (fw_loaded)
> -		intel_runtime_pm_put(dev_priv);
> +		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
>   	else
>   		intel_csr_load_status_set(dev_priv, FW_FAILED);
>   
> @@ -429,7 +429,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);

Thanks Daniel for helping me in redesigning the dmc firmware loading flow.
I can see a issue here - intel_power_domain_init is called after intel_csr_ucode_init in i915_driver_load function, we can move the init call for csr after power domain init.

>   
>   	/* CSR supported for platform, load firmware */
>   	ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,

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

Re: [PATCH 02/12] drm/i915: Only allow rpm on gen9+ with dmc loaded

[Animesh Manna]

On 7/10/2015 1:34 AM, Daniel Vetter wrote:
> Instead of trying to deal with this complexity we'll simply require
> that the dmc firmware is available for runtime pm support. We do that
> by not releasing the rpm reference we acquire when starting the
> firmware loader work. Note that since we hold a rpm reference (and rpm
> get/put is synchronized with its own locking already) there's no need
> for any additional synchronization between the dmc loader and the rpm
> entry/exit code.
>
> Hence we can remove all dmc_load_status_get calls, they don't do
> anything any more.
>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

For BXT, without DMC firmware display engine can goto DC9, so if firmware failed to load then as we are holding rpm reference - it will block dc9 entry.
Do we really want to block dc9 if firmware is not present or failed to load?

Regards,
Animesh

> ---
>   drivers/gpu/drm/i915/intel_csr.c        |  9 +++++----
>   drivers/gpu/drm/i915/intel_runtime_pm.c | 17 +++--------------
>   2 files changed, 8 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 16cd9dae1c1b..03d83892cdb0 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -393,8 +393,11 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>   out:
>   	if (fw_loaded)
>   		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> -	else
> -		intel_csr_load_status_set(dev_priv, FW_FAILED);
> +
> +	/*
> +	 * We require the dmc firmware for runtime pm on gen9+ - leak the rpm
> +	 * reference in case this failed to disable rpm on.
> +	 */
>   
>   	release_firmware(fw);
>   }
> @@ -462,8 +465,6 @@ void intel_csr_ucode_fini(struct drm_device *dev)
>   
>   void assert_csr_loaded(struct drm_i915_private *dev_priv)
>   {
> -	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
> -	     "CSR is not loaded.\n");
>   	WARN(!I915_READ(CSR_PROGRAM_BASE),
>   				"CSR program storage start is NULL\n");
>   	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 2628b21ff2c0..ed8c0cee738f 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -644,21 +644,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>   
>   			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
>   				power_well->data == SKL_DISP_PW_2) {
> -				enum csr_state state;
> -				/* TODO: wait for a completion event or
> -				 * similar here instead of busy
> -				 * waiting using wait_for function.
> -				 */
> -				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> -						FW_UNINITIALIZED, 1000);
> -				if (state != FW_LOADED)
> -					DRM_ERROR("CSR firmware not ready (%d)\n",
> -							state);
> +				if (SKL_ENABLE_DC6(dev))
> +					skl_enable_dc6(dev_priv);
>   				else
> -					if (SKL_ENABLE_DC6(dev))
> -						skl_enable_dc6(dev_priv);
> -					else
> -						gen9_enable_dc5(dev_priv);
> +					gen9_enable_dc5(dev_priv);
>   			}
>   		}
>   	}

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

Re: [PATCH 02/12] drm/i915: Only allow rpm on gen9+ with dmc loaded

[Daniel Vetter]

On Fri, Jul 10, 2015 at 01:50:23PM +0530, Animesh Manna wrote:
> 
> 
> On 7/10/2015 1:34 AM, Daniel Vetter wrote:
> >Instead of trying to deal with this complexity we'll simply require
> >that the dmc firmware is available for runtime pm support. We do that
> >by not releasing the rpm reference we acquire when starting the
> >firmware loader work. Note that since we hold a rpm reference (and rpm
> >get/put is synchronized with its own locking already) there's no need
> >for any additional synchronization between the dmc loader and the rpm
> >entry/exit code.
> >
> >Hence we can remove all dmc_load_status_get calls, they don't do
> >anything any more.
> >
> >Cc: Animesh Manna <animesh.manna@intel.com>
> >Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> For BXT, without DMC firmware display engine can goto DC9, so if
> firmware failed to load then as we are holding rpm reference - it will
> block dc9 entry.  Do we really want to block dc9 if firmware is not
> present or failed to load?

I guess for bxt we can lift the restriction and check
dev_priv->csr.dmc_payload in the bxt power well code and drop the power
well reference here by adjusting the condition.

This really was just to showcase what I had in mind with grabbing the
right power well reference: We use get/put to make sure that any of the
platform power well code is guaranteed to never run, instead of perhaps
adding another mutex or something similar (which usually results in more
design problems than just a get/put pair).

In short: This is just to demonstrate the flow, details can of course be
adjusted.
-Daniel

> 
> Regards,
> Animesh
> 
> >---
> >  drivers/gpu/drm/i915/intel_csr.c        |  9 +++++----
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 17 +++--------------
> >  2 files changed, 8 insertions(+), 18 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> >index 16cd9dae1c1b..03d83892cdb0 100644
> >--- a/drivers/gpu/drm/i915/intel_csr.c
> >+++ b/drivers/gpu/drm/i915/intel_csr.c
> >@@ -393,8 +393,11 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >  out:
> >  	if (fw_loaded)
> >  		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> >-	else
> >-		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >+
> >+	/*
> >+	 * We require the dmc firmware for runtime pm on gen9+ - leak the rpm
> >+	 * reference in case this failed to disable rpm on.
> >+	 */
> >  	release_firmware(fw);
> >  }
> >@@ -462,8 +465,6 @@ void intel_csr_ucode_fini(struct drm_device *dev)
> >  void assert_csr_loaded(struct drm_i915_private *dev_priv)
> >  {
> >-	WARN(intel_csr_load_status_get(dev_priv) != FW_LOADED,
> >-	     "CSR is not loaded.\n");
> >  	WARN(!I915_READ(CSR_PROGRAM_BASE),
> >  				"CSR program storage start is NULL\n");
> >  	WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
> >diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >index 2628b21ff2c0..ed8c0cee738f 100644
> >--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >@@ -644,21 +644,10 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> >  			if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> >  				power_well->data == SKL_DISP_PW_2) {
> >-				enum csr_state state;
> >-				/* TODO: wait for a completion event or
> >-				 * similar here instead of busy
> >-				 * waiting using wait_for function.
> >-				 */
> >-				wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> >-						FW_UNINITIALIZED, 1000);
> >-				if (state != FW_LOADED)
> >-					DRM_ERROR("CSR firmware not ready (%d)\n",
> >-							state);
> >+				if (SKL_ENABLE_DC6(dev))
> >+					skl_enable_dc6(dev_priv);
> >  				else
> >-					if (SKL_ENABLE_DC6(dev))
> >-						skl_enable_dc6(dev_priv);
> >-					else
> >-						gen9_enable_dc5(dev_priv);
> >+					gen9_enable_dc5(dev_priv);
> >  			}
> >  		}
> >  	}
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: use correct power domain for csr loading

[Daniel Vetter]

On Fri, Jul 10, 2015 at 01:42:09PM +0530, Animesh Manna wrote:
> 
> 
> On 7/10/2015 1:34 AM, Daniel Vetter wrote:
> >Grabbing a runtime pm reference with intel_runtime_pm_get will only
> >prevent device D3. But dmc firmware is required even earlier (namely
> >for the skl power well 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: Animesh Manna <animesh.manna@intel.com>
> >Signed-off-by: Daniel Vetter <daniel.vetter@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 6d8a7bf06dfc..16cd9dae1c1b 100644
> >--- a/drivers/gpu/drm/i915/intel_csr.c
> >+++ b/drivers/gpu/drm/i915/intel_csr.c
> >@@ -392,7 +392,7 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> >  	DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
> >  out:
> >  	if (fw_loaded)
> >-		intel_runtime_pm_put(dev_priv);
> >+		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> >  	else
> >  		intel_csr_load_status_set(dev_priv, FW_FAILED);
> >@@ -429,7 +429,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);
> 
> Thanks Daniel for helping me in redesigning the dmc firmware loading flow.
> I can see a issue here - intel_power_domain_init is called after
> intel_csr_ucode_init in i915_driver_load function, we can move the init
> call for csr after power domain init.

Yeah there's definitely some issues in those patches since they're totally
untested ;-) My idea here was just to showcase the concepts, if a given
patch needs to be adjusted or completely rewritten then just throw it out
and replace it with yours.

There's another FIXME in a later patch about where exactly we need to make
sure that the dmc firmware is loaded correctly. I don't have any idea how
the hardware works and what's possible/allowed, so we might need to move
the flush_work around a bit in the suspend/unload code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915/csr: Simplify stepping computation

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6764
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  303/303              303/303
SNB              +3                 309/316              312/316
IVB                                  343/343              343/343
BYT                 -1              285/285              284/285
HSW              +13                 367/381              380/381
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@kms_mmio_vs_cs_flip@setcrtc_vs_cs_flip      DMESG_WARN(1)      PASS(1)
*SNB  igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip      DMESG_WARN(1)      PASS(1)
*SNB  igt@pm_rpm@cursor      DMESG_WARN(1)      PASS(1)
*SNB  igt@pm_rpm@cursor-dpms      DMESG_FAIL(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
*HSW  igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_lpsp@non-edp      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@debugfs-read      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@gem-idle      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@gem-mmap-gtt      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@gem-pread      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@i2c      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@modeset-non-lpsp      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@modeset-non-lpsp-stress-no-wait      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@pci-d3-state      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@reg-read-ioctl      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@rte      DMESG_WARN(1)      PASS(1)
*HSW  igt@pm_rpm@sysfs-read      DMESG_WARN(1)      PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx