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

[Lucas De Marchi]

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.

Lucas De Marchi

>
>Daniele
>
>>BR,
>>Jani.
>>
>>