Re: [RFC 01/14] drm/i915: Make i915_check_and_clear_faults take uncore

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

On 10/06/2019 17:26, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-10 16:54:06)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Continuing the conversion and elimination of implicit dev_priv.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Suggested-by: Rodrigo Vivi <rodrigo.vivi@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 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.

Regards,

Tvrtko

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx