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

[Chris Wilson]

Quoting Sagar Arun Kamble (2018-01-22 10:38:10)
> 
> 
> On 1/22/2018 3:46 PM, Chris Wilson wrote:
> > Quoting Sagar Arun Kamble (2018-01-22 08:26:01)
> >> +int intel_guc_log_relay_create(struct intel_guc *guc)
> >> +{
> >> +       struct drm_i915_private *dev_priv = guc_to_i915(guc);
> >> +       struct rchan *guc_log_relay_chan;
> >> +       size_t n_subbufs, subbuf_size;
> >> +       int ret;
> >> +
> >> +       if (!i915_modparams.guc_log_level)
> >> +               return 0;
> >> +
> >> +       GEM_BUG_ON(guc_log_has_relay(guc));
> >> +
> >>           /* Keep the size of sub buffers same as shared log buffer */
> >> -       subbuf_size = guc->log.vma->obj->base.size;
> >> +       subbuf_size = GUC_LOG_SIZE;
> >>   
> >>          /* Store up to 8 snapshots, which is large enough to buffer sufficient
> >>           * boot time logs and provides enough leeway to User, in terms of
> >> @@ -407,33 +442,31 @@ static int guc_log_runtime_create(struct intel_guc *guc)
> >>                  DRM_ERROR("Couldn't create relay chan for GuC logging\n");
> >>   
> >>                  ret = -ENOMEM;
> >> -               goto err_vaddr;
> >> +               goto err;
> >>          }
> >>   
> >>          GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
> >>          guc->log.runtime.relay_chan = guc_log_relay_chan;
> >>   
> >> -       INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
> >>          return 0;
> >>   
> >> -err_vaddr:
> >> -       i915_gem_object_unpin_map(guc->log.vma->obj);
> >> -       guc->log.runtime.buf_addr = NULL;
> >> +err:
> >> +       /* logging will be off */
> >> +       i915_modparams.guc_log_level = 0;
> >>          return ret;
> >>   }
> >>   
> >> -static void guc_log_runtime_destroy(struct intel_guc *guc)
> >> +void intel_guc_log_relay_destroy(struct intel_guc *guc)
> >>   {
> > Now exposed to multiple users, we need to document what the locking
> > requirements are here. Or add some local locking.
> Do you mean locking between relay_create and relay_destroy?

We need a lock around guc->log.runtime.relay_chan as the destroy path is
not ostensibly serialised between multiple potential callers. Ordinarily
I would have said that serialisation for create/destroy/access of
relay_chan was guaranteed by init/fini ordering, but that's no longer
clear (based on a 5min read of the patch).

The most important question is "can relay_destroy be called while some
user still has access to the relay_chan?"

> >   Looks like at the
> > moment, _create is using struct_mutex,
> relay_create and relay_destroy are now to be done outside of struct_mutex.
> I will add this documentation to the functions.

(lockdep_assert_held :)
-Chris