Re: [Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions
On 18/07/16 12:31, Goel, Akash wrote: On 7/18/2016 4:48 PM, Tvrtko Ursulin wrote: On 18/07/16 11:46, Goel, Akash wrote: On 7/18/2016 3:42 PM, Tvrtko Ursulin wrote: On 15/07/16 16:51, Goel, Akash wrote: On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: On 10/07/16 14:41, akash.g...@intel.com wrote: From: Akash Goel With the addition of new Host2GuC actions related to GuC logging, there is a need of a lock to serialize them, as they can execute concurrently with each other and also with other existing actions. After which patch in this series is this required? From patch 6 or 7 saw the problem, when enabled flush interrupts from boot (guc_log_level >= 0). That means this patch should come before 6 or 7. :) Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & UK_LOG_ENABLE_LOGGING can execute concurrently with each other. Right I see, from the worker/thread vs debugfs activity. Will use mutex to serialize and place the patch earlier in the series. Please suggest which would be better, mutex_lock() or mutex_lock_interruptible(). Interruptible from the debugfs paths, otherwise not. Yes calls from debugfs path should ideally use interruptible version, but then how to determine that whether the given host2guc_action call came from debugfs path. Should I add a new argument 'interruptible_wait' to host2guc_action() or to keep things simple use mutex_lock() only ? I thought it would be cleaner to abstract the lock usage, for serialization, entirely inside the host2guc_action only. Hm yes, good point. I think a simple mutex lock should be fine then. You don't expect to be holding it for more than 10ms in the absolutely worst case and I think simplicity of the implementation wins in this case even if it stalls userspace a bit. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions
On 7/18/2016 4:48 PM, Tvrtko Ursulin wrote: On 18/07/16 11:46, Goel, Akash wrote: On 7/18/2016 3:42 PM, Tvrtko Ursulin wrote: On 15/07/16 16:51, Goel, Akash wrote: On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: On 10/07/16 14:41, akash.g...@intel.com wrote: From: Akash Goel With the addition of new Host2GuC actions related to GuC logging, there is a need of a lock to serialize them, as they can execute concurrently with each other and also with other existing actions. After which patch in this series is this required? From patch 6 or 7 saw the problem, when enabled flush interrupts from boot (guc_log_level >= 0). That means this patch should come before 6 or 7. :) Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & UK_LOG_ENABLE_LOGGING can execute concurrently with each other. Right I see, from the worker/thread vs debugfs activity. Will use mutex to serialize and place the patch earlier in the series. Please suggest which would be better, mutex_lock() or mutex_lock_interruptible(). Interruptible from the debugfs paths, otherwise not. Yes calls from debugfs path should ideally use interruptible version, but then how to determine that whether the given host2guc_action call came from debugfs path. Should I add a new argument 'interruptible_wait' to host2guc_action() or to keep things simple use mutex_lock() only ? I thought it would be cleaner to abstract the lock usage, for serialization, entirely inside the host2guc_action only. --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -88,6 +88,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len) return -EINVAL; intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); +spin_lock(&guc->action_lock); The code below can sleep waiting for a response from GuC so you cannot use a spinlock. Mutex I suppose... Sorry I missed the sleep. Probably I did not see any problem, in spite of a spinlock, as _wait_for macro does not sleep when used in atomic context, does a busy wait instead. I wonder about that in general, since in_atomic is not a reliable indicator. But that is beside the point. You probably haven't seen it because the action completes in the first shorter, atomic sleep, check. Actually I had profiled host2guc_logbuffer_flush_complete() and saw that on some occasions it was taking more than 100 micro seconds, so presumably it would have went past the first wait. But most of the times it was less than 10 micro seconds only. ret = wait_for_us(host2guc_action_response(dev_priv, &status), 10); if (ret) ret = wait_for(host2guc_action_response(dev_priv, &status), 10); Yes presumably so. In that case keep in mind that in_atomic always returns false in spinlock sections unless the kernel has CONFIG_PREEMPT_COUNT enabled. Thanks for this info, will be mindful of this in future. Best regards Akash Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions
On 18/07/16 11:46, Goel, Akash wrote: On 7/18/2016 3:42 PM, Tvrtko Ursulin wrote: On 15/07/16 16:51, Goel, Akash wrote: On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: On 10/07/16 14:41, akash.g...@intel.com wrote: From: Akash Goel With the addition of new Host2GuC actions related to GuC logging, there is a need of a lock to serialize them, as they can execute concurrently with each other and also with other existing actions. After which patch in this series is this required? From patch 6 or 7 saw the problem, when enabled flush interrupts from boot (guc_log_level >= 0). That means this patch should come before 6 or 7. :) Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & UK_LOG_ENABLE_LOGGING can execute concurrently with each other. Right I see, from the worker/thread vs debugfs activity. Will use mutex to serialize and place the patch earlier in the series. Please suggest which would be better, mutex_lock() or mutex_lock_interruptible(). Interruptible from the debugfs paths, otherwise not. Signed-off-by: Akash Goel --- drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++ drivers/gpu/drm/i915/intel_guc.h | 3 +++ 2 files changed, 6 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 6043166..c1e637f 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -88,6 +88,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len) return -EINVAL; intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); +spin_lock(&guc->action_lock); The code below can sleep waiting for a response from GuC so you cannot use a spinlock. Mutex I suppose... Sorry I missed the sleep. Probably I did not see any problem, in spite of a spinlock, as _wait_for macro does not sleep when used in atomic context, does a busy wait instead. I wonder about that in general, since in_atomic is not a reliable indicator. But that is beside the point. You probably haven't seen it because the action completes in the first shorter, atomic sleep, check. Actually I had profiled host2guc_logbuffer_flush_complete() and saw that on some occasions it was taking more than 100 micro seconds, so presumably it would have went past the first wait. But most of the times it was less than 10 micro seconds only. ret = wait_for_us(host2guc_action_response(dev_priv, &status), 10); if (ret) ret = wait_for(host2guc_action_response(dev_priv, &status), 10); Yes presumably so. In that case keep in mind that in_atomic always returns false in spinlock sections unless the kernel has CONFIG_PREEMPT_COUNT enabled. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions
On 7/18/2016 3:42 PM, Tvrtko Ursulin wrote: On 15/07/16 16:51, Goel, Akash wrote: On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: On 10/07/16 14:41, akash.g...@intel.com wrote: From: Akash Goel With the addition of new Host2GuC actions related to GuC logging, there is a need of a lock to serialize them, as they can execute concurrently with each other and also with other existing actions. After which patch in this series is this required? From patch 6 or 7 saw the problem, when enabled flush interrupts from boot (guc_log_level >= 0). That means this patch should come before 6 or 7. :) Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & UK_LOG_ENABLE_LOGGING can execute concurrently with each other. Right I see, from the worker/thread vs debugfs activity. Will use mutex to serialize and place the patch earlier in the series. Please suggest which would be better, mutex_lock() or mutex_lock_interruptible(). Signed-off-by: Akash Goel --- drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++ drivers/gpu/drm/i915/intel_guc.h | 3 +++ 2 files changed, 6 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 6043166..c1e637f 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -88,6 +88,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len) return -EINVAL; intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); +spin_lock(&guc->action_lock); The code below can sleep waiting for a response from GuC so you cannot use a spinlock. Mutex I suppose... Sorry I missed the sleep. Probably I did not see any problem, in spite of a spinlock, as _wait_for macro does not sleep when used in atomic context, does a busy wait instead. I wonder about that in general, since in_atomic is not a reliable indicator. But that is beside the point. You probably haven't seen it because the action completes in the first shorter, atomic sleep, check. Actually I had profiled host2guc_logbuffer_flush_complete() and saw that on some occasions it was taking more than 100 micro seconds, so presumably it would have went past the first wait. But most of the times it was less than 10 micro seconds only. ret = wait_for_us(host2guc_action_response(dev_priv, &status), 10); if (ret) ret = wait_for(host2guc_action_response(dev_priv, &status), 10); Best regards Akash Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions
On 15/07/16 16:51, Goel, Akash wrote: On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote: On 10/07/16 14:41, akash.g...@intel.com wrote: From: Akash Goel With the addition of new Host2GuC actions related to GuC logging, there is a need of a lock to serialize them, as they can execute concurrently with each other and also with other existing actions. After which patch in this series is this required? From patch 6 or 7 saw the problem, when enabled flush interrupts from boot (guc_log_level >= 0). That means this patch should come before 6 or 7. :) Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE & UK_LOG_ENABLE_LOGGING can execute concurrently with each other. Right I see, from the worker/thread vs debugfs activity. Signed-off-by: Akash Goel --- drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++ drivers/gpu/drm/i915/intel_guc.h | 3 +++ 2 files changed, 6 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 6043166..c1e637f 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -88,6 +88,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data, u32 len) return -EINVAL; intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); +spin_lock(&guc->action_lock); The code below can sleep waiting for a response from GuC so you cannot use a spinlock. Mutex I suppose... Sorry I missed the sleep. Probably I did not see any problem, in spite of a spinlock, as _wait_for macro does not sleep when used in atomic context, does a busy wait instead. I wonder about that in general, since in_atomic is not a reliable indicator. But that is beside the point. You probably haven't seen it because the action completes in the first shorter, atomic sleep, check. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions
On 7/15/2016 5:10 PM, Tvrtko Ursulin wrote:
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Akash Goel
With the addition of new Host2GuC actions related to GuC logging, there
is a need of a lock to serialize them, as they can execute concurrently
with each other and also with other existing actions.
After which patch in this series is this required?
From patch 6 or 7 saw the problem, when enabled flush interrupts from
boot (guc_log_level >= 0).
Also new HOST2GUC actions LOG_BUFFER_FILE_FLUSH_COMPLETE &
UK_LOG_ENABLE_LOGGING can execute concurrently with each other.
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++
drivers/gpu/drm/i915/intel_guc.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6043166..c1e637f 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -88,6 +88,7 @@ static int host2guc_action(struct intel_guc *guc,
u32 *data, u32 len)
return -EINVAL;
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+spin_lock(&guc->action_lock);
The code below can sleep waiting for a response from GuC so you cannot
use a spinlock. Mutex I suppose...
Sorry I missed the sleep.
Probably I did not see any problem, in spite of a spinlock, as _wait_for
macro does not sleep when used in atomic context, does a busy wait instead.
Best Regards
Akash
dev_priv->guc.action_count += 1;
dev_priv->guc.action_cmd = data[0];
@@ -126,6 +127,7 @@ static int host2guc_action(struct intel_guc *guc,
u32 *data, u32 len)
}
dev_priv->guc.action_status = status;
+spin_unlock(&guc->action_lock);
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
return ret;
@@ -1304,6 +1306,7 @@ int i915_guc_submission_init(struct
drm_i915_private *dev_priv)
return -ENOMEM;
ida_init(&guc->ctx_ids);
+spin_lock_init(&guc->action_lock);
I think this should go to guc_client_alloc which is where the guc client
object is allocated and initialized.
guc_create_log(guc);
guc_create_ads(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc.h
b/drivers/gpu/drm/i915/intel_guc.h
index d56bde6..611f4a7 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -157,6 +157,9 @@ struct intel_guc {
uint64_t submissions[I915_NUM_ENGINES];
uint32_t last_seqno[I915_NUM_ENGINES];
+
+/* To serialize the Host2GuC actions */
+spinlock_t action_lock;
};
/* intel_guc_loader.c */
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions
On 10/07/16 14:41, akash.g...@intel.com wrote:
From: Akash Goel
With the addition of new Host2GuC actions related to GuC logging, there
is a need of a lock to serialize them, as they can execute concurrently
with each other and also with other existing actions.
After which patch in this series is this required?
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++
drivers/gpu/drm/i915/intel_guc.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6043166..c1e637f 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -88,6 +88,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data,
u32 len)
return -EINVAL;
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+ spin_lock(&guc->action_lock);
The code below can sleep waiting for a response from GuC so you cannot
use a spinlock. Mutex I suppose...
dev_priv->guc.action_count += 1;
dev_priv->guc.action_cmd = data[0];
@@ -126,6 +127,7 @@ static int host2guc_action(struct intel_guc *guc, u32
*data, u32 len)
}
dev_priv->guc.action_status = status;
+ spin_unlock(&guc->action_lock);
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
return ret;
@@ -1304,6 +1306,7 @@ int i915_guc_submission_init(struct drm_i915_private
*dev_priv)
return -ENOMEM;
ida_init(&guc->ctx_ids);
+ spin_lock_init(&guc->action_lock);
I think this should go to guc_client_alloc which is where the guc client
object is allocated and initialized.
guc_create_log(guc);
guc_create_ads(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index d56bde6..611f4a7 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -157,6 +157,9 @@ struct intel_guc {
uint64_t submissions[I915_NUM_ENGINES];
uint32_t last_seqno[I915_NUM_ENGINES];
+
+ /* To serialize the Host2GuC actions */
+ spinlock_t action_lock;
};
/* intel_guc_loader.c */
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 13/17] drm/i915: New lock to serialize the Host2GuC actions
From: Akash Goel
With the addition of new Host2GuC actions related to GuC logging, there
is a need of a lock to serialize them, as they can execute concurrently
with each other and also with other existing actions.
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_guc_submission.c | 3 +++
drivers/gpu/drm/i915/intel_guc.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 6043166..c1e637f 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -88,6 +88,7 @@ static int host2guc_action(struct intel_guc *guc, u32 *data,
u32 len)
return -EINVAL;
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+ spin_lock(&guc->action_lock);
dev_priv->guc.action_count += 1;
dev_priv->guc.action_cmd = data[0];
@@ -126,6 +127,7 @@ static int host2guc_action(struct intel_guc *guc, u32
*data, u32 len)
}
dev_priv->guc.action_status = status;
+ spin_unlock(&guc->action_lock);
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
return ret;
@@ -1304,6 +1306,7 @@ int i915_guc_submission_init(struct drm_i915_private
*dev_priv)
return -ENOMEM;
ida_init(&guc->ctx_ids);
+ spin_lock_init(&guc->action_lock);
guc_create_log(guc);
guc_create_ads(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index d56bde6..611f4a7 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -157,6 +157,9 @@ struct intel_guc {
uint64_t submissions[I915_NUM_ENGINES];
uint32_t last_seqno[I915_NUM_ENGINES];
+
+ /* To serialize the Host2GuC actions */
+ spinlock_t action_lock;
};
/* intel_guc_loader.c */
--
1.9.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
