Re: [RFC 0/3] Display uncore

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 08 Aug 2019, Chris Wilson <chris@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.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx