[PATCH 00/13] drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD

[PATCH 00/13] drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD

[Gustavo Sousa]

Using the DMC wakelock is the official recommendation for Xe3_LPD. This
series apply fixes to the current DMC wakelock implementation and
enables it by default for Xe3_LPD. The series has been tested with a PTL
machine.

Gustavo Sousa (13):
  drm/xe: Mimic i915 behavior for non-sleeping MMIO wait
  drm/i915/dmc_wl: Use non-sleeping variant of MMIO wait
  drm/i915/dmc_wl: Check for non-zero refcount in release work
  drm/i915/dmc_wl: Get wakelock when disabling dynamic DC states
  drm/i915/dmc_wl: Use sentinel item for range tables
  drm/i915/dmc_wl: Extract intel_dmc_wl_addr_in_range()
  drm/i915/dmc_wl: Check ranges specific to DC states
  drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables
  drm/i915/dmc_wl: Deal with existing references when disabling
  drm/i915/dmc_wl: Couple enable/disable with dynamic DC states
  drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()
  drm/i915/dmc_wl: Sanitize enable_dmc_wl according to hardware support
  drm/i915/xe3lpd: Use DMC wakelock by default

 drivers/gpu/drm/i915/display/intel_de.h       |  11 +
 .../drm/i915/display/intel_display_device.h   |   1 +
 .../drm/i915/display/intel_display_driver.c   |   2 +-
 .../drm/i915/display/intel_display_params.c   |   4 +-
 .../drm/i915/display/intel_display_params.h   |   2 +-
 .../i915/display/intel_display_power_well.c   |  15 +-
 drivers/gpu/drm/i915/display/intel_dmc.c      |   4 -
 drivers/gpu/drm/i915/display/intel_dmc_wl.c   | 282 ++++++++++++++----
 drivers/gpu/drm/i915/display/intel_dmc_wl.h   |   2 +
 .../drm/xe/compat-i915-headers/intel_uncore.h |  13 +-
 10 files changed, 269 insertions(+), 67 deletions(-)

-- 
2.47.0

[PATCH 01/13] drm/xe: Mimic i915 behavior for non-sleeping MMIO wait

[Gustavo Sousa]

In upcoming display changes, we will modify the DMC wakelock MMIO
waiting code to choose a non-sleeping variant implementation, because
the wakelock is also taking in atomic context.

While xe provides an explicit parameter (namely "atomic") to prevent
xe_mmio_wait32() from sleeping, i915 does not and implements that
behavior when slow_timeout_ms is zero.

So, for now, let's mimic what i915 does to allow for display to use
non-sleeping MMIO wait. In the future, we should come up with a better
and explicit interface for this behavior in i915, at least while display
code is not an independent entity with proper interfaces between xe and
i915.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 .../gpu/drm/xe/compat-i915-headers/intel_uncore.h   | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
index 0382beb4035b..5a57f76c1760 100644
--- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
+++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
@@ -117,10 +117,21 @@ __intel_wait_for_register(struct intel_uncore *uncore, i915_reg_t i915_reg,
 			  unsigned int slow_timeout_ms, u32 *out_value)
 {
 	struct xe_reg reg = XE_REG(i915_mmio_reg_offset(i915_reg));
+	bool atomic;
+
+	/*
+	 * FIXME: We are trying to replicate the behavior from i915 here, in
+	 * which sleep is not performed if slow_timeout_ms == 0. This hack is
+	 * necessary because of paths in display code that are executed in
+	 * atomic context. Setting the atomic flag based on timeout values
+	 * doesn't feel very robust. Ideally, we should have a proper interface
+	 * for explicitly choosing non-sleeping behavior.
+	 */
+	atomic = !slow_timeout_ms && fast_timeout_us > 0;
 
 	return xe_mmio_wait32(__compat_uncore_to_mmio(uncore), reg, mask, value,
 			      fast_timeout_us + 1000 * slow_timeout_ms,
-			      out_value, false);
+			      out_value, atomic);
 }
 
 static inline u32 intel_uncore_read_fw(struct intel_uncore *uncore,
-- 
2.47.0

[PATCH 02/13] drm/i915/dmc_wl: Use non-sleeping variant of MMIO wait

[Gustavo Sousa]

Some display MMIO transactions for offsets in the range that requires
the DMC wakelock happen in atomic context (this has been confirmed
during tests on PTL). That means that we need to use a non-sleeping
variant of MMIO waiting function.

Implement __intel_de_wait_for_register_atomic_nowl() and use it when
waiting for acknowledgment of acquire/release.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_de.h     | 11 +++++++++++
 drivers/gpu/drm/i915/display/intel_dmc_wl.c | 20 ++++++++++++--------
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
index e017cd4a8168..4116783a62dd 100644
--- a/drivers/gpu/drm/i915/display/intel_de.h
+++ b/drivers/gpu/drm/i915/display/intel_de.h
@@ -121,6 +121,17 @@ ____intel_de_wait_for_register_nowl(struct intel_display *display,
 }
 #define __intel_de_wait_for_register_nowl(p,...) ____intel_de_wait_for_register_nowl(__to_intel_display(p), __VA_ARGS__)
 
+static inline int
+____intel_de_wait_for_register_atomic_nowl(struct intel_display *display,
+					   i915_reg_t reg,
+					   u32 mask, u32 value,
+					   unsigned int fast_timeout_us)
+{
+	return __intel_wait_for_register(__to_uncore(display), reg, mask,
+					 value, fast_timeout_us, 0, NULL);
+}
+#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)
+
 static inline int
 __intel_de_wait(struct intel_display *display, i915_reg_t reg,
 		u32 mask, u32 value, unsigned int timeout)
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index 5634ff07269d..8056a3c8666c 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -39,7 +39,7 @@
  * potential future use.
  */
 
-#define DMC_WAKELOCK_CTL_TIMEOUT 5
+#define DMC_WAKELOCK_CTL_TIMEOUT_US 5000
 #define DMC_WAKELOCK_HOLD_TIME 50
 
 struct intel_dmc_wl_range {
@@ -78,9 +78,9 @@ static void intel_dmc_wl_work(struct work_struct *work)
 
 	__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0);
 
-	if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL,
-					      DMC_WAKELOCK_CTL_ACK, 0,
-					      DMC_WAKELOCK_CTL_TIMEOUT)) {
+	if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
+						     DMC_WAKELOCK_CTL_ACK, 0,
+						     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
 		WARN_RATELIMIT(1, "DMC wakelock release timed out");
 		goto out_unlock;
 	}
@@ -216,10 +216,14 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
 		__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, 0,
 				    DMC_WAKELOCK_CTL_REQ);
 
-		if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL,
-						      DMC_WAKELOCK_CTL_ACK,
-						      DMC_WAKELOCK_CTL_ACK,
-						      DMC_WAKELOCK_CTL_TIMEOUT)) {
+		/*
+		 * We need to use the atomic variant of the waiting routine
+		 * because the DMC wakelock is also taken in atomic context.
+		 */
+		if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
+							     DMC_WAKELOCK_CTL_ACK,
+							     DMC_WAKELOCK_CTL_ACK,
+							     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
 			WARN_RATELIMIT(1, "DMC wakelock ack timed out");
 			goto out_unlock;
 		}
-- 
2.47.0

[PATCH 03/13] drm/i915/dmc_wl: Check for non-zero refcount in release work

[Gustavo Sousa]

When the DMC wakelock refcount reaches zero, we know that there are no
users and that we can do the actual release operation on the hardware,
which is queued with a delayed work. The idea of the delayed work is to
avoid performing the release if a new lock user appears (i.e. refcount
gets incremented) in a very short period of time.

Based on the above, the release work should bail out if refcount is
non-zero (meaning new lock users appeared in the meantime), but our
current code actually does the opposite: it bails when refcount is zero.
That means that the wakelock is not released when it should be; and
that, when the work is not canceled in time, it ends up being releasing
when it should not.

Fix that by inverting the condition.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc_wl.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index 8056a3c8666c..c298aef89449 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -72,8 +72,11 @@ static void intel_dmc_wl_work(struct work_struct *work)
 
 	spin_lock_irqsave(&wl->lock, flags);
 
-	/* Bail out if refcount reached zero while waiting for the spinlock */
-	if (!refcount_read(&wl->refcount))
+	/*
+	 * Bail out if refcount became non-zero while waiting for the spinlock,
+	 * meaning that the lock is now taken again.
+	 */
+	if (refcount_read(&wl->refcount))
 		goto out_unlock;
 
 	__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0);
-- 
2.47.0

[PATCH 04/13] drm/i915/dmc_wl: Get wakelock when disabling dynamic DC states

[Gustavo Sousa]

Bspec says that disabling dynamic DC states require taking the DMC
wakelock to cause an DC exit before writing to DC_STATE_EN. Implement
that.

In fact, testing on PTL revealed we end up failing to exit DC5/6 without
this step.

Bspec: 71583
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 .../drm/i915/display/intel_display_power_well.c    | 10 +++++++---
 drivers/gpu/drm/i915/display/intel_dmc_wl.c        | 14 ++++++++++++--
 drivers/gpu/drm/i915/display/intel_dmc_wl.h        |  2 ++
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
index adaf7cf3a33b..e8946ce86aaa 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
@@ -987,10 +987,14 @@ void gen9_disable_dc_states(struct intel_display *display)
 		return;
 	}
 
-	gen9_set_dc_state(display, DC_STATE_DISABLE);
-
-	if (!HAS_DISPLAY(display))
+	if (HAS_DISPLAY(display)) {
+		intel_dmc_wl_get_noreg(display);
+		gen9_set_dc_state(display, DC_STATE_DISABLE);
+		intel_dmc_wl_put_noreg(display);
+	} else {
+		gen9_set_dc_state(display, DC_STATE_DISABLE);
 		return;
+	}
 
 	intel_dmc_wl_disable(display);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index c298aef89449..5ed610c9be39 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -194,7 +194,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
 	if (!__intel_dmc_wl_supported(display))
 		return;
 
-	if (!intel_dmc_wl_check_range(reg.reg))
+	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
 		return;
 
 	spin_lock_irqsave(&wl->lock, flags);
@@ -246,7 +246,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
 	if (!__intel_dmc_wl_supported(display))
 		return;
 
-	if (!intel_dmc_wl_check_range(reg.reg))
+	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
 		return;
 
 	spin_lock_irqsave(&wl->lock, flags);
@@ -267,3 +267,13 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
 out_unlock:
 	spin_unlock_irqrestore(&wl->lock, flags);
 }
+
+void intel_dmc_wl_get_noreg(struct intel_display *display)
+{
+	intel_dmc_wl_get(display, INVALID_MMIO_REG);
+}
+
+void intel_dmc_wl_put_noreg(struct intel_display *display)
+{
+	intel_dmc_wl_put(display, INVALID_MMIO_REG);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.h b/drivers/gpu/drm/i915/display/intel_dmc_wl.h
index adab51208d0a..9aa72a4bf153 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.h
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.h
@@ -27,5 +27,7 @@ void intel_dmc_wl_enable(struct intel_display *display);
 void intel_dmc_wl_disable(struct intel_display *display);
 void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg);
 void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg);
+void intel_dmc_wl_get_noreg(struct intel_display *display);
+void intel_dmc_wl_put_noreg(struct intel_display *display);
 
 #endif /* __INTEL_WAKELOCK_H__ */
-- 
2.47.0

[PATCH 05/13] drm/i915/dmc_wl: Use sentinel item for range tables

[Gustavo Sousa]

We are currently using ARRAY_SIZE() to iterate address ranges in
intel_dmc_wl_check_range(). In upcoming changes, we will be using more
than a single table and will extract the range checking logic into a
dedicated function that takes a range table as argument. As we will not
able to use ARRAY_SIZE() then, let's make range tables contain a
sentinel item at the end and use that instead of having to pass the size
as parameter in this future function.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc_wl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index 5ed610c9be39..82eb9166e5f8 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -49,6 +49,7 @@ struct intel_dmc_wl_range {
 
 static struct intel_dmc_wl_range lnl_wl_range[] = {
 	{ .start = 0x60000, .end = 0x7ffff },
+	{},
 };
 
 static void __intel_dmc_wl_release(struct intel_display *display)
@@ -99,7 +100,7 @@ static bool intel_dmc_wl_check_range(u32 address)
 	int i;
 	bool wl_needed = false;
 
-	for (i = 0; i < ARRAY_SIZE(lnl_wl_range); i++) {
+	for (i = 0; lnl_wl_range[i].start; i++) {
 		if (address >= lnl_wl_range[i].start &&
 		    address <= lnl_wl_range[i].end) {
 			wl_needed = true;
-- 
2.47.0

[PATCH 06/13] drm/i915/dmc_wl: Extract intel_dmc_wl_addr_in_range()

[Gustavo Sousa]

We will be using more than one range table in
intel_dmc_wl_check_range(). As such, move the logic to a new function
and name it intel_dmc_wl_addr_in_range().

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc_wl.c | 22 ++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index 82eb9166e5f8..d597cc825f64 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -95,20 +95,20 @@ static void intel_dmc_wl_work(struct work_struct *work)
 	spin_unlock_irqrestore(&wl->lock, flags);
 }
 
-static bool intel_dmc_wl_check_range(u32 address)
+static bool intel_dmc_wl_addr_in_range(u32 address,
+				       const struct intel_dmc_wl_range ranges[])
 {
-	int i;
-	bool wl_needed = false;
-
-	for (i = 0; lnl_wl_range[i].start; i++) {
-		if (address >= lnl_wl_range[i].start &&
-		    address <= lnl_wl_range[i].end) {
-			wl_needed = true;
-			break;
-		}
+	for (int i = 0; ranges[i].start; i++) {
+		if (ranges[i].start <= address && address <= ranges[i].end)
+			return true;
 	}
 
-	return wl_needed;
+	return false;
+}
+
+static bool intel_dmc_wl_check_range(u32 address)
+{
+	return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
 }
 
 static bool __intel_dmc_wl_supported(struct intel_display *display)
-- 
2.47.0

[PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Gustavo Sousa]

There are extra registers that require the DMC wakelock when specific
dynamic DC states are in place. Add the table ranges for them and use
the correct table depending on the allowed DC states.

Bspec: 71583
Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
 1 file changed, 108 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index d597cc825f64..8bf2f32be859 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -5,6 +5,7 @@
 
 #include <linux/kernel.h>
 
+#include "i915_reg.h"
 #include "intel_de.h"
 #include "intel_dmc.h"
 #include "intel_dmc_regs.h"
@@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
 	{},
 };
 
+static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
+	{ .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
+	{ .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
+	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
+	{ .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
+	{ .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
+
+	/* DBUF_CTL_* */
+	{ .start = 0x44300, .end = 0x44300 },
+	{ .start = 0x44304, .end = 0x44304 },
+	{ .start = 0x44f00, .end = 0x44f00 },
+	{ .start = 0x44f04, .end = 0x44f04 },
+	{ .start = 0x44fe8, .end = 0x44fe8 },
+	{ .start = 0x45008, .end = 0x45008 },
+
+	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
+	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
+	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
+
+	/* TRANS_CMTG_CTL_* */
+	{ .start = 0x6fa88, .end = 0x6fa88 },
+	{ .start = 0x6fb88, .end = 0x6fb88 },
+
+	{ .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
+	{ .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
+	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
+	{ .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
+	{ .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
+	{ .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
+	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
+
+	{},
+};
+
+static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
+	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
+
+	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
+
+	/* DBUF_CTL_* */
+	{ .start = 0x44300, .end = 0x44300 },
+	{ .start = 0x44304, .end = 0x44304 },
+	{ .start = 0x44f00, .end = 0x44f00 },
+	{ .start = 0x44f04, .end = 0x44f04 },
+	{ .start = 0x44fe8, .end = 0x44fe8 },
+	{ .start = 0x45008, .end = 0x45008 },
+
+	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
+	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
+	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
+	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
+
+	/* Scanline registers */
+	{ .start = 0x70000, .end = 0x70000 },
+	{ .start = 0x70004, .end = 0x70004 },
+	{ .start = 0x70014, .end = 0x70014 },
+	{ .start = 0x70018, .end = 0x70018 },
+	{ .start = 0x71000, .end = 0x71000 },
+	{ .start = 0x71004, .end = 0x71004 },
+	{ .start = 0x71014, .end = 0x71014 },
+	{ .start = 0x71018, .end = 0x71018 },
+	{ .start = 0x72000, .end = 0x72000 },
+	{ .start = 0x72004, .end = 0x72004 },
+	{ .start = 0x72014, .end = 0x72014 },
+	{ .start = 0x72018, .end = 0x72018 },
+	{ .start = 0x73000, .end = 0x73000 },
+	{ .start = 0x73004, .end = 0x73004 },
+	{ .start = 0x73014, .end = 0x73014 },
+	{ .start = 0x73018, .end = 0x73018 },
+	{ .start = 0x7b000, .end = 0x7b000 },
+	{ .start = 0x7b004, .end = 0x7b004 },
+	{ .start = 0x7b014, .end = 0x7b014 },
+	{ .start = 0x7b018, .end = 0x7b018 },
+	{ .start = 0x7c000, .end = 0x7c000 },
+	{ .start = 0x7c004, .end = 0x7c004 },
+	{ .start = 0x7c014, .end = 0x7c014 },
+	{ .start = 0x7c018, .end = 0x7c018 },
+
+	{},
+};
+
 static void __intel_dmc_wl_release(struct intel_display *display)
 {
 	struct drm_i915_private *i915 = to_i915(display->drm);
@@ -106,9 +188,31 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
 	return false;
 }
 
-static bool intel_dmc_wl_check_range(u32 address)
+static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
 {
-	return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
+	const struct intel_dmc_wl_range *ranges;
+
+	ranges = lnl_wl_range;
+
+	if (intel_dmc_wl_addr_in_range(address, ranges))
+		return true;
+
+	switch (display->power.domains.dc_state) {
+	case DC_STATE_EN_DC3CO:
+		ranges = xe3lpd_dc3co_wl_ranges;
+		break;
+	case DC_STATE_EN_UPTO_DC5:
+	case DC_STATE_EN_UPTO_DC6:
+		ranges = xe3lpd_dc5_dc6_wl_ranges;
+		break;
+	default:
+		ranges = NULL;
+	}
+
+	if (ranges && intel_dmc_wl_addr_in_range(address, ranges))
+		return true;
+
+	return false;
 }
 
 static bool __intel_dmc_wl_supported(struct intel_display *display)
@@ -195,7 +299,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
 	if (!__intel_dmc_wl_supported(display))
 		return;
 
-	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
+	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
 		return;
 
 	spin_lock_irqsave(&wl->lock, flags);
@@ -247,7 +351,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
 	if (!__intel_dmc_wl_supported(display))
 		return;
 
-	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
+	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
 		return;
 
 	spin_lock_irqsave(&wl->lock, flags);
-- 
2.47.0

[PATCH 08/13] drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables

[Gustavo Sousa]

Allow simpler syntax for defining entries for single registers in range
tables. That makes them easier to type as well as to read, allowing one
to quickly tell whether a range actually refers to a single register or
a "true range".

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc_wl.c | 118 ++++++++++----------
 1 file changed, 60 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index 8bf2f32be859..6992ce654e75 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -54,82 +54,82 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
 };
 
 static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
-	{ .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
+	{ .start = 0x45500 }, /* DC_STATE_SEL */
 	{ .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
-	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
+	{ .start = 0x45504 }, /* DC_STATE_EN */
 	{ .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
-	{ .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
+	{ .start = 0x454f0 }, /* RETENTION_CTRL */
 
 	/* DBUF_CTL_* */
-	{ .start = 0x44300, .end = 0x44300 },
-	{ .start = 0x44304, .end = 0x44304 },
-	{ .start = 0x44f00, .end = 0x44f00 },
-	{ .start = 0x44f04, .end = 0x44f04 },
-	{ .start = 0x44fe8, .end = 0x44fe8 },
-	{ .start = 0x45008, .end = 0x45008 },
+	{ .start = 0x44300 },
+	{ .start = 0x44304 },
+	{ .start = 0x44f00 },
+	{ .start = 0x44f04 },
+	{ .start = 0x44fe8 },
+	{ .start = 0x45008 },
 
-	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
-	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
-	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
+	{ .start = 0x46070 }, /* CDCLK_PLL_ENABLE */
+	{ .start = 0x46000 }, /* CDCLK_CTL */
+	{ .start = 0x46008 }, /* CDCLK_SQUASH_CTL */
 
 	/* TRANS_CMTG_CTL_* */
-	{ .start = 0x6fa88, .end = 0x6fa88 },
-	{ .start = 0x6fb88, .end = 0x6fb88 },
-
-	{ .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
-	{ .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
-	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
-	{ .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
-	{ .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
-	{ .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
+	{ .start = 0x6fa88 },
+	{ .start = 0x6fb88 },
+
+	{ .start = 0x46430 }, /* CHICKEN_DCPR_1 */
+	{ .start = 0x46434 }, /* CHICKEN_DCPR_2 */
+	{ .start = 0x454a0 }, /* CHICKEN_DCPR_4 */
+	{ .start = 0x42084 }, /* CHICKEN_MISC_2 */
+	{ .start = 0x42088 }, /* CHICKEN_MISC_3 */
+	{ .start = 0x46160 }, /* CMTG_CLK_SEL */
 	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
 
 	{},
 };
 
 static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
-	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
+	{ .start = 0x454a0 }, /* CHICKEN_DCPR_4 */
 
-	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
+	{ .start = 0x45504 }, /* DC_STATE_EN */
 
 	/* DBUF_CTL_* */
-	{ .start = 0x44300, .end = 0x44300 },
-	{ .start = 0x44304, .end = 0x44304 },
-	{ .start = 0x44f00, .end = 0x44f00 },
-	{ .start = 0x44f04, .end = 0x44f04 },
-	{ .start = 0x44fe8, .end = 0x44fe8 },
-	{ .start = 0x45008, .end = 0x45008 },
-
-	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
-	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
-	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
+	{ .start = 0x44300 },
+	{ .start = 0x44304 },
+	{ .start = 0x44f00 },
+	{ .start = 0x44f04 },
+	{ .start = 0x44fe8 },
+	{ .start = 0x45008 },
+
+	{ .start = 0x46070 }, /* CDCLK_PLL_ENABLE */
+	{ .start = 0x46000 }, /* CDCLK_CTL */
+	{ .start = 0x46008 }, /* CDCLK_SQUASH_CTL */
 	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
 
 	/* Scanline registers */
-	{ .start = 0x70000, .end = 0x70000 },
-	{ .start = 0x70004, .end = 0x70004 },
-	{ .start = 0x70014, .end = 0x70014 },
-	{ .start = 0x70018, .end = 0x70018 },
-	{ .start = 0x71000, .end = 0x71000 },
-	{ .start = 0x71004, .end = 0x71004 },
-	{ .start = 0x71014, .end = 0x71014 },
-	{ .start = 0x71018, .end = 0x71018 },
-	{ .start = 0x72000, .end = 0x72000 },
-	{ .start = 0x72004, .end = 0x72004 },
-	{ .start = 0x72014, .end = 0x72014 },
-	{ .start = 0x72018, .end = 0x72018 },
-	{ .start = 0x73000, .end = 0x73000 },
-	{ .start = 0x73004, .end = 0x73004 },
-	{ .start = 0x73014, .end = 0x73014 },
-	{ .start = 0x73018, .end = 0x73018 },
-	{ .start = 0x7b000, .end = 0x7b000 },
-	{ .start = 0x7b004, .end = 0x7b004 },
-	{ .start = 0x7b014, .end = 0x7b014 },
-	{ .start = 0x7b018, .end = 0x7b018 },
-	{ .start = 0x7c000, .end = 0x7c000 },
-	{ .start = 0x7c004, .end = 0x7c004 },
-	{ .start = 0x7c014, .end = 0x7c014 },
-	{ .start = 0x7c018, .end = 0x7c018 },
+	{ .start = 0x70000 },
+	{ .start = 0x70004 },
+	{ .start = 0x70014 },
+	{ .start = 0x70018 },
+	{ .start = 0x71000 },
+	{ .start = 0x71004 },
+	{ .start = 0x71014 },
+	{ .start = 0x71018 },
+	{ .start = 0x72000 },
+	{ .start = 0x72004 },
+	{ .start = 0x72014 },
+	{ .start = 0x72018 },
+	{ .start = 0x73000 },
+	{ .start = 0x73004 },
+	{ .start = 0x73014 },
+	{ .start = 0x73018 },
+	{ .start = 0x7b000 },
+	{ .start = 0x7b004 },
+	{ .start = 0x7b014 },
+	{ .start = 0x7b018 },
+	{ .start = 0x7c000 },
+	{ .start = 0x7c004 },
+	{ .start = 0x7c014 },
+	{ .start = 0x7c018 },
 
 	{},
 };
@@ -181,7 +181,9 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
 				       const struct intel_dmc_wl_range ranges[])
 {
 	for (int i = 0; ranges[i].start; i++) {
-		if (ranges[i].start <= address && address <= ranges[i].end)
+		u32 end = ranges[i].end ?: ranges[i].start;
+
+		if (ranges[i].start <= address && address <= end)
 			return true;
 	}
 
-- 
2.47.0

[PATCH 09/13] drm/i915/dmc_wl: Deal with existing references when disabling

[Gustavo Sousa]

It is possible that there are active wakelock references at the time we
are disabling the DMC wakelock mechanism. We need to deal with that in
two ways:

(A) Implement the missing step from Bspec:

    The Bspec instructs us to clear any existing wakelock request bit
    after disabling the mechanism. That gives a clue that it is okay to
    disable while there are locks held and we do not need to wait for
    them. However, since the spec is not explicit about it, we need
    still to get confirmation with the hardware team. Let's thus
    implement the spec and add a TODO note.

(B) Ensure a consistent driver state:

    The enable/disable logic would be problematic if the following
    sequence of events would happen:

    1. Function A calls intel_dmc_wl_get();
    2. Some function calls intel_dmc_wl_disable();
    3. Some function calls intel_dmc_wl_enable();
    4. Function A is done and calls intel_dmc_wl_put().

    At (2), the refcount becomes zero and then (4) causes an invalid
    decrement to the refcount. That would cause some issues:

        - At the time between (3) and (4), function A would think that
          the hardware lock is held but it could not be really held
          until intel_dmc_wl_get() is called by something else.
        - The call made to (4) could cause the refcount to become zero
          and consequently the hardware lock to be released while there
          could be innocent paths trusting they still have the lock.

    To fix that, we need to keep the refcount correctly in sync with
    intel_dmc_wl_{get,put}() calls and retake the hardware lock when
    enabling the DMC wakelock with a non-zero refcount.

    One missing piece left to be handled here is the following scenario:

    1. Function A calls intel_dmc_wl_get();
    2. Some function calls intel_dmc_wl_disable();
    3. Some function calls intel_dmc_wl_enable();
    4. Concurrently with (3), function A performs the MMIO in between
       setting DMC_WAKELOCK_CFG_ENABLE and asserting the lock with
       __intel_dmc_wl_take().

    I'm mostly sure this would cause issues future display IPs if DMC
    trap implementation was completely removed. We need to check with
    the hardware team whether it would be safe to assert the hardware
    lock before setting DMC_WAKELOCK_CFG_ENABLE to avoid this scenario.
    If not, then we would have to deal with that via software
    synchronization.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc_wl.c | 97 ++++++++++++++-------
 1 file changed, 67 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index 6992ce654e75..e27c06b7c42f 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -177,6 +177,37 @@ static void intel_dmc_wl_work(struct work_struct *work)
 	spin_unlock_irqrestore(&wl->lock, flags);
 }
 
+static void __intel_dmc_wl_take(struct intel_display *display)
+{
+	struct intel_dmc_wl *wl = &display->wl;
+
+	/*
+	 * Only try to take the wakelock if it's not marked as taken
+	 * yet.  It may be already taken at this point if we have
+	 * already released the last reference, but the work has not
+	 * run yet.
+	 */
+	if (wl->taken)
+		return;
+
+	__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, 0,
+			    DMC_WAKELOCK_CTL_REQ);
+
+	/*
+	 * We need to use the atomic variant of the waiting routine
+	 * because the DMC wakelock is also taken in atomic context.
+	 */
+	if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
+						     DMC_WAKELOCK_CTL_ACK,
+						     DMC_WAKELOCK_CTL_ACK,
+						     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
+		WARN_RATELIMIT(1, "DMC wakelock ack timed out");
+		return;
+	}
+
+	wl->taken = true;
+}
+
 static bool intel_dmc_wl_addr_in_range(u32 address,
 				       const struct intel_dmc_wl_range ranges[])
 {
@@ -261,7 +292,23 @@ void intel_dmc_wl_enable(struct intel_display *display)
 	__intel_de_rmw_nowl(display, DMC_WAKELOCK_CFG, 0, DMC_WAKELOCK_CFG_ENABLE);
 
 	wl->enabled = true;
-	wl->taken = false;
+
+	/*
+	 * This would be racy in the following scenario:
+	 *
+	 *   1. Function A calls intel_dmc_wl_get();
+	 *   2. Some function calls intel_dmc_wl_disable();
+	 *   3. Some function calls intel_dmc_wl_enable();
+	 *   4. Concurrently with (3), function A performs the MMIO in between
+	 *      setting DMC_WAKELOCK_CFG_ENABLE and asserting the lock with
+	 *      __intel_dmc_wl_take().
+	 *
+	 * TODO: Check with the hardware team whether it is safe to assert the
+	 * hardware lock before enabling to avoid such a scenario. Otherwise, we
+	 * would need to deal with it via software synchronization.
+	 */
+	if (refcount_read(&wl->refcount))
+		__intel_dmc_wl_take(display);
 
 out_unlock:
 	spin_unlock_irqrestore(&wl->lock, flags);
@@ -285,8 +332,18 @@ void intel_dmc_wl_disable(struct intel_display *display)
 	/* Disable wakelock in DMC */
 	__intel_de_rmw_nowl(display, DMC_WAKELOCK_CFG, DMC_WAKELOCK_CFG_ENABLE, 0);
 
-	refcount_set(&wl->refcount, 0);
 	wl->enabled = false;
+
+	/*
+	 * The spec is not explicit about the expectation of existing
+	 * lock users at the moment of disabling, but it does say that we must
+	 * clear DMC_WAKELOCK_CTL_REQ, which gives us a clue that it is okay to
+	 * disable with existing lock users.
+	 *
+	 * TODO: Get the correct expectation from the hardware team.
+	 */
+	__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0);
+
 	wl->taken = false;
 
 out_unlock:
@@ -306,8 +363,11 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
 
 	spin_lock_irqsave(&wl->lock, flags);
 
-	if (!wl->enabled)
+	if (!wl->enabled) {
+		if (!refcount_inc_not_zero(&wl->refcount))
+			refcount_set(&wl->refcount, 1);
 		goto out_unlock;
+	}
 
 	cancel_delayed_work(&wl->work);
 
@@ -316,30 +376,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
 
 	refcount_set(&wl->refcount, 1);
 
-	/*
-	 * Only try to take the wakelock if it's not marked as taken
-	 * yet.  It may be already taken at this point if we have
-	 * already released the last reference, but the work has not
-	 * run yet.
-	 */
-	if (!wl->taken) {
-		__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, 0,
-				    DMC_WAKELOCK_CTL_REQ);
-
-		/*
-		 * We need to use the atomic variant of the waiting routine
-		 * because the DMC wakelock is also taken in atomic context.
-		 */
-		if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
-							     DMC_WAKELOCK_CTL_ACK,
-							     DMC_WAKELOCK_CTL_ACK,
-							     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
-			WARN_RATELIMIT(1, "DMC wakelock ack timed out");
-			goto out_unlock;
-		}
-
-		wl->taken = true;
-	}
+	__intel_dmc_wl_take(display);
 
 out_unlock:
 	spin_unlock_irqrestore(&wl->lock, flags);
@@ -358,14 +395,14 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
 
 	spin_lock_irqsave(&wl->lock, flags);
 
-	if (!wl->enabled)
-		goto out_unlock;
-
 	if (WARN_RATELIMIT(!refcount_read(&wl->refcount),
 			   "Tried to put wakelock with refcount zero\n"))
 		goto out_unlock;
 
 	if (refcount_dec_and_test(&wl->refcount)) {
+		if (!wl->enabled)
+			goto out_unlock;
+
 		__intel_dmc_wl_release(display);
 
 		goto out_unlock;
-- 
2.47.0

[PATCH 10/13] drm/i915/dmc_wl: Couple enable/disable with dynamic DC states

[Gustavo Sousa]

Enabling and disabling the DMC wakelock should be done as part of
enabling and disabling of dynamic DC states, respectively. We should not
enable or disable DMC wakelock independently of DC states, otherwise we
would risk ending up with an inconsistent state where dynamic DC states
are enabled and the DMC wakelock is disabled, going against current
recommendations and making MMIO transactions potentially slower. In
future display IPs that could have a worse outcome if DMC trap
implementation is completely removed.

So, let's make things safer by tying stuff together, removing the
independent calls, and also put warnings in place to detect inconsistent
calls.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_power_well.c | 5 ++++-
 drivers/gpu/drm/i915/display/intel_dmc.c                | 4 ----
 drivers/gpu/drm/i915/display/intel_dmc_wl.c             | 6 ++++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
index e8946ce86aaa..1a6c93170a5a 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
@@ -981,6 +981,7 @@ void gen9_disable_dc_states(struct intel_display *display)
 	struct drm_i915_private *dev_priv = to_i915(display->drm);
 	struct i915_power_domains *power_domains = &display->power.domains;
 	struct intel_cdclk_config cdclk_config = {};
+	u32 old_state = power_domains->dc_state;
 
 	if (power_domains->target_dc_state == DC_STATE_EN_DC3CO) {
 		tgl_disable_dc3co(display);
@@ -996,7 +997,9 @@ void gen9_disable_dc_states(struct intel_display *display)
 		return;
 	}
 
-	intel_dmc_wl_disable(display);
+	if (old_state == DC_STATE_EN_UPTO_DC5 ||
+	    old_state == DC_STATE_EN_UPTO_DC6)
+		intel_dmc_wl_disable(display);
 
 	intel_cdclk_get_cdclk(display, &cdclk_config);
 	/* Can't read out voltage_level so can't use intel_cdclk_changed() */
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 48bbbf8f312c..f0b12c609884 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -632,8 +632,6 @@ void intel_dmc_disable_program(struct intel_display *display)
 	pipedmc_clock_gating_wa(display, true);
 	disable_all_event_handlers(display);
 	pipedmc_clock_gating_wa(display, false);
-
-	intel_dmc_wl_disable(display);
 }
 
 void assert_dmc_loaded(struct intel_display *display)
@@ -1140,8 +1138,6 @@ void intel_dmc_suspend(struct intel_display *display)
 	if (dmc)
 		flush_work(&dmc->work);
 
-	intel_dmc_wl_disable(display);
-
 	/* Drop the reference held in case DMC isn't loaded. */
 	if (!intel_dmc_has_payload(display))
 		intel_dmc_runtime_pm_put(display);
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index e27c06b7c42f..8283b607aac4 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -271,6 +271,7 @@ void intel_dmc_wl_init(struct intel_display *display)
 	refcount_set(&wl->refcount, 0);
 }
 
+/* Must only be called as part of enabling dynamic DC states. */
 void intel_dmc_wl_enable(struct intel_display *display)
 {
 	struct intel_dmc_wl *wl = &display->wl;
@@ -281,7 +282,7 @@ void intel_dmc_wl_enable(struct intel_display *display)
 
 	spin_lock_irqsave(&wl->lock, flags);
 
-	if (wl->enabled)
+	if (drm_WARN_ON(display->drm, wl->enabled))
 		goto out_unlock;
 
 	/*
@@ -314,6 +315,7 @@ void intel_dmc_wl_enable(struct intel_display *display)
 	spin_unlock_irqrestore(&wl->lock, flags);
 }
 
+/* Must only be called as part of disabling dynamic DC states. */
 void intel_dmc_wl_disable(struct intel_display *display)
 {
 	struct intel_dmc_wl *wl = &display->wl;
@@ -326,7 +328,7 @@ void intel_dmc_wl_disable(struct intel_display *display)
 
 	spin_lock_irqsave(&wl->lock, flags);
 
-	if (!wl->enabled)
+	if (drm_WARN_ON(display->drm, !wl->enabled))
 		goto out_unlock;
 
 	/* Disable wakelock in DMC */
-- 
2.47.0

[PATCH 11/13] drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()

[Gustavo Sousa]

In order to be able to use the DMC wakelock, we also need to know that
the display hardware has support for DMC, which is a runtime info.
Define HAS_DMC_WAKELOCK(), which checks for both DMC availability and IP
version, and use it in place of directly checking the display version.

Since we depend on runtime info, also make sure to call
intel_dmc_wl_init() only after we have probed the hardware for such info
(i.e. after intel_display_device_info_runtime_init()).

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
 drivers/gpu/drm/i915/display/intel_display_driver.c | 2 +-
 drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
index 071a36b51f79..5f78fd127fe0 100644
--- a/drivers/gpu/drm/i915/display/intel_display_device.h
+++ b/drivers/gpu/drm/i915/display/intel_display_device.h
@@ -128,6 +128,7 @@ enum intel_display_subplatform {
 #define HAS_DDI(i915)			(DISPLAY_INFO(i915)->has_ddi)
 #define HAS_DISPLAY(i915)		(DISPLAY_RUNTIME_INFO(i915)->pipe_mask != 0)
 #define HAS_DMC(i915)			(DISPLAY_RUNTIME_INFO(i915)->has_dmc)
+#define HAS_DMC_WAKELOCK(i915)		(HAS_DMC(i915) && DISPLAY_VER(i915) >= 20)
 #define HAS_DOUBLE_BUFFERED_M_N(i915)	(DISPLAY_VER(i915) >= 9 || IS_BROADWELL(i915))
 #define HAS_DP_MST(i915)		(DISPLAY_INFO(i915)->has_dp_mst)
 #define HAS_DP20(i915)			(IS_DG2(i915) || DISPLAY_VER(i915) >= 14)
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index 673f9b965494..8afaa9cb89d2 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -200,7 +200,6 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915)
 	intel_dpll_init_clock_hook(i915);
 	intel_init_display_hooks(i915);
 	intel_fdi_init_hook(i915);
-	intel_dmc_wl_init(&i915->display);
 }
 
 /* part #1: call before irq install */
@@ -238,6 +237,7 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
 		return 0;
 
 	intel_dmc_init(display);
+	intel_dmc_wl_init(display);
 
 	i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
 	i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index 8283b607aac4..f6ec79b0e39d 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -250,7 +250,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
 
 static bool __intel_dmc_wl_supported(struct intel_display *display)
 {
-	if (DISPLAY_VER(display) < 20 ||
+	if (!HAS_DMC_WAKELOCK(display) ||
 	    !intel_dmc_has_payload(display) ||
 	    !display->params.enable_dmc_wl)
 		return false;
@@ -263,7 +263,7 @@ void intel_dmc_wl_init(struct intel_display *display)
 	struct intel_dmc_wl *wl = &display->wl;
 
 	/* don't call __intel_dmc_wl_supported(), DMC is not loaded yet */
-	if (DISPLAY_VER(display) < 20 || !display->params.enable_dmc_wl)
+	if (!HAS_DMC_WAKELOCK(display) || !display->params.enable_dmc_wl)
 		return;
 
 	INIT_DELAYED_WORK(&wl->work, intel_dmc_wl_work);
-- 
2.47.0

[PATCH 12/13] drm/i915/dmc_wl: Sanitize enable_dmc_wl according to hardware support

[Gustavo Sousa]

Instead of checking for HAS_DMC_WAKELOCK() multiple times, let's use it
to sanitize the enable_dmc_wl parameter and use that variable when
necessary.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dmc_wl.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index f6ec79b0e39d..55f07f3c9863 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -5,6 +5,8 @@
 
 #include <linux/kernel.h>
 
+#include <drm/drm_print.h>
+
 #include "i915_reg.h"
 #include "intel_de.h"
 #include "intel_dmc.h"
@@ -250,20 +252,25 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
 
 static bool __intel_dmc_wl_supported(struct intel_display *display)
 {
-	if (!HAS_DMC_WAKELOCK(display) ||
-	    !intel_dmc_has_payload(display) ||
-	    !display->params.enable_dmc_wl)
-		return false;
+	return display->params.enable_dmc_wl && intel_dmc_has_payload(display);
+}
 
-	return true;
+static void intel_dmc_wl_sanitize_param(struct intel_display *display)
+{
+	if (!HAS_DMC_WAKELOCK(display))
+		display->params.enable_dmc_wl = false;
+
+	drm_dbg_kms(display->drm, "Sanitized enable_dmc_wl value: %d\n",
+		    display->params.enable_dmc_wl);
 }
 
 void intel_dmc_wl_init(struct intel_display *display)
 {
 	struct intel_dmc_wl *wl = &display->wl;
 
-	/* don't call __intel_dmc_wl_supported(), DMC is not loaded yet */
-	if (!HAS_DMC_WAKELOCK(display) || !display->params.enable_dmc_wl)
+	intel_dmc_wl_sanitize_param(display);
+
+	if (!display->params.enable_dmc_wl)
 		return;
 
 	INIT_DELAYED_WORK(&wl->work, intel_dmc_wl_work);
-- 
2.47.0

[PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default

[Gustavo Sousa]

Although Bspec doesn't explicitly mentions that, as of Xe3_LPD, using
DMC wakelock is the officially recommended way of accessing registers
that would be off during DC5/DC6 and the legacy method (where the DMC
intercepts MMIO to wake up the hardware) is to be avoided.

As such, update the driver to use the DMC wakelock by default starting
with Xe3_LPD. Since the feature is somewhat new to the driver, also
allow disabling it via a module parameter for debugging purposes.

For that, make the existing parameter allow values -1 (per-chip
default), 0 (disabled) and 1 (enabled), similarly to what is done for
other parameters.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
 drivers/gpu/drm/i915/display/intel_display_params.h | 2 +-
 drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 6 +++++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
index 024de8abcb1a..bf00e5f1f145 100644
--- a/drivers/gpu/drm/i915/display/intel_display_params.c
+++ b/drivers/gpu/drm/i915/display/intel_display_params.c
@@ -123,10 +123,10 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
 	"(0=disabled, 1=enabled) "
 	"Default: 1");
 
-intel_display_param_named_unsafe(enable_dmc_wl, bool, 0400,
+intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
 	"Enable DMC wakelock "
 	"(0=disabled, 1=enabled) "
-	"Default: 0");
+	"Default: -1 (use per-chip default)");
 
 __maybe_unused
 static void _param_print_bool(struct drm_printer *p, const char *driver_name,
diff --git a/drivers/gpu/drm/i915/display/intel_display_params.h b/drivers/gpu/drm/i915/display/intel_display_params.h
index dcb6face936a..5317138e6044 100644
--- a/drivers/gpu/drm/i915/display/intel_display_params.h
+++ b/drivers/gpu/drm/i915/display/intel_display_params.h
@@ -47,7 +47,7 @@ struct drm_printer;
 	param(int, enable_psr, -1, 0600) \
 	param(bool, psr_safest_params, false, 0400) \
 	param(bool, enable_psr2_sel_fetch, true, 0400) \
-	param(bool, enable_dmc_wl, false, 0400) \
+	param(int, enable_dmc_wl, -1, 0400) \
 
 #define MEMBER(T, member, ...) T member;
 struct intel_display_params {
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
index 55f07f3c9863..f58031811e79 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
@@ -258,7 +258,11 @@ static bool __intel_dmc_wl_supported(struct intel_display *display)
 static void intel_dmc_wl_sanitize_param(struct intel_display *display)
 {
 	if (!HAS_DMC_WAKELOCK(display))
-		display->params.enable_dmc_wl = false;
+		display->params.enable_dmc_wl = 0;
+	else if (display->params.enable_dmc_wl >= 0)
+		display->params.enable_dmc_wl = !!display->params.enable_dmc_wl;
+	else
+		display->params.enable_dmc_wl = DISPLAY_VER(display) >= 30;
 
 	drm_dbg_kms(display->drm, "Sanitized enable_dmc_wl value: %d\n",
 		    display->params.enable_dmc_wl);
-- 
2.47.0

✗ Fi.CI.CHECKPATCH: warning for drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD

[Patchwork]

== Series Details ==

Series: drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD
URL   : https://patchwork.freedesktop.org/series/140282/
State : warning

== Summary ==

Error: dim checkpatch failed
22a0e1dad66b drm/xe: Mimic i915 behavior for non-sleeping MMIO wait
21b66390096d drm/i915/dmc_wl: Use non-sleeping variant of MMIO wait
-:33: WARNING:LONG_LINE: line length of 134 exceeds 100 columns
#33: FILE: drivers/gpu/drm/i915/display/intel_de.h:133:
+#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)

-:33: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
#33: FILE: drivers/gpu/drm/i915/display/intel_de.h:133:
+}
+#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)

-:33: ERROR:SPACING: space required after that ',' (ctx:VxV)
#33: FILE: drivers/gpu/drm/i915/display/intel_de.h:133:
+#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)
                                                   ^

total: 1 errors, 1 warnings, 1 checks, 55 lines checked
87054766f121 drm/i915/dmc_wl: Check for non-zero refcount in release work
504337ac549f drm/i915/dmc_wl: Get wakelock when disabling dynamic DC states
6f7e7bfc4fc1 drm/i915/dmc_wl: Use sentinel item for range tables
ed1087ec8d7b drm/i915/dmc_wl: Extract intel_dmc_wl_addr_in_range()
a9d0463d8f1a drm/i915/dmc_wl: Check ranges specific to DC states
f843b90ccfa9 drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables
1b4fb02d409c drm/i915/dmc_wl: Deal with existing references when disabling
e54a65822ae0 drm/i915/dmc_wl: Couple enable/disable with dynamic DC states
ad28b8ed1d2d drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()
-:25: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'i915' - possible side-effects?
#25: FILE: drivers/gpu/drm/i915/display/intel_display_device.h:131:
+#define HAS_DMC_WAKELOCK(i915)		(HAS_DMC(i915) && DISPLAY_VER(i915) >= 20)

total: 0 errors, 0 warnings, 1 checks, 37 lines checked
c90342162700 drm/i915/dmc_wl: Sanitize enable_dmc_wl according to hardware support
aef567f023a4 drm/i915/xe3lpd: Use DMC wakelock by default
-:31: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#31: FILE: drivers/gpu/drm/i915/display/intel_display_params.c:127:
+intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
 	"Enable DMC wakelock "

total: 0 errors, 0 warnings, 1 checks, 32 lines checked

✗ Fi.CI.SPARSE: warning for drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD

[Patchwork]

== Series Details ==

Series: drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD
URL   : https://patchwork.freedesktop.org/series/140282/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ Fi.CI.BAT: failure for drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 11609 bytes --]

== Series Details ==

Series: drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD
URL   : https://patchwork.freedesktop.org/series/140282/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15574 -> Patchwork_140282v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_140282v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_140282v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/index.html

Participating hosts (44 -> 39)
------------------------------

  Additional (1): bat-arls-2 
  Missing    (6): bat-dg1-7 bat-dg2-8 bat-dg2-9 fi-snb-2520m bat-dg2-14 bat-dg2-11 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_140282v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - fi-glk-j4005:       [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/fi-glk-j4005/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/fi-glk-j4005/igt@i915_selftest@live@execlists.html

  
Known issues
------------

  Here are the changes found in Patchwork_140282v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-arls-2:         NOTRUN -> [SKIP][3] ([i915#9318])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@debugfs_test@basic-hwmon.html

  * igt@gem_lmem_swapping@verify-random:
    - bat-arls-2:         NOTRUN -> [SKIP][4] ([i915#10213] / [i915#11671]) +3 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@gem_lmem_swapping@verify-random.html

  * igt@gem_mmap@basic:
    - bat-arls-2:         NOTRUN -> [SKIP][5] ([i915#11343] / [i915#4083])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-arls-2:         NOTRUN -> [SKIP][6] ([i915#10196] / [i915#4077]) +2 other tests skip
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-arls-2:         NOTRUN -> [SKIP][7] ([i915#10197] / [i915#10211] / [i915#4079])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-arls-2:         NOTRUN -> [SKIP][8] ([i915#10206] / [i915#4079])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-arls-2:         NOTRUN -> [SKIP][9] ([i915#10209] / [i915#11681])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live:
    - bat-arlh-3:         [PASS][10] -> [ABORT][11] ([i915#12133])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/bat-arlh-3/igt@i915_selftest@live.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arlh-3/igt@i915_selftest@live.html
    - fi-glk-j4005:       [PASS][12] -> [ABORT][13] ([i915#12133])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/fi-glk-j4005/igt@i915_selftest@live.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/fi-glk-j4005/igt@i915_selftest@live.html
    - bat-arlh-2:         [PASS][14] -> [ABORT][15] ([i915#12133])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/bat-arlh-2/igt@i915_selftest@live.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arlh-2/igt@i915_selftest@live.html

  * igt@i915_selftest@live@workarounds:
    - bat-arlh-3:         [PASS][16] -> [ABORT][17] ([i915#12061])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/bat-arlh-3/igt@i915_selftest@live@workarounds.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arlh-3/igt@i915_selftest@live@workarounds.html
    - bat-arlh-2:         [PASS][18] -> [ABORT][19] ([i915#12061])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/bat-arlh-2/igt@i915_selftest@live@workarounds.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arlh-2/igt@i915_selftest@live@workarounds.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-arls-2:         NOTRUN -> [SKIP][20] ([i915#10200] / [i915#12203])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@framebuffer-vs-set-tiling:
    - bat-arls-2:         NOTRUN -> [SKIP][21] ([i915#10200]) +8 other tests skip
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html

  * igt@kms_chamelium_frames@dp-crc-fast:
    - bat-dg2-13:         [PASS][22] -> [DMESG-WARN][23] ([i915#12253])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/bat-dg2-13/igt@kms_chamelium_frames@dp-crc-fast.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-dg2-13/igt@kms_chamelium_frames@dp-crc-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-arls-2:         NOTRUN -> [SKIP][24] ([i915#10202] / [i915#11346]) +1 other test skip
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-arls-2:         NOTRUN -> [SKIP][25] ([i915#11346] / [i915#9886])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-arls-2:         NOTRUN -> [SKIP][26] ([i915#10207] / [i915#11346])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@psr-primary-mmap-gtt:
    - bat-arls-2:         NOTRUN -> [SKIP][27] ([i915#11346] / [i915#4077] / [i915#9688])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@kms_psr@psr-primary-mmap-gtt.html

  * igt@kms_psr@psr-primary-mmap-gtt@edp-1:
    - bat-arls-2:         NOTRUN -> [SKIP][28] ([i915#10196] / [i915#4077] / [i915#9688])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@kms_psr@psr-primary-mmap-gtt@edp-1.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-arls-2:         NOTRUN -> [SKIP][29] ([i915#10208] / [i915#8809])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-arls-2:         NOTRUN -> [SKIP][30] ([i915#10196] / [i915#3708] / [i915#4077]) +1 other test skip
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-fence-read:
    - bat-arls-2:         NOTRUN -> [SKIP][31] ([i915#10212] / [i915#3708])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@prime_vgem@basic-fence-read.html

  * igt@prime_vgem@basic-read:
    - bat-arls-2:         NOTRUN -> [SKIP][32] ([i915#10214] / [i915#3708])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-write:
    - bat-arls-2:         NOTRUN -> [SKIP][33] ([i915#10216] / [i915#3708])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-arls-2/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [ABORT][34] ([i915#12216]) -> [PASS][35] +1 other test pass
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/bat-mtlp-8/igt@i915_selftest@live.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-mtlp-8/igt@i915_selftest@live.html

  * igt@kms_chamelium_edid@hdmi-edid-read:
    - bat-dg2-13:         [DMESG-WARN][36] ([i915#12253]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15574/bat-dg2-13/igt@kms_chamelium_edid@hdmi-edid-read.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/bat-dg2-13/igt@kms_chamelium_edid@hdmi-edid-read.html

  
  [i915#10196]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10196
  [i915#10197]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10197
  [i915#10200]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10200
  [i915#10202]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10202
  [i915#10206]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10206
  [i915#10207]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10207
  [i915#10208]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10208
  [i915#10209]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10209
  [i915#10211]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10211
  [i915#10212]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10212
  [i915#10213]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10213
  [i915#10214]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10214
  [i915#10216]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10216
  [i915#11343]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11343
  [i915#11346]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11346
  [i915#11671]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11671
  [i915#11681]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11681
  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [i915#12133]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12133
  [i915#12203]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12203
  [i915#12216]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12216
  [i915#12253]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12253
  [i915#3708]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4083
  [i915#8809]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/8809
  [i915#9318]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9318
  [i915#9688]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9688
  [i915#9886]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9886


Build changes
-------------

  * Linux: CI_DRM_15574 -> Patchwork_140282v1

  CI-20190529: 20190529
  CI_DRM_15574: aa0898115bcff3eda6d021cc66eb8a1c3b264c56 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8080: 20fcbc59241a16c84d12f4f6ba390fb46fd65a36 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_140282v1: aa0898115bcff3eda6d021cc66eb8a1c3b264c56 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_140282v1/index.html

[-- Attachment #2: Type: text/html, Size: 14157 bytes --]

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Jani Nikula]

On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> There are extra registers that require the DMC wakelock when specific
> dynamic DC states are in place. Add the table ranges for them and use
> the correct table depending on the allowed DC states.
>
> Bspec: 71583
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>  1 file changed, 108 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index d597cc825f64..8bf2f32be859 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/kernel.h>
>  
> +#include "i915_reg.h"
>  #include "intel_de.h"
>  #include "intel_dmc.h"
>  #include "intel_dmc_regs.h"
> @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>  	{},
>  };
>  
> +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
> +	{ .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
> +	{ .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
> +	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> +	{ .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
> +	{ .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
> +
> +	/* DBUF_CTL_* */
> +	{ .start = 0x44300, .end = 0x44300 },
> +	{ .start = 0x44304, .end = 0x44304 },
> +	{ .start = 0x44f00, .end = 0x44f00 },
> +	{ .start = 0x44f04, .end = 0x44f04 },
> +	{ .start = 0x44fe8, .end = 0x44fe8 },
> +	{ .start = 0x45008, .end = 0x45008 },
> +
> +	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> +	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> +	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> +
> +	/* TRANS_CMTG_CTL_* */
> +	{ .start = 0x6fa88, .end = 0x6fa88 },
> +	{ .start = 0x6fb88, .end = 0x6fb88 },
> +
> +	{ .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
> +	{ .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
> +	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
> +	{ .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
> +	{ .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
> +	{ .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
> +	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
> +
> +	{},
> +};
> +
> +static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
> +	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
> +
> +	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> +
> +	/* DBUF_CTL_* */
> +	{ .start = 0x44300, .end = 0x44300 },
> +	{ .start = 0x44304, .end = 0x44304 },
> +	{ .start = 0x44f00, .end = 0x44f00 },
> +	{ .start = 0x44f04, .end = 0x44f04 },
> +	{ .start = 0x44fe8, .end = 0x44fe8 },
> +	{ .start = 0x45008, .end = 0x45008 },
> +
> +	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> +	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> +	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> +	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
> +
> +	/* Scanline registers */
> +	{ .start = 0x70000, .end = 0x70000 },
> +	{ .start = 0x70004, .end = 0x70004 },
> +	{ .start = 0x70014, .end = 0x70014 },
> +	{ .start = 0x70018, .end = 0x70018 },
> +	{ .start = 0x71000, .end = 0x71000 },
> +	{ .start = 0x71004, .end = 0x71004 },
> +	{ .start = 0x71014, .end = 0x71014 },
> +	{ .start = 0x71018, .end = 0x71018 },
> +	{ .start = 0x72000, .end = 0x72000 },
> +	{ .start = 0x72004, .end = 0x72004 },
> +	{ .start = 0x72014, .end = 0x72014 },
> +	{ .start = 0x72018, .end = 0x72018 },
> +	{ .start = 0x73000, .end = 0x73000 },
> +	{ .start = 0x73004, .end = 0x73004 },
> +	{ .start = 0x73014, .end = 0x73014 },
> +	{ .start = 0x73018, .end = 0x73018 },
> +	{ .start = 0x7b000, .end = 0x7b000 },
> +	{ .start = 0x7b004, .end = 0x7b004 },
> +	{ .start = 0x7b014, .end = 0x7b014 },
> +	{ .start = 0x7b018, .end = 0x7b018 },
> +	{ .start = 0x7c000, .end = 0x7c000 },
> +	{ .start = 0x7c004, .end = 0x7c004 },
> +	{ .start = 0x7c014, .end = 0x7c014 },
> +	{ .start = 0x7c018, .end = 0x7c018 },
> +
> +	{},
> +};
> +
>  static void __intel_dmc_wl_release(struct intel_display *display)
>  {
>  	struct drm_i915_private *i915 = to_i915(display->drm);
> @@ -106,9 +188,31 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
>  	return false;
>  }
>  
> -static bool intel_dmc_wl_check_range(u32 address)
> +static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>  {
> -	return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
> +	const struct intel_dmc_wl_range *ranges;
> +
> +	ranges = lnl_wl_range;
> +
> +	if (intel_dmc_wl_addr_in_range(address, ranges))
> +		return true;
> +
> +	switch (display->power.domains.dc_state) {

This file has no business looking at power domain guts. Use or add
interfaces instead of poking at data directly.

> +	case DC_STATE_EN_DC3CO:
> +		ranges = xe3lpd_dc3co_wl_ranges;
> +		break;
> +	case DC_STATE_EN_UPTO_DC5:
> +	case DC_STATE_EN_UPTO_DC6:
> +		ranges = xe3lpd_dc5_dc6_wl_ranges;
> +		break;
> +	default:
> +		ranges = NULL;
> +	}
> +
> +	if (ranges && intel_dmc_wl_addr_in_range(address, ranges))
> +		return true;
> +
> +	return false;
>  }
>  
>  static bool __intel_dmc_wl_supported(struct intel_display *display)
> @@ -195,7 +299,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>  	if (!__intel_dmc_wl_supported(display))
>  		return;
>  
> -	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
> +	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))

Side note, unrelated to this patch, i915_reg_t is supposed to be opaque,
nobody should look at reg.reg directly, there's i915_mmio_reg_offset()
for it.

>  		return;
>  
>  	spin_lock_irqsave(&wl->lock, flags);
> @@ -247,7 +351,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
>  	if (!__intel_dmc_wl_supported(display))
>  		return;
>  
> -	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
> +	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>  		return;
>  
>  	spin_lock_irqsave(&wl->lock, flags);

-- 
Jani Nikula, Intel

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Jani Nikula]

On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> There are extra registers that require the DMC wakelock when specific
> dynamic DC states are in place. Add the table ranges for them and use
> the correct table depending on the allowed DC states.
>
> Bspec: 71583
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>  1 file changed, 108 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index d597cc825f64..8bf2f32be859 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/kernel.h>
>  
> +#include "i915_reg.h"

I think you mean i915_reg_defs.h

>  #include "intel_de.h"
>  #include "intel_dmc.h"
>  #include "intel_dmc_regs.h"
> @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>  	{},
>  };
>  
> +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
> +	{ .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
> +	{ .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
> +	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> +	{ .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
> +	{ .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
> +
> +	/* DBUF_CTL_* */
> +	{ .start = 0x44300, .end = 0x44300 },
> +	{ .start = 0x44304, .end = 0x44304 },
> +	{ .start = 0x44f00, .end = 0x44f00 },
> +	{ .start = 0x44f04, .end = 0x44f04 },
> +	{ .start = 0x44fe8, .end = 0x44fe8 },
> +	{ .start = 0x45008, .end = 0x45008 },
> +
> +	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> +	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> +	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> +
> +	/* TRANS_CMTG_CTL_* */
> +	{ .start = 0x6fa88, .end = 0x6fa88 },
> +	{ .start = 0x6fb88, .end = 0x6fb88 },
> +
> +	{ .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
> +	{ .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
> +	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
> +	{ .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
> +	{ .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
> +	{ .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
> +	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
> +
> +	{},
> +};
> +
> +static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
> +	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
> +
> +	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> +
> +	/* DBUF_CTL_* */
> +	{ .start = 0x44300, .end = 0x44300 },
> +	{ .start = 0x44304, .end = 0x44304 },
> +	{ .start = 0x44f00, .end = 0x44f00 },
> +	{ .start = 0x44f04, .end = 0x44f04 },
> +	{ .start = 0x44fe8, .end = 0x44fe8 },
> +	{ .start = 0x45008, .end = 0x45008 },
> +
> +	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> +	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> +	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> +	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
> +
> +	/* Scanline registers */
> +	{ .start = 0x70000, .end = 0x70000 },
> +	{ .start = 0x70004, .end = 0x70004 },
> +	{ .start = 0x70014, .end = 0x70014 },
> +	{ .start = 0x70018, .end = 0x70018 },
> +	{ .start = 0x71000, .end = 0x71000 },
> +	{ .start = 0x71004, .end = 0x71004 },
> +	{ .start = 0x71014, .end = 0x71014 },
> +	{ .start = 0x71018, .end = 0x71018 },
> +	{ .start = 0x72000, .end = 0x72000 },
> +	{ .start = 0x72004, .end = 0x72004 },
> +	{ .start = 0x72014, .end = 0x72014 },
> +	{ .start = 0x72018, .end = 0x72018 },
> +	{ .start = 0x73000, .end = 0x73000 },
> +	{ .start = 0x73004, .end = 0x73004 },
> +	{ .start = 0x73014, .end = 0x73014 },
> +	{ .start = 0x73018, .end = 0x73018 },
> +	{ .start = 0x7b000, .end = 0x7b000 },
> +	{ .start = 0x7b004, .end = 0x7b004 },
> +	{ .start = 0x7b014, .end = 0x7b014 },
> +	{ .start = 0x7b018, .end = 0x7b018 },
> +	{ .start = 0x7c000, .end = 0x7c000 },
> +	{ .start = 0x7c004, .end = 0x7c004 },
> +	{ .start = 0x7c014, .end = 0x7c014 },
> +	{ .start = 0x7c018, .end = 0x7c018 },
> +
> +	{},
> +};
> +
>  static void __intel_dmc_wl_release(struct intel_display *display)
>  {
>  	struct drm_i915_private *i915 = to_i915(display->drm);
> @@ -106,9 +188,31 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
>  	return false;
>  }
>  
> -static bool intel_dmc_wl_check_range(u32 address)
> +static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>  {
> -	return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
> +	const struct intel_dmc_wl_range *ranges;
> +
> +	ranges = lnl_wl_range;
> +
> +	if (intel_dmc_wl_addr_in_range(address, ranges))
> +		return true;
> +
> +	switch (display->power.domains.dc_state) {
> +	case DC_STATE_EN_DC3CO:
> +		ranges = xe3lpd_dc3co_wl_ranges;
> +		break;
> +	case DC_STATE_EN_UPTO_DC5:
> +	case DC_STATE_EN_UPTO_DC6:
> +		ranges = xe3lpd_dc5_dc6_wl_ranges;
> +		break;
> +	default:
> +		ranges = NULL;
> +	}
> +
> +	if (ranges && intel_dmc_wl_addr_in_range(address, ranges))
> +		return true;
> +
> +	return false;
>  }
>  
>  static bool __intel_dmc_wl_supported(struct intel_display *display)
> @@ -195,7 +299,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>  	if (!__intel_dmc_wl_supported(display))
>  		return;
>  
> -	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
> +	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>  		return;
>  
>  	spin_lock_irqsave(&wl->lock, flags);
> @@ -247,7 +351,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
>  	if (!__intel_dmc_wl_supported(display))
>  		return;
>  
> -	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
> +	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>  		return;
>  
>  	spin_lock_irqsave(&wl->lock, flags);

-- 
Jani Nikula, Intel

Re: [PATCH 02/13] drm/i915/dmc_wl: Use non-sleeping variant of MMIO wait

[Jani Nikula]

On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Some display MMIO transactions for offsets in the range that requires
> the DMC wakelock happen in atomic context (this has been confirmed
> during tests on PTL). That means that we need to use a non-sleeping
> variant of MMIO waiting function.
>
> Implement __intel_de_wait_for_register_atomic_nowl() and use it when
> waiting for acknowledgment of acquire/release.
>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_de.h     | 11 +++++++++++
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 20 ++++++++++++--------
>  2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index e017cd4a8168..4116783a62dd 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -121,6 +121,17 @@ ____intel_de_wait_for_register_nowl(struct intel_display *display,
>  }
>  #define __intel_de_wait_for_register_nowl(p,...) ____intel_de_wait_for_register_nowl(__to_intel_display(p), __VA_ARGS__)
>  
> +static inline int
> +____intel_de_wait_for_register_atomic_nowl(struct intel_display *display,
> +					   i915_reg_t reg,
> +					   u32 mask, u32 value,
> +					   unsigned int fast_timeout_us)
> +{
> +	return __intel_wait_for_register(__to_uncore(display), reg, mask,
> +					 value, fast_timeout_us, 0, NULL);
> +}
> +#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)
> +

There's no need to add the wrapper when all users pass struct
intel_display. And we don't want new users that pass i915.

And why are we adding new stuff and users with double underscores?

>  static inline int
>  __intel_de_wait(struct intel_display *display, i915_reg_t reg,
>  		u32 mask, u32 value, unsigned int timeout)
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index 5634ff07269d..8056a3c8666c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -39,7 +39,7 @@
>   * potential future use.
>   */
>  
> -#define DMC_WAKELOCK_CTL_TIMEOUT 5
> +#define DMC_WAKELOCK_CTL_TIMEOUT_US 5000
>  #define DMC_WAKELOCK_HOLD_TIME 50
>  
>  struct intel_dmc_wl_range {
> @@ -78,9 +78,9 @@ static void intel_dmc_wl_work(struct work_struct *work)
>  
>  	__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0);
>  
> -	if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL,
> -					      DMC_WAKELOCK_CTL_ACK, 0,
> -					      DMC_WAKELOCK_CTL_TIMEOUT)) {
> +	if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
> +						     DMC_WAKELOCK_CTL_ACK, 0,
> +						     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
>  		WARN_RATELIMIT(1, "DMC wakelock release timed out");
>  		goto out_unlock;
>  	}
> @@ -216,10 +216,14 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>  		__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, 0,
>  				    DMC_WAKELOCK_CTL_REQ);
>  
> -		if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL,
> -						      DMC_WAKELOCK_CTL_ACK,
> -						      DMC_WAKELOCK_CTL_ACK,
> -						      DMC_WAKELOCK_CTL_TIMEOUT)) {
> +		/*
> +		 * We need to use the atomic variant of the waiting routine
> +		 * because the DMC wakelock is also taken in atomic context.
> +		 */
> +		if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
> +							     DMC_WAKELOCK_CTL_ACK,
> +							     DMC_WAKELOCK_CTL_ACK,
> +							     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
>  			WARN_RATELIMIT(1, "DMC wakelock ack timed out");
>  			goto out_unlock;
>  		}

-- 
Jani Nikula, Intel

Re: [PATCH 11/13] drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()

[Jani Nikula]

On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> In order to be able to use the DMC wakelock, we also need to know that
> the display hardware has support for DMC, which is a runtime info.
> Define HAS_DMC_WAKELOCK(), which checks for both DMC availability and IP
> version, and use it in place of directly checking the display version.
>
> Since we depend on runtime info, also make sure to call
> intel_dmc_wl_init() only after we have probed the hardware for such info
> (i.e. after intel_display_device_info_runtime_init()).

Non-functional changes combined with functional changes. Please split.

BR,
Jani.

>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
>  drivers/gpu/drm/i915/display/intel_display_driver.c | 2 +-
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 4 ++--
>  3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> index 071a36b51f79..5f78fd127fe0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -128,6 +128,7 @@ enum intel_display_subplatform {
>  #define HAS_DDI(i915)			(DISPLAY_INFO(i915)->has_ddi)
>  #define HAS_DISPLAY(i915)		(DISPLAY_RUNTIME_INFO(i915)->pipe_mask != 0)
>  #define HAS_DMC(i915)			(DISPLAY_RUNTIME_INFO(i915)->has_dmc)
> +#define HAS_DMC_WAKELOCK(i915)		(HAS_DMC(i915) && DISPLAY_VER(i915) >= 20)
>  #define HAS_DOUBLE_BUFFERED_M_N(i915)	(DISPLAY_VER(i915) >= 9 || IS_BROADWELL(i915))
>  #define HAS_DP_MST(i915)		(DISPLAY_INFO(i915)->has_dp_mst)
>  #define HAS_DP20(i915)			(IS_DG2(i915) || DISPLAY_VER(i915) >= 14)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 673f9b965494..8afaa9cb89d2 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -200,7 +200,6 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915)
>  	intel_dpll_init_clock_hook(i915);
>  	intel_init_display_hooks(i915);
>  	intel_fdi_init_hook(i915);
> -	intel_dmc_wl_init(&i915->display);
>  }
>  
>  /* part #1: call before irq install */
> @@ -238,6 +237,7 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
>  		return 0;
>  
>  	intel_dmc_init(display);
> +	intel_dmc_wl_init(display);
>  
>  	i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
>  	i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index 8283b607aac4..f6ec79b0e39d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -250,7 +250,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>  
>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>  {
> -	if (DISPLAY_VER(display) < 20 ||
> +	if (!HAS_DMC_WAKELOCK(display) ||
>  	    !intel_dmc_has_payload(display) ||
>  	    !display->params.enable_dmc_wl)
>  		return false;
> @@ -263,7 +263,7 @@ void intel_dmc_wl_init(struct intel_display *display)
>  	struct intel_dmc_wl *wl = &display->wl;
>  
>  	/* don't call __intel_dmc_wl_supported(), DMC is not loaded yet */
> -	if (DISPLAY_VER(display) < 20 || !display->params.enable_dmc_wl)
> +	if (!HAS_DMC_WAKELOCK(display) || !display->params.enable_dmc_wl)
>  		return;
>  
>  	INIT_DELAYED_WORK(&wl->work, intel_dmc_wl_work);

-- 
Jani Nikula, Intel

Re: [PATCH 02/13] drm/i915/dmc_wl: Use non-sleeping variant of MMIO wait

[Gustavo Sousa]

Quoting Jani Nikula (2024-10-22 06:34:44-03:00)
>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Some display MMIO transactions for offsets in the range that requires
>> the DMC wakelock happen in atomic context (this has been confirmed
>> during tests on PTL). That means that we need to use a non-sleeping
>> variant of MMIO waiting function.
>>
>> Implement __intel_de_wait_for_register_atomic_nowl() and use it when
>> waiting for acknowledgment of acquire/release.
>>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_de.h     | 11 +++++++++++
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 20 ++++++++++++--------
>>  2 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
>> index e017cd4a8168..4116783a62dd 100644
>> --- a/drivers/gpu/drm/i915/display/intel_de.h
>> +++ b/drivers/gpu/drm/i915/display/intel_de.h
>> @@ -121,6 +121,17 @@ ____intel_de_wait_for_register_nowl(struct intel_display *display,
>>  }
>>  #define __intel_de_wait_for_register_nowl(p,...) ____intel_de_wait_for_register_nowl(__to_intel_display(p), __VA_ARGS__)
>>  
>> +static inline int
>> +____intel_de_wait_for_register_atomic_nowl(struct intel_display *display,
>> +                                           i915_reg_t reg,
>> +                                           u32 mask, u32 value,
>> +                                           unsigned int fast_timeout_us)
>> +{
>> +        return __intel_wait_for_register(__to_uncore(display), reg, mask,
>> +                                         value, fast_timeout_us, 0, NULL);
>> +}
>> +#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)
>> +
>
>There's no need to add the wrapper when all users pass struct
>intel_display. And we don't want new users that pass i915.

Ah, okay. Thanks.

>
>And why are we adding new stuff and users with double underscores?

Well, this is a no-wakelock variant and it shouldn't be used broadly. I
believe that was the motivation of all "__intel_de_*nowl" variants being
prefixed with the underscores.

--
Gustavo Sousa

>
>>  static inline int
>>  __intel_de_wait(struct intel_display *display, i915_reg_t reg,
>>                  u32 mask, u32 value, unsigned int timeout)
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> index 5634ff07269d..8056a3c8666c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> @@ -39,7 +39,7 @@
>>   * potential future use.
>>   */
>>  
>> -#define DMC_WAKELOCK_CTL_TIMEOUT 5
>> +#define DMC_WAKELOCK_CTL_TIMEOUT_US 5000
>>  #define DMC_WAKELOCK_HOLD_TIME 50
>>  
>>  struct intel_dmc_wl_range {
>> @@ -78,9 +78,9 @@ static void intel_dmc_wl_work(struct work_struct *work)
>>  
>>          __intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0);
>>  
>> -        if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL,
>> -                                              DMC_WAKELOCK_CTL_ACK, 0,
>> -                                              DMC_WAKELOCK_CTL_TIMEOUT)) {
>> +        if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
>> +                                                     DMC_WAKELOCK_CTL_ACK, 0,
>> +                                                     DMC_WAKELOCK_CTL_TIMEOUT_US)) {
>>                  WARN_RATELIMIT(1, "DMC wakelock release timed out");
>>                  goto out_unlock;
>>          }
>> @@ -216,10 +216,14 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>>                  __intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, 0,
>>                                      DMC_WAKELOCK_CTL_REQ);
>>  
>> -                if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL,
>> -                                                      DMC_WAKELOCK_CTL_ACK,
>> -                                                      DMC_WAKELOCK_CTL_ACK,
>> -                                                      DMC_WAKELOCK_CTL_TIMEOUT)) {
>> +                /*
>> +                 * We need to use the atomic variant of the waiting routine
>> +                 * because the DMC wakelock is also taken in atomic context.
>> +                 */
>> +                if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL,
>> +                                                             DMC_WAKELOCK_CTL_ACK,
>> +                                                             DMC_WAKELOCK_CTL_ACK,
>> +                                                             DMC_WAKELOCK_CTL_TIMEOUT_US)) {
>>                          WARN_RATELIMIT(1, "DMC wakelock ack timed out");
>>                          goto out_unlock;
>>                  }
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 11/13] drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()

[Gustavo Sousa]

Quoting Jani Nikula (2024-10-22 06:37:51-03:00)
>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> In order to be able to use the DMC wakelock, we also need to know that
>> the display hardware has support for DMC, which is a runtime info.
>> Define HAS_DMC_WAKELOCK(), which checks for both DMC availability and IP
>> version, and use it in place of directly checking the display version.
>>
>> Since we depend on runtime info, also make sure to call
>> intel_dmc_wl_init() only after we have probed the hardware for such info
>> (i.e. after intel_display_device_info_runtime_init()).
>
>Non-functional changes combined with functional changes. Please split.

Do you mean changing the call site of intel_dmc_wl_init() as being
non-functional? Or is it something else?

If this is about the former, I would argue that's not really
non-functional, because we are changing the order of how things are
done... But if making that a standalone patch is preferred, I can do
that.

--
Gustavo Sousa

>
>BR,
>Jani.
>
>>
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
>>  drivers/gpu/drm/i915/display/intel_display_driver.c | 2 +-
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 4 ++--
>>  3 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>> index 071a36b51f79..5f78fd127fe0 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>> @@ -128,6 +128,7 @@ enum intel_display_subplatform {
>>  #define HAS_DDI(i915)                        (DISPLAY_INFO(i915)->has_ddi)
>>  #define HAS_DISPLAY(i915)                (DISPLAY_RUNTIME_INFO(i915)->pipe_mask != 0)
>>  #define HAS_DMC(i915)                        (DISPLAY_RUNTIME_INFO(i915)->has_dmc)
>> +#define HAS_DMC_WAKELOCK(i915)                (HAS_DMC(i915) && DISPLAY_VER(i915) >= 20)
>>  #define HAS_DOUBLE_BUFFERED_M_N(i915)        (DISPLAY_VER(i915) >= 9 || IS_BROADWELL(i915))
>>  #define HAS_DP_MST(i915)                (DISPLAY_INFO(i915)->has_dp_mst)
>>  #define HAS_DP20(i915)                        (IS_DG2(i915) || DISPLAY_VER(i915) >= 14)
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> index 673f9b965494..8afaa9cb89d2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>> @@ -200,7 +200,6 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915)
>>          intel_dpll_init_clock_hook(i915);
>>          intel_init_display_hooks(i915);
>>          intel_fdi_init_hook(i915);
>> -        intel_dmc_wl_init(&i915->display);
>>  }
>>  
>>  /* part #1: call before irq install */
>> @@ -238,6 +237,7 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
>>                  return 0;
>>  
>>          intel_dmc_init(display);
>> +        intel_dmc_wl_init(display);
>>  
>>          i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
>>          i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> index 8283b607aac4..f6ec79b0e39d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> @@ -250,7 +250,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>  
>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>>  {
>> -        if (DISPLAY_VER(display) < 20 ||
>> +        if (!HAS_DMC_WAKELOCK(display) ||
>>              !intel_dmc_has_payload(display) ||
>>              !display->params.enable_dmc_wl)
>>                  return false;
>> @@ -263,7 +263,7 @@ void intel_dmc_wl_init(struct intel_display *display)
>>          struct intel_dmc_wl *wl = &display->wl;
>>  
>>          /* don't call __intel_dmc_wl_supported(), DMC is not loaded yet */
>> -        if (DISPLAY_VER(display) < 20 || !display->params.enable_dmc_wl)
>> +        if (!HAS_DMC_WAKELOCK(display) || !display->params.enable_dmc_wl)
>>                  return;
>>  
>>          INIT_DELAYED_WORK(&wl->work, intel_dmc_wl_work);
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Gustavo Sousa]

Quoting Jani Nikula (2024-10-22 05:03:21-03:00)
>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> There are extra registers that require the DMC wakelock when specific
>> dynamic DC states are in place. Add the table ranges for them and use
>> the correct table depending on the allowed DC states.
>>
>> Bspec: 71583
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>>  1 file changed, 108 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> index d597cc825f64..8bf2f32be859 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> @@ -5,6 +5,7 @@
>>  
>>  #include <linux/kernel.h>
>>  
>> +#include "i915_reg.h"
>>  #include "intel_de.h"
>>  #include "intel_dmc.h"
>>  #include "intel_dmc_regs.h"
>> @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>>          {},
>>  };
>>  
>> +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>> +        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>> +        { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +        { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>> +        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>> +
>> +        /* DBUF_CTL_* */
>> +        { .start = 0x44300, .end = 0x44300 },
>> +        { .start = 0x44304, .end = 0x44304 },
>> +        { .start = 0x44f00, .end = 0x44f00 },
>> +        { .start = 0x44f04, .end = 0x44f04 },
>> +        { .start = 0x44fe8, .end = 0x44fe8 },
>> +        { .start = 0x45008, .end = 0x45008 },
>> +
>> +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +
>> +        /* TRANS_CMTG_CTL_* */
>> +        { .start = 0x6fa88, .end = 0x6fa88 },
>> +        { .start = 0x6fb88, .end = 0x6fb88 },
>> +
>> +        { .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
>> +        { .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +        { .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
>> +        { .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
>> +        { .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        {},
>> +};
>> +
>> +static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +
>> +        /* DBUF_CTL_* */
>> +        { .start = 0x44300, .end = 0x44300 },
>> +        { .start = 0x44304, .end = 0x44304 },
>> +        { .start = 0x44f00, .end = 0x44f00 },
>> +        { .start = 0x44f04, .end = 0x44f04 },
>> +        { .start = 0x44fe8, .end = 0x44fe8 },
>> +        { .start = 0x45008, .end = 0x45008 },
>> +
>> +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        /* Scanline registers */
>> +        { .start = 0x70000, .end = 0x70000 },
>> +        { .start = 0x70004, .end = 0x70004 },
>> +        { .start = 0x70014, .end = 0x70014 },
>> +        { .start = 0x70018, .end = 0x70018 },
>> +        { .start = 0x71000, .end = 0x71000 },
>> +        { .start = 0x71004, .end = 0x71004 },
>> +        { .start = 0x71014, .end = 0x71014 },
>> +        { .start = 0x71018, .end = 0x71018 },
>> +        { .start = 0x72000, .end = 0x72000 },
>> +        { .start = 0x72004, .end = 0x72004 },
>> +        { .start = 0x72014, .end = 0x72014 },
>> +        { .start = 0x72018, .end = 0x72018 },
>> +        { .start = 0x73000, .end = 0x73000 },
>> +        { .start = 0x73004, .end = 0x73004 },
>> +        { .start = 0x73014, .end = 0x73014 },
>> +        { .start = 0x73018, .end = 0x73018 },
>> +        { .start = 0x7b000, .end = 0x7b000 },
>> +        { .start = 0x7b004, .end = 0x7b004 },
>> +        { .start = 0x7b014, .end = 0x7b014 },
>> +        { .start = 0x7b018, .end = 0x7b018 },
>> +        { .start = 0x7c000, .end = 0x7c000 },
>> +        { .start = 0x7c004, .end = 0x7c004 },
>> +        { .start = 0x7c014, .end = 0x7c014 },
>> +        { .start = 0x7c018, .end = 0x7c018 },
>> +
>> +        {},
>> +};
>> +
>>  static void __intel_dmc_wl_release(struct intel_display *display)
>>  {
>>          struct drm_i915_private *i915 = to_i915(display->drm);
>> @@ -106,9 +188,31 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
>>          return false;
>>  }
>>  
>> -static bool intel_dmc_wl_check_range(u32 address)
>> +static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>  {
>> -        return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
>> +        const struct intel_dmc_wl_range *ranges;
>> +
>> +        ranges = lnl_wl_range;
>> +
>> +        if (intel_dmc_wl_addr_in_range(address, ranges))
>> +                return true;
>> +
>> +        switch (display->power.domains.dc_state) {
>
>This file has no business looking at power domain guts. Use or add
>interfaces instead of poking at data directly.

Good point. Thanks.

>
>> +        case DC_STATE_EN_DC3CO:
>> +                ranges = xe3lpd_dc3co_wl_ranges;
>> +                break;
>> +        case DC_STATE_EN_UPTO_DC5:
>> +        case DC_STATE_EN_UPTO_DC6:
>> +                ranges = xe3lpd_dc5_dc6_wl_ranges;
>> +                break;
>> +        default:
>> +                ranges = NULL;
>> +        }
>> +
>> +        if (ranges && intel_dmc_wl_addr_in_range(address, ranges))
>> +                return true;
>> +
>> +        return false;
>>  }
>>  
>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>> @@ -195,7 +299,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>>          if (!__intel_dmc_wl_supported(display))
>>                  return;
>>  
>> -        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>> +        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>
>Side note, unrelated to this patch, i915_reg_t is supposed to be opaque,
>nobody should look at reg.reg directly, there's i915_mmio_reg_offset()
>for it.

Ah, cool. I'll add a patch in v2 to have the whole file using
i915_mmio_reg_offset(). Thanks.

--
Gustavo Sousa

>
>>                  return;
>>  
>>          spin_lock_irqsave(&wl->lock, flags);
>> @@ -247,7 +351,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
>>          if (!__intel_dmc_wl_supported(display))
>>                  return;
>>  
>> -        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>> +        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>>                  return;
>>  
>>          spin_lock_irqsave(&wl->lock, flags);
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Gustavo Sousa]

Quoting Jani Nikula (2024-10-22 05:03:50-03:00)
>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> There are extra registers that require the DMC wakelock when specific
>> dynamic DC states are in place. Add the table ranges for them and use
>> the correct table depending on the allowed DC states.
>>
>> Bspec: 71583
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>>  1 file changed, 108 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> index d597cc825f64..8bf2f32be859 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> @@ -5,6 +5,7 @@
>>  
>>  #include <linux/kernel.h>
>>  
>> +#include "i915_reg.h"
>
>I think you mean i915_reg_defs.h

Not really. I included i915_reg.h because of the DC_STATE_EN_* defines,
which are currently being defined there.

--
Gustavo Sousa

>
>>  #include "intel_de.h"
>>  #include "intel_dmc.h"
>>  #include "intel_dmc_regs.h"
>> @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>>          {},
>>  };
>>  
>> +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>> +        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>> +        { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +        { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>> +        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>> +
>> +        /* DBUF_CTL_* */
>> +        { .start = 0x44300, .end = 0x44300 },
>> +        { .start = 0x44304, .end = 0x44304 },
>> +        { .start = 0x44f00, .end = 0x44f00 },
>> +        { .start = 0x44f04, .end = 0x44f04 },
>> +        { .start = 0x44fe8, .end = 0x44fe8 },
>> +        { .start = 0x45008, .end = 0x45008 },
>> +
>> +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +
>> +        /* TRANS_CMTG_CTL_* */
>> +        { .start = 0x6fa88, .end = 0x6fa88 },
>> +        { .start = 0x6fb88, .end = 0x6fb88 },
>> +
>> +        { .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
>> +        { .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +        { .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
>> +        { .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
>> +        { .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        {},
>> +};
>> +
>> +static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +
>> +        /* DBUF_CTL_* */
>> +        { .start = 0x44300, .end = 0x44300 },
>> +        { .start = 0x44304, .end = 0x44304 },
>> +        { .start = 0x44f00, .end = 0x44f00 },
>> +        { .start = 0x44f04, .end = 0x44f04 },
>> +        { .start = 0x44fe8, .end = 0x44fe8 },
>> +        { .start = 0x45008, .end = 0x45008 },
>> +
>> +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        /* Scanline registers */
>> +        { .start = 0x70000, .end = 0x70000 },
>> +        { .start = 0x70004, .end = 0x70004 },
>> +        { .start = 0x70014, .end = 0x70014 },
>> +        { .start = 0x70018, .end = 0x70018 },
>> +        { .start = 0x71000, .end = 0x71000 },
>> +        { .start = 0x71004, .end = 0x71004 },
>> +        { .start = 0x71014, .end = 0x71014 },
>> +        { .start = 0x71018, .end = 0x71018 },
>> +        { .start = 0x72000, .end = 0x72000 },
>> +        { .start = 0x72004, .end = 0x72004 },
>> +        { .start = 0x72014, .end = 0x72014 },
>> +        { .start = 0x72018, .end = 0x72018 },
>> +        { .start = 0x73000, .end = 0x73000 },
>> +        { .start = 0x73004, .end = 0x73004 },
>> +        { .start = 0x73014, .end = 0x73014 },
>> +        { .start = 0x73018, .end = 0x73018 },
>> +        { .start = 0x7b000, .end = 0x7b000 },
>> +        { .start = 0x7b004, .end = 0x7b004 },
>> +        { .start = 0x7b014, .end = 0x7b014 },
>> +        { .start = 0x7b018, .end = 0x7b018 },
>> +        { .start = 0x7c000, .end = 0x7c000 },
>> +        { .start = 0x7c004, .end = 0x7c004 },
>> +        { .start = 0x7c014, .end = 0x7c014 },
>> +        { .start = 0x7c018, .end = 0x7c018 },
>> +
>> +        {},
>> +};
>> +
>>  static void __intel_dmc_wl_release(struct intel_display *display)
>>  {
>>          struct drm_i915_private *i915 = to_i915(display->drm);
>> @@ -106,9 +188,31 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
>>          return false;
>>  }
>>  
>> -static bool intel_dmc_wl_check_range(u32 address)
>> +static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>  {
>> -        return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
>> +        const struct intel_dmc_wl_range *ranges;
>> +
>> +        ranges = lnl_wl_range;
>> +
>> +        if (intel_dmc_wl_addr_in_range(address, ranges))
>> +                return true;
>> +
>> +        switch (display->power.domains.dc_state) {
>> +        case DC_STATE_EN_DC3CO:
>> +                ranges = xe3lpd_dc3co_wl_ranges;
>> +                break;
>> +        case DC_STATE_EN_UPTO_DC5:
>> +        case DC_STATE_EN_UPTO_DC6:
>> +                ranges = xe3lpd_dc5_dc6_wl_ranges;
>> +                break;
>> +        default:
>> +                ranges = NULL;
>> +        }
>> +
>> +        if (ranges && intel_dmc_wl_addr_in_range(address, ranges))
>> +                return true;
>> +
>> +        return false;
>>  }
>>  
>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>> @@ -195,7 +299,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>>          if (!__intel_dmc_wl_supported(display))
>>                  return;
>>  
>> -        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>> +        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>>                  return;
>>  
>>          spin_lock_irqsave(&wl->lock, flags);
>> @@ -247,7 +351,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
>>          if (!__intel_dmc_wl_supported(display))
>>                  return;
>>  
>> -        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>> +        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>>                  return;
>>  
>>          spin_lock_irqsave(&wl->lock, flags);
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Gustavo Sousa]

Quoting Gustavo Sousa (2024-10-21 19:27:26-03:00)
>There are extra registers that require the DMC wakelock when specific
>dynamic DC states are in place. Add the table ranges for them and use
>the correct table depending on the allowed DC states.
>
>Bspec: 71583

For anyone who is reviewing this and doesn't find the ranges in that
Bspec page: I forgot to mention that the ranges are not yet listed there
and that I got them directly from the display hardware team with a note
that they would be updating the Bspec.

--
Gustavo Sousa

>Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>---
> drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
> 1 file changed, 108 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>index d597cc825f64..8bf2f32be859 100644
>--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>@@ -5,6 +5,7 @@
> 
> #include <linux/kernel.h>
> 
>+#include "i915_reg.h"
> #include "intel_de.h"
> #include "intel_dmc.h"
> #include "intel_dmc_regs.h"
>@@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>         {},
> };
> 
>+static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>+        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>+        { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>+        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>+        { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>+        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>+
>+        /* DBUF_CTL_* */
>+        { .start = 0x44300, .end = 0x44300 },
>+        { .start = 0x44304, .end = 0x44304 },
>+        { .start = 0x44f00, .end = 0x44f00 },
>+        { .start = 0x44f04, .end = 0x44f04 },
>+        { .start = 0x44fe8, .end = 0x44fe8 },
>+        { .start = 0x45008, .end = 0x45008 },
>+
>+        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>+        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>+        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>+
>+        /* TRANS_CMTG_CTL_* */
>+        { .start = 0x6fa88, .end = 0x6fa88 },
>+        { .start = 0x6fb88, .end = 0x6fb88 },
>+
>+        { .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
>+        { .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
>+        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>+        { .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
>+        { .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
>+        { .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
>+        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>+
>+        {},
>+};
>+
>+static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
>+        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>+
>+        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>+
>+        /* DBUF_CTL_* */
>+        { .start = 0x44300, .end = 0x44300 },
>+        { .start = 0x44304, .end = 0x44304 },
>+        { .start = 0x44f00, .end = 0x44f00 },
>+        { .start = 0x44f04, .end = 0x44f04 },
>+        { .start = 0x44fe8, .end = 0x44fe8 },
>+        { .start = 0x45008, .end = 0x45008 },
>+
>+        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>+        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>+        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>+        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>+
>+        /* Scanline registers */
>+        { .start = 0x70000, .end = 0x70000 },
>+        { .start = 0x70004, .end = 0x70004 },
>+        { .start = 0x70014, .end = 0x70014 },
>+        { .start = 0x70018, .end = 0x70018 },
>+        { .start = 0x71000, .end = 0x71000 },
>+        { .start = 0x71004, .end = 0x71004 },
>+        { .start = 0x71014, .end = 0x71014 },
>+        { .start = 0x71018, .end = 0x71018 },
>+        { .start = 0x72000, .end = 0x72000 },
>+        { .start = 0x72004, .end = 0x72004 },
>+        { .start = 0x72014, .end = 0x72014 },
>+        { .start = 0x72018, .end = 0x72018 },
>+        { .start = 0x73000, .end = 0x73000 },
>+        { .start = 0x73004, .end = 0x73004 },
>+        { .start = 0x73014, .end = 0x73014 },
>+        { .start = 0x73018, .end = 0x73018 },
>+        { .start = 0x7b000, .end = 0x7b000 },
>+        { .start = 0x7b004, .end = 0x7b004 },
>+        { .start = 0x7b014, .end = 0x7b014 },
>+        { .start = 0x7b018, .end = 0x7b018 },
>+        { .start = 0x7c000, .end = 0x7c000 },
>+        { .start = 0x7c004, .end = 0x7c004 },
>+        { .start = 0x7c014, .end = 0x7c014 },
>+        { .start = 0x7c018, .end = 0x7c018 },
>+
>+        {},
>+};
>+
> static void __intel_dmc_wl_release(struct intel_display *display)
> {
>         struct drm_i915_private *i915 = to_i915(display->drm);
>@@ -106,9 +188,31 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
>         return false;
> }
> 
>-static bool intel_dmc_wl_check_range(u32 address)
>+static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
> {
>-        return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
>+        const struct intel_dmc_wl_range *ranges;
>+
>+        ranges = lnl_wl_range;
>+
>+        if (intel_dmc_wl_addr_in_range(address, ranges))
>+                return true;
>+
>+        switch (display->power.domains.dc_state) {
>+        case DC_STATE_EN_DC3CO:
>+                ranges = xe3lpd_dc3co_wl_ranges;
>+                break;
>+        case DC_STATE_EN_UPTO_DC5:
>+        case DC_STATE_EN_UPTO_DC6:
>+                ranges = xe3lpd_dc5_dc6_wl_ranges;
>+                break;
>+        default:
>+                ranges = NULL;
>+        }
>+
>+        if (ranges && intel_dmc_wl_addr_in_range(address, ranges))
>+                return true;
>+
>+        return false;
> }
> 
> static bool __intel_dmc_wl_supported(struct intel_display *display)
>@@ -195,7 +299,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>         if (!__intel_dmc_wl_supported(display))
>                 return;
> 
>-        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>+        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>                 return;
> 
>         spin_lock_irqsave(&wl->lock, flags);
>@@ -247,7 +351,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
>         if (!__intel_dmc_wl_supported(display))
>                 return;
> 
>-        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>+        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>                 return;
> 
>         spin_lock_irqsave(&wl->lock, flags);
>-- 
>2.47.0
>

Re: [PATCH 01/13] drm/xe: Mimic i915 behavior for non-sleeping MMIO wait

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> In upcoming display changes, we will modify the DMC wakelock MMIO
> waiting code to choose a non-sleeping variant implementation, because
> the wakelock is also taking in atomic context.
> 
> While xe provides an explicit parameter (namely "atomic") to prevent
> xe_mmio_wait32() from sleeping, i915 does not and implements that
> behavior when slow_timeout_ms is zero.
> 
> So, for now, let's mimic what i915 does to allow for display to use
> non-sleeping MMIO wait. In the future, we should come up with a better
> and explicit interface for this behavior in i915, at least while display
> code is not an independent entity with proper interfaces between xe and
> i915.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---

Makes sense.

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

Just one question/comment below.


>  .../gpu/drm/xe/compat-i915-headers/intel_uncore.h   | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
> index 0382beb4035b..5a57f76c1760 100644
> --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
> @@ -117,10 +117,21 @@ __intel_wait_for_register(struct intel_uncore *uncore, i915_reg_t i915_reg,
>  			  unsigned int slow_timeout_ms, u32 *out_value)
>  {
>  	struct xe_reg reg = XE_REG(i915_mmio_reg_offset(i915_reg));
> +	bool atomic;
> +
> +	/*
> +	 * FIXME: We are trying to replicate the behavior from i915 here, in
> +	 * which sleep is not performed if slow_timeout_ms == 0. This hack is
> +	 * necessary because of paths in display code that are executed in
> +	 * atomic context. Setting the atomic flag based on timeout values
> +	 * doesn't feel very robust. Ideally, we should have a proper interface
> +	 * for explicitly choosing non-sleeping behavior.

I think this is just a matter of semantics.  It would look nicer to
have a more intuitive interface, but I don't think the i915
implementation is any less robust per se.  If this behavior is
documented properly, I don't see it as a real issue.

--
Cheers,
Luca.

Re: [PATCH 02/13] drm/i915/dmc_wl: Use non-sleeping variant of MMIO wait

[Luca Coelho]

On Tue, 2024-10-22 at 07:55 -0300, Gustavo Sousa wrote:
> Quoting Jani Nikula (2024-10-22 06:34:44-03:00)
> > On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> > > Some display MMIO transactions for offsets in the range that requires
> > > the DMC wakelock happen in atomic context (this has been confirmed
> > > during tests on PTL). That means that we need to use a non-sleeping
> > > variant of MMIO waiting function.
> > > 
> > > Implement __intel_de_wait_for_register_atomic_nowl() and use it when
> > > waiting for acknowledgment of acquire/release.
> > > 
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_de.h     | 11 +++++++++++
> > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 20 ++++++++++++--------
> > >  2 files changed, 23 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> > > index e017cd4a8168..4116783a62dd 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_de.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_de.h
> > > @@ -121,6 +121,17 @@ ____intel_de_wait_for_register_nowl(struct intel_display *display,
> > >  }
> > >  #define __intel_de_wait_for_register_nowl(p,...) ____intel_de_wait_for_register_nowl(__to_intel_display(p), __VA_ARGS__)
> > >  
> > > +static inline int
> > > +____intel_de_wait_for_register_atomic_nowl(struct intel_display *display,
> > > +                                           i915_reg_t reg,
> > > +                                           u32 mask, u32 value,
> > > +                                           unsigned int fast_timeout_us)
> > > +{
> > > +        return __intel_wait_for_register(__to_uncore(display), reg, mask,
> > > +                                         value, fast_timeout_us, 0, NULL);
> > > +}
> > > +#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)
> > > +
> > 
> > There's no need to add the wrapper when all users pass struct
> > intel_display. And we don't want new users that pass i915.
> 
> Ah, okay. Thanks.
> 
> > 
> > And why are we adding new stuff and users with double underscores?
> 
> Well, this is a no-wakelock variant and it shouldn't be used broadly. I
> believe that was the motivation of all "__intel_de_*nowl" variants being
> prefixed with the underscores.

Yes, that's exactly the idea in the code I added earlier.  The double
underscore is used for non-locking functions that are called by their
locking versions.  And should only be used elsewhere in very specific
cases.

--
Cheers,
Luca.

Re: [PATCH 02/13] drm/i915/dmc_wl: Use non-sleeping variant of MMIO wait

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> Some display MMIO transactions for offsets in the range that requires
> the DMC wakelock happen in atomic context (this has been confirmed
> during tests on PTL). That means that we need to use a non-sleeping
> variant of MMIO waiting function.
> 
> Implement __intel_de_wait_for_register_atomic_nowl() and use it when
> waiting for acknowledgment of acquire/release.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_de.h     | 11 +++++++++++
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 20 ++++++++++++--------
>  2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index e017cd4a8168..4116783a62dd 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -121,6 +121,17 @@ ____intel_de_wait_for_register_nowl(struct intel_display *display,
>  }
>  #define __intel_de_wait_for_register_nowl(p,...) ____intel_de_wait_for_register_nowl(__to_intel_display(p), __VA_ARGS__)
>  
> +static inline int
> +____intel_de_wait_for_register_atomic_nowl(struct intel_display *display,
> +					   i915_reg_t reg,
> +					   u32 mask, u32 value,
> +					   unsigned int fast_timeout_us)
> +{
> +	return __intel_wait_for_register(__to_uncore(display), reg, mask,
> +					 value, fast_timeout_us, 0, NULL);
> +}
> +#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__)
> +
>  static inline int
>  __intel_de_wait(struct intel_display *display, i915_reg_t reg,
>  		u32 mask, u32 value, unsigned int timeout)
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index 5634ff07269d..8056a3c8666c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -39,7 +39,7 @@
>   * potential future use.
>   */
>  
> -#define DMC_WAKELOCK_CTL_TIMEOUT 5
> +#define DMC_WAKELOCK_CTL_TIMEOUT_US 5000

Maybe this deserves a small explanation in the commit message saying
that you need to use the microsecond variant (fast_timeout_us) because
that makes it atomic further down the call chain?

--
Cheers,
Luca.

Re: [PATCH 03/13] drm/i915/dmc_wl: Check for non-zero refcount in release work

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> When the DMC wakelock refcount reaches zero, we know that there are no
> users and that we can do the actual release operation on the hardware,
> which is queued with a delayed work. The idea of the delayed work is to
> avoid performing the release if a new lock user appears (i.e. refcount
> gets incremented) in a very short period of time.
> 
> Based on the above, the release work should bail out if refcount is
> non-zero (meaning new lock users appeared in the meantime), but our
> current code actually does the opposite: it bails when refcount is zero.
> That means that the wakelock is not released when it should be; and
> that, when the work is not canceled in time, it ends up being releasing
> when it should not.
> 
> Fix that by inverting the condition.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index 8056a3c8666c..c298aef89449 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -72,8 +72,11 @@ static void intel_dmc_wl_work(struct work_struct *work)
>  
>  	spin_lock_irqsave(&wl->lock, flags);
>  
> -	/* Bail out if refcount reached zero while waiting for the spinlock */
> -	if (!refcount_read(&wl->refcount))
> +	/*
> +	 * Bail out if refcount became non-zero while waiting for the spinlock,
> +	 * meaning that the lock is now taken again.
> +	 */
> +	if (refcount_read(&wl->refcount))
>  		goto out_unlock;
>  
>  	__intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0);

Oh, this was probably hard to catch.  I think this is a buggy leftover
from my earlier implementations where I was decreasing the refcount
only here.

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [PATCH 04/13] drm/i915/dmc_wl: Get wakelock when disabling dynamic DC states

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> Bspec says that disabling dynamic DC states require taking the DMC
> wakelock to cause an DC exit before writing to DC_STATE_EN. Implement
> that.
> 
> In fact, testing on PTL revealed we end up failing to exit DC5/6 without
> this step.
> 
> Bspec: 71583
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  .../drm/i915/display/intel_display_power_well.c    | 10 +++++++---
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c        | 14 ++++++++++++--
>  drivers/gpu/drm/i915/display/intel_dmc_wl.h        |  2 ++
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> index adaf7cf3a33b..e8946ce86aaa 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -987,10 +987,14 @@ void gen9_disable_dc_states(struct intel_display *display)
>  		return;
>  	}
>  
> -	gen9_set_dc_state(display, DC_STATE_DISABLE);
> -
> -	if (!HAS_DISPLAY(display))
> +	if (HAS_DISPLAY(display)) {
> +		intel_dmc_wl_get_noreg(display);
> +		gen9_set_dc_state(display, DC_STATE_DISABLE);
> +		intel_dmc_wl_put_noreg(display);
> +	} else {
> +		gen9_set_dc_state(display, DC_STATE_DISABLE);
>  		return;
> +	}

I think intel_dmc_get/put() already protect indirectly on
HAS_DISPLAY(), doesn't it? If that's the case, then the if here is
unnecessary.

--
Cheers,
Luca.

Re: [PATCH 05/13] drm/i915/dmc_wl: Use sentinel item for range tables

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> We are currently using ARRAY_SIZE() to iterate address ranges in
> intel_dmc_wl_check_range(). In upcoming changes, we will be using more
> than a single table and will extract the range checking logic into a
> dedicated function that takes a range table as argument. As we will not
> able to use ARRAY_SIZE() then, let's make range tables contain a
> sentinel item at the end and use that instead of having to pass the size
> as parameter in this future function.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> There are extra registers that require the DMC wakelock when specific
> dynamic DC states are in place. Add the table ranges for them and use
> the correct table depending on the allowed DC states.
> 
> Bspec: 71583
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>  1 file changed, 108 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index d597cc825f64..8bf2f32be859 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/kernel.h>
>  
> +#include "i915_reg.h"
>  #include "intel_de.h"
>  #include "intel_dmc.h"
>  #include "intel_dmc_regs.h"
> @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>  	{},
>  };

Do we still need the lnl_wl_range[]? This was sort of a place-holder
with a very large range just for testing.  I can see that there are at
least some ranges in common between lnl_wl_range[] and the new range
tables defined below.


> +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
> +	{ .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
> +	{ .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
> +	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> +	{ .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
> +	{ .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
> +
> +	/* DBUF_CTL_* */
> +	{ .start = 0x44300, .end = 0x44300 },
> +	{ .start = 0x44304, .end = 0x44304 },
> +	{ .start = 0x44f00, .end = 0x44f00 },
> +	{ .start = 0x44f04, .end = 0x44f04 },
> +	{ .start = 0x44fe8, .end = 0x44fe8 },
> +	{ .start = 0x45008, .end = 0x45008 },
> +
> +	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> +	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> +	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> +
> +	/* TRANS_CMTG_CTL_* */
> +	{ .start = 0x6fa88, .end = 0x6fa88 },
> +	{ .start = 0x6fb88, .end = 0x6fb88 },

These, for instance, are part of lnl_wl_range[].


> +
> +	{ .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
> +	{ .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
> +	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
> +	{ .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
> +	{ .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
> +	{ .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
> +	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
> +
> +	{},
> +};
> +
> +static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
> +	{ .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
> +
> +	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> +
> +	/* DBUF_CTL_* */
> +	{ .start = 0x44300, .end = 0x44300 },
> +	{ .start = 0x44304, .end = 0x44304 },
> +	{ .start = 0x44f00, .end = 0x44f00 },
> +	{ .start = 0x44f04, .end = 0x44f04 },
> +	{ .start = 0x44fe8, .end = 0x44fe8 },
> +	{ .start = 0x45008, .end = 0x45008 },
> +
> +	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> +	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> +	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> +	{ .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
> +
> +	/* Scanline registers */
> +	{ .start = 0x70000, .end = 0x70000 },
> +	{ .start = 0x70004, .end = 0x70004 },
> +	{ .start = 0x70014, .end = 0x70014 },
> +	{ .start = 0x70018, .end = 0x70018 },
> +	{ .start = 0x71000, .end = 0x71000 },
> +	{ .start = 0x71004, .end = 0x71004 },
> +	{ .start = 0x71014, .end = 0x71014 },
> +	{ .start = 0x71018, .end = 0x71018 },
> +	{ .start = 0x72000, .end = 0x72000 },
> +	{ .start = 0x72004, .end = 0x72004 },
> +	{ .start = 0x72014, .end = 0x72014 },
> +	{ .start = 0x72018, .end = 0x72018 },
> +	{ .start = 0x73000, .end = 0x73000 },
> +	{ .start = 0x73004, .end = 0x73004 },
> +	{ .start = 0x73014, .end = 0x73014 },
> +	{ .start = 0x73018, .end = 0x73018 },
> +	{ .start = 0x7b000, .end = 0x7b000 },
> +	{ .start = 0x7b004, .end = 0x7b004 },
> +	{ .start = 0x7b014, .end = 0x7b014 },
> +	{ .start = 0x7b018, .end = 0x7b018 },
> +	{ .start = 0x7c000, .end = 0x7c000 },
> +	{ .start = 0x7c004, .end = 0x7c004 },
> +	{ .start = 0x7c014, .end = 0x7c014 },
> +	{ .start = 0x7c018, .end = 0x7c018 },

And so are all these.


--
Cheers,
Luca.

Re: [PATCH 08/13] drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> Allow simpler syntax for defining entries for single registers in range
> tables. That makes them easier to type as well as to read, allowing one
> to quickly tell whether a range actually refers to a single register or
> a "true range".
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 118 ++++++++++----------
>  1 file changed, 60 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> index 8bf2f32be859..6992ce654e75 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> @@ -54,82 +54,82 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>  };
>  
>  static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
> -	{ .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
> +	{ .start = 0x45500 }, /* DC_STATE_SEL */
>  	{ .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
> -	{ .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> +	{ .start = 0x45504 }, /* DC_STATE_EN */
>  	{ .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
> -	{ .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
> +	{ .start = 0x454f0 }, /* RETENTION_CTRL */
>  
>  	/* DBUF_CTL_* */
> -	{ .start = 0x44300, .end = 0x44300 },
> -	{ .start = 0x44304, .end = 0x44304 },
> -	{ .start = 0x44f00, .end = 0x44f00 },
> -	{ .start = 0x44f04, .end = 0x44f04 },
> -	{ .start = 0x44fe8, .end = 0x44fe8 },
> -	{ .start = 0x45008, .end = 0x45008 },
> +	{ .start = 0x44300 },
> +	{ .start = 0x44304 },
> +	{ .start = 0x44f00 },
> +	{ .start = 0x44f04 },
> +	{ .start = 0x44fe8 },
> +	{ .start = 0x45008 },
>  
> -	{ .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> -	{ .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> -	{ .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> +	{ .start = 0x46070 }, /* CDCLK_PLL_ENABLE */
> +	{ .start = 0x46000 }, /* CDCLK_CTL */
> +	{ .start = 0x46008 }, /* CDCLK_SQUASH_CTL */

Many of these are probably actually ranges.  I'm not a HW guy, but
these are probably blocks that need the wakelock and it just happens
that many of those addresses are actually not used, but would need a
wakelock if they were used?

IOW, e.g. all these DBUF_CTL registers are probably in the same range
that needs wakelocks (i.e. 0x44300-0x46fff)? Do we really need to
define many of these individually?

This is related to the previous patch as well, but I decided to comment
it here because it becomes clearer.

--
Cheers,
Luca.

Re: [PATCH 09/13] drm/i915/dmc_wl: Deal with existing references when disabling

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> It is possible that there are active wakelock references at the time we
> are disabling the DMC wakelock mechanism. We need to deal with that in
> two ways:
> 
> (A) Implement the missing step from Bspec:
> 
>     The Bspec instructs us to clear any existing wakelock request bit
>     after disabling the mechanism. That gives a clue that it is okay to
>     disable while there are locks held and we do not need to wait for
>     them. However, since the spec is not explicit about it, we need
>     still to get confirmation with the hardware team. Let's thus
>     implement the spec and add a TODO note.
> 
> (B) Ensure a consistent driver state:
> 
>     The enable/disable logic would be problematic if the following
>     sequence of events would happen:
> 
>     1. Function A calls intel_dmc_wl_get();
>     2. Some function calls intel_dmc_wl_disable();
>     3. Some function calls intel_dmc_wl_enable();
>     4. Function A is done and calls intel_dmc_wl_put().
> 
>     At (2), the refcount becomes zero and then (4) causes an invalid
>     decrement to the refcount. That would cause some issues:
> 
>         - At the time between (3) and (4), function A would think that
>           the hardware lock is held but it could not be really held
>           until intel_dmc_wl_get() is called by something else.
>         - The call made to (4) could cause the refcount to become zero
>           and consequently the hardware lock to be released while there
>           could be innocent paths trusting they still have the lock.
> 
>     To fix that, we need to keep the refcount correctly in sync with
>     intel_dmc_wl_{get,put}() calls and retake the hardware lock when
>     enabling the DMC wakelock with a non-zero refcount.
> 
>     One missing piece left to be handled here is the following scenario:
> 
>     1. Function A calls intel_dmc_wl_get();
>     2. Some function calls intel_dmc_wl_disable();
>     3. Some function calls intel_dmc_wl_enable();
>     4. Concurrently with (3), function A performs the MMIO in between
>        setting DMC_WAKELOCK_CFG_ENABLE and asserting the lock with
>        __intel_dmc_wl_take().
> 
>     I'm mostly sure this would cause issues future display IPs if DMC
>     trap implementation was completely removed. We need to check with
>     the hardware team whether it would be safe to assert the hardware
>     lock before setting DMC_WAKELOCK_CFG_ENABLE to avoid this scenario.
>     If not, then we would have to deal with that via software
>     synchronization.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [PATCH 10/13] drm/i915/dmc_wl: Couple enable/disable with dynamic DC states

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> Enabling and disabling the DMC wakelock should be done as part of
> enabling and disabling of dynamic DC states, respectively. We should not
> enable or disable DMC wakelock independently of DC states, otherwise we
> would risk ending up with an inconsistent state where dynamic DC states
> are enabled and the DMC wakelock is disabled, going against current
> recommendations and making MMIO transactions potentially slower. In
> future display IPs that could have a worse outcome if DMC trap
> implementation is completely removed.
> 
> So, let's make things safer by tying stuff together, removing the
> independent calls, and also put warnings in place to detect inconsistent
> calls.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [PATCH 12/13] drm/i915/dmc_wl: Sanitize enable_dmc_wl according to hardware support

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> Instead of checking for HAS_DMC_WAKELOCK() multiple times, let's use it
> to sanitize the enable_dmc_wl parameter and use that variable when
> necessary.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.

Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default

[Luca Coelho]

On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> Although Bspec doesn't explicitly mentions that, as of Xe3_LPD, using
> DMC wakelock is the officially recommended way of accessing registers
> that would be off during DC5/DC6 and the legacy method (where the DMC
> intercepts MMIO to wake up the hardware) is to be avoided.
> 
> As such, update the driver to use the DMC wakelock by default starting
> with Xe3_LPD. Since the feature is somewhat new to the driver, also
> allow disabling it via a module parameter for debugging purposes.
> 
> For that, make the existing parameter allow values -1 (per-chip
> default), 0 (disabled) and 1 (enabled), similarly to what is done for
> other parameters.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
>  drivers/gpu/drm/i915/display/intel_display_params.h | 2 +-
>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 6 +++++-
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> index 024de8abcb1a..bf00e5f1f145 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> @@ -123,10 +123,10 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
>  	"(0=disabled, 1=enabled) "
>  	"Default: 1");
>  
> -intel_display_param_named_unsafe(enable_dmc_wl, bool, 0400,
> +intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
>  	"Enable DMC wakelock "
>  	"(0=disabled, 1=enabled) "
> -	"Default: 0");
> +	"Default: -1 (use per-chip default)");

We're already explaining the possible values in the previous
parentheses, so maybe the -1 should also be explained there?

--
Cheers,
Luca.

Re: [PATCH 01/13] drm/xe: Mimic i915 behavior for non-sleeping MMIO wait

[Gustavo Sousa]

Quoting Luca Coelho (2024-11-01 07:57:58-03:00)
>On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>> In upcoming display changes, we will modify the DMC wakelock MMIO
>> waiting code to choose a non-sleeping variant implementation, because
>> the wakelock is also taking in atomic context.
>> 
>> While xe provides an explicit parameter (namely "atomic") to prevent
>> xe_mmio_wait32() from sleeping, i915 does not and implements that
>> behavior when slow_timeout_ms is zero.
>> 
>> So, for now, let's mimic what i915 does to allow for display to use
>> non-sleeping MMIO wait. In the future, we should come up with a better
>> and explicit interface for this behavior in i915, at least while display
>> code is not an independent entity with proper interfaces between xe and
>> i915.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>
>Makes sense.
>
>Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

Thanks!

>
>Just one question/comment below.
>
>
>>  .../gpu/drm/xe/compat-i915-headers/intel_uncore.h   | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
>> index 0382beb4035b..5a57f76c1760 100644
>> --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
>> +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
>> @@ -117,10 +117,21 @@ __intel_wait_for_register(struct intel_uncore *uncore, i915_reg_t i915_reg,
>>                            unsigned int slow_timeout_ms, u32 *out_value)
>>  {
>>          struct xe_reg reg = XE_REG(i915_mmio_reg_offset(i915_reg));
>> +        bool atomic;
>> +
>> +        /*
>> +         * FIXME: We are trying to replicate the behavior from i915 here, in
>> +         * which sleep is not performed if slow_timeout_ms == 0. This hack is
>> +         * necessary because of paths in display code that are executed in
>> +         * atomic context. Setting the atomic flag based on timeout values
>> +         * doesn't feel very robust. Ideally, we should have a proper interface
>> +         * for explicitly choosing non-sleeping behavior.
>
>I think this is just a matter of semantics.  It would look nicer to
>have a more intuitive interface, but I don't think the i915
>implementation is any less robust per se.  If this behavior is
>documented properly, I don't see it as a real issue.

Ah, well... Yeah, I guess I was too hard on i915. I'll replace this
comment with a quick note only mentioning that we are replicating the
behavior then.

Thanks!

--
Gustavo Sousa

Re: [PATCH 04/13] drm/i915/dmc_wl: Get wakelock when disabling dynamic DC states

[Gustavo Sousa]

Quoting Luca Coelho (2024-11-01 09:24:08-03:00)
>On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>> Bspec says that disabling dynamic DC states require taking the DMC
>> wakelock to cause an DC exit before writing to DC_STATE_EN. Implement
>> that.
>> 
>> In fact, testing on PTL revealed we end up failing to exit DC5/6 without
>> this step.
>> 
>> Bspec: 71583
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  .../drm/i915/display/intel_display_power_well.c    | 10 +++++++---
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c        | 14 ++++++++++++--
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.h        |  2 ++
>>  3 files changed, 21 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> index adaf7cf3a33b..e8946ce86aaa 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
>> @@ -987,10 +987,14 @@ void gen9_disable_dc_states(struct intel_display *display)
>>                  return;
>>          }
>>  
>> -        gen9_set_dc_state(display, DC_STATE_DISABLE);
>> -
>> -        if (!HAS_DISPLAY(display))
>> +        if (HAS_DISPLAY(display)) {
>> +                intel_dmc_wl_get_noreg(display);
>> +                gen9_set_dc_state(display, DC_STATE_DISABLE);
>> +                intel_dmc_wl_put_noreg(display);
>> +        } else {
>> +                gen9_set_dc_state(display, DC_STATE_DISABLE);
>>                  return;
>> +        }
>
>I think intel_dmc_get/put() already protect indirectly on
>HAS_DISPLAY(), doesn't it? If that's the case, then the if here is
>unnecessary.

Actually, intel_dmc_wl_init() gets called only when HAS_DISPLAY() is
true, so I think using intel_dmc_wl_{get,put}_noreg() for the
!HAS_DISPLAY() case would not be right, at least not with the current
state of the code.

--
Gustavo Sousa

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Gustavo Sousa]

Quoting Luca Coelho (2024-11-01 09:51:48-03:00)
>On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>> There are extra registers that require the DMC wakelock when specific
>> dynamic DC states are in place. Add the table ranges for them and use
>> the correct table depending on the allowed DC states.
>> 
>> Bspec: 71583
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>>  1 file changed, 108 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> index d597cc825f64..8bf2f32be859 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> @@ -5,6 +5,7 @@
>>  
>>  #include <linux/kernel.h>
>>  
>> +#include "i915_reg.h"
>>  #include "intel_de.h"
>>  #include "intel_dmc.h"
>>  #include "intel_dmc_regs.h"
>> @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>>          {},
>>  };
>
>Do we still need the lnl_wl_range[]? This was sort of a place-holder
>with a very large range just for testing.  I can see that there are at
>least some ranges in common between lnl_wl_range[] and the new range
>tables defined below.

Yes, although we could do some homework to get a more accurate set of
ranges.

Now, about the different tables:

 - lnl_wl_range should be about ranges of registers that are powered
   down during DC states and that the HW requires DC exit for proper
   access.
 - xe3lpd_{dc5_dc6,dc3co}_wl_ranges are registers that are touched by
   the DMC and need the wakelock for properly restoring the correct
   value before accessing them.

Maybe a comment in the code explaining the above is warranted?

>
>
>> +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>> +        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>> +        { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +        { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>> +        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>> +
>> +        /* DBUF_CTL_* */
>> +        { .start = 0x44300, .end = 0x44300 },
>> +        { .start = 0x44304, .end = 0x44304 },
>> +        { .start = 0x44f00, .end = 0x44f00 },
>> +        { .start = 0x44f04, .end = 0x44f04 },
>> +        { .start = 0x44fe8, .end = 0x44fe8 },
>> +        { .start = 0x45008, .end = 0x45008 },
>> +
>> +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +
>> +        /* TRANS_CMTG_CTL_* */
>> +        { .start = 0x6fa88, .end = 0x6fa88 },
>> +        { .start = 0x6fb88, .end = 0x6fb88 },
>
>These, for instance, are part of lnl_wl_range[].

Given my clarification above about the different purposes of the ranges,
I think we should stick to keeping the same values from the (soon to
be?) documented tables, even if there is some small redundancy.
Otherwise we would require the programmer to remember to check ranges in
the "more general" table every time a DC state-specific one needs to be
added or updated.

--
Gustavo Sousa

>
>
>> +
>> +        { .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
>> +        { .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +        { .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
>> +        { .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
>> +        { .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        {},
>> +};
>> +
>> +static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +
>> +        /* DBUF_CTL_* */
>> +        { .start = 0x44300, .end = 0x44300 },
>> +        { .start = 0x44304, .end = 0x44304 },
>> +        { .start = 0x44f00, .end = 0x44f00 },
>> +        { .start = 0x44f04, .end = 0x44f04 },
>> +        { .start = 0x44fe8, .end = 0x44fe8 },
>> +        { .start = 0x45008, .end = 0x45008 },
>> +
>> +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        /* Scanline registers */
>> +        { .start = 0x70000, .end = 0x70000 },
>> +        { .start = 0x70004, .end = 0x70004 },
>> +        { .start = 0x70014, .end = 0x70014 },
>> +        { .start = 0x70018, .end = 0x70018 },
>> +        { .start = 0x71000, .end = 0x71000 },
>> +        { .start = 0x71004, .end = 0x71004 },
>> +        { .start = 0x71014, .end = 0x71014 },
>> +        { .start = 0x71018, .end = 0x71018 },
>> +        { .start = 0x72000, .end = 0x72000 },
>> +        { .start = 0x72004, .end = 0x72004 },
>> +        { .start = 0x72014, .end = 0x72014 },
>> +        { .start = 0x72018, .end = 0x72018 },
>> +        { .start = 0x73000, .end = 0x73000 },
>> +        { .start = 0x73004, .end = 0x73004 },
>> +        { .start = 0x73014, .end = 0x73014 },
>> +        { .start = 0x73018, .end = 0x73018 },
>> +        { .start = 0x7b000, .end = 0x7b000 },
>> +        { .start = 0x7b004, .end = 0x7b004 },
>> +        { .start = 0x7b014, .end = 0x7b014 },
>> +        { .start = 0x7b018, .end = 0x7b018 },
>> +        { .start = 0x7c000, .end = 0x7c000 },
>> +        { .start = 0x7c004, .end = 0x7c004 },
>> +        { .start = 0x7c014, .end = 0x7c014 },
>> +        { .start = 0x7c018, .end = 0x7c018 },
>
>And so are all these.
>
>
>--
>Cheers,
>Luca.

Re: [PATCH 08/13] drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables

[Gustavo Sousa]

Quoting Luca Coelho (2024-11-01 09:58:33-03:00)
>On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>> Allow simpler syntax for defining entries for single registers in range
>> tables. That makes them easier to type as well as to read, allowing one
>> to quickly tell whether a range actually refers to a single register or
>> a "true range".
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 118 ++++++++++----------
>>  1 file changed, 60 insertions(+), 58 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> index 8bf2f32be859..6992ce654e75 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> @@ -54,82 +54,82 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>>  };
>>  
>>  static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>> -        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>> +        { .start = 0x45500 }, /* DC_STATE_SEL */
>>          { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>> -        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +        { .start = 0x45504 }, /* DC_STATE_EN */
>>          { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>> -        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>> +        { .start = 0x454f0 }, /* RETENTION_CTRL */
>>  
>>          /* DBUF_CTL_* */
>> -        { .start = 0x44300, .end = 0x44300 },
>> -        { .start = 0x44304, .end = 0x44304 },
>> -        { .start = 0x44f00, .end = 0x44f00 },
>> -        { .start = 0x44f04, .end = 0x44f04 },
>> -        { .start = 0x44fe8, .end = 0x44fe8 },
>> -        { .start = 0x45008, .end = 0x45008 },
>> +        { .start = 0x44300 },
>> +        { .start = 0x44304 },
>> +        { .start = 0x44f00 },
>> +        { .start = 0x44f04 },
>> +        { .start = 0x44fe8 },
>> +        { .start = 0x45008 },
>>  
>> -        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> -        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> -        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +        { .start = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008 }, /* CDCLK_SQUASH_CTL */
>
>Many of these are probably actually ranges.  I'm not a HW guy, but
>these are probably blocks that need the wakelock and it just happens
>that many of those addresses are actually not used, but would need a
>wakelock if they were used?
>
>IOW, e.g. all these DBUF_CTL registers are probably in the same range
>that needs wakelocks (i.e. 0x44300-0x46fff)? Do we really need to
>define many of these individually?
>
>This is related to the previous patch as well, but I decided to comment
>it here because it becomes clearer.

Maybe my reply on the previous patch clarifies this? I.e., these
offset or offset ranges represent offsets that the DMC touches when on
specific DC states.

--
Gustavo Sousa

>
>--
>Cheers,
>Luca.

Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default

[Gustavo Sousa]

Quoting Luca Coelho (2024-11-01 11:27:10-03:00)
>On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>> Although Bspec doesn't explicitly mentions that, as of Xe3_LPD, using
>> DMC wakelock is the officially recommended way of accessing registers
>> that would be off during DC5/DC6 and the legacy method (where the DMC
>> intercepts MMIO to wake up the hardware) is to be avoided.
>> 
>> As such, update the driver to use the DMC wakelock by default starting
>> with Xe3_LPD. Since the feature is somewhat new to the driver, also
>> allow disabling it via a module parameter for debugging purposes.
>> 
>> For that, make the existing parameter allow values -1 (per-chip
>> default), 0 (disabled) and 1 (enabled), similarly to what is done for
>> other parameters.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
>>  drivers/gpu/drm/i915/display/intel_display_params.h | 2 +-
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 6 +++++-
>>  3 files changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
>> index 024de8abcb1a..bf00e5f1f145 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
>> @@ -123,10 +123,10 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
>>          "(0=disabled, 1=enabled) "
>>          "Default: 1");
>>  
>> -intel_display_param_named_unsafe(enable_dmc_wl, bool, 0400,
>> +intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
>>          "Enable DMC wakelock "
>>          "(0=disabled, 1=enabled) "
>> -        "Default: 0");
>> +        "Default: -1 (use per-chip default)");
>
>We're already explaining the possible values in the previous
>parentheses, so maybe the -1 should also be explained there?

Yep that makes sense. I was following the trend of what was done for
enable_fbc and enable_psr, but I guess following other examples in this
same file where we tag the default one with "[default]" is better.

Thanks! I'll update this on the next version.

--
Gustavo Sousa

Re: [PATCH 11/13] drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()

[Gustavo Sousa]

Quoting Gustavo Sousa (2024-10-22 08:03:39-03:00)
>Quoting Jani Nikula (2024-10-22 06:37:51-03:00)
>>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>> In order to be able to use the DMC wakelock, we also need to know that
>>> the display hardware has support for DMC, which is a runtime info.
>>> Define HAS_DMC_WAKELOCK(), which checks for both DMC availability and IP
>>> version, and use it in place of directly checking the display version.
>>>
>>> Since we depend on runtime info, also make sure to call
>>> intel_dmc_wl_init() only after we have probed the hardware for such info
>>> (i.e. after intel_display_device_info_runtime_init()).
>>
>>Non-functional changes combined with functional changes. Please split.
>
>Do you mean changing the call site of intel_dmc_wl_init() as being
>non-functional? Or is it something else?

Jani, I'll send a v2 soon-ish. I'll go ahead and assume the anwser for
the above is the former. Please stop me if otherwise :-)

--
Gustavo Sousa

>
>If this is about the former, I would argue that's not really
>non-functional, because we are changing the order of how things are
>done... But if making that a standalone patch is preferred, I can do
>that.
>
>--
>Gustavo Sousa
>
>>
>>BR,
>>Jani.
>>
>>>
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
>>>  drivers/gpu/drm/i915/display/intel_display_driver.c | 2 +-
>>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 4 ++--
>>>  3 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>>> index 071a36b51f79..5f78fd127fe0 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>>> @@ -128,6 +128,7 @@ enum intel_display_subplatform {
>>>  #define HAS_DDI(i915)                        (DISPLAY_INFO(i915)->has_ddi)
>>>  #define HAS_DISPLAY(i915)                (DISPLAY_RUNTIME_INFO(i915)->pipe_mask != 0)
>>>  #define HAS_DMC(i915)                        (DISPLAY_RUNTIME_INFO(i915)->has_dmc)
>>> +#define HAS_DMC_WAKELOCK(i915)                (HAS_DMC(i915) && DISPLAY_VER(i915) >= 20)
>>>  #define HAS_DOUBLE_BUFFERED_M_N(i915)        (DISPLAY_VER(i915) >= 9 || IS_BROADWELL(i915))
>>>  #define HAS_DP_MST(i915)                (DISPLAY_INFO(i915)->has_dp_mst)
>>>  #define HAS_DP20(i915)                        (IS_DG2(i915) || DISPLAY_VER(i915) >= 14)
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> index 673f9b965494..8afaa9cb89d2 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> @@ -200,7 +200,6 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915)
>>>          intel_dpll_init_clock_hook(i915);
>>>          intel_init_display_hooks(i915);
>>>          intel_fdi_init_hook(i915);
>>> -        intel_dmc_wl_init(&i915->display);
>>>  }
>>>  
>>>  /* part #1: call before irq install */
>>> @@ -238,6 +237,7 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
>>>                  return 0;
>>>  
>>>          intel_dmc_init(display);
>>> +        intel_dmc_wl_init(display);
>>>  
>>>          i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
>>>          i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>> index 8283b607aac4..f6ec79b0e39d 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>> @@ -250,7 +250,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>>  
>>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>>>  {
>>> -        if (DISPLAY_VER(display) < 20 ||
>>> +        if (!HAS_DMC_WAKELOCK(display) ||
>>>              !intel_dmc_has_payload(display) ||
>>>              !display->params.enable_dmc_wl)
>>>                  return false;
>>> @@ -263,7 +263,7 @@ void intel_dmc_wl_init(struct intel_display *display)
>>>          struct intel_dmc_wl *wl = &display->wl;
>>>  
>>>          /* don't call __intel_dmc_wl_supported(), DMC is not loaded yet */
>>> -        if (DISPLAY_VER(display) < 20 || !display->params.enable_dmc_wl)
>>> +        if (!HAS_DMC_WAKELOCK(display) || !display->params.enable_dmc_wl)
>>>                  return;
>>>  
>>>          INIT_DELAYED_WORK(&wl->work, intel_dmc_wl_work);
>>
>>-- 
>>Jani Nikula, Intel

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Gustavo Sousa]

Quoting Jani Nikula (2024-10-22 05:03:21-03:00)
>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> There are extra registers that require the DMC wakelock when specific
>> dynamic DC states are in place. Add the table ranges for them and use
>> the correct table depending on the allowed DC states.
>>
>> Bspec: 71583
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>>  1 file changed, 108 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> index d597cc825f64..8bf2f32be859 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> @@ -5,6 +5,7 @@
>>  
>>  #include <linux/kernel.h>
>>  
>> +#include "i915_reg.h"
>>  #include "intel_de.h"
>>  #include "intel_dmc.h"
>>  #include "intel_dmc_regs.h"
>> @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>>          {},
>>  };
>>  
>> +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>> +        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>> +        { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +        { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>> +        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>> +
>> +        /* DBUF_CTL_* */
>> +        { .start = 0x44300, .end = 0x44300 },
>> +        { .start = 0x44304, .end = 0x44304 },
>> +        { .start = 0x44f00, .end = 0x44f00 },
>> +        { .start = 0x44f04, .end = 0x44f04 },
>> +        { .start = 0x44fe8, .end = 0x44fe8 },
>> +        { .start = 0x45008, .end = 0x45008 },
>> +
>> +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +
>> +        /* TRANS_CMTG_CTL_* */
>> +        { .start = 0x6fa88, .end = 0x6fa88 },
>> +        { .start = 0x6fb88, .end = 0x6fb88 },
>> +
>> +        { .start = 0x46430, .end = 0x46430 }, /* CHICKEN_DCPR_1 */
>> +        { .start = 0x46434, .end = 0x46434 }, /* CHICKEN_DCPR_2 */
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +        { .start = 0x42084, .end = 0x42084 }, /* CHICKEN_MISC_2 */
>> +        { .start = 0x42088, .end = 0x42088 }, /* CHICKEN_MISC_3 */
>> +        { .start = 0x46160, .end = 0x46160 }, /* CMTG_CLK_SEL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        {},
>> +};
>> +
>> +static struct intel_dmc_wl_range xe3lpd_dc3co_wl_ranges[] = {
>> +        { .start = 0x454a0, .end = 0x454a0 }, /* CHICKEN_DCPR_4 */
>> +
>> +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> +
>> +        /* DBUF_CTL_* */
>> +        { .start = 0x44300, .end = 0x44300 },
>> +        { .start = 0x44304, .end = 0x44304 },
>> +        { .start = 0x44f00, .end = 0x44f00 },
>> +        { .start = 0x44f04, .end = 0x44f04 },
>> +        { .start = 0x44fe8, .end = 0x44fe8 },
>> +        { .start = 0x45008, .end = 0x45008 },
>> +
>> +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> +        { .start = 0x8f000, .end = 0x8ffff }, /* Main DMC registers */
>> +
>> +        /* Scanline registers */
>> +        { .start = 0x70000, .end = 0x70000 },
>> +        { .start = 0x70004, .end = 0x70004 },
>> +        { .start = 0x70014, .end = 0x70014 },
>> +        { .start = 0x70018, .end = 0x70018 },
>> +        { .start = 0x71000, .end = 0x71000 },
>> +        { .start = 0x71004, .end = 0x71004 },
>> +        { .start = 0x71014, .end = 0x71014 },
>> +        { .start = 0x71018, .end = 0x71018 },
>> +        { .start = 0x72000, .end = 0x72000 },
>> +        { .start = 0x72004, .end = 0x72004 },
>> +        { .start = 0x72014, .end = 0x72014 },
>> +        { .start = 0x72018, .end = 0x72018 },
>> +        { .start = 0x73000, .end = 0x73000 },
>> +        { .start = 0x73004, .end = 0x73004 },
>> +        { .start = 0x73014, .end = 0x73014 },
>> +        { .start = 0x73018, .end = 0x73018 },
>> +        { .start = 0x7b000, .end = 0x7b000 },
>> +        { .start = 0x7b004, .end = 0x7b004 },
>> +        { .start = 0x7b014, .end = 0x7b014 },
>> +        { .start = 0x7b018, .end = 0x7b018 },
>> +        { .start = 0x7c000, .end = 0x7c000 },
>> +        { .start = 0x7c004, .end = 0x7c004 },
>> +        { .start = 0x7c014, .end = 0x7c014 },
>> +        { .start = 0x7c018, .end = 0x7c018 },
>> +
>> +        {},
>> +};
>> +
>>  static void __intel_dmc_wl_release(struct intel_display *display)
>>  {
>>          struct drm_i915_private *i915 = to_i915(display->drm);
>> @@ -106,9 +188,31 @@ static bool intel_dmc_wl_addr_in_range(u32 address,
>>          return false;
>>  }
>>  
>> -static bool intel_dmc_wl_check_range(u32 address)
>> +static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>  {
>> -        return intel_dmc_wl_addr_in_range(address, lnl_wl_range);
>> +        const struct intel_dmc_wl_range *ranges;
>> +
>> +        ranges = lnl_wl_range;
>> +
>> +        if (intel_dmc_wl_addr_in_range(address, ranges))
>> +                return true;
>> +
>> +        switch (display->power.domains.dc_state) {
>
>This file has no business looking at power domain guts. Use or add
>interfaces instead of poking at data directly.

I started adding a function intel_display_power_get_current_dc_state()
here, but then realized that display->power.domains is protected by a
mutex and we do not want to use it in an atomic context.

So, in v2, to avoid rewriting the whole power domains code to use
spinlocks, I decided to go with having a copy of dc_state struct
intel_dmc_wl, which is set by intel_dmc_wl_enable().

--
Gustavo Sousa

>
>> +        case DC_STATE_EN_DC3CO:
>> +                ranges = xe3lpd_dc3co_wl_ranges;
>> +                break;
>> +        case DC_STATE_EN_UPTO_DC5:
>> +        case DC_STATE_EN_UPTO_DC6:
>> +                ranges = xe3lpd_dc5_dc6_wl_ranges;
>> +                break;
>> +        default:
>> +                ranges = NULL;
>> +        }
>> +
>> +        if (ranges && intel_dmc_wl_addr_in_range(address, ranges))
>> +                return true;
>> +
>> +        return false;
>>  }
>>  
>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>> @@ -195,7 +299,7 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>>          if (!__intel_dmc_wl_supported(display))
>>                  return;
>>  
>> -        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>> +        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>
>Side note, unrelated to this patch, i915_reg_t is supposed to be opaque,
>nobody should look at reg.reg directly, there's i915_mmio_reg_offset()
>for it.
>
>>                  return;
>>  
>>          spin_lock_irqsave(&wl->lock, flags);
>> @@ -247,7 +351,7 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
>>          if (!__intel_dmc_wl_supported(display))
>>                  return;
>>  
>> -        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg.reg))
>> +        if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(display, reg.reg))
>>                  return;
>>  
>>          spin_lock_irqsave(&wl->lock, flags);
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default

[Gustavo Sousa]

Quoting Gustavo Sousa (2024-11-05 10:46:52-03:00)
>Quoting Luca Coelho (2024-11-01 11:27:10-03:00)
>>On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>>> Although Bspec doesn't explicitly mentions that, as of Xe3_LPD, using
>>> DMC wakelock is the officially recommended way of accessing registers
>>> that would be off during DC5/DC6 and the legacy method (where the DMC
>>> intercepts MMIO to wake up the hardware) is to be avoided.
>>> 
>>> As such, update the driver to use the DMC wakelock by default starting
>>> with Xe3_LPD. Since the feature is somewhat new to the driver, also
>>> allow disabling it via a module parameter for debugging purposes.
>>> 
>>> For that, make the existing parameter allow values -1 (per-chip
>>> default), 0 (disabled) and 1 (enabled), similarly to what is done for
>>> other parameters.
>>> 
>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
>>>  drivers/gpu/drm/i915/display/intel_display_params.h | 2 +-
>>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 6 +++++-
>>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
>>> index 024de8abcb1a..bf00e5f1f145 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_params.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
>>> @@ -123,10 +123,10 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
>>>          "(0=disabled, 1=enabled) "
>>>          "Default: 1");
>>>  
>>> -intel_display_param_named_unsafe(enable_dmc_wl, bool, 0400,
>>> +intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
>>>          "Enable DMC wakelock "
>>>          "(0=disabled, 1=enabled) "
>>> -        "Default: 0");
>>> +        "Default: -1 (use per-chip default)");
>>
>>We're already explaining the possible values in the previous
>>parentheses, so maybe the -1 should also be explained there?
>
>Yep that makes sense. I was following the trend of what was done for
>enable_fbc and enable_psr, but I guess following other examples in this
>same file where we tag the default one with "[default]" is better.

Ended up simply doing this:

    diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
    index bf00e5f1f145..dc666aefa362 100644
    --- a/drivers/gpu/drm/i915/display/intel_display_params.c
    +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
    @@ -125,8 +125,8 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
     
     intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
     	"Enable DMC wakelock "
    -	"(0=disabled, 1=enabled) "
    -	"Default: -1 (use per-chip default)");
    +	"(-1=use per-chip default, 0=disabled, 1=enabled) "
    +	"Default: -1");
     
     __maybe_unused
     static void _param_print_bool(struct drm_printer *p, const char *driver_name,

, because repeating the word "default" in "(-1=use per-chip default
[default], 0=disabled, 1=enabled)" looked weird.

--
Gustavo Sousa

>
>Thanks! I'll update this on the next version.
>
>--
>Gustavo Sousa

Re: [PATCH 11/13] drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()

[Jani Nikula]

On Tue, 05 Nov 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Gustavo Sousa (2024-10-22 08:03:39-03:00)
>>Quoting Jani Nikula (2024-10-22 06:37:51-03:00)
>>>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>>> In order to be able to use the DMC wakelock, we also need to know that
>>>> the display hardware has support for DMC, which is a runtime info.
>>>> Define HAS_DMC_WAKELOCK(), which checks for both DMC availability and IP
>>>> version, and use it in place of directly checking the display version.
>>>>
>>>> Since we depend on runtime info, also make sure to call
>>>> intel_dmc_wl_init() only after we have probed the hardware for such info
>>>> (i.e. after intel_display_device_info_runtime_init()).
>>>
>>>Non-functional changes combined with functional changes. Please split.
>>
>>Do you mean changing the call site of intel_dmc_wl_init() as being
>>non-functional? Or is it something else?
>
> Jani, I'll send a v2 soon-ish. I'll go ahead and assume the anwser for
> the above is the former. Please stop me if otherwise :-)

Sorry, inbox overflowing. I think I meant that adding HAS_DMC_WAKELOCK()
is a non-functional change.

BR,
Jani.

>
> --
> Gustavo Sousa
>
>>
>>If this is about the former, I would argue that's not really
>>non-functional, because we are changing the order of how things are
>>done... But if making that a standalone patch is preferred, I can do
>>that.
>>
>>--
>>Gustavo Sousa
>>
>>>
>>>BR,
>>>Jani.
>>>
>>>>
>>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
>>>>  drivers/gpu/drm/i915/display/intel_display_driver.c | 2 +-
>>>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 4 ++--
>>>>  3 files changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>>>> index 071a36b51f79..5f78fd127fe0 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>>>> @@ -128,6 +128,7 @@ enum intel_display_subplatform {
>>>>  #define HAS_DDI(i915)                        (DISPLAY_INFO(i915)->has_ddi)
>>>>  #define HAS_DISPLAY(i915)                (DISPLAY_RUNTIME_INFO(i915)->pipe_mask != 0)
>>>>  #define HAS_DMC(i915)                        (DISPLAY_RUNTIME_INFO(i915)->has_dmc)
>>>> +#define HAS_DMC_WAKELOCK(i915)                (HAS_DMC(i915) && DISPLAY_VER(i915) >= 20)
>>>>  #define HAS_DOUBLE_BUFFERED_M_N(i915)        (DISPLAY_VER(i915) >= 9 || IS_BROADWELL(i915))
>>>>  #define HAS_DP_MST(i915)                (DISPLAY_INFO(i915)->has_dp_mst)
>>>>  #define HAS_DP20(i915)                        (IS_DG2(i915) || DISPLAY_VER(i915) >= 14)
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>> index 673f9b965494..8afaa9cb89d2 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>> @@ -200,7 +200,6 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915)
>>>>          intel_dpll_init_clock_hook(i915);
>>>>          intel_init_display_hooks(i915);
>>>>          intel_fdi_init_hook(i915);
>>>> -        intel_dmc_wl_init(&i915->display);
>>>>  }
>>>>  
>>>>  /* part #1: call before irq install */
>>>> @@ -238,6 +237,7 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
>>>>                  return 0;
>>>>  
>>>>          intel_dmc_init(display);
>>>> +        intel_dmc_wl_init(display);
>>>>  
>>>>          i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
>>>>          i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>> index 8283b607aac4..f6ec79b0e39d 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>> @@ -250,7 +250,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>>>  
>>>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>>>>  {
>>>> -        if (DISPLAY_VER(display) < 20 ||
>>>> +        if (!HAS_DMC_WAKELOCK(display) ||
>>>>              !intel_dmc_has_payload(display) ||
>>>>              !display->params.enable_dmc_wl)
>>>>                  return false;
>>>> @@ -263,7 +263,7 @@ void intel_dmc_wl_init(struct intel_display *display)
>>>>          struct intel_dmc_wl *wl = &display->wl;
>>>>  
>>>>          /* don't call __intel_dmc_wl_supported(), DMC is not loaded yet */
>>>> -        if (DISPLAY_VER(display) < 20 || !display->params.enable_dmc_wl)
>>>> +        if (!HAS_DMC_WAKELOCK(display) || !display->params.enable_dmc_wl)
>>>>                  return;
>>>>  
>>>>          INIT_DELAYED_WORK(&wl->work, intel_dmc_wl_work);
>>>
>>>-- 
>>>Jani Nikula, Intel

-- 
Jani Nikula, Intel

Re: [PATCH 04/13] drm/i915/dmc_wl: Get wakelock when disabling dynamic DC states

[Luca Coelho]

On Tue, 2024-11-05 at 09:44 -0300, Gustavo Sousa wrote:
> Quoting Luca Coelho (2024-11-01 09:24:08-03:00)
> > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> > > Bspec says that disabling dynamic DC states require taking the DMC
> > > wakelock to cause an DC exit before writing to DC_STATE_EN. Implement
> > > that.
> > > 
> > > In fact, testing on PTL revealed we end up failing to exit DC5/6 without
> > > this step.
> > > 
> > > Bspec: 71583
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_power_well.c    | 10 +++++++---
> > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c        | 14 ++++++++++++--
> > >  drivers/gpu/drm/i915/display/intel_dmc_wl.h        |  2 ++
> > >  3 files changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > index adaf7cf3a33b..e8946ce86aaa 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> > > @@ -987,10 +987,14 @@ void gen9_disable_dc_states(struct intel_display *display)
> > >                  return;
> > >          }
> > >  
> > > -        gen9_set_dc_state(display, DC_STATE_DISABLE);
> > > -
> > > -        if (!HAS_DISPLAY(display))
> > > +        if (HAS_DISPLAY(display)) {
> > > +                intel_dmc_wl_get_noreg(display);
> > > +                gen9_set_dc_state(display, DC_STATE_DISABLE);
> > > +                intel_dmc_wl_put_noreg(display);
> > > +        } else {
> > > +                gen9_set_dc_state(display, DC_STATE_DISABLE);
> > >                  return;
> > > +        }
> > 
> > I think intel_dmc_get/put() already protect indirectly on
> > HAS_DISPLAY(), doesn't it? If that's the case, then the if here is
> > unnecessary.
> 
> Actually, intel_dmc_wl_init() gets called only when HAS_DISPLAY() is
> true, so I think using intel_dmc_wl_{get,put}_noreg() for the
> !HAS_DISPLAY() case would not be right, at least not with the current
> state of the code.

Okay, fair enough.

--
Cheers,
Luca.

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Luca Coelho]

On Tue, 2024-11-05 at 10:00 -0300, Gustavo Sousa wrote:
> Quoting Luca Coelho (2024-11-01 09:51:48-03:00)
> > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> > > There are extra registers that require the DMC wakelock when specific
> > > dynamic DC states are in place. Add the table ranges for them and use
> > > the correct table depending on the allowed DC states.
> > > 
> > > Bspec: 71583
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
> > >  1 file changed, 108 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > index d597cc825f64..8bf2f32be859 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > @@ -5,6 +5,7 @@
> > >  
> > >  #include <linux/kernel.h>
> > >  
> > > +#include "i915_reg.h"
> > >  #include "intel_de.h"
> > >  #include "intel_dmc.h"
> > >  #include "intel_dmc_regs.h"
> > > @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
> > >          {},
> > >  };
> > 
> > Do we still need the lnl_wl_range[]? This was sort of a place-holder
> > with a very large range just for testing.  I can see that there are at
> > least some ranges in common between lnl_wl_range[] and the new range
> > tables defined below.
> 
> Yes, although we could do some homework to get a more accurate set of
> ranges.
> 
> Now, about the different tables:
> 
>  - lnl_wl_range should be about ranges of registers that are powered
>    down during DC states and that the HW requires DC exit for proper
>    access.
>  - xe3lpd_{dc5_dc6,dc3co}_wl_ranges are registers that are touched by
>    the DMC and need the wakelock for properly restoring the correct
>    value before accessing them.
> 
> Maybe a comment in the code explaining the above is warranted?

I think a better naming for the arrays is warranted. :) Wouldn't
changing lnl_wl_range to base_wl_range or so be better? My point is
that LNL is not related at all here (anymore).


> > > +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
> > > +        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
> > > +        { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
> > > +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> > > +        { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
> > > +        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
> > > +
> > > +        /* DBUF_CTL_* */
> > > +        { .start = 0x44300, .end = 0x44300 },
> > > +        { .start = 0x44304, .end = 0x44304 },
> > > +        { .start = 0x44f00, .end = 0x44f00 },
> > > +        { .start = 0x44f04, .end = 0x44f04 },
> > > +        { .start = 0x44fe8, .end = 0x44fe8 },
> > > +        { .start = 0x45008, .end = 0x45008 },
> > > +
> > > +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> > > +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> > > +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> > > +
> > > +        /* TRANS_CMTG_CTL_* */
> > > +        { .start = 0x6fa88, .end = 0x6fa88 },
> > > +        { .start = 0x6fb88, .end = 0x6fb88 },
> > 
> > These, for instance, are part of lnl_wl_range[].
> 
> Given my clarification above about the different purposes of the ranges,
> I think we should stick to keeping the same values from the (soon to
> be?) documented tables, even if there is some small redundancy.
> Otherwise we would require the programmer to remember to check ranges in
> the "more general" table every time a DC state-specific one needs to be
> added or updated.

Makes sense, I guess it's okay that the base table and the specialized
tables are slightly redundant then.

--
Cheers,
Luca.

Re: [PATCH 08/13] drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables

[Luca Coelho]

On Tue, 2024-11-05 at 10:42 -0300, Gustavo Sousa wrote:
> Quoting Luca Coelho (2024-11-01 09:58:33-03:00)
> > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> > > Allow simpler syntax for defining entries for single registers in range
> > > tables. That makes them easier to type as well as to read, allowing one
> > > to quickly tell whether a range actually refers to a single register or
> > > a "true range".
> > > 
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 118 ++++++++++----------
> > >  1 file changed, 60 insertions(+), 58 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > index 8bf2f32be859..6992ce654e75 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > @@ -54,82 +54,82 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
> > >  };
> > >  
> > >  static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
> > > -        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
> > > +        { .start = 0x45500 }, /* DC_STATE_SEL */
> > >          { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
> > > -        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> > > +        { .start = 0x45504 }, /* DC_STATE_EN */
> > >          { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
> > > -        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
> > > +        { .start = 0x454f0 }, /* RETENTION_CTRL */
> > >  
> > >          /* DBUF_CTL_* */
> > > -        { .start = 0x44300, .end = 0x44300 },
> > > -        { .start = 0x44304, .end = 0x44304 },
> > > -        { .start = 0x44f00, .end = 0x44f00 },
> > > -        { .start = 0x44f04, .end = 0x44f04 },
> > > -        { .start = 0x44fe8, .end = 0x44fe8 },
> > > -        { .start = 0x45008, .end = 0x45008 },
> > > +        { .start = 0x44300 },
> > > +        { .start = 0x44304 },
> > > +        { .start = 0x44f00 },
> > > +        { .start = 0x44f04 },
> > > +        { .start = 0x44fe8 },
> > > +        { .start = 0x45008 },
> > >  
> > > -        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> > > -        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> > > -        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> > > +        { .start = 0x46070 }, /* CDCLK_PLL_ENABLE */
> > > +        { .start = 0x46000 }, /* CDCLK_CTL */
> > > +        { .start = 0x46008 }, /* CDCLK_SQUASH_CTL */
> > 
> > Many of these are probably actually ranges.  I'm not a HW guy, but
> > these are probably blocks that need the wakelock and it just happens
> > that many of those addresses are actually not used, but would need a
> > wakelock if they were used?
> > 
> > IOW, e.g. all these DBUF_CTL registers are probably in the same range
> > that needs wakelocks (i.e. 0x44300-0x46fff)? Do we really need to
> > define many of these individually?
> > 
> > This is related to the previous patch as well, but I decided to comment
> > it here because it becomes clearer.
> 
> Maybe my reply on the previous patch clarifies this? I.e., these
> offset or offset ranges represent offsets that the DMC touches when on
> specific DC states.

Yeah, but I think this idea of blocks is still valid.  I think it's
very unlikely that only certain _addresses_ and not full blocks of
addresses are affected in the HW.

For instance:

         /* DBUF_CTL_* */
-        { .start = 0x44300, .end = 0x44300 },
-        { .start = 0x44304, .end = 0x44304 },
-        { .start = 0x44f00, .end = 0x44f00 },
-        { .start = 0x44f04, .end = 0x44f04 },
-        { .start = 0x44fe8, .end = 0x44fe8 },

This probably means that _all_ the block, from at least 0x44300 till
0x44fff, needs to be protected.  What I'm trying to say is that even
though we don't access e.g. 0x44400, if we did, it would most likely
also have to be protected, because it's in the same block of addresses.

I guess this doesn't matter _that_ much, but it would be just cleaner
to know the actual ranges where the wakelocks are _potentially_ needed.

--
Cheers,
Luca.

Re: [PATCH 13/13] drm/i915/xe3lpd: Use DMC wakelock by default

[Luca Coelho]

On Tue, 2024-11-05 at 18:12 -0300, Gustavo Sousa wrote:
> Quoting Gustavo Sousa (2024-11-05 10:46:52-03:00)
> > Quoting Luca Coelho (2024-11-01 11:27:10-03:00)
> > > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> > > > Although Bspec doesn't explicitly mentions that, as of Xe3_LPD, using
> > > > DMC wakelock is the officially recommended way of accessing registers
> > > > that would be off during DC5/DC6 and the legacy method (where the DMC
> > > > intercepts MMIO to wake up the hardware) is to be avoided.
> > > > 
> > > > As such, update the driver to use the DMC wakelock by default starting
> > > > with Xe3_LPD. Since the feature is somewhat new to the driver, also
> > > > allow disabling it via a module parameter for debugging purposes.
> > > > 
> > > > For that, make the existing parameter allow values -1 (per-chip
> > > > default), 0 (disabled) and 1 (enabled), similarly to what is done for
> > > > other parameters.
> > > > 
> > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display_params.c | 4 ++--
> > > >  drivers/gpu/drm/i915/display/intel_display_params.h | 2 +-
> > > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 6 +++++-
> > > >  3 files changed, 8 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > index 024de8abcb1a..bf00e5f1f145 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
> > > > @@ -123,10 +123,10 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
> > > >          "(0=disabled, 1=enabled) "
> > > >          "Default: 1");
> > > >  
> > > > -intel_display_param_named_unsafe(enable_dmc_wl, bool, 0400,
> > > > +intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
> > > >          "Enable DMC wakelock "
> > > >          "(0=disabled, 1=enabled) "
> > > > -        "Default: 0");
> > > > +        "Default: -1 (use per-chip default)");
> > > 
> > > We're already explaining the possible values in the previous
> > > parentheses, so maybe the -1 should also be explained there?
> > 
> > Yep that makes sense. I was following the trend of what was done for
> > enable_fbc and enable_psr, but I guess following other examples in this
> > same file where we tag the default one with "[default]" is better.
> 
> Ended up simply doing this:
> 
>     diff --git a/drivers/gpu/drm/i915/display/intel_display_params.c b/drivers/gpu/drm/i915/display/intel_display_params.c
>     index bf00e5f1f145..dc666aefa362 100644
>     --- a/drivers/gpu/drm/i915/display/intel_display_params.c
>     +++ b/drivers/gpu/drm/i915/display/intel_display_params.c
>     @@ -125,8 +125,8 @@ intel_display_param_named_unsafe(enable_psr2_sel_fetch, bool, 0400,
>      
>      intel_display_param_named_unsafe(enable_dmc_wl, int, 0400,
>      	"Enable DMC wakelock "
>     -	"(0=disabled, 1=enabled) "
>     -	"Default: -1 (use per-chip default)");
>     +	"(-1=use per-chip default, 0=disabled, 1=enabled) "
>     +	"Default: -1");
>      
>      __maybe_unused
>      static void _param_print_bool(struct drm_printer *p, const char *driver_name,
> 
> , because repeating the word "default" in "(-1=use per-chip default
> [default], 0=disabled, 1=enabled)" looked weird.

Yep, this looks good!

--
Cheers,
Luca.

Re: [PATCH 08/13] drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables

[Gustavo Sousa]

Quoting Luca Coelho (2024-11-06 09:23:32-03:00)
>On Tue, 2024-11-05 at 10:42 -0300, Gustavo Sousa wrote:
>> Quoting Luca Coelho (2024-11-01 09:58:33-03:00)
>> > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>> > > Allow simpler syntax for defining entries for single registers in range
>> > > tables. That makes them easier to type as well as to read, allowing one
>> > > to quickly tell whether a range actually refers to a single register or
>> > > a "true range".
>> > > 
>> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 118 ++++++++++----------
>> > >  1 file changed, 60 insertions(+), 58 deletions(-)
>> > > 
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > index 8bf2f32be859..6992ce654e75 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > @@ -54,82 +54,82 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>> > >  };
>> > >  
>> > >  static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>> > > -        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>> > > +        { .start = 0x45500 }, /* DC_STATE_SEL */
>> > >          { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>> > > -        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> > > +        { .start = 0x45504 }, /* DC_STATE_EN */
>> > >          { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>> > > -        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>> > > +        { .start = 0x454f0 }, /* RETENTION_CTRL */
>> > >  
>> > >          /* DBUF_CTL_* */
>> > > -        { .start = 0x44300, .end = 0x44300 },
>> > > -        { .start = 0x44304, .end = 0x44304 },
>> > > -        { .start = 0x44f00, .end = 0x44f00 },
>> > > -        { .start = 0x44f04, .end = 0x44f04 },
>> > > -        { .start = 0x44fe8, .end = 0x44fe8 },
>> > > -        { .start = 0x45008, .end = 0x45008 },
>> > > +        { .start = 0x44300 },
>> > > +        { .start = 0x44304 },
>> > > +        { .start = 0x44f00 },
>> > > +        { .start = 0x44f04 },
>> > > +        { .start = 0x44fe8 },
>> > > +        { .start = 0x45008 },
>> > >  
>> > > -        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> > > -        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> > > -        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> > > +        { .start = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> > > +        { .start = 0x46000 }, /* CDCLK_CTL */
>> > > +        { .start = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> > 
>> > Many of these are probably actually ranges.  I'm not a HW guy, but
>> > these are probably blocks that need the wakelock and it just happens
>> > that many of those addresses are actually not used, but would need a
>> > wakelock if they were used?
>> > 
>> > IOW, e.g. all these DBUF_CTL registers are probably in the same range
>> > that needs wakelocks (i.e. 0x44300-0x46fff)? Do we really need to
>> > define many of these individually?
>> > 
>> > This is related to the previous patch as well, but I decided to comment
>> > it here because it becomes clearer.
>> 
>> Maybe my reply on the previous patch clarifies this? I.e., these
>> offset or offset ranges represent offsets that the DMC touches when on
>> specific DC states.
>
>Yeah, but I think this idea of blocks is still valid.  I think it's
>very unlikely that only certain _addresses_ and not full blocks of
>addresses are affected in the HW.

Except that this is not about the hardware per se, this is about
registers that are touched by the *DMC* during DC states and that need
DC exit for properly accessing them from the driver. So, I think blocks
are not applicable here.

--
Gustavo Sousa

>
>For instance:
>
>         /* DBUF_CTL_* */
>-        { .start = 0x44300, .end = 0x44300 },
>-        { .start = 0x44304, .end = 0x44304 },
>-        { .start = 0x44f00, .end = 0x44f00 },
>-        { .start = 0x44f04, .end = 0x44f04 },
>-        { .start = 0x44fe8, .end = 0x44fe8 },
>
>This probably means that _all_ the block, from at least 0x44300 till
>0x44fff, needs to be protected.  What I'm trying to say is that even
>though we don't access e.g. 0x44400, if we did, it would most likely
>also have to be protected, because it's in the same block of addresses.
>
>I guess this doesn't matter _that_ much, but it would be just cleaner
>to know the actual ranges where the wakelocks are _potentially_ needed.
>
>--
>Cheers,
>Luca.

Re: [PATCH 08/13] drm/i915/dmc_wl: Allow simpler syntax for single reg in range tables

[Luca Coelho]

On Wed, 2024-11-06 at 09:29 -0300, Gustavo Sousa wrote:
> Quoting Luca Coelho (2024-11-06 09:23:32-03:00)
> > On Tue, 2024-11-05 at 10:42 -0300, Gustavo Sousa wrote:
> > > Quoting Luca Coelho (2024-11-01 09:58:33-03:00)
> > > > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
> > > > > Allow simpler syntax for defining entries for single registers in range
> > > > > tables. That makes them easier to type as well as to read, allowing one
> > > > > to quickly tell whether a range actually refers to a single register or
> > > > > a "true range".
> > > > > 
> > > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 118 ++++++++++----------
> > > > >  1 file changed, 60 insertions(+), 58 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > > > index 8bf2f32be859..6992ce654e75 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
> > > > > @@ -54,82 +54,82 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
> > > > >  };
> > > > >  
> > > > >  static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
> > > > > -        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
> > > > > +        { .start = 0x45500 }, /* DC_STATE_SEL */
> > > > >          { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
> > > > > -        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
> > > > > +        { .start = 0x45504 }, /* DC_STATE_EN */
> > > > >          { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
> > > > > -        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
> > > > > +        { .start = 0x454f0 }, /* RETENTION_CTRL */
> > > > >  
> > > > >          /* DBUF_CTL_* */
> > > > > -        { .start = 0x44300, .end = 0x44300 },
> > > > > -        { .start = 0x44304, .end = 0x44304 },
> > > > > -        { .start = 0x44f00, .end = 0x44f00 },
> > > > > -        { .start = 0x44f04, .end = 0x44f04 },
> > > > > -        { .start = 0x44fe8, .end = 0x44fe8 },
> > > > > -        { .start = 0x45008, .end = 0x45008 },
> > > > > +        { .start = 0x44300 },
> > > > > +        { .start = 0x44304 },
> > > > > +        { .start = 0x44f00 },
> > > > > +        { .start = 0x44f04 },
> > > > > +        { .start = 0x44fe8 },
> > > > > +        { .start = 0x45008 },
> > > > >  
> > > > > -        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
> > > > > -        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
> > > > > -        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
> > > > > +        { .start = 0x46070 }, /* CDCLK_PLL_ENABLE */
> > > > > +        { .start = 0x46000 }, /* CDCLK_CTL */
> > > > > +        { .start = 0x46008 }, /* CDCLK_SQUASH_CTL */
> > > > 
> > > > Many of these are probably actually ranges.  I'm not a HW guy, but
> > > > these are probably blocks that need the wakelock and it just happens
> > > > that many of those addresses are actually not used, but would need a
> > > > wakelock if they were used?
> > > > 
> > > > IOW, e.g. all these DBUF_CTL registers are probably in the same range
> > > > that needs wakelocks (i.e. 0x44300-0x46fff)? Do we really need to
> > > > define many of these individually?
> > > > 
> > > > This is related to the previous patch as well, but I decided to comment
> > > > it here because it becomes clearer.
> > > 
> > > Maybe my reply on the previous patch clarifies this? I.e., these
> > > offset or offset ranges represent offsets that the DMC touches when on
> > > specific DC states.
> > 
> > Yeah, but I think this idea of blocks is still valid.  I think it's
> > very unlikely that only certain _addresses_ and not full blocks of
> > addresses are affected in the HW.
> 
> Except that this is not about the hardware per se, this is about
> registers that are touched by the *DMC* during DC states and that need
> DC exit for properly accessing them from the driver. So, I think blocks
> are not applicable here.

Ah, okay, makes sense now. This could be explained in the awesome
comment you're planning to add, as discussed in the previous patch.

--
Cheers,
Luca.

Re: [PATCH 11/13] drm/i915/dmc_wl: Add and use HAS_DMC_WAKELOCK()

[Gustavo Sousa]

Quoting Jani Nikula (2024-11-06 06:25:52-03:00)
>On Tue, 05 Nov 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Quoting Gustavo Sousa (2024-10-22 08:03:39-03:00)
>>>Quoting Jani Nikula (2024-10-22 06:37:51-03:00)
>>>>On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>>>> In order to be able to use the DMC wakelock, we also need to know that
>>>>> the display hardware has support for DMC, which is a runtime info.
>>>>> Define HAS_DMC_WAKELOCK(), which checks for both DMC availability and IP
>>>>> version, and use it in place of directly checking the display version.
>>>>>
>>>>> Since we depend on runtime info, also make sure to call
>>>>> intel_dmc_wl_init() only after we have probed the hardware for such info
>>>>> (i.e. after intel_display_device_info_runtime_init()).
>>>>
>>>>Non-functional changes combined with functional changes. Please split.
>>>
>>>Do you mean changing the call site of intel_dmc_wl_init() as being
>>>non-functional? Or is it something else?
>>
>> Jani, I'll send a v2 soon-ish. I'll go ahead and assume the anwser for
>> the above is the former. Please stop me if otherwise :-)
>
>Sorry, inbox overflowing. I think I meant that adding HAS_DMC_WAKELOCK()
>is a non-functional change.

Ah, okay.

Well, I think the use of HAS_DMC() in the definition of
HAS_DMC_WAKELOCK() makes it a functional change when intel_dmc_wl.c uses
it (because we were not checking HAS_DMC() before). So, for an earlier
"non-functional" patch, maybe the way to go is something like the
following then?

- A patch defining HAS_DMC_WAKELOCK() with only DISPLAY_VER(i915) >=
  20 and use that macro in the DMC wakelock code.
- A modified version of this patch discarding stuff already done in the
  patch above.

Is that what you meant?

--
Gustavo Sousa

>
>BR,
>Jani.
>
>>
>> --
>> Gustavo Sousa
>>
>>>
>>>If this is about the former, I would argue that's not really
>>>non-functional, because we are changing the order of how things are
>>>done... But if making that a standalone patch is preferred, I can do
>>>that.
>>>
>>>--
>>>Gustavo Sousa
>>>
>>>>
>>>>BR,
>>>>Jani.
>>>>
>>>>>
>>>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/display/intel_display_device.h | 1 +
>>>>>  drivers/gpu/drm/i915/display/intel_display_driver.c | 2 +-
>>>>>  drivers/gpu/drm/i915/display/intel_dmc_wl.c         | 4 ++--
>>>>>  3 files changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
>>>>> index 071a36b51f79..5f78fd127fe0 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>>>>> @@ -128,6 +128,7 @@ enum intel_display_subplatform {
>>>>>  #define HAS_DDI(i915)                        (DISPLAY_INFO(i915)->has_ddi)
>>>>>  #define HAS_DISPLAY(i915)                (DISPLAY_RUNTIME_INFO(i915)->pipe_mask != 0)
>>>>>  #define HAS_DMC(i915)                        (DISPLAY_RUNTIME_INFO(i915)->has_dmc)
>>>>> +#define HAS_DMC_WAKELOCK(i915)                (HAS_DMC(i915) && DISPLAY_VER(i915) >= 20)
>>>>>  #define HAS_DOUBLE_BUFFERED_M_N(i915)        (DISPLAY_VER(i915) >= 9 || IS_BROADWELL(i915))
>>>>>  #define HAS_DP_MST(i915)                (DISPLAY_INFO(i915)->has_dp_mst)
>>>>>  #define HAS_DP20(i915)                        (IS_DG2(i915) || DISPLAY_VER(i915) >= 14)
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>>> index 673f9b965494..8afaa9cb89d2 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>>>> @@ -200,7 +200,6 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915)
>>>>>          intel_dpll_init_clock_hook(i915);
>>>>>          intel_init_display_hooks(i915);
>>>>>          intel_fdi_init_hook(i915);
>>>>> -        intel_dmc_wl_init(&i915->display);
>>>>>  }
>>>>>  
>>>>>  /* part #1: call before irq install */
>>>>> @@ -238,6 +237,7 @@ int intel_display_driver_probe_noirq(struct drm_i915_private *i915)
>>>>>                  return 0;
>>>>>  
>>>>>          intel_dmc_init(display);
>>>>> +        intel_dmc_wl_init(display);
>>>>>  
>>>>>          i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
>>>>>          i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>>> index 8283b607aac4..f6ec79b0e39d 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>>>>> @@ -250,7 +250,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, u32 address)
>>>>>  
>>>>>  static bool __intel_dmc_wl_supported(struct intel_display *display)
>>>>>  {
>>>>> -        if (DISPLAY_VER(display) < 20 ||
>>>>> +        if (!HAS_DMC_WAKELOCK(display) ||
>>>>>              !intel_dmc_has_payload(display) ||
>>>>>              !display->params.enable_dmc_wl)
>>>>>                  return false;
>>>>> @@ -263,7 +263,7 @@ void intel_dmc_wl_init(struct intel_display *display)
>>>>>          struct intel_dmc_wl *wl = &display->wl;
>>>>>  
>>>>>          /* don't call __intel_dmc_wl_supported(), DMC is not loaded yet */
>>>>> -        if (DISPLAY_VER(display) < 20 || !display->params.enable_dmc_wl)
>>>>> +        if (!HAS_DMC_WAKELOCK(display) || !display->params.enable_dmc_wl)
>>>>>                  return;
>>>>>  
>>>>>          INIT_DELAYED_WORK(&wl->work, intel_dmc_wl_work);
>>>>
>>>>-- 
>>>>Jani Nikula, Intel
>
>-- 
>Jani Nikula, Intel

Re: [PATCH 07/13] drm/i915/dmc_wl: Check ranges specific to DC states

[Gustavo Sousa]

Quoting Luca Coelho (2024-11-06 08:47:07-03:00)
>On Tue, 2024-11-05 at 10:00 -0300, Gustavo Sousa wrote:
>> Quoting Luca Coelho (2024-11-01 09:51:48-03:00)
>> > On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote:
>> > > There are extra registers that require the DMC wakelock when specific
>> > > dynamic DC states are in place. Add the table ranges for them and use
>> > > the correct table depending on the allowed DC states.
>> > > 
>> > > Bspec: 71583
>> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/display/intel_dmc_wl.c | 112 +++++++++++++++++++-
>> > >  1 file changed, 108 insertions(+), 4 deletions(-)
>> > > 
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > index d597cc825f64..8bf2f32be859 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>> > > @@ -5,6 +5,7 @@
>> > >  
>> > >  #include <linux/kernel.h>
>> > >  
>> > > +#include "i915_reg.h"
>> > >  #include "intel_de.h"
>> > >  #include "intel_dmc.h"
>> > >  #include "intel_dmc_regs.h"
>> > > @@ -52,6 +53,87 @@ static struct intel_dmc_wl_range lnl_wl_range[] = {
>> > >          {},
>> > >  };
>> > 
>> > Do we still need the lnl_wl_range[]? This was sort of a place-holder
>> > with a very large range just for testing.  I can see that there are at
>> > least some ranges in common between lnl_wl_range[] and the new range
>> > tables defined below.
>> 
>> Yes, although we could do some homework to get a more accurate set of
>> ranges.
>> 
>> Now, about the different tables:
>> 
>>  - lnl_wl_range should be about ranges of registers that are powered
>>    down during DC states and that the HW requires DC exit for proper
>>    access.
>>  - xe3lpd_{dc5_dc6,dc3co}_wl_ranges are registers that are touched by
>>    the DMC and need the wakelock for properly restoring the correct
>>    value before accessing them.
>> 
>> Maybe a comment in the code explaining the above is warranted?
>
>I think a better naming for the arrays is warranted. :) Wouldn't
>changing lnl_wl_range to base_wl_range or so be better? My point is
>that LNL is not related at all here (anymore).

Yep, we could come up with better names for those variables. I went
with:

    s/lnl_wl_range/powered_off_ranges/
    s/xe3lpd_dc3co_wl_ranges/xe3lpd_dc3co_dmc_ranges/
    s/xe3lpd_dc5_dc6_wl_ranges/xe3lpd_dc5_dc6_dmc_ranges/

And also added comments to differentiate their purposes.

--
Gustavo Sousa

>
>
>> > > +static struct intel_dmc_wl_range xe3lpd_dc5_dc6_wl_ranges[] = {
>> > > +        { .start = 0x45500, .end = 0x45500 }, /* DC_STATE_SEL */
>> > > +        { .start = 0x457a0, .end = 0x457b0 }, /* DC*_RESIDENCY_COUNTER */
>> > > +        { .start = 0x45504, .end = 0x45504 }, /* DC_STATE_EN */
>> > > +        { .start = 0x45400, .end = 0x4540c }, /* PWR_WELL_CTL_* */
>> > > +        { .start = 0x454f0, .end = 0x454f0 }, /* RETENTION_CTRL */
>> > > +
>> > > +        /* DBUF_CTL_* */
>> > > +        { .start = 0x44300, .end = 0x44300 },
>> > > +        { .start = 0x44304, .end = 0x44304 },
>> > > +        { .start = 0x44f00, .end = 0x44f00 },
>> > > +        { .start = 0x44f04, .end = 0x44f04 },
>> > > +        { .start = 0x44fe8, .end = 0x44fe8 },
>> > > +        { .start = 0x45008, .end = 0x45008 },
>> > > +
>> > > +        { .start = 0x46070, .end = 0x46070 }, /* CDCLK_PLL_ENABLE */
>> > > +        { .start = 0x46000, .end = 0x46000 }, /* CDCLK_CTL */
>> > > +        { .start = 0x46008, .end = 0x46008 }, /* CDCLK_SQUASH_CTL */
>> > > +
>> > > +        /* TRANS_CMTG_CTL_* */
>> > > +        { .start = 0x6fa88, .end = 0x6fa88 },
>> > > +        { .start = 0x6fb88, .end = 0x6fb88 },
>> > 
>> > These, for instance, are part of lnl_wl_range[].
>> 
>> Given my clarification above about the different purposes of the ranges,
>> I think we should stick to keeping the same values from the (soon to
>> be?) documented tables, even if there is some small redundancy.
>> Otherwise we would require the programmer to remember to check ranges in
>> the "more general" table every time a DC state-specific one needs to be
>> added or updated.
>
>Makes sense, I guess it's okay that the base table and the specialized
>tables are slightly redundant then.
>
>--
>Cheers,
>Luca.