diff mbox

drm/i915: Resign firmware loading for dmc

Message ID 1436272812-14934-1-git-send-email-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Manna, Animesh July 7, 2015, 12:40 p.m. UTC
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(-)

Comments

Daniel Vetter July 7, 2015, 1:18 p.m. UTC | #1
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
Shuang He July 8, 2015, 10:31 a.m. UTC | #2
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 '*'
Manna, Animesh July 8, 2015, 2:24 p.m. UTC | #3
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(-)
diff mbox

Patch

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);
 			}
 		}
 	}