[Intel-gfx] [PATCH 1/8] drm/i915/guc: Use _FW variants for mmio access in GuC irq handler

[Michał Winiarski]

On Wed, Mar 28, 2018 at 04:51:55PM +0300, Joonas Lahtinen wrote:
> Quoting Daniele Ceraolo Spurio (2018-03-23 19:17:49)
> > 
> > 
> > On 23/03/18 05:34, Michał Winiarski wrote:
> > > We're seeing "RPM wakelock ref not held during HW access" warning
> > > otherwise. And since IRQ are synced for runtime suspend, we can use the
> > > variant without wakeref assert.
> > > 
> > > Reported-by: Marta Löfstedt <marta.lofstedt at intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710
> > > Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Marta Löfstedt <marta.lofstedt at intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_guc.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> > > index 8f93f5bef8fd..6787a3116783 100644
> > > --- a/drivers/gpu/drm/i915/intel_guc.c
> > > +++ b/drivers/gpu/drm/i915/intel_guc.c
> > > @@ -391,9 +391,9 @@ void intel_guc_to_host_event_handler(struct intel_guc *guc)
> > >        * clears out the bit on handling the 1st interrupt.
> > >        */
> > >       spin_lock(&guc->irq_lock);
> > > -     val = I915_READ(SOFT_SCRATCH(15));
> > > +     val = I915_READ_FW(SOFT_SCRATCH(15));
> > 
> > GuC registers are in forcewake range, so don't we need to manually grab 
> > forcewake if we use the _FW variant of the read/write macros?
> 
> Hmm, with the Bugzilla tag and all, wasn't this patch tested
> specifically to fix the bug?
> 
> Regards, Joonas
> 
> PS. If there's a bugfix, it should really be a separate patch that can
> be immediately merged and the bug should get fixed by the patch with
> Bugzilla: is merged.

Daniele is correct - we do need the forcewake. The WARN is caused by the fact
that we're not doing pm_get (and we don't need to do it).

The correct fix would be to do the disable/enable_rpm_wakeref_asserts dance,
however Chris suggested that since the forcewake takes time, maybe we could move
the logic to tasklet. And... why not?

I'll follow your advice and post it as a separate patch.

-Michał

> 
> > 
> > Daniele
> > 
> > >       msg = val & guc->msg_enabled_mask;
> > > -     I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
> > > +     I915_WRITE_FW(SOFT_SCRATCH(15), val & ~msg);
> > >       spin_unlock(&guc->irq_lock);
> > >   
> > >       if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
> > > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx