[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