[Intel-gfx] [PATCH] drm/i915/perf: introduce global sseu pinning

[Lionel Landwerlin]

On 28/02/2020 18:44, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2020-02-28 16:02:29)
>> On Gen11 powergating half the execution units is a functional
>> requirement when using the VME samplers. Not fullfilling this
>> requirement can lead to hangs.
>>
>> This unfortunately plays fairly poorly with the NOA requirements. NOA
>> requires a stable power configuration to maintain its configuration.
>>
>> As a result using OA (and NOA feeding into it) so far has required us
>> to use a power configuration that can work for all contexts. The only
>> power configuration fullfilling this is powergating half the execution
>> units.
>>
>> This makes performance analysis for 3D workloads somewhat pointless.
>>
>> Failing to find a solution that would work for everybody, this change
>> introduces a new i915-perf stream open parameter that punts the
>> decision off to userspace. If this parameter is omitted, the existing
>> Gen11 behavior remains (half EU array powergating).
>>
>> This change takes the initiative to move all perf related sseu
>> configuration into i915_perf.c
> The code looks fine, your argument is sound. My only reservation is the
> danger of this becoming the defacto default and so catching users's
> profiling their system by surprise.


As far as I can see, this will only be using non default (default = full 
EU array on !gen11, default = half EU array on gen11) on Gen11.

Except Gen11 we don't have those asymetric subslices (remember gen12 has 
dual slices of 16 EUs).


It all bad choices :

    - let everybody do what they want but risk invalid OA data with no 
warning

    - force everybody to the same config and only on gen11 risk a hang 
if VME samplers end up being used (which is a subset of all media workloads)


>
>> @@ -3628,6 +3678,16 @@ static int read_properties_unlocked(struct i915_perf *perf,
>>                  case DRM_I915_PERF_PROP_HOLD_PREEMPTION:
>>                          props->hold_preemption = !!value;
>>                          break;
>> +               case DRM_I915_PERF_PROP_GLOBAL_SSEU: {
>> +                       if (copy_from_user(&props->user_sseu,
>> +                                          u64_to_user_ptr(value),
>> +                                          sizeof(props->user_sseu))) {
>> +                               DRM_DEBUG("Unable to copy global sseu parameter\n");
>> +                               return -EFAULT;
>> +                       }
> Since this affects system state for other users, I would suggest this
> has a privilege check
>
>> +                       props->user_sseu_present = true;
>> +                       break;
> i915_perf_ioctl_open_locked:
> 	if (props->user_sseu_present && IS_GEN(11))
> 		privileged_op = true;
> ?


I'm going to go with priviliged for all gens except if user request == 
default.


Thanks a lot,


-Lionel


> -Chris