| Message ID | 20190613133539.12620-14-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implicit dev_priv removal and GT compartmentalization | expand |
Quoting Tvrtko Ursulin (2019-06-13 14:35:24) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > More removal of implicit dev_priv from using old mmio accessors. > > Actually the top level function remains but is split into a part which > writes to i915 and part which operates on intel_gt in order to initialize > the hardware. > > GuC and engines are the only odd ones out remaining. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 66 ++++++++++++++++++++------------- > 1 file changed, 40 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index e54cd30534dc..b6f450e782e7 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1234,28 +1234,32 @@ static void init_unused_rings(struct intel_gt *gt) > } > } > > -int i915_gem_init_hw(struct drm_i915_private *dev_priv) > +static int init_hw(struct intel_gt *gt) > { > + struct drm_i915_private *i915 = gt->i915; > + struct intel_uncore *uncore = gt->uncore; > int ret; > > - dev_priv->gt.last_init_time = ktime_get(); > + gt->last_init_time = ktime_get(); > > /* Double layer security blanket, see i915_gem_init() */ > - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL); > + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); > > - if (HAS_EDRAM(dev_priv) && INTEL_GEN(dev_priv) < 9) > - I915_WRITE(HSW_IDICR, I915_READ(HSW_IDICR) | IDIHASHMSK(0xf)); > + if (HAS_EDRAM(i915) && INTEL_GEN(i915) < 9) > + intel_uncore_rmw(uncore, HSW_IDICR, 0, IDIHASHMSK(0xf)); > > - if (IS_HASWELL(dev_priv)) > - I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ? > - LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); > + if (IS_HASWELL(i915)) > + intel_uncore_write(uncore, > + MI_PREDICATE_RESULT_2, > + IS_HSW_GT3(i915) ? > + LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); > > /* Apply the GT workarounds... */ > - intel_gt_apply_workarounds(&dev_priv->gt); > + intel_gt_apply_workarounds(gt); Would it be worth moving the above mmio into workarounds? Whilst you are doing some spring cleaning :) > /* ...and determine whether they are sticking. */ > - intel_gt_verify_workarounds(&dev_priv->gt, "init"); > + intel_gt_verify_workarounds(gt, "init"); > > - intel_gt_init_swizzling(&dev_priv->gt); > + intel_gt_init_swizzling(gt); > > /* > * At least 830 can leave some of the unused rings > @@ -1263,48 +1267,58 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv) > * will prevent c3 entry. Makes sure all unused rings > * are totally idle. > */ > - init_unused_rings(&dev_priv->gt); > - > - BUG_ON(!dev_priv->kernel_context); > - ret = i915_terminally_wedged(dev_priv); > - if (ret) > - goto out; > + init_unused_rings(gt); > > - ret = i915_ppgtt_init_hw(&dev_priv->gt); > + ret = i915_ppgtt_init_hw(gt); > if (ret) { > DRM_ERROR("Enabling PPGTT failed (%d)\n", ret); > goto out; > } > > - ret = intel_wopcm_init_hw(&dev_priv->wopcm); > + ret = intel_wopcm_init_hw(&i915->wopcm); > if (ret) { > DRM_ERROR("Enabling WOPCM failed (%d)\n", ret); > goto out; > } > > /* We can't enable contexts until all firmware is loaded */ > - ret = intel_uc_init_hw(dev_priv); > + ret = intel_uc_init_hw(i915); Sorting out the uc layering is an ongoing task. I think it probably means our init_hw needs splitting. > if (ret) { > DRM_ERROR("Enabling uc failed (%d)\n", ret); > goto out; > } > > - intel_mocs_init_l3cc_table(&dev_priv->gt); > + intel_mocs_init_l3cc_table(gt); > > /* Only when the HW is re-initialised, can we replay the requests */ > - ret = intel_engines_resume(dev_priv); > + ret = intel_engines_resume(i915); > if (ret) > goto cleanup_uc; > > - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); > + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); > > - intel_engines_set_scheduler_caps(dev_priv); > return 0; > > cleanup_uc: > - intel_uc_fini_hw(dev_priv); > + intel_uc_fini_hw(i915); > out: > - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); > + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); > + > + return ret; > +} > + > +int i915_gem_init_hw(struct drm_i915_private *i915) Do we also start to recognise this as i915_init_hw()? This is the driver talking to the intel_gt and friends, not the driver setting up the GEM api. -Chris
On 13/06/2019 14:59, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-06-13 14:35:24) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> More removal of implicit dev_priv from using old mmio accessors. >> >> Actually the top level function remains but is split into a part which >> writes to i915 and part which operates on intel_gt in order to initialize >> the hardware. >> >> GuC and engines are the only odd ones out remaining. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem.c | 66 ++++++++++++++++++++------------- >> 1 file changed, 40 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index e54cd30534dc..b6f450e782e7 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -1234,28 +1234,32 @@ static void init_unused_rings(struct intel_gt *gt) >> } >> } >> >> -int i915_gem_init_hw(struct drm_i915_private *dev_priv) >> +static int init_hw(struct intel_gt *gt) >> { >> + struct drm_i915_private *i915 = gt->i915; >> + struct intel_uncore *uncore = gt->uncore; >> int ret; >> >> - dev_priv->gt.last_init_time = ktime_get(); >> + gt->last_init_time = ktime_get(); >> >> /* Double layer security blanket, see i915_gem_init() */ >> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL); >> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); >> >> - if (HAS_EDRAM(dev_priv) && INTEL_GEN(dev_priv) < 9) >> - I915_WRITE(HSW_IDICR, I915_READ(HSW_IDICR) | IDIHASHMSK(0xf)); >> + if (HAS_EDRAM(i915) && INTEL_GEN(i915) < 9) >> + intel_uncore_rmw(uncore, HSW_IDICR, 0, IDIHASHMSK(0xf)); >> >> - if (IS_HASWELL(dev_priv)) >> - I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ? >> - LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); >> + if (IS_HASWELL(i915)) >> + intel_uncore_write(uncore, >> + MI_PREDICATE_RESULT_2, >> + IS_HSW_GT3(i915) ? >> + LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); >> >> /* Apply the GT workarounds... */ >> - intel_gt_apply_workarounds(&dev_priv->gt); >> + intel_gt_apply_workarounds(gt); > > Would it be worth moving the above mmio into workarounds? Whilst you are > doing some spring cleaning :) To GT workarounds? Are the above two workarounds? Do they have an official designation? >> /* ...and determine whether they are sticking. */ >> - intel_gt_verify_workarounds(&dev_priv->gt, "init"); >> + intel_gt_verify_workarounds(gt, "init"); >> >> - intel_gt_init_swizzling(&dev_priv->gt); >> + intel_gt_init_swizzling(gt); >> >> /* >> * At least 830 can leave some of the unused rings >> @@ -1263,48 +1267,58 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv) >> * will prevent c3 entry. Makes sure all unused rings >> * are totally idle. >> */ >> - init_unused_rings(&dev_priv->gt); >> - >> - BUG_ON(!dev_priv->kernel_context); >> - ret = i915_terminally_wedged(dev_priv); >> - if (ret) >> - goto out; >> + init_unused_rings(gt); >> >> - ret = i915_ppgtt_init_hw(&dev_priv->gt); >> + ret = i915_ppgtt_init_hw(gt); >> if (ret) { >> DRM_ERROR("Enabling PPGTT failed (%d)\n", ret); >> goto out; >> } >> >> - ret = intel_wopcm_init_hw(&dev_priv->wopcm); >> + ret = intel_wopcm_init_hw(&i915->wopcm); >> if (ret) { >> DRM_ERROR("Enabling WOPCM failed (%d)\n", ret); >> goto out; >> } >> >> /* We can't enable contexts until all firmware is loaded */ >> - ret = intel_uc_init_hw(dev_priv); >> + ret = intel_uc_init_hw(i915); > > Sorting out the uc layering is an ongoing task. I think it probably > means our init_hw needs splitting. I think guc and huc could be made children of intel_gt so this could be changed to take gt. It's a lot of code which I am not sure has much yet to live so I opted not to touch it. > >> if (ret) { >> DRM_ERROR("Enabling uc failed (%d)\n", ret); >> goto out; >> } >> >> - intel_mocs_init_l3cc_table(&dev_priv->gt); >> + intel_mocs_init_l3cc_table(gt); >> >> /* Only when the HW is re-initialised, can we replay the requests */ >> - ret = intel_engines_resume(dev_priv); >> + ret = intel_engines_resume(i915); >> if (ret) >> goto cleanup_uc; >> >> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); >> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); >> >> - intel_engines_set_scheduler_caps(dev_priv); >> return 0; >> >> cleanup_uc: >> - intel_uc_fini_hw(dev_priv); >> + intel_uc_fini_hw(i915); >> out: >> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); >> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); >> + >> + return ret; >> +} >> + >> +int i915_gem_init_hw(struct drm_i915_private *i915) > > Do we also start to recognise this as i915_init_hw()? This is the driver > talking to the intel_gt and friends, not the driver setting up the GEM > api. Not sure. There are some GEM bits inside like wedged status and scheduler caps. So it sounds passable to leave it like it is for now. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-06-13 17:11:43) > > On 13/06/2019 14:59, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-06-13 14:35:24) > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> More removal of implicit dev_priv from using old mmio accessors. > >> > >> Actually the top level function remains but is split into a part which > >> writes to i915 and part which operates on intel_gt in order to initialize > >> the hardware. > >> > >> GuC and engines are the only odd ones out remaining. > >> > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_gem.c | 66 ++++++++++++++++++++------------- > >> 1 file changed, 40 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >> index e54cd30534dc..b6f450e782e7 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem.c > >> +++ b/drivers/gpu/drm/i915/i915_gem.c > >> @@ -1234,28 +1234,32 @@ static void init_unused_rings(struct intel_gt *gt) > >> } > >> } > >> > >> -int i915_gem_init_hw(struct drm_i915_private *dev_priv) > >> +static int init_hw(struct intel_gt *gt) > >> { > >> + struct drm_i915_private *i915 = gt->i915; > >> + struct intel_uncore *uncore = gt->uncore; > >> int ret; > >> > >> - dev_priv->gt.last_init_time = ktime_get(); > >> + gt->last_init_time = ktime_get(); > >> > >> /* Double layer security blanket, see i915_gem_init() */ > >> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL); > >> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); > >> > >> - if (HAS_EDRAM(dev_priv) && INTEL_GEN(dev_priv) < 9) > >> - I915_WRITE(HSW_IDICR, I915_READ(HSW_IDICR) | IDIHASHMSK(0xf)); > >> + if (HAS_EDRAM(i915) && INTEL_GEN(i915) < 9) > >> + intel_uncore_rmw(uncore, HSW_IDICR, 0, IDIHASHMSK(0xf)); > >> > >> - if (IS_HASWELL(dev_priv)) > >> - I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ? > >> - LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); > >> + if (IS_HASWELL(i915)) > >> + intel_uncore_write(uncore, > >> + MI_PREDICATE_RESULT_2, > >> + IS_HSW_GT3(i915) ? > >> + LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); > >> > >> /* Apply the GT workarounds... */ > >> - intel_gt_apply_workarounds(&dev_priv->gt); > >> + intel_gt_apply_workarounds(gt); > > > > Would it be worth moving the above mmio into workarounds? Whilst you are > > doing some spring cleaning :) > > To GT workarounds? Are the above two workarounds? Do they have an > official designation? To intel_gt_workarounds_apply, I'm sure you can find something ;) > >> /* ...and determine whether they are sticking. */ > >> - intel_gt_verify_workarounds(&dev_priv->gt, "init"); > >> + intel_gt_verify_workarounds(gt, "init"); > >> > >> - intel_gt_init_swizzling(&dev_priv->gt); > >> + intel_gt_init_swizzling(gt); > >> > >> /* > >> * At least 830 can leave some of the unused rings > >> @@ -1263,48 +1267,58 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv) > >> * will prevent c3 entry. Makes sure all unused rings > >> * are totally idle. > >> */ > >> - init_unused_rings(&dev_priv->gt); > >> - > >> - BUG_ON(!dev_priv->kernel_context); > >> - ret = i915_terminally_wedged(dev_priv); > >> - if (ret) > >> - goto out; > >> + init_unused_rings(gt); > >> > >> - ret = i915_ppgtt_init_hw(&dev_priv->gt); > >> + ret = i915_ppgtt_init_hw(gt); > >> if (ret) { > >> DRM_ERROR("Enabling PPGTT failed (%d)\n", ret); > >> goto out; > >> } > >> > >> - ret = intel_wopcm_init_hw(&dev_priv->wopcm); > >> + ret = intel_wopcm_init_hw(&i915->wopcm); > >> if (ret) { > >> DRM_ERROR("Enabling WOPCM failed (%d)\n", ret); > >> goto out; > >> } > >> > >> /* We can't enable contexts until all firmware is loaded */ > >> - ret = intel_uc_init_hw(dev_priv); > >> + ret = intel_uc_init_hw(i915); > > > > Sorting out the uc layering is an ongoing task. I think it probably > > means our init_hw needs splitting. > > I think guc and huc could be made children of intel_gt so this could be > changed to take gt. It's a lot of code which I am not sure has much yet > to live so I opted not to touch it. I can go either way. There are a few hooks into e.g. i915_ggtt, but mostly it is a different means of driving HW (by passing through to a second driver of our HW, grumble) via a separate communication channel. On the whole, I think it replaces gt/. In passing, as we move i915_ggtt/i915_ppgtt beneath gt/, we should also consider using the intel_ prefix. The goal is that these are the HW interface for tracking the page tables with the i915_gem_context being the API interface. (And if the GEM vm api grows more we should introduce a i915_gem_vm/gtt wrapper around intel_ppgtt.) > >> if (ret) { > >> DRM_ERROR("Enabling uc failed (%d)\n", ret); > >> goto out; > >> } > >> > >> - intel_mocs_init_l3cc_table(&dev_priv->gt); > >> + intel_mocs_init_l3cc_table(gt); > >> > >> /* Only when the HW is re-initialised, can we replay the requests */ > >> - ret = intel_engines_resume(dev_priv); > >> + ret = intel_engines_resume(i915); > >> if (ret) > >> goto cleanup_uc; > >> > >> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); > >> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); > >> > >> - intel_engines_set_scheduler_caps(dev_priv); > >> return 0; > >> > >> cleanup_uc: > >> - intel_uc_fini_hw(dev_priv); > >> + intel_uc_fini_hw(i915); > >> out: > >> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); > >> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); > >> + > >> + return ret; > >> +} > >> + > >> +int i915_gem_init_hw(struct drm_i915_private *i915) > > > > Do we also start to recognise this as i915_init_hw()? This is the driver > > talking to the intel_gt and friends, not the driver setting up the GEM > > api. > > Not sure. There are some GEM bits inside like wedged status and > scheduler caps. So it sounds passable to leave it like it is for now. wedged is mostly our response to HW, and is controlled by gt/intel_reset.c. But we also use to disable the GEM uAPI. However that is just the API layer looking at the underlying HW state and saying "no can do". Hopefully. i915->gpu_error is definitely an interesting beast. I actually think it doesn't belong inside i915->gt as it is a separate HW snapshot for different API, but not actually a part of driving the HW. The caps.sched are an interesting wart, we are summarising the gt features and they change depending on how we mistreat gt (wedged). -Chris
On 13/06/2019 17:30, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-06-13 17:11:43) >> >> On 13/06/2019 14:59, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2019-06-13 14:35:24) >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> More removal of implicit dev_priv from using old mmio accessors. >>>> >>>> Actually the top level function remains but is split into a part which >>>> writes to i915 and part which operates on intel_gt in order to initialize >>>> the hardware. >>>> >>>> GuC and engines are the only odd ones out remaining. >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_gem.c | 66 ++++++++++++++++++++------------- >>>> 1 file changed, 40 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>>> index e54cd30534dc..b6f450e782e7 100644 >>>> --- a/drivers/gpu/drm/i915/i915_gem.c >>>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>>> @@ -1234,28 +1234,32 @@ static void init_unused_rings(struct intel_gt *gt) >>>> } >>>> } >>>> >>>> -int i915_gem_init_hw(struct drm_i915_private *dev_priv) >>>> +static int init_hw(struct intel_gt *gt) >>>> { >>>> + struct drm_i915_private *i915 = gt->i915; >>>> + struct intel_uncore *uncore = gt->uncore; >>>> int ret; >>>> >>>> - dev_priv->gt.last_init_time = ktime_get(); >>>> + gt->last_init_time = ktime_get(); >>>> >>>> /* Double layer security blanket, see i915_gem_init() */ >>>> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL); >>>> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); >>>> >>>> - if (HAS_EDRAM(dev_priv) && INTEL_GEN(dev_priv) < 9) >>>> - I915_WRITE(HSW_IDICR, I915_READ(HSW_IDICR) | IDIHASHMSK(0xf)); >>>> + if (HAS_EDRAM(i915) && INTEL_GEN(i915) < 9) >>>> + intel_uncore_rmw(uncore, HSW_IDICR, 0, IDIHASHMSK(0xf)); >>>> >>>> - if (IS_HASWELL(dev_priv)) >>>> - I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ? >>>> - LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); >>>> + if (IS_HASWELL(i915)) >>>> + intel_uncore_write(uncore, >>>> + MI_PREDICATE_RESULT_2, >>>> + IS_HSW_GT3(i915) ? >>>> + LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); >>>> >>>> /* Apply the GT workarounds... */ >>>> - intel_gt_apply_workarounds(&dev_priv->gt); >>>> + intel_gt_apply_workarounds(gt); >>> >>> Would it be worth moving the above mmio into workarounds? Whilst you are >>> doing some spring cleaning :) >> >> To GT workarounds? Are the above two workarounds? Do they have an >> official designation? > > To intel_gt_workarounds_apply, I'm sure you can find something ;) Having looked up the docs for HSW_IDCR and MI_PREDICATE_RESULT_2, neither of the programming looks like workarounds but normal device init to me. As such I don't see how it would be appropriate to move them into workarounds. > >>>> /* ...and determine whether they are sticking. */ >>>> - intel_gt_verify_workarounds(&dev_priv->gt, "init"); >>>> + intel_gt_verify_workarounds(gt, "init"); >>>> >>>> - intel_gt_init_swizzling(&dev_priv->gt); >>>> + intel_gt_init_swizzling(gt); >>>> >>>> /* >>>> * At least 830 can leave some of the unused rings >>>> @@ -1263,48 +1267,58 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv) >>>> * will prevent c3 entry. Makes sure all unused rings >>>> * are totally idle. >>>> */ >>>> - init_unused_rings(&dev_priv->gt); >>>> - >>>> - BUG_ON(!dev_priv->kernel_context); >>>> - ret = i915_terminally_wedged(dev_priv); >>>> - if (ret) >>>> - goto out; >>>> + init_unused_rings(gt); >>>> >>>> - ret = i915_ppgtt_init_hw(&dev_priv->gt); >>>> + ret = i915_ppgtt_init_hw(gt); >>>> if (ret) { >>>> DRM_ERROR("Enabling PPGTT failed (%d)\n", ret); >>>> goto out; >>>> } >>>> >>>> - ret = intel_wopcm_init_hw(&dev_priv->wopcm); >>>> + ret = intel_wopcm_init_hw(&i915->wopcm); >>>> if (ret) { >>>> DRM_ERROR("Enabling WOPCM failed (%d)\n", ret); >>>> goto out; >>>> } >>>> >>>> /* We can't enable contexts until all firmware is loaded */ >>>> - ret = intel_uc_init_hw(dev_priv); >>>> + ret = intel_uc_init_hw(i915); >>> >>> Sorting out the uc layering is an ongoing task. I think it probably >>> means our init_hw needs splitting. >> >> I think guc and huc could be made children of intel_gt so this could be >> changed to take gt. It's a lot of code which I am not sure has much yet >> to live so I opted not to touch it. > > I can go either way. There are a few hooks into e.g. i915_ggtt, but > mostly it is a different means of driving HW (by passing through to a > second driver of our HW, grumble) via a separate communication > channel. On the whole, I think it replaces gt/. > > In passing, as we move i915_ggtt/i915_ppgtt beneath gt/, we should also > consider using the intel_ prefix. The goal is that these are the HW > interface for tracking the page tables with the i915_gem_context being > the API interface. (And if the GEM vm api grows more we should > introduce a i915_gem_vm/gtt wrapper around intel_ppgtt.) > >>>> if (ret) { >>>> DRM_ERROR("Enabling uc failed (%d)\n", ret); >>>> goto out; >>>> } >>>> >>>> - intel_mocs_init_l3cc_table(&dev_priv->gt); >>>> + intel_mocs_init_l3cc_table(gt); >>>> >>>> /* Only when the HW is re-initialised, can we replay the requests */ >>>> - ret = intel_engines_resume(dev_priv); >>>> + ret = intel_engines_resume(i915); >>>> if (ret) >>>> goto cleanup_uc; >>>> >>>> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); >>>> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); >>>> >>>> - intel_engines_set_scheduler_caps(dev_priv); >>>> return 0; >>>> >>>> cleanup_uc: >>>> - intel_uc_fini_hw(dev_priv); >>>> + intel_uc_fini_hw(i915); >>>> out: >>>> - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); >>>> + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +int i915_gem_init_hw(struct drm_i915_private *i915) >>> >>> Do we also start to recognise this as i915_init_hw()? This is the driver >>> talking to the intel_gt and friends, not the driver setting up the GEM >>> api. >> >> Not sure. There are some GEM bits inside like wedged status and >> scheduler caps. So it sounds passable to leave it like it is for now. > > wedged is mostly our response to HW, and is controlled by > gt/intel_reset.c. But we also use to disable the GEM uAPI. However that > is just the API layer looking at the underlying HW state and saying "no can do". > Hopefully. > > i915->gpu_error is definitely an interesting beast. I actually think it > doesn't belong inside i915->gt as it is a separate HW snapshot for > different API, but not actually a part of driving the HW. > > The caps.sched are an interesting wart, we are summarising the gt > features and they change depending on how we mistreat gt (wedged). I read this as leave as is for now. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-06-14 10:34:06) > > On 13/06/2019 17:30, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-06-13 17:11:43) > >> > >> On 13/06/2019 14:59, Chris Wilson wrote: > >>> Quoting Tvrtko Ursulin (2019-06-13 14:35:24) > >>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >>>> index e54cd30534dc..b6f450e782e7 100644 > >>>> --- a/drivers/gpu/drm/i915/i915_gem.c > >>>> +++ b/drivers/gpu/drm/i915/i915_gem.c > >>>> @@ -1234,28 +1234,32 @@ static void init_unused_rings(struct intel_gt *gt) > >>>> } > >>>> } > >>>> > >>>> -int i915_gem_init_hw(struct drm_i915_private *dev_priv) > >>>> +static int init_hw(struct intel_gt *gt) > >>>> { > >>>> + struct drm_i915_private *i915 = gt->i915; > >>>> + struct intel_uncore *uncore = gt->uncore; > >>>> int ret; > >>>> > >>>> - dev_priv->gt.last_init_time = ktime_get(); > >>>> + gt->last_init_time = ktime_get(); > >>>> > >>>> /* Double layer security blanket, see i915_gem_init() */ > >>>> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL); > >>>> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); > >>>> > >>>> - if (HAS_EDRAM(dev_priv) && INTEL_GEN(dev_priv) < 9) > >>>> - I915_WRITE(HSW_IDICR, I915_READ(HSW_IDICR) | IDIHASHMSK(0xf)); > >>>> + if (HAS_EDRAM(i915) && INTEL_GEN(i915) < 9) > >>>> + intel_uncore_rmw(uncore, HSW_IDICR, 0, IDIHASHMSK(0xf)); > >>>> > >>>> - if (IS_HASWELL(dev_priv)) > >>>> - I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ? > >>>> - LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); > >>>> + if (IS_HASWELL(i915)) > >>>> + intel_uncore_write(uncore, > >>>> + MI_PREDICATE_RESULT_2, > >>>> + IS_HSW_GT3(i915) ? > >>>> + LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); > >>>> > >>>> /* Apply the GT workarounds... */ > >>>> - intel_gt_apply_workarounds(&dev_priv->gt); > >>>> + intel_gt_apply_workarounds(gt); > >>> > >>> Would it be worth moving the above mmio into workarounds? Whilst you are > >>> doing some spring cleaning :) > >> > >> To GT workarounds? Are the above two workarounds? Do they have an > >> official designation? > > > > To intel_gt_workarounds_apply, I'm sure you can find something ;) > > Having looked up the docs for HSW_IDCR and MI_PREDICATE_RESULT_2, > neither of the programming looks like workarounds but normal device init > to me. As such I don't see how it would be appropriate to move them into > workarounds. Where they are is definitely not where they should be. I'm sure I've complained about this since they were put there. And normal device init == workarounds, does it not? It is just a list of registers that need to be programmed to default values, where those default values were decided upon after the fact. -Chris
On 14/06/2019 10:41, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-06-14 10:34:06) >> >> On 13/06/2019 17:30, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2019-06-13 17:11:43) >>>> >>>> On 13/06/2019 14:59, Chris Wilson wrote: >>>>> Quoting Tvrtko Ursulin (2019-06-13 14:35:24) >>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >>>>>> index e54cd30534dc..b6f450e782e7 100644 >>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c >>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c >>>>>> @@ -1234,28 +1234,32 @@ static void init_unused_rings(struct intel_gt *gt) >>>>>> } >>>>>> } >>>>>> >>>>>> -int i915_gem_init_hw(struct drm_i915_private *dev_priv) >>>>>> +static int init_hw(struct intel_gt *gt) >>>>>> { >>>>>> + struct drm_i915_private *i915 = gt->i915; >>>>>> + struct intel_uncore *uncore = gt->uncore; >>>>>> int ret; >>>>>> >>>>>> - dev_priv->gt.last_init_time = ktime_get(); >>>>>> + gt->last_init_time = ktime_get(); >>>>>> >>>>>> /* Double layer security blanket, see i915_gem_init() */ >>>>>> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL); >>>>>> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); >>>>>> >>>>>> - if (HAS_EDRAM(dev_priv) && INTEL_GEN(dev_priv) < 9) >>>>>> - I915_WRITE(HSW_IDICR, I915_READ(HSW_IDICR) | IDIHASHMSK(0xf)); >>>>>> + if (HAS_EDRAM(i915) && INTEL_GEN(i915) < 9) >>>>>> + intel_uncore_rmw(uncore, HSW_IDICR, 0, IDIHASHMSK(0xf)); >>>>>> >>>>>> - if (IS_HASWELL(dev_priv)) >>>>>> - I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ? >>>>>> - LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); >>>>>> + if (IS_HASWELL(i915)) >>>>>> + intel_uncore_write(uncore, >>>>>> + MI_PREDICATE_RESULT_2, >>>>>> + IS_HSW_GT3(i915) ? >>>>>> + LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); >>>>>> >>>>>> /* Apply the GT workarounds... */ >>>>>> - intel_gt_apply_workarounds(&dev_priv->gt); >>>>>> + intel_gt_apply_workarounds(gt); >>>>> >>>>> Would it be worth moving the above mmio into workarounds? Whilst you are >>>>> doing some spring cleaning :) >>>> >>>> To GT workarounds? Are the above two workarounds? Do they have an >>>> official designation? >>> >>> To intel_gt_workarounds_apply, I'm sure you can find something ;) >> >> Having looked up the docs for HSW_IDCR and MI_PREDICATE_RESULT_2, >> neither of the programming looks like workarounds but normal device init >> to me. As such I don't see how it would be appropriate to move them into >> workarounds. > > Where they are is definitely not where they should be. I'm sure I've > complained about this since they were put there. And normal device init == > workarounds, does it not? It is just a list of registers that need to be > programmed to default values, where those default values were decided > upon after the fact. Well we could argue.. :) I see stuff in intel_workarounds as having WaXXXX names, give or take. So I prefer to leave this here for now. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-06-14 16:00:28) > > On 14/06/2019 10:41, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-06-14 10:34:06) > >> > >> On 13/06/2019 17:30, Chris Wilson wrote: > >>> Quoting Tvrtko Ursulin (2019-06-13 17:11:43) > >>>> > >>>> On 13/06/2019 14:59, Chris Wilson wrote: > >>>>> Quoting Tvrtko Ursulin (2019-06-13 14:35:24) > >>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >>>>>> index e54cd30534dc..b6f450e782e7 100644 > >>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c > >>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c > >>>>>> @@ -1234,28 +1234,32 @@ static void init_unused_rings(struct intel_gt *gt) > >>>>>> } > >>>>>> } > >>>>>> > >>>>>> -int i915_gem_init_hw(struct drm_i915_private *dev_priv) > >>>>>> +static int init_hw(struct intel_gt *gt) > >>>>>> { > >>>>>> + struct drm_i915_private *i915 = gt->i915; > >>>>>> + struct intel_uncore *uncore = gt->uncore; > >>>>>> int ret; > >>>>>> > >>>>>> - dev_priv->gt.last_init_time = ktime_get(); > >>>>>> + gt->last_init_time = ktime_get(); > >>>>>> > >>>>>> /* Double layer security blanket, see i915_gem_init() */ > >>>>>> - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL); > >>>>>> + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); > >>>>>> > >>>>>> - if (HAS_EDRAM(dev_priv) && INTEL_GEN(dev_priv) < 9) > >>>>>> - I915_WRITE(HSW_IDICR, I915_READ(HSW_IDICR) | IDIHASHMSK(0xf)); > >>>>>> + if (HAS_EDRAM(i915) && INTEL_GEN(i915) < 9) > >>>>>> + intel_uncore_rmw(uncore, HSW_IDICR, 0, IDIHASHMSK(0xf)); > >>>>>> > >>>>>> - if (IS_HASWELL(dev_priv)) > >>>>>> - I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ? > >>>>>> - LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); > >>>>>> + if (IS_HASWELL(i915)) > >>>>>> + intel_uncore_write(uncore, > >>>>>> + MI_PREDICATE_RESULT_2, > >>>>>> + IS_HSW_GT3(i915) ? > >>>>>> + LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); > >>>>>> > >>>>>> /* Apply the GT workarounds... */ > >>>>>> - intel_gt_apply_workarounds(&dev_priv->gt); > >>>>>> + intel_gt_apply_workarounds(gt); > >>>>> > >>>>> Would it be worth moving the above mmio into workarounds? Whilst you are > >>>>> doing some spring cleaning :) > >>>> > >>>> To GT workarounds? Are the above two workarounds? Do they have an > >>>> official designation? > >>> > >>> To intel_gt_workarounds_apply, I'm sure you can find something ;) > >> > >> Having looked up the docs for HSW_IDCR and MI_PREDICATE_RESULT_2, > >> neither of the programming looks like workarounds but normal device init > >> to me. As such I don't see how it would be appropriate to move them into > >> workarounds. > > > > Where they are is definitely not where they should be. I'm sure I've > > complained about this since they were put there. And normal device init == > > workarounds, does it not? It is just a list of registers that need to be > > programmed to default values, where those default values were decided > > upon after the fact. > > Well we could argue.. :) I see stuff in intel_workarounds as having > WaXXXX names, give or take. So I prefer to leave this here for now. Give or take the fake Wa names made up to cover normal device init :-p -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e54cd30534dc..b6f450e782e7 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1234,28 +1234,32 @@ static void init_unused_rings(struct intel_gt *gt) } } -int i915_gem_init_hw(struct drm_i915_private *dev_priv) +static int init_hw(struct intel_gt *gt) { + struct drm_i915_private *i915 = gt->i915; + struct intel_uncore *uncore = gt->uncore; int ret; - dev_priv->gt.last_init_time = ktime_get(); + gt->last_init_time = ktime_get(); /* Double layer security blanket, see i915_gem_init() */ - intel_uncore_forcewake_get(&dev_priv->uncore, FORCEWAKE_ALL); + intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); - if (HAS_EDRAM(dev_priv) && INTEL_GEN(dev_priv) < 9) - I915_WRITE(HSW_IDICR, I915_READ(HSW_IDICR) | IDIHASHMSK(0xf)); + if (HAS_EDRAM(i915) && INTEL_GEN(i915) < 9) + intel_uncore_rmw(uncore, HSW_IDICR, 0, IDIHASHMSK(0xf)); - if (IS_HASWELL(dev_priv)) - I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ? - LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); + if (IS_HASWELL(i915)) + intel_uncore_write(uncore, + MI_PREDICATE_RESULT_2, + IS_HSW_GT3(i915) ? + LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED); /* Apply the GT workarounds... */ - intel_gt_apply_workarounds(&dev_priv->gt); + intel_gt_apply_workarounds(gt); /* ...and determine whether they are sticking. */ - intel_gt_verify_workarounds(&dev_priv->gt, "init"); + intel_gt_verify_workarounds(gt, "init"); - intel_gt_init_swizzling(&dev_priv->gt); + intel_gt_init_swizzling(gt); /* * At least 830 can leave some of the unused rings @@ -1263,48 +1267,58 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv) * will prevent c3 entry. Makes sure all unused rings * are totally idle. */ - init_unused_rings(&dev_priv->gt); - - BUG_ON(!dev_priv->kernel_context); - ret = i915_terminally_wedged(dev_priv); - if (ret) - goto out; + init_unused_rings(gt); - ret = i915_ppgtt_init_hw(&dev_priv->gt); + ret = i915_ppgtt_init_hw(gt); if (ret) { DRM_ERROR("Enabling PPGTT failed (%d)\n", ret); goto out; } - ret = intel_wopcm_init_hw(&dev_priv->wopcm); + ret = intel_wopcm_init_hw(&i915->wopcm); if (ret) { DRM_ERROR("Enabling WOPCM failed (%d)\n", ret); goto out; } /* We can't enable contexts until all firmware is loaded */ - ret = intel_uc_init_hw(dev_priv); + ret = intel_uc_init_hw(i915); if (ret) { DRM_ERROR("Enabling uc failed (%d)\n", ret); goto out; } - intel_mocs_init_l3cc_table(&dev_priv->gt); + intel_mocs_init_l3cc_table(gt); /* Only when the HW is re-initialised, can we replay the requests */ - ret = intel_engines_resume(dev_priv); + ret = intel_engines_resume(i915); if (ret) goto cleanup_uc; - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); - intel_engines_set_scheduler_caps(dev_priv); return 0; cleanup_uc: - intel_uc_fini_hw(dev_priv); + intel_uc_fini_hw(i915); out: - intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL); + intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL); + + return ret; +} + +int i915_gem_init_hw(struct drm_i915_private *i915) +{ + int ret; + + BUG_ON(!i915->kernel_context); + ret = i915_terminally_wedged(i915); + if (ret) + return ret; + + ret = init_hw(&i915->gt); + + intel_engines_set_scheduler_caps(i915); return ret; }