Re: [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

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

 



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




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