[Intel-gfx] [PATCH v3 3/6] drm/i915/guc: Fix lockdep due to log relay channel handling under struct_mutex

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-24 10:52:28)
> 
> 
> On 1/24/2018 3:48 PM, Chris Wilson wrote:
> > Quoting Sagar Arun Kamble (2018-01-24 04:09:09)
> >> @@ -197,7 +212,7 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
> >>   
> >>   static void *guc_get_write_buffer(struct intel_guc *guc)
> >>   {
> >> -       if (!guc->log.runtime.relay_chan)
> >> +       if (!guc_log_has_relay(guc))
> >>                  return NULL;
> >>   
> >>          /* Just get the base address of a new sub buffer and copy data into it
> >> @@ -265,6 +280,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> >>          /* Get the pointer to shared GuC log buffer */
> >>          log_buf_state = src_data = guc->log.runtime.buf_addr;
> >>   
> >> +       mutex_lock(&guc->log.runtime.relay_lock);
> >> +
> >>          /* Get the pointer to local buffer to store the logs */
> >>          log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
> > Hmm. The locking here tells me that we are being careful in case the
> > relay_chan disappears, but we don't handle the NULL pointer here.
> >   
> There is check for log_bug_snapshot_state below in for loop. But yes, we 
> should return from here.
> Will update.
> >> @@ -643,6 +724,8 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> >>          gen9_disable_guc_interrupts(dev_priv);
> >>          intel_runtime_pm_put(dev_priv);
> >>   
> >> -       guc_log_runtime_destroy(&dev_priv->guc);
> >> +       guc_log_runtime_destroy(guc);
> >>          mutex_unlock(&dev_priv->drm.struct_mutex);
> >> +
> >> +       intel_guc_log_relay_destroy(guc);
> >>   }
> > This looks all reasonably well described by the addition of the
> > relay_lock and the interactions look fine. The only mistake I could see,
> > in the story told by this patch, was the runtime checking.
> Could you please elaborate more on this.

The previous comment :)
-Chris