[Intel-gfx] [PATCH v2] drm/i915: Always program CSR if CSR is uninitialized

[Imre Deak]

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 at linux.intel.com>
> > > Tested-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at 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