[PATCH] drm/i915/xe3lpd: Power request asserting/deasserting

[Gustavo Sousa]

Quoting Mika Kahola (2024-10-28 09:58:35-03:00)
>There is a HW issue that arises when there are race conditions
>between TCSS entering/exiting TC7 or TC10 states while the
>driver is asserting/deasserting TCSS power request. As a
>workaround, Display driver will implement a mailbox sequence
>to ensure that the TCSS is in TC0 when TCSS power request is
>asserted/deasserted.
>
>The sequence is the following
>
>1. Read mailbox command status and wait until run/busy bit is
>   clear
>2. Write mailbox data value '1' for power request asserting
>   and '0' for power request deasserting
>3. Write mailbox command run/busy bit and command value with 0x1
>4. Read mailbox command and wait until run/busy bit is clear
>   before continuing power request.
>
>v2: Rename WA function (Gustavo)
>    Limit WA only for PTL platform with a TODO note (Gustavo)
>    Add TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY for clarity when writing
>    register data (Gustavo)
>    Move register defs from i915_reg.h to intel_cx0_phy_regs.h (Gustavo)
>
>Signed-off-by: Mika Kahola <mika.kahola at intel.com>
>---
> .../gpu/drm/i915/display/intel_cx0_phy_regs.h |  7 +++
> drivers/gpu/drm/i915/display/intel_tc.c       | 46 +++++++++++++++++++
> 2 files changed, 53 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>index ab3ae110b68f..e46c07cd20e0 100644
>--- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>@@ -363,4 +363,11 @@
> #define HDMI_DIV_MASK                REG_GENMASK16(2, 0)
> #define HDMI_DIV(val)                REG_FIELD_PREP16(HDMI_DIV_MASK, val)
> 
>+#define TCSS_DISP_MAILBOX_IN_CMD                _MMIO(0x161300)

This part of the header contains definitions specific to the PHY
components that are not memory mapped by our driver, but rather accessed
via message bus. As such, I think TCSS_DISP_MAILBOX_IN_CMD and
TCSS_DISP_MAILBOX_IN_DATA are better defined at the end of the MMIO
register definitions (i.e. before the "/* C10 Vendor Registers */"
line).

>+#define   TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY        REG_BIT(31)
>+#define   TCSS_DISP_MAILBOX_IN_CMD_CMD_MASK        REG_GENMASK(7, 0)
>+#define   TCSS_DISP_MAILBOX_IN_CMD_DATA(x)        TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY | \
>+                                                REG_FIELD_PREP(TCSS_DISP_MAILBOX_IN_CMD_CMD_MASK, (x))

Missing blank line here.

>+#define TCSS_DISP_MAILBOX_IN_DATA                _MMIO(0x161304)
>+
> #endif /* __INTEL_CX0_REG_DEFS_H__ */
>diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
>index 6f2ee7dbc43b..924c3ff04eb6 100644
>--- a/drivers/gpu/drm/i915/display/intel_tc.c
>+++ b/drivers/gpu/drm/i915/display/intel_tc.c
>@@ -1013,6 +1013,45 @@ xelpdp_tc_phy_wait_for_tcss_power(struct intel_tc_port *tc, bool enabled)
>         return true;
> }
> 
>+static bool wa_tcss_power_request_assert(struct drm_i915_private *i915,
>+                                         bool enable)

I think we should either name this function wa_14020908590 or add a /*
Wa_14020908590 */ comment above it. I would go with the former, but I'm
okay with the latter if you prefer.

>+{
>+        /*
>+         * Limit to PTL only
>+         * TODO: Add check for PICA IP and use that instead limiting WA for
>+         * platform
>+         */
>+        if (DISPLAY_VER(i915) != 30)
>+                return true;
>+
>+        /* check if mailbox is running busy */
>+        if (intel_de_wait_for_clear(i915, TCSS_DISP_MAILBOX_IN_CMD,
>+                                    TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY, 10)) {
>+                drm_dbg_kms(&i915->drm,
>+                            "Power request assert WA timeout waiting for TCSS mailbox run/busy bit to clear\n");
>+                return false;
>+        }
>+
>+        if (enable)
>+                intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, 1);
>+        else
>+                intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, 0);
>+
>+        intel_de_write(i915, TCSS_DISP_MAILBOX_IN_CMD,
>+                       TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY |
>+                       TCSS_DISP_MAILBOX_IN_CMD_DATA(0x1));
>+
>+        /* wait to clear mailbox running busy bit before continuing */
>+        if (intel_de_wait_for_clear(i915, TCSS_DISP_MAILBOX_IN_CMD,
>+                                    TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY, 10)) {
>+                drm_dbg_kms(&i915->drm,
>+                            "Power request assert WA timeout waiting for TCSS mailbox run/busy bit to clear\n");

Hm. I think I missed to get my point accross with my earlier comment,
sorry.

What I meant was that I think it would be good if the first and second
debug messages were different. That way it is easy to spot the point of
failure (waiting to use the mailbox vs waiting for our message to be
handled) in case any happens.

>+                return false;
>+        }
>+
>+        return true;
>+}
>+
> static void __xelpdp_tc_phy_enable_tcss_power(struct intel_tc_port *tc, bool enable)
> {
>         struct drm_i915_private *i915 = tc_to_i915(tc);
>@@ -1022,6 +1061,13 @@ static void __xelpdp_tc_phy_enable_tcss_power(struct intel_tc_port *tc, bool ena
> 
>         assert_tc_cold_blocked(tc);
> 
>+        /*
>+         * Gfx driver workaround for PTL tcss_rxdetect_clkswb_req/ack handshake
>+         * violation when pwwreq= 0->1 during TC7/10 entry
>+         */
>+        drm_WARN_ON(&i915->drm,
>+                    !wa_tcss_power_request_assert(i915, enable));

As mentioned in another message, maybe we could have this warning done
inside the function to make things self-containined and not rely on the
caller to do it.

--
Gustavo Sousa

>+
>         val = intel_de_read(i915, reg);
>         if (enable)
>                 val |= XELPDP_TCSS_POWER_REQUEST;
>-- 
>2.43.0
>