Re: [PATCH v2] drm/i915: Always program CSR if CSR is uninitialized

From: Imre Deak <imre.deak@intel.com>
To: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, rodrigo.vivi@intel.com
Subject: Re: [PATCH v2] drm/i915: Always program CSR if CSR is uninitialized
Date: Tue, 27 Oct 2015 20:41:31 +0200	[thread overview]
Message-ID: <1445971291.2408.68.camel@intel.com> (raw)
In-Reply-To: <1445593310-26596-1-git-send-email-patrik.jakobsson@linux.intel.com>

On pe, 2015-10-23 at 11:41 +0200, Patrik Jakobsson wrote:
> The current CSR loading code depends on the CSR program memory to be
> cleared after boot. This is unfortunately not true on all hardware.
> Instead make use of the FW_UNINITIALIZED state in init and check for
> FW_LOADED to prevent init path from skipping the actual programming.
> 
> v2: Move initialization of state to after HAS_CSR() check
> 
> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_csr.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index 9e530a7..e74c09e 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -271,7 +271,7 @@ void intel_csr_load_program(struct drm_device *dev)
>  	 * Unfortunately the ACPI subsystem doesn't yet give us a way to
>  	 * differentiate this, hence figure it out with this hack.
>  	 */
> -	if (I915_READ(CSR_PROGRAM(0)))
> +	if (I915_READ(CSR_PROGRAM(0)) && dev_priv->csr.state == FW_LOADED)
>  		return;

As Animesh said csr->state is being removed, so I'd prefer if we didn't
add new users for it.

I think the proper solution for this would be to reprogram the firmware
only when we know we have to and not have here either of the above
checks. The points we need to reprogram the firmware:

S3/S4 both on SKL.
S3/S4/DC9 (runtime suspend) on BXT.

To fix this particular bug you could also just add an
intel_csr_reprogram() that would check for CSR_PROGRAM(0) as now, while
during driver loading you would program the firmware unconditionally.

--Imre

>  
>  	mutex_lock(&dev_priv->csr_lock);
> @@ -428,6 +428,8 @@ void intel_csr_ucode_init(struct drm_device *dev)
>  	if (!HAS_CSR(dev))
>  		return;
>  
> +	intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> +
>  	if (IS_SKYLAKE(dev))
>  		csr->fw_path = I915_CSR_SKL;
>  	else if (IS_BROXTON(dev_priv))

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