[Intel-gfx] [PATCH] drm/i915/perf: allow holding preemption on filtered ctx

[Lionel Landwerlin]

On 05/06/18 17:28, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2018-06-05 17:19:11)
>> We would like to make use of perf in Vulkan. The Vulkan API is much
>> lower level than OpenGL, with applications directly exposed to the
>> concept of command buffers (pretty much equivalent to our batch
>> buffers). In Vulkan, queries are always limited in scope to a command
>> buffer. In OpenGL, the lack of command buffer concept meant that
>> queries' duration could span multiple command buffers.
>>
>> With that restriction gone in Vulkan, we would like to simplify
>> measuring performance just by measuring the deltas between the counter
>> snapshots written by 2 MI_RECORD_PERF_COUNT commands, rather than the
>> more complex scheme with currently have in the GL driver, using 2
>> MI_RECORD_PERF_COUNT commands and doing some post processing on the
>> stream of OA reports to remove any unrelated deltas in the OA stream.
>>
>> Disabling preemption only apply to the context with which want to
>> query performance counters and is considered a privileged operation
>> (disabled by default).
> and protected by CAP_SYS_ADMIN (give or take it requiring root
> privilege to let an ordinary user set it themselves).
>
> Please make that clear in the commit log for the likes of myself who
> panicked at the mere thought of being able to stop preemption (because
> it's an outright priority inversion). Note that this will not escape a
> fast reset timer so the batch had better be short, or they will end up
> being banned.
>   

Sure, I thought people would be scared.
Adding it locally.

>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 7d63c6d2f687..417ca93ea606 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -2099,7 +2099,9 @@ static int gen8_emit_bb_start(struct i915_request *rq,
>>           *
>>           * That satisfies both the GPGPU w/a and our heavy-handed paranoia.
>>           */
>> -       *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
>> +       *cs++ = MI_ARB_ON_OFF |
> I'd prefer having it as a u32 enable/disable. Could it be intel_context
> even, i.e. selectable on engine?
>
> 	*cs++ = MI_ARB_ON_OFF | rq->hw_context->arb_enable;

Yeah, I just thought about putting it on the engine. Will do that too.
Right now it only make sense on the RCS anyway, that's where the OA unit 
works.

Thanks,

-
Lionel

> -Chris
>