[Intel-gfx] [RFC 01/14] drm/i915: Make i915_check_and_clear_faults take uncore

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-06-11 13:05:58)
> 
> On 11/06/2019 09:52, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-06-11 09:35:07)
> >>
> >> On 10/06/2019 17:26, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-06-10 16:54:06)
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>>
> >>>> Continuing the conversion and elimination of implicit dev_priv.
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>> Suggested-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/gt/intel_engine_cs.c |  2 +-
> >>>>    drivers/gpu/drm/i915/gt/intel_reset.c     | 28 ++++++++++++-----------
> >>>>    drivers/gpu/drm/i915/gt/intel_reset.h     |  2 +-
> >>>>    drivers/gpu/drm/i915/i915_drv.c           |  2 +-
> >>>>    drivers/gpu/drm/i915/i915_gem_gtt.c       |  4 ++--
> >>>>    5 files changed, 20 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> >>>> index c0d986db5a75..a046e8dccc96 100644
> >>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> >>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> >>>> @@ -453,7 +453,7 @@ int intel_engines_init_mmio(struct drm_i915_private *i915)
> >>>>    
> >>>>           RUNTIME_INFO(i915)->num_engines = hweight32(mask);
> >>>>    
> >>>> -       i915_check_and_clear_faults(i915);
> >>>> +       i915_check_and_clear_faults(&i915->uncore);
> >>>
> >>> This name is still setting off red flags for me, but I have to confess
> >>> that staring at it, passing uncore does make sense.
> >>
> >> Rename to intel_uncore_check_and_clear_faults?
> >>
> >> Or move later in the series as intel_gt_check_and_clear_faults?
> > 
> > I think I prefer the latter option, intel_gt_check_and_clear_faults.
> 
> Yep agreed.
> 
> Any comments on the intel_gt.c the series added?

Good. It's the direction I think we need.
 
> And the end result in i915_gem_init(_hw)?

Definitely not the end yet, but passable for now :)

> >>> I just wish we have per-engines faults everywhere and this could be
> >>> reduced to passing engine.
> >>>
> >>> Hmm, this I guess we will just have to revisit in the near future as we
> >>> may get the opportunity to put these regs under more scrutiny.
> >>>
> >>>>    
> >>>>           intel_setup_engine_capabilities(i915);
> >>>>    
> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> >>>> index 60d24110af80..13471916559b 100644
> >>>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> >>>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> >>>> @@ -1166,10 +1166,10 @@ static void gen8_clear_engine_error_register(struct intel_engine_cs *engine)
> >>>>           GEN6_RING_FAULT_REG_POSTING_READ(engine);
> >>>>    }
> >>>>    
> >>>> -static void clear_error_registers(struct drm_i915_private *i915,
> >>>> +static void clear_error_registers(struct intel_uncore *uncore,
> >>>>                                     intel_engine_mask_t engine_mask)
> >>>>    {
> >>>> -       struct intel_uncore *uncore = &i915->uncore;
> >>>> +       struct drm_i915_private *i915 = uncore_to_i915(uncore);
> >>>
> >>> Grr, I should have objected to uncore_to_i915() loudly from the
> >>> beginning
> >>>
> >>> What's done is done,
> >>
> >> Is it too late already? Shouldn't be. My thinking was the implementation
> >> can easily be changed if/when backpointer is added (instead of
> >> container_of). But if you would prefer we start without a helper, but
> >> with a direct access to backpointer straight away that is fine by me.
> > 
> > I'm optimistic that we can land a split display/gt intel_uncore early
> > and so the churn is in the not too distant future.
> 
> Okay but that doesn't explicitly answer whether you prefer I just drop 
> all the XXX_to_YYY wrappers in favour of using direct pointer dereferences.

I'm in favour of uncore->i915 over uncore_to_i915(uncore) and drop the
embedding knowledge.

> You are also in favour of replacing engine->i915 with engine->gt 
> straight away?

I can live with an extra back pointer. I think it will ultimately be
redundant, but expect a incremental evolution will be easier.
-Chris