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

[Daniele Ceraolo Spurio]

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.

Daniele

> BR,
> Jani.
> 
>