[Intel-gfx] [RFC 0/3] Display uncore

[Jani Nikula]

On Thu, 08 Aug 2019, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
> On Thu, Aug 08, 2019 at 09:47:56AM -0700, Daniele Ceraolo Spurio wrote:
>>
>>
>>On 8/8/19 6:58 AM, Jani Nikula wrote:
>>>On Thu, 08 Aug 2019, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>>>>Quoting Daniele Ceraolo Spurio (2019-08-08 02:44:20)
>>>>>I've been trying to identify MMIO ranges to clearly define what belongs
>>>>>to display_uncore to do a check on access, but there are lots of
>>>>>exceptions and differences across gens (with a few more coming with TGL),
>>>>>so I don't think that's a viable way. The alternative option implemented
>>>>>here is to differentiate the register by type, which should ensure we
>>>>>never mix them up, but at the cost of a more complex transition.
>>>>
>>>>One thing we could try with this approach is to tag every _MMIO() as
>>>>either DE or GT and have the uncore accessors check the magic bits
>>>>before scrubbing them. (With enough magic macros to make it disappear
>>>>
>>>>Huge task, but not insurmountable. The danger is that if we do this
>>>>piecemeal is that we may end up with two simultaneous accesses via the
>>>>separate uncore accessors. Hmm.
>>>
>>>You mean something like this?
>>>
>>>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>index b362ca0663a6..401490f79935 100644
>>>--- a/drivers/gpu/drm/i915/i915_reg.h
>>>+++ b/drivers/gpu/drm/i915/i915_reg.h
>>>@@ -179,7 +179,8 @@
>>>  #define REG_FIELD_GET(__mask, __val)	((u32)FIELD_GET(__mask, __val))
>>>  typedef struct {
>>>-	u32 reg;
>>>+	u32 de:1;
>>>+	u32 reg:31;
>>>  } i915_reg_t;
>>>  #define _MMIO(r) ((const i915_reg_t){ .reg = (r) })
>>>---
>>>
>>>bloat-o-meter tells me just that, with no other changes is +0.53%
>>>increase. Perhaps still acceptable.
>>>
>>>I think we could just add something like
>>>
>>>#define _MMIO_DE(r) ((const i915_reg_t){ .de = 1, .reg = (r) })
>>>
>>>and update i915_reg.h to use that as the first step, with no other
>>>changes, and build on top of that. I don't think there should be large
>>>scale changes outside of i915_reg.h required at all at first. The update
>>>to move away from I915_READ and I915_WRITE could come afterwards and
>>>piecemeal AFAICT.
>>>
>>>>On thing though is that Jani may find the intel_de_write (or just
>>>>de_write, the frequency may be worth bending the naming rules) as being
>>>>palatable.
>>>
>>>Indeed. Already intel_de_write(i915, ...) is so much better than
>>>intel_uncore_write(&i915->uncore, ...).
>>>
>>>Though... with de encoded in i915_reg_t, we could have intel_write(i915,
>>>...) do the right thing based on .de. It could internally choose the
>>>right uncore for intel_uncore_write(). Even if most non-de would
>>>directly use the uncore versions.
>>>
>>
>>I'd prefer to avoid the implicit selection in the new functions, but, 
>>for compatibility during the transition, we could add the selection 
>>inside the old I915_READ/WRITE() calls. This way we'll be able to 
>>ensure that even the non yet converted accesses will go through the 
>>correct uncore.
>
> Yeah, I'm with you on this one. For new functions I think it's better to
> be explicit.

I suppose my question is, how is it implicit if it's explicitly
specified in the i915_reg_t?

BR,
Jani.


>
> Lucas De Marchi
>
>>
>>Daniele
>>
>>>BR,
>>>Jani.
>>>
>>>

-- 
Jani Nikula, Intel Open Source Graphics Center