[Intel-xe] [RFC] drm/xe: A minimal assert for some forcewake domain on xe_mmio.

* [Intel-xe] [RFC] drm/xe: A minimal assert for some forcewake domain on xe_mmio.
@ 2023-05-22 22:15 Rodrigo Vivi
  2023-05-22 22:17 ` [Intel-xe] ✓ CI.Patch_applied: success for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 25+ messages in thread
From: Rodrigo Vivi @ 2023-05-22 22:15 UTC (permalink / raw)
  To: intel-xe
  Cc: Jani Nikula, Lucas De Marchi, Ville Syrjälä,
	Matthew Auld, Rodrigo Vivi, Matt Roper

It is the maximum protection we can do with the current infrastructure.

Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Francois Dugast <francois.dugast@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---


RFC
===

Okay, so, this is more an RFC to brainstorm the future of the force_wake in
Xe than anything else.

On i915 the force_wake is built-in the mmio functions at uncore component.

With that approach we had few historical issues iirc:

1. Display performance with vblank evasion that requested uncore to provide
the 'fw' variantes that are actually the ones that avoid fw (contrary to what
the name suggests).

2. Missed ranges updates. Sometimes we messed up with the ranges, there were
other times that the spec was updated and we didn't get the notification, and
there were cases that the BSpec had bugs.

For these reasons in Xe we have decided to let to the caller the
responsibility to set the force_wake bits for their domains before doing
the MMIO.

However I'm seeing many questions and doubts popping up:

1. Are we confident that we are not missing any wake-up?
2. Are we confident that the domains are set correctly?
3. Are we not wasting power if we are waking up ALL the domains instead
   of some specific one?
4. What about the display disconnection now because i915 and xe have different
   mmio approaches but reusing i915-display?

It looks to me that the cons of the current approach are superseding the
cons of the i915 approach. But I want to hear more thoughts here before
we decide which route to take.

Maybe we have that domain as part of the mmio calls themselves? Maybe
a double approach where the caller is responsible but mmio has the range
information and double check it?

Any other idea? Thoughts?

Thanks,
Rodrigo.

 drivers/gpu/drm/xe/xe_mmio.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
index 1407f1189b0d..6302ca85e3f4 100644
--- a/drivers/gpu/drm/xe/xe_mmio.h
+++ b/drivers/gpu/drm/xe/xe_mmio.h
@@ -18,8 +18,26 @@ struct xe_device;
 
 int xe_mmio_init(struct xe_device *xe);
 
+static inline void xe_mmio_assert_forcewake(struct xe_gt *gt,
+					    struct xe_reg reg)
+{
+	struct xe_force_wake *fw = &gt->mmio.fw;
+
+	/* No need for forcewake in this range */
+	if (reg.addr >= 0x40000 && reg.addr < 0x116000)
+		return;
+
+	/*
+	 * XXX: Weak. It checks for some domain, but impossible to determine
+	 * if the right one is selected, or if it was set by the same caller
+	 */
+	WARN_ON(!fw->awake_domains);
+}
+
 static inline u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
 {
+	xe_mmio_assert_forcewake(gt, reg);
+
 	if (reg.addr < gt->mmio.adj_limit)
 		reg.addr += gt->mmio.adj_offset;
 
@@ -29,6 +47,8 @@ static inline u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
 static inline void xe_mmio_write32(struct xe_gt *gt,
 				   struct xe_reg reg, u32 val)
 {
+	xe_mmio_assert_forcewake(gt, reg);
+
 	if (reg.addr < gt->mmio.adj_limit)
 		reg.addr += gt->mmio.adj_offset;
 
@@ -37,6 +57,8 @@ static inline void xe_mmio_write32(struct xe_gt *gt,
 
 static inline u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg reg)
 {
+	xe_mmio_assert_forcewake(gt, reg);
+
 	if (reg.addr < gt->mmio.adj_limit)
 		reg.addr += gt->mmio.adj_offset;
 
@@ -58,6 +80,8 @@ static inline u32 xe_mmio_rmw32(struct xe_gt *gt, struct xe_reg reg, u32 clr,
 static inline void xe_mmio_write64(struct xe_gt *gt,
 				   struct xe_reg reg, u64 val)
 {
+	xe_mmio_assert_forcewake(gt, reg);
+
 	if (reg.addr < gt->mmio.adj_limit)
 		reg.addr += gt->mmio.adj_offset;
 
@@ -66,6 +90,8 @@ static inline void xe_mmio_write64(struct xe_gt *gt,
 
 static inline u64 xe_mmio_read64(struct xe_gt *gt, struct xe_reg reg)
 {
+	xe_mmio_assert_forcewake(gt, reg);
+
 	if (reg.addr < gt->mmio.adj_limit)
 		reg.addr += gt->mmio.adj_offset;
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread