[Intel-gfx] [PATCH v2 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

Sagar Arun Kamble sagar.a.kamble at intel.com
Fri Mar 9 07:51:12 UTC 2018



On 3/8/2018 9:16 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.
>
> v2: Tidy locking (Sagar)
>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_log.c | 113 +++++++++++------------------------
>   drivers/gpu/drm/i915/intel_guc_log.h |   3 +-
>   2 files changed, 36 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 89cb3939467f..4eb3ebd8d6c3 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -155,10 +155,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
>   	struct dentry *log_dir;
>   	int ret;
>   
> -	if (!i915_modparams.guc_log_level)
> -		return 0;
> -
> -	mutex_lock(&guc->log.runtime.relay_lock);
> +	lockdep_assert_held(&guc->log.runtime.lock);
>   
>   	/* For now create the log file in /sys/kernel/debug/dri/0 dir */
>   	log_dir = dev_priv->drm.primary->debugfs_root;
> @@ -177,28 +174,16 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
>   	 */
>   	if (!log_dir) {
>   		DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
> -		ret = -ENODEV;
> -		goto out_unlock;
> +		return -ENODEV;
>   	}
>   
>   	ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);
>   	if (ret < 0 && ret != -EEXIST) {
>   		DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> -		goto out_unlock;
> +		return ret;
>   	}
>   
> -	ret = 0;
> -
> -out_unlock:
> -	mutex_unlock(&guc->log.runtime.relay_lock);
> -	return ret;
> -}
> -
> -static bool guc_log_has_relay(struct intel_guc *guc)
> -{
> -	lockdep_assert_held(&guc->log.runtime.relay_lock);
> -
> -	return guc->log.runtime.relay_chan != NULL;
> +	return 0;
>   }
>   
>   static void guc_move_to_next_buf(struct intel_guc *guc)
> @@ -209,9 +194,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
>   	 */
>   	smp_wmb();
>   
> -	if (!guc_log_has_relay(guc))
> -		return;
> -
>   	/* All data has been written, so now move the offset of sub buffer. */
>   	relay_reserve(guc->log.runtime.relay_chan, guc->log.vma->obj->base.size);
>   
> @@ -221,9 +203,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
>   
>   static void *guc_get_write_buffer(struct intel_guc *guc)
>   {
> -	if (!guc_log_has_relay(guc))
> -		return NULL;
> -
>   	/*
>   	 * Just get the base address of a new sub buffer and copy data into it
>   	 * ourselves. NULL will be returned in no-overwrite mode, if all sub
> @@ -284,13 +263,14 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   	void *src_data, *dst_data;
>   	bool new_overflow;
>   
> +	mutex_lock(&guc->log.runtime.lock);
> +
>   	if (WARN_ON(!guc->log.runtime.buf_addr))
> -		return;
> +		goto out_unlock;
>   
>   	/* 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);
> @@ -302,9 +282,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   		 */
>   		DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
>   		guc->log.capture_miss_count++;
> -		mutex_unlock(&guc->log.runtime.relay_lock);
>   
> -		return;
> +		goto out_unlock;
>   	}
>   
>   	/* Actual logs are present from the 2nd page */
> @@ -375,7 +354,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
>   
>   	guc_move_to_next_buf(guc);
>   
> -	mutex_unlock(&guc->log.runtime.relay_lock);
> +out_unlock:
> +	mutex_unlock(&guc->log.runtime.lock);
>   }
>   
>   static void capture_logs_work(struct work_struct *work)
> @@ -391,20 +371,20 @@ 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_held(&guc->log.runtime.lock);
>   
>   	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);
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
>   	if (ret)
>   		return ret;
>   
> @@ -424,14 +404,9 @@ static int guc_log_runtime_create(struct intel_guc *guc)
>   	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);
>   
>   	i915_gem_object_unpin_map(guc->log.vma->obj);
>   	guc->log.runtime.buf_addr = NULL;
> @@ -439,7 +414,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);
>   	INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work);
>   }
>   
> @@ -450,12 +425,7 @@ static 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;
> @@ -485,12 +455,9 @@ static 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;
>   	return ret;
> @@ -498,20 +465,10 @@ static int guc_log_relay_create(struct intel_guc *guc)
>   
>   static 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)
> @@ -610,7 +567,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);
>   }
>   
> @@ -687,9 +643,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 *i915 = guc_to_i915(guc);
>   	int ret;
>   
> +	mutex_lock(&guc->log.runtime.lock);
> +
>   	GEM_BUG_ON(guc_log_has_runtime(guc));
>   
>   	/*
> @@ -701,35 +658,32 @@ int intel_guc_log_register(struct intel_guc *guc)
>   	if (ret)
>   		goto err;
>   
> -	mutex_lock(&i915->drm.struct_mutex);
> -	ret = guc_log_runtime_create(guc);
> -	mutex_unlock(&i915->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(&i915->drm.struct_mutex);
> -	guc_log_runtime_destroy(guc);
> -	mutex_unlock(&i915->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 *i915 = guc_to_i915(guc);
> -
>   	guc_log_flush_irq_disable(guc);
>   
>   	/*
> @@ -740,9 +694,12 @@ void intel_guc_log_unregister(struct intel_guc *guc)
>   	 */
>   	guc_flush_logs(guc);
>   
> -	mutex_lock(&i915->drm.struct_mutex);
> -	guc_log_runtime_destroy(guc);
> -	mutex_unlock(&i915->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 */
> -		struct mutex relay_lock;
> +		struct mutex lock;
>   	} runtime;
>   	/* logging related stats */
>   	u32 capture_miss_count;

-- 
Thanks,
Sagar



More information about the Intel-gfx mailing list