[Intel-gfx] [PATCH 4/7] drm/i915/perf: lock powergating configuration to default when active

[Tvrtko Ursulin]

On 05/09/2018 16:21, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-09-05 15:22:19)
>> -static u32 make_rpcs(struct drm_i915_private *dev_priv,
>> -                    struct intel_sseu *ctx_sseu)
>> +u32 gen8_make_rpcs(struct drm_i915_private *dev_priv,
>> +                  struct intel_sseu *req_sseu)
> 
> Should we retrospectively make this const?

Can do, but generally I try to avoid it kernel code since most of the 
time it is way more pain than benefit.

> (And anychance for a s/dev_priv/i915?)

Will check if it is doable without much noise at any of the stages.

>>   {
>>          const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>>          bool subslice_pg = sseu->has_subslice_pg;
>> -       u8 slices = hweight8(ctx_sseu->slice_mask);
>> -       u8 subslices = hweight8(ctx_sseu->subslice_mask);
>> +       struct intel_sseu ctx_sseu;
>> +       u8 slices, subslices;
>>          u32 rpcs = 0;
>>   
>> +       /*
>> +        * If i915/perf is active, we want a stable powergating configuration
>> +        * on the system. The most natural configuration to take in that case
>> +        * is the default (i.e maximum the hardware can do).
>> +        */
>> +       if (unlikely(dev_priv->perf.oa.exclusive_stream))
>> +               ctx_sseu = intel_device_default_sseu(dev_priv);
>> +       else
>> +               ctx_sseu = *req_sseu;
> 
> :(
> 
> I'm not sure if I can suggest anything better, but this does feel like a
> layering violation.
> 
> It makes sense which makes it only feel worse.

It used to be a helper which applied the adjustment but I wasn't happy 
with how callers then had to know to call the helper and decided 
handling it at the core is better in more than one way.

I think bottom line is there is fundamental interaction between the two 
so some layering violation has to happen.

Regards,

Tvrtko