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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 1/22/2018 8:57 PM, Sagar Arun Kamble wrote:


On 1/22/2018 4:17 PM, Chris Wilson wrote:
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,
I was also initially thinking that init/fini ordering should take care of synchronization. Checked further and I see that relay_open and relay_destroy are synchronized by relay_channels_mutex and internally if needed through debugfs inode synchronization. So I feel we need not add new lock.
Sorry. after some more thinking, I am now convinced that lock will be needed as guc_log_has_relay was accessing
it without any locking. Will share new rev. Thanks for the review.
  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


--
Thanks,
Sagar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux