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

[Daniel Vetter <daniel@ffwll.ch>]

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