Re: [PATCH v5] drm/i915/icl: Preempt-to-idle support in execlists.

["Lis, Tomasz" <tomasz.lis@intel.com>]

On 2018-10-23 11:13, Joonas Lahtinen wrote:
> Quoting Lis, Tomasz (2018-10-19 19:00:15)
>>
>> On 2018-10-16 12:53, Joonas Lahtinen wrote:
>>> Quoting Tomasz Lis (2018-10-15 20:29:18)
>>>> The patch adds support of preempt-to-idle requesting by setting a proper
>>>> bit within Execlist Control Register, and receiving preemption result from
>>>> Context Status Buffer.
>>>>
>>>> Preemption in previous gens required a special batch buffer to be executed,
>>>> so the Command Streamer never preempted to idle directly. In Icelake it is
>>>> possible, as there is a hardware mechanism to inform the kernel about
>>>> status of the preemption request.
>>>>
>>>> This patch does not cover using the new preemption mechanism when GuC is
>>>> active.
>>>>
>>>> v2: Added needs_preempt_context() change so that it is not created when
>>>>       preempt-to-idle is supported. (Chris)
>>>>       Updated setting HWACK flag so that it is cleared after
>>>>       preempt-to-dle. (Chris, Daniele)
>>>>       Updated to use I915_ENGINE_HAS_PREEMPTION flag. (Chris)
>>>>
>>>> v3: Fixed needs_preempt_context() change. (Chris)
>>>>       Merged preemption trigger functions to one. (Chris)
>>>>       Fixed conyext state tonot assume COMPLETED_MASK after preemption,
>>>>       since idle-to-idle case will not have it set.
>>>>
>>>> v4: Simplified needs_preempt_context() change. (Daniele)
>>>>       Removed clearing HWACK flag in idle-to-idle preempt. (Daniele)
>>>>
>>>> v5: Renamed inject_preempt_context(). (Daniele)
>>>>       Removed duplicated GEM_BUG_ON() on HWACK (Daniele)
>>>>
>>>> Bspec: 18922
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: Michal Winiarski <michal.winiarski@intel.com>
>>>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> This R-b was on v4, and should be indicated with # v4 comment.
>>>
>>> The commit message doesn't say much about why preempting to idle is
>>> beneficial? The pre-Gen11 codepath needs to be maintained anyway.
>>>
>>> Regards, Joonas
>> The benefit is one less context switch - there is no "preempt context".
> Yes.
>
> But that still doesn't quite explain what material benefits there are? :)
>
> Is there some actual workloads/microbenchmarks that get an improvement?
>
> This alters the behavior between different platforms for a very delicate
> feature, probably resulting in slightly different bugs. So there should
> be some more reasoning than just because we can.
>
> Regards, Joonas
Less context switching does imply perf improvement, though it would 
require measurement - it might be hardly detectable. We may even lose 
performance - without measurements, we don't know. So not a strong argument.

One more benefit I could think of is - GuC path will use 
preempt-to-idle, so this would make execlists use the same path as GuC. 
But that's not a strong argument as well.

I must agree - there doesn't seem to be any strong enough reason to go 
with this change.
We might consider it after we have performance data.

-Tomasz

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx