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.goel@xxxxxxxxx wrote:
From: Akash Goel <akash.goel@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
