| Message ID | 20190610155419.23723-9-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implicit dev_priv removal | expand |
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. I'm sure I'm missing a link or two :) -Chris
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. > 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. Daniele > I'm sure I'm missing a link or two :) > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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 Regards, Tvrtko
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
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
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index a046e8dccc96..d4385422e2b3 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -314,6 +314,7 @@ intel_engine_setup(struct drm_i915_private *dev_priv, engine->id = id; engine->mask = BIT(id); engine->i915 = dev_priv; + engine->gt = &dev_priv->gt; engine->uncore = &dev_priv->uncore; __sprint_engine_name(engine->name, info); engine->hw_id = engine->guc_id = info->hw_id; 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; struct intel_uncore *uncore; char name[INTEL_ENGINE_CS_MAX_NAME];