On Mon, Mar 05, 2018 at 04:01:18PM +0530, Sagar Arun Kamble wrote:
>
>
> On 2/27/2018 6:22 PM, Michał Winiarski wrote:
> > Currently, we're treating relay and mapping of GuC log as a separate
> > concepts. We're also using inconsistent locking, sometimes using
> > relay_lock, sometimes using struct mutex.
> > Let's correct that. Anything touching the runtime is now serialized
> > using runtime.lock, while we're still using struct mutex as inner lock
> > for mapping.
> > We're still racy in setting the log level - but we'll take care of that
> > in the following patches.
> >
> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_guc_log.c | 110 ++++++++++-------------------------
> > drivers/gpu/drm/i915/intel_guc_log.h | 3 +-
> > drivers/gpu/drm/i915/intel_uc.c | 14 +++--
> > 3 files changed, 42 insertions(+), 85 deletions(-)
> >
[SNIP]
> > @@ -372,8 +349,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> > }
> > guc_move_to_next_buf(guc);
> > -
> > - mutex_unlock(&guc->log.runtime.relay_lock);
> > }
> > static void capture_logs_work(struct work_struct *work)
> > @@ -381,7 +356,9 @@ static void capture_logs_work(struct work_struct *work)
> > struct intel_guc *guc =
> > container_of(work, struct intel_guc, log.runtime.flush_work);
> > + mutex_lock(&guc->log.runtime.lock);
> > guc_log_capture_logs(guc);
> I think we should just lock guc_read_update_log_buffer as
> guc_log_flush_complete is independent action
Agreed - I'll change this and apply all the other suggestions to the locking.
> > + mutex_unlock(&guc->log.runtime.lock);
> > }
> > static bool guc_log_has_runtime(struct intel_guc *guc)
> > @@ -389,19 +366,16 @@ static bool guc_log_has_runtime(struct intel_guc *guc)
> > return guc->log.runtime.buf_addr != NULL;
> > }
> > -static int guc_log_runtime_create(struct intel_guc *guc)
> > +static int guc_log_map(struct intel_guc *guc)
> > {
> > struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > void *vaddr;
> > int ret;
> > - lockdep_assert_held(&dev_priv->drm.struct_mutex);
> > -
> lockdep_assert for runtime.lock here?
> > if (!guc->log.vma)
> > return -ENODEV;
> > - GEM_BUG_ON(guc_log_has_runtime(guc));
> > -
> > + mutex_lock(&dev_priv->drm.struct_mutex);
> > ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true);
> > if (ret)
> mutex not unlocked
> > return ret;
> > @@ -416,20 +390,16 @@ static int guc_log_runtime_create(struct intel_guc *guc)
> > DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> mutex not unlocked
> > return PTR_ERR(vaddr);
> > }
> > + mutex_unlock(&dev_priv->drm.struct_mutex);
> > guc->log.runtime.buf_addr = vaddr;
> > return 0;
> > }
> > -static void guc_log_runtime_destroy(struct intel_guc *guc)
> > +static void guc_log_unmap(struct intel_guc *guc)
> > {
> > - /*
> > - * It's possible that the runtime stuff was never allocated because
> > - * GuC log was disabled at the boot time.
> > - */
> > - if (!guc_log_has_runtime(guc))
> > - return;
> > + lockdep_assert_held(&guc->log.runtime.lock);
> struct_mutex locking here?
Except this one. AFACT, we don't really need struct mutex here. We need it only
for set_domain - I'll reduce the scope to set_domain on the map path.
> > i915_gem_object_unpin_map(guc->log.vma->obj);
> > guc->log.runtime.buf_addr = NULL;
> > @@ -437,7 +407,7 @@ static void guc_log_runtime_destroy(struct intel_guc *guc)
> > void intel_guc_log_init_early(struct intel_guc *guc)
> > {
> > - mutex_init(&guc->log.runtime.relay_lock);
> > + mutex_init(&guc->log.runtime.lock);
> rename and move of members from runtime to log can precede this patch?
How strongly do you feel about this one?
I wanted to tidy first (decouple things), rename later.
> > INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
> > }
> > @@ -448,12 +418,7 @@ int guc_log_relay_create(struct intel_guc *guc)
> > size_t n_subbufs, subbuf_size;
> > int ret;
> > - if (!i915_modparams.guc_log_level)
> > - return 0;
> > -
> > - mutex_lock(&guc->log.runtime.relay_lock);
> > -
> > - GEM_BUG_ON(guc_log_has_relay(guc));
> > + lockdep_assert_held(&guc->log.runtime.lock);
> > /* Keep the size of sub buffers same as shared log buffer */
> > subbuf_size = GUC_LOG_SIZE;
> > @@ -483,12 +448,9 @@ int guc_log_relay_create(struct intel_guc *guc)
> > GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
> > guc->log.runtime.relay_chan = guc_log_relay_chan;
> > - mutex_unlock(&guc->log.runtime.relay_lock);
> > -
> > return 0;
> > err:
> > - mutex_unlock(&guc->log.runtime.relay_lock);
> > /* logging will be off */
> > i915_modparams.guc_log_level = 0;
> This log_level decoupling is not taken care
Yup, though it belongs in "drm/i915/guc: Split relay control and GuC log level",
I'll add it there.
> > return ret;
> > @@ -496,20 +458,10 @@ int guc_log_relay_create(struct intel_guc *guc)
> > void guc_log_relay_destroy(struct intel_guc *guc)
> > {
> > - mutex_lock(&guc->log.runtime.relay_lock);
> > -
> > - /*
> > - * It's possible that the relay was never allocated because
> > - * GuC log was disabled at the boot time.
> > - */
> > - if (!guc_log_has_relay(guc))
> > - goto out_unlock;
> > + lockdep_assert_held(&guc->log.runtime.lock);
> > relay_close(guc->log.runtime.relay_chan);
> > guc->log.runtime.relay_chan = NULL;
> > -
> > -out_unlock:
> > - mutex_unlock(&guc->log.runtime.relay_lock);
> > }
> > static void guc_log_capture_logs(struct intel_guc *guc)
> > @@ -608,7 +560,6 @@ static void guc_log_flush_irq_disable(struct intel_guc *guc)
> > void intel_guc_log_destroy(struct intel_guc *guc)
> > {
> > - guc_log_runtime_destroy(guc);
> > i915_vma_unpin_and_release(&guc->log.vma);
> > }
> > @@ -678,9 +629,10 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
> > int intel_guc_log_register(struct intel_guc *guc)
> > {
> > - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > int ret;
> > + mutex_lock(&guc->log.runtime.lock);
> > +
> > GEM_BUG_ON(guc_log_has_runtime(guc));
> > /*
> > @@ -692,35 +644,33 @@ int intel_guc_log_register(struct intel_guc *guc)
> > if (ret)
> > goto err;
> > - mutex_lock(&dev_priv->drm.struct_mutex);
> > - ret = guc_log_runtime_create(guc);
> > - mutex_unlock(&dev_priv->drm.struct_mutex);
> > -
> > + ret = guc_log_map(guc);
> > if (ret)
> > goto err_relay;
> > ret = guc_log_relay_file_create(guc);
> > if (ret)
> > - goto err_runtime;
> > + goto err_unmap;
> > guc_log_flush_irq_enable(guc);
> > + mutex_unlock(&guc->log.runtime.lock);
> > +
> > return 0;
> > -err_runtime:
> > - mutex_lock(&dev_priv->drm.struct_mutex);
> > - guc_log_runtime_destroy(guc);
> > - mutex_unlock(&dev_priv->drm.struct_mutex);
> > +err_unmap:
> > + guc_log_unmap(guc);
> > err_relay:
> > guc_log_relay_destroy(guc);
> > err:
> > + mutex_unlock(&guc->log.runtime.lock);
> > +
> > return ret;
> > }
> > void intel_guc_log_unregister(struct intel_guc *guc)
> > {
> > - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > -
> > + guc_log_flush_irq_disable(guc);
> This move could be part of earlier patch.
> > /*
> > * Once logging is disabled, GuC won't generate logs & send an
> > * interrupt. But there could be some data in the log buffer
> > @@ -728,11 +678,13 @@ void intel_guc_log_unregister(struct intel_guc *guc)
> > * buffer state and then collect the left over logs.
> > */
> > guc_flush_logs(guc);
> > - guc_log_flush_irq_disable(guc);
> > - mutex_lock(&dev_priv->drm.struct_mutex);
> > - guc_log_runtime_destroy(guc);
> > - mutex_unlock(&dev_priv->drm.struct_mutex);
> > + mutex_lock(&guc->log.runtime.lock);
> > +
> > + GEM_BUG_ON(!guc_log_has_runtime(guc));
> > + guc_log_unmap(guc);
> > guc_log_relay_destroy(guc);
> > +
> > + mutex_unlock(&guc->log.runtime.lock);
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
> > index 09dd2ef1933d..8c26cce77a98 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_log.h
> > +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> > @@ -48,8 +48,7 @@ struct intel_guc_log {
> > struct workqueue_struct *flush_wq;
> > struct work_struct flush_work;
> > struct rchan *relay_chan;
> > - /* To serialize the access to relay_chan */
> Interesting, checkpatch does not complain about comment being removed for
> mutex lock :)
Well, it's a member of intel_guc_log, and it protects other members of
intel_guc_log.
-Michał
> > - struct mutex relay_lock;
> > + struct mutex lock;
> > } runtime;
> > /* logging related stats */
> > u32 capture_miss_count;
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index e9aba3c35264..55a9b5b673e0 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -221,18 +221,24 @@ static void guc_free_load_err_log(struct intel_guc *guc)
> > int intel_uc_log_register(struct drm_i915_private *dev_priv)
> > {
> > - if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level)
> > + int ret = 0;
> > +
> > + if (!USES_GUC(dev_priv))
> > return 0;
> > - return intel_guc_log_register(&dev_priv->guc);
> > + if (i915_modparams.guc_log_level)
> > + ret = intel_guc_log_register(&dev_priv->guc);
> > +
> > + return ret;
> > }
> > void intel_uc_log_unregister(struct drm_i915_private *dev_priv)
> > {
> > - if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level)
> > + if (!USES_GUC(dev_priv))
> > return;
> > - intel_guc_log_unregister(&dev_priv->guc);
> > + if (i915_modparams.guc_log_level)
> > + intel_guc_log_unregister(&dev_priv->guc);
> > }
> > static int guc_enable_communication(struct intel_guc *guc)
>
> --
> Thanks,
> Sagar
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
