On 3/5/2018 7:44 PM, Michał Winiarski wrote:
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]
<snip>
How strongly do you feel about this one?
I wanted to tidy first (decouple things), rename later.
fine. it's ok.
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.
Yes. I was saying that if we just define new lock without comment,
checkpatch complains but
if we remove comment of earlier defined lock it doesn't :)
-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
--
Thanks,
Sagar
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
