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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx