[Intel-gfx] [PATCH 17/17] drm/i915: Use rt priority kthread to do GuC log buffer sampling

[Goel, Akash]

On 7/21/2016 11:13 AM, Chris Wilson wrote:
> On Thu, Jul 21, 2016 at 09:11:42AM +0530, Goel, Akash wrote:
>>
>>
>> On 7/21/2016 1:04 AM, Chris Wilson wrote:
>>> In the end, just the silly locking and placement of complete_all() is
>>> dangerous. reinit_completion() lacks the barrier to be used like this
>>> really, at any rate, racy with the irq handler, so use sparingly or when
>>> you control the irq handler.
>> Sorry I forgot to add a comment that
>> guc_cancel_log_flush_work_sync() should be invoked only after
>> ensuring that there will be no more flush interrupts, which will
>> happen either by explicitly disabling the interrupt or disabling the
>> logging and that's what is done at the 2 call sites.
>>
>> Since had covered reinit_completion() under the irq_lock, thought an
>> explicit barrier is not needed.
>
> You hadn't controlled everything via the irq_lock, and nor should you.
>
>>
>> 	spin_lock_irq(&dev_priv->irq_lock);
>> 	if (guc->log.flush_signal) {
>> 		guc->log.flush_signal = false;
>> 		reinit_completion(&guc->log.flush_completion);
>> 		spin_unlock_irq(&dev_priv->irq_lock);
>> 		i915_guc_capture_logs(&dev_priv->drm);
>> 	 	complete_all(&guc->log.flush_completion);
>>
>> The placement of complete_all isn't right for the case, where
>> a guc_cancel_log_flush_work_sync() is called but there was no prior
>> flush interrupt received.
>
> Exactly.
>
>>> (Also not sure if log.signal = 0 is sane,
>> Did log.signal = 0 for fast cancellation. Will remove that.
>>
>> A smp_wmb() after reinit_completion(&flush_completion) would be fine ?
>
> Don't worry, the race can only be controlled by controlling the irq.


> In the end, I think something more like
>
> 	while (signal) ...
>
> 	complete_all();
> 	schedule();
> 	reinit_completion();
>
> is the simplest.
Thanks much, so will have the task body like this.
do {
	set_current_state(TASK_INT);
	while (cmpxchg(&log.signal, 1, 0)) {
		i915_guc_capture_logs();
	};
	complete_all(log.complete);
	if (kthread_should_stop())
		break;
	schedule();
	reinit_completion();
} while(1);

>
>>> or the current callsites really require the flush.)
>>
>> Sync against a ongoing/pending flush is being done for the 2
>> forceful flush cases, which will be effective only if the pending
>> flush is completed, so forceful flush should be serialized with a
>> pending flush.
>
> Or you just signal=true, wakeup task, wait_timeout. Otherwise you
> haven't really serialized anything without disabling the interrupt.
Agree without disabling the interrupt, serialization cannot be provided,

For the sync can use,
{
	WARN_ON(guc->interrupts_enabled);
	wait_for_completion_interruptible_timeout(
			guc->log.complete, 5 /* in jiffies*/);
}


Best regards
Akash
> -Chris
>