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

From: Daniel Vetter <daniel@ffwll.ch>
To: Animesh Manna <animesh.manna@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 05/18] drm/i915/gen9: csr_init after runtime pm enable
Date: Tue, 28 Jul 2015 09:56:10 +0200	[thread overview]
Message-ID: <20150728075610.GZ16722@phenom.ffwll.local> (raw)
In-Reply-To: <1437850839-16782-6-git-send-email-animesh.manna@intel.com>

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

Imo the IS_BXT should be separate patch.

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

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

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

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