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

Commit Message

Matt Atwood Oct. 18, 2024, 8:49 p.m. UTC

From: Mika Kahola <[e-mail]>

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 <[e-mail]>
Signed-off-by: Matt Atwood <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_tc.c | 40 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h         |  7 +++++
 2 files changed, 47 insertions(+)

Comments

Jani Nikula Oct. 21, 2024, 12:08 p.m. UTC | #1

On Fri, 18 Oct 2024, Matt Atwood <[e-mail]> wrote:
> From: Mika Kahola <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Matt Atwood <[e-mail]>
> ---
>  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,

No new struct drm_i915_private params or variables please.

BR,
Jani.

> +					      bool enable)
> +{
> +	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,
> +			    "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));
> +
> +	/* 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");
> +		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
> @@ -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))
> +#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)

Gustavo Sousa Oct. 21, 2024, 12:30 p.m. UTC | #2

Quoting Matt Atwood (2024-10-18 17:49:41-03:00)
>From: Mika Kahola <[e-mail]>
>
>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.

Is there a WA lineage number we can refer to for this?

--
Gustavo Sousa

>
>Signed-off-by: Mika Kahola <[e-mail]>
>Signed-off-by: Matt Atwood <[e-mail]>
>---
> 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,
>+                                              bool enable)
>+{
>+        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,
>+                            "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));
>+
>+        /* 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");
>+                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
>@@ -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))
>+#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
>

Mika Kahola Oct. 21, 2024, 1:03 p.m. UTC | #3

> -----Original Message-----
> From: Sousa, Gustavo <[e-mail]>
> Sent: Monday, 21 October 2024 15.31
> To: Atwood, Matthew S <[e-mail]>; intel-
> [e-mail]; [e-mail]
> Cc: Kahola, Mika <[e-mail]>; Atwood, Matthew S
> <[e-mail]>
> Subject: Re: [PATCH 12/12] drm/i915/xe3lpd: Power request
> asserting/deasserting
> 
> Quoting Matt Atwood (2024-10-18 17:49:41-03:00)
> >From: Mika Kahola <[e-mail]>
> >
> >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.
> 
> Is there a WA lineage number we can refer to for this?

Unfortunately, there isn't any official WA number for this. This is somewhat unofficially proposed fix.

-Mika-

> 
> --
> Gustavo Sousa
> 
> >
> >Signed-off-by: Mika Kahola <[e-mail]>
> >Signed-off-by: Matt Atwood <[e-mail]>
> >---
> > 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,
> >+                                              bool enable) {
> >+        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,
> >+                            "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));
> >+
> >+        /* 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");
> >+                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
> >@@ -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))
> >+#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
> >

Gustavo Sousa Oct. 23, 2024, 8:39 p.m. UTC | #4

Quoting Matt Atwood (2024-10-18 17:49:41-03:00)
>From: Mika Kahola <[e-mail]>
>
>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 <[e-mail]>
>Signed-off-by: Matt Atwood <[e-mail]>
>---
> 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
>

Mika Kahola Oct. 28, 2024, 9:11 a.m. UTC | #5

> -----Original Message-----
> From: Sousa, Gustavo <[e-mail]>
> Sent: Wednesday, 23 October 2024 23.40
> To: Atwood, Matthew S <[e-mail]>; intel-
> [e-mail]; [e-mail]
> Cc: Kahola, Mika <[e-mail]>; Atwood, Matthew S
> <[e-mail]>; Taylor, Clinton A <[e-mail]>
> Subject: Re: [PATCH 12/12] drm/i915/xe3lpd: Power request
> asserting/deasserting
> 
> Quoting Matt Atwood (2024-10-18 17:49:41-03:00)
> >From: Mika Kahola <[e-mail]>
> >
> >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 <[e-mail]>
> >Signed-off-by: Matt Atwood <[e-mail]>
> >---
> > 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().

I couldn't find any workarounds with this 14020908590 number. Maybe we could rename the function wa_tcss_power_assert()?

> 
> >+                                              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).

I'm assuming this is more related to PICA IP than platform but I cannot confirm that yet. In the meantime, I could add a comment and check display version only for the PTL 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.

I'll rephrase the wording here.

> 
> >+                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.

Ok, I will move these defs to intel_cx0_phy_regs.h

> 
> >@@ -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.
Ok

I will make few adjustments to the patch. Thanks for the comments and a review!

-Mika-

> 
> --
> 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
> >

Patch

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,
+					      bool enable)
+{
+	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,
+			    "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));
+
+	/* 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");
+		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
@@ -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))
+#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)