Re: [RFC 08/14] drm/i915: Store backpointer to intel_gt in the engine

From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Intel-gfx@lists.freedesktop.org,
	Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Subject: Re: [RFC 08/14] drm/i915: Store backpointer to intel_gt in the engine
Date: Tue, 11 Jun 2019 09:42:57 -0700	[thread overview]
Message-ID: <bbbb1dd5-c357-9b24-6662-61b9aebeafb4@intel.com> (raw)
In-Reply-To: <156024578887.2497.16355057690352237734@skylake-alporthouse-com>

On 6/11/2019 2:36 AM, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-11 09:41:02)
>> On 10/06/2019 19:17, Daniele Ceraolo Spurio wrote:
>>> On 6/10/19 9:16 AM, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2019-06-10 16:54:13)
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>>> index 01223864237a..343c4459e8a3 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>>> @@ -34,6 +34,7 @@ struct drm_i915_reg_table;
>>>>>    struct i915_gem_context;
>>>>>    struct i915_request;
>>>>>    struct i915_sched_attr;
>>>>> +struct intel_gt;
>>>>>    struct intel_uncore;
>>>>>    typedef u8 intel_engine_mask_t;
>>>>> @@ -266,6 +267,7 @@ struct intel_engine_execlists {
>>>>>    struct intel_engine_cs {
>>>>>           struct drm_i915_private *i915;
>>>>> +       struct intel_gt *gt;
>>>> I'd push for gt as being the backpointer, and i915 its distant grand
>>>> parent. Not sure how much pain that would bring just for the elimination
>>>> of one more drm_i915_private, but that's how I picture the
>>>> encapsulation.
>> It depends on overall direction. Are we going to go with helpers
>> (XXX_to_i915) or not. Well for removing engine->i915 there would be
>> churn already. But same churn regardless of whether we pick
>> engine_to_i915 or engine->gt->i915.
>>
>> But I don't see a problem with having both i915 and gt pointers in the
>> engine. It's a short cut to avoid pointer chasing and verbosity. Our
>> code is fundamentally still very dependent on runtime checks against
>> INTEL_GEN and INTEL_INFO, so i915 is pretty much in need all over the place.
>>
>>> Would it be worth moving some of the flags in the device_info structure
>>> in a gt substructure, like we did for display, and get a pointer to that
>>> in intel_gt? We could save some jumps back that way and be more coherent
>>> in where we store the info.
>> So even with this we maybe reduce the need to chase all the way to i915
>> a bit, but not fully. Unless we decide to duplicate gen in intel_gt as
>> well. Well.. now I am scared we will just decide to do that. :D
> Kind off, we are already reducing the runtime checks into feature flags
> or vfuncs for hot paths. I do hope the only time we need to go back to
> i915 is during init. This should be reasonably true for engine; looking
> at intel_lrc.c the common access is for i915->scratch, which we need to
> move under intel_gt. And I expect that we will see similar natural
> transitions for engine->i915.
> -Chris

There was also a mention a while back of splitting gt and display gens 
(https://patchwork.freedesktop.org/series/51860/), if we ever decide 
that that makes sense the gt gen will just naturally move and we'll save 
most of the jumps to i915.

Daniele

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx