Quoting Matt Atwood (2024-10-18 17:49:41-03:00)
>From: Mika Kahola <mika.kahola@xxxxxxxxx>
>
>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.
>
>Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx>
>Signed-off-by: Matt Atwood <matthew.s.atwood@xxxxxxxxx>
>---
> drivers/gpu/drm/i915/display/intel_tc.c | 40 +++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h | 7 +++++
> 2 files changed, 47 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
>index 6f2ee7dbc43b..7d9f87db381c 100644
>--- a/drivers/gpu/drm/i915/display/intel_tc.c
>+++ b/drivers/gpu/drm/i915/display/intel_tc.c
>@@ -1013,6 +1013,39 @@ xelpdp_tc_phy_wait_for_tcss_power(struct intel_tc_port *tc, bool enabled)
> return true;
> }
>
>+static bool xelpdp_tc_phy_wait_for_tcss_ready(struct drm_i915_private *i915,
I think xelpdp_ is not right here as this does not apply to Xe_LPD+. I
think we could simply use the workaround lineage number for the name of
this function. Something like wa_14020908590().
>+ bool enable)
>+{
>+ if (DISPLAY_VER(i915) < 30)
The description of the internal ticket that resulted in this workaround
makes me wonder if this is actually an issue associated to the SoC
instead of display or PICA IP. However the ticket metadata indicates the
PICA IP as the one affected. It would be good to confirm the correct
association here.
In any case, this seems not really related to the display IP, so
checking DISPLAY_VER(i915) seems not very precise here.
If it turns out that this is a SoC-related issue, it would be better to
check if the platform is PTL.
Now, if this is indeed an issue associated to the PICA IP, then I see
the following alternatives:
- add an earlier patch to detect the PICA IP and add that info to
intel_display_runtime_info. Then, here we use that info in the
condition for this workaround;
- at least add a comment here that we are checking the display version
because we do not have PICA IP detection in the driver yet. In this
case.
I tend to think that checking version equality would make more sense
(assuming the issue would not be seen in a future platform).
>+ 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,
>+ "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_DATA(1));
Nitpick: I would prefer a more explicit version of this. Something like:
intel_de_write(i915, TCSS_DISP_MAILBOX_IN_CMD,
TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY |
TCSS_DISP_MAILBOX_IN_CMD_CMD(0x1));
With the current version, I had to go and check that
TCSS_DISP_MAILBOX_IN_CMD_DATA() also includes the run/busy bit.
>+
>+ /* 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,
>+ "timeout waiting for TCSS mailbox run/busy bit to clear\n");
I think would be good to have different timeout messages so that it is
easy to differentiate whether we timed out while waiting for our turn to
use the mailbox or while waiting for our command to be handled.
>+ 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 +1055,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,
>+ !xelpdp_tc_phy_wait_for_tcss_ready(i915, enable));
>+
> val = intel_de_read(i915, reg);
> if (enable)
> val |= XELPDP_TCSS_POWER_REQUEST;
>diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>index 2743a2dd0a3d..d2775a32bf18 100644
>--- a/drivers/gpu/drm/i915/i915_reg.h
>+++ b/drivers/gpu/drm/i915/i915_reg.h
Maybe intel_cx0_phy_regs.h would be a better home for the mailbox
registers, since it is where XELPDP_PORT_BUF_CTL1 and
XELPDP_TCSS_POWER_{REQUEST,STATE} are defined?
Not the perfect place, but at least we would not add new definitions to
i915_reg.h and add to the work of separating display code from i915.
>@@ -4539,6 +4539,13 @@ enum skl_power_gate {
> #define TCSS_DDI_STATUS_HPD_LIVE_STATUS_TBT REG_BIT(1)
> #define TCSS_DDI_STATUS_HPD_LIVE_STATUS_ALT REG_BIT(0)
>
>+#define TCSS_DISP_MAILBOX_IN_CMD _MMIO(0x161300)
>+#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 a blank line here.
--
Gustavo Sousa
>+#define TCSS_DISP_MAILBOX_IN_DATA _MMIO(0x161304)
>+
> #define PRIMARY_SPI_TRIGGER _MMIO(0x102040)
> #define PRIMARY_SPI_ADDRESS _MMIO(0x102080)
> #define PRIMARY_SPI_REGIONID _MMIO(0x102084)
>--
>2.45.0
>
