Re: [PATCH 3/7] drm/i915: Record the sseu configuration per-context & engine

Tvrtko Ursulin
2018-09-06T09:36:45Z

On 05/09/2018 16:18, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-09-05 15:22:18)
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> We want to expose the ability to reconfigure the slices, subslice and
>> eu per context and per engine. To facilitate that, store the current
>> configuration on the context for each engine, which is initially set
>> to the device default upon creation.
>>
>> v2: record sseu configuration per context & engine (Chris)
>>
>> v3: introduce the i915_gem_context_sseu to store powergating
>> programming, sseu_dev_info has grown quite a bit (Lionel)
>>
>> v4: rename i915_gem_sseu into intel_sseu (Chris)
>> use to_intel_context() (Chris)
>>
>> v5: More to_intel_context() (Tvrtko)
>> Switch intel_sseu from union to struct (Tvrtko)
>> Move context default sseu in existing loop (Chris)
>>
>> v6: s/intel_sseu_from_device_sseu/intel_device_default_sseu/ (Tvrtko)
>>
>> Tvrtko Ursulin:
>>
>> v7:
>> * Pass intel_sseu by pointer instead of value to make_rpcs.
>> * Rebase for make_rpcs changes.
>>
>> v8:
>> * Rebase for RPCS edit on pin.
>>
>> v9:
>> * Rebase for context image setup changes.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> I feel this is substantially different (since I just outlined a v1!) to
> merit a
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> and probably deserves a different author. I think Lionel is still the
> principle author here, but Tvrtko has done a lot of refactoring and
> integrating in the new scheme.

Agreed Lionel is the real author here - mine were just small tweaks.

>> -static u32 make_rpcs(struct drm_i915_private *dev_priv);
>> +static u32 make_rpcs(struct drm_i915_private *dev_priv,
>> + struct intel_sseu *ctx_sseu);
>>
>> static struct intel_context *
>> __execlists_context_pin(struct intel_engine_cs *engine,
>> @@ -1349,7 +1350,7 @@ __execlists_context_pin(struct intel_engine_cs *engine,
>> /* RPCS */
>> if (engine->class == RENDER_CLASS) {
>> ce->lrc_reg_state[CTX_R_PWR_CLK_STATE + 1] =
>> - make_rpcs(engine->i915);
>> + make_rpcs(engine->i915, &ce->sseu);
>
> We have different habits here; my vim config just gives this a single
> tab indent beyond the incomplete line. (Was going to say it earlier ;)

Not sure if my approach here is always consistent, but I *think* I first
try to indent it to where it "looks good". If neither indentation looks
decidedly better, then I push it so end aligns with the wrap marker. I
in this particular case I wasn't too happy with any of the options. :(

Regards,

Tvrtko