diff mbox

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

Message ID 1445593310-26596-1-git-send-email-patrik.jakobsson@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Patrik Jakobsson Oct. 23, 2015, 9:41 a.m. UTC 
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(-)

Comments

Patrik Jakobsson Oct. 23, 2015, 11:28 a.m. UTC | #1 
Not sure why all my CCs disappeared...

Animesh, is it true that CSR_PROGRAM(0) is cleared on bxt when no program is
loaded? This is not the case on skl. If it's not true on bxt either we can just
skip check CSR_PROGRAM(0) altogether.

Thanks
Patrik

On Fri, Oct 23, 2015 at 11:41:50AM +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;
>  
>  	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))
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Manna, Animesh Oct. 24, 2015, 5:33 a.m. UTC | #2 
On 10/23/2015 3:11 PM, 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).state

As I will be removing csr.state in dmc-redesign patch series my suggestion would be to
compare register read with first element of payload like below:

if (I915_READ(CSR_PROGRAM(0)) == dmc_payload[0]) and then skip the f/w loading part.

-Animesh


>   		return;
>   
>   	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))
Patrik Jakobsson Oct. 26, 2015, 9:57 a.m. UTC | #3 
On Sat, Oct 24, 2015 at 11:03:05AM +0530, Animesh Manna wrote:
> 
> 
> On 10/23/2015 3:11 PM, 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).state
> 
> As I will be removing csr.state in dmc-redesign patch series my suggestion would be to
> compare register read with first element of payload like below:

Is the redesign series likely to land soon? Currently the dmc fw isn't loading
at all on skl which is a stopper for quite a few features.

> if (I915_READ(CSR_PROGRAM(0)) == dmc_payload[0]) and then skip the f/w loading part.

I haven't looked at what is stored in CSR_PROGRAM(0) on skl after boot but can
we really trust this check? Is there really no other way to verify if it's up
and running already? What was the conclusion from your discussion with Daniel
about keeping track of this manually?

-Patrik

> -Animesh
> 
> 
> >  		return;
> >  	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))
>
Imre Deak Oct. 27, 2015, 6:41 p.m. UTC | #4 
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))
Patrik Jakobsson Oct. 28, 2015, 3:52 p.m. UTC | #5 
On Tue, Oct 27, 2015 at 08:41:31PM +0200, Imre Deak wrote:
> 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.

Isn't this what csr->state was intended to solve in the first place? It's very
easy to get lost in the discussions around DMC so I've might have missed
something.

> 
> 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.

Did some more digging and it seems the CSR program is not cleared on warm boot.
So checking CSR_PROGRAM(0) == payload[0] will not work. Don't think we can
figure out the CSR status by probing hardware so we must keep a flag for this
somewhere. The CSR program loading is async so the state need to be stored
somewhere and not just passed along as a function argument.

-Patrik

> 
> --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))
> 
> 
>
Imre Deak Oct. 28, 2015, 5:12 p.m. UTC | #6 
On ke, 2015-10-28 at 16:52 +0100, Patrik Jakobsson wrote:
> On Tue, Oct 27, 2015 at 08:41:31PM +0200, Imre Deak wrote:
> > 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.
> 
> Isn't this what csr->state was intended to solve in the first place? It's very
> easy to get lost in the discussions around DMC so I've might have missed
> something.

It was used to track if we need to enable/disable DC5/6 (depending on
the firmware load status), but that check won't be needed any more since
runtime PM itself will be disabled while the firmware is not loaded. So
we can enable/disable DC5/6 unconditionally.

We know where we need to reprogram the firmware (resuming from the above
PM states), so we could reprogram it at those times unconditionally
without using a flag.

> > 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.
> 
> Did some more digging and it seems the CSR program is not cleared on warm boot.
> So checking CSR_PROGRAM(0) == payload[0] will not work.

Yes, the check will not work for S3/S4, but that's a separate existing
issue. The solution for that is instead of this check or a flag, program
the FW unconditionally when we know we have to. But that could be also
fixed separately.

> Don't think we can figure out the CSR status by probing hardware so we
> must keep a flag for this somewhere.

We can't probe the HW, but we know the places the FW must be
reprogrammed, so we wouldn't need to probe.

> The CSR program loading is async so the state need to be stored
> somewhere and not just passed along as a function argument.

The programming itself is not async. So for this fix we could program
the firmware unconditionally (without checking for CSR_PROGRAM(0)) from
finish_csr_load() while still checking CSR_PROGRAM(0) when called from
skl_resume_prepare().

As a follow-up we could also fix the S3/S4 cases by removing the
CSR_PROGRAM(0) check altogether, and calling intel_csr_load_program()
only when resuming from the above PM events.

--Imre
diff mbox

Patch

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