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

Tvrtko Ursulin
2018-09-06T09:41:10Z

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