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@xxxxxxxxxxxxxxxxxx> 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
