| Message ID | 20180323123411.3214-1-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710 > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Marta Löfstedt <marta.lofstedt@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@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? 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 | >
Quoting Michał Winiarski (2018-03-23 14:34:04) > 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@intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710 > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Marta Löfstedt <marta.lofstedt@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
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@intel.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710 > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Marta Löfstedt <marta.lofstedt@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@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 > > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710 > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Marta Löfstedt <marta.lofstedt@intel.com> > > > Cc: Michal Wajdeczko <michal.wajdeczko@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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)); 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 |
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@intel.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105710 Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Marta Löfstedt <marta.lofstedt@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/intel_guc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)