[PATCH] drm/i915/xe3lpd: Power request asserting/deasserting
Kahola, Mika
mika.kahola at intel.com
Tue Oct 29 11:58:08 UTC 2024
> -----Original Message-----
> From: Sousa, Gustavo <gustavo.sousa at intel.com>
> Sent: Monday, 28 October 2024 17.51
> To: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Kahola, Mika <mika.kahola at intel.com>
> Subject: Re: [PATCH] drm/i915/xe3lpd: Power request asserting/deasserting
>
> 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).
Ok. I will move these lines upwards in the file.
>
> >+#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.
Ok
>
> >+#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.
I will rename this function as wa_14020908590(). After all it is a workaround.
>
> >+{
> >+ /*
> >+ * 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.
Ok got it. Even though it is the same bit that we are waiting for to be cleared it's first checked that the mailbox is available and the second time after we have written a command to the mailbox and wait for it to become available again. I will reword the debug message.
>
> >+ 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.
I will move this inside the wa function as well.
Thanks for the comments!
-Mika-
>
> --
> Gustavo Sousa
>
> >+
> > val = intel_de_read(i915, reg);
> > if (enable)
> > val |= XELPDP_TCSS_POWER_REQUEST;
> >--
> >2.43.0
> >
More information about the Intel-gfx
mailing list