| Message ID | 20241028125835.78639-1-mika.kahola@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/xe3lpd: Power request asserting/deasserting | expand |
On Mon, Oct 28, 2024 at 02:58:35PM +0200, Mika Kahola wrote: ... > +static bool wa_tcss_power_request_assert(struct drm_i915_private *i915, > + bool enable) > +{ > + /* > + * Limit to PTL only > + * TODO: Add check for PICA IP and use that instead limiting WA for > + * platform > + */ > + if (DISPLAY_VER(i915) != 30) Not sure if hardcoding constants is the best of the approach, but I found similar patterns in other places, so upto you. > + 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"); Is timeout considered a failure? Not sure _dbg is the right helper if it. > + 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); Since enable it already a bool, this can be intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, enable); > + > + 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 */ Any specific reason to do this on exit? I'm assuming anyone trying to use the mailbox further down would be doing this anyway since it's a prerequisite, right? > + 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"); Ditto. > + return false; > + } > + > + return true; > +} Raag
Quoting Raag Jadav (2024-10-28 11:02:02-03:00) >On Mon, Oct 28, 2024 at 02:58:35PM +0200, Mika Kahola wrote: > >... > >> +static bool wa_tcss_power_request_assert(struct drm_i915_private *i915, >> + bool enable) >> +{ >> + /* >> + * Limit to PTL only >> + * TODO: Add check for PICA IP and use that instead limiting WA for >> + * platform >> + */ >> + if (DISPLAY_VER(i915) != 30) > >Not sure if hardcoding constants is the best of the approach, but I found >similar patterns in other places, so upto you. Using version number directly makes it easier to concurrently merge independent patches for some display IP without having to wait some #define to become available. That comes with the cost of having to remember the version numbers (or checking somewhere) though. > >> + 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"); > >Is timeout considered a failure? Not sure _dbg is the right helper if it. I believe it is okay to have _dbg here, because the returned value is checked and a warning is raised. Althought we could make this more self-contained by jumping an error path inside this function. > >> + 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); > >Since enable it already a bool, this can be > > intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, enable); > >> + >> + 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 */ > >Any specific reason to do this on exit? >I'm assuming anyone trying to use the mailbox further down would be doing >this anyway since it's a prerequisite, right? This wait is part of the "DE to IOM Mailbox" flow. I believe this is necessary and the workaround might even not be effective without it. -- Gustavo Sousa > >> + 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"); > >Ditto. > >> + return false; >> + } >> + >> + return true; >> +} > >Raag
On Mon, Oct 28, 2024 at 11:21:59AM -0300, Gustavo Sousa wrote: > Quoting Raag Jadav (2024-10-28 11:02:02-03:00) > >On Mon, Oct 28, 2024 at 02:58:35PM +0200, Mika Kahola wrote: > > > >... > > > >> +static bool wa_tcss_power_request_assert(struct drm_i915_private *i915, > >> + bool enable) > >> +{ > >> + /* > >> + * Limit to PTL only > >> + * TODO: Add check for PICA IP and use that instead limiting WA for > >> + * platform > >> + */ > >> + if (DISPLAY_VER(i915) != 30) > > > >Not sure if hardcoding constants is the best of the approach, but I found > >similar patterns in other places, so upto you. > > Using version number directly makes it easier to concurrently merge > independent patches for some display IP without having to wait some > #define to become available. That comes with the cost of having > to remember the version numbers (or checking somewhere) though. Agree. Although usually we have codenames for this, which is easier to track or grep. > > > >> + 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"); > > > >Is timeout considered a failure? Not sure _dbg is the right helper if it. > > I believe it is okay to have _dbg here, because the returned value is > checked and a warning is raised. Which makes the logging redundant... > Althought we could make this more > self-contained by jumping an error path inside this function. ... but if we'd still want it I think this will be best. > > > > >> + 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); > > > >Since enable it already a bool, this can be > > > > intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, enable); > > > >> + > >> + 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 */ > > > >Any specific reason to do this on exit? > >I'm assuming anyone trying to use the mailbox further down would be doing > >this anyway since it's a prerequisite, right? > > This wait is part of the "DE to IOM Mailbox" flow. I believe this is > necessary and the workaround might even not be effective without it. My understanding is that the wait is for the availability of mailbox which is not equivalent to command success, but I could be wrong. Raag
On Mon, 28 Oct 2024, Mika Kahola <mika.kahola@intel.com> wrote: > 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@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) > +#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) > + > #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, Please use struct intel_display *display for new code. > + bool enable) > +{ > + /* > + * Limit to PTL only > + * TODO: Add check for PICA IP and use that instead limiting WA for > + * platform > + */ > + if (DISPLAY_VER(i915) != 30) s/i915/display/ > + 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)) { s/i915/display/ > + drm_dbg_kms(&i915->drm, display->drm Ditto below, this function doesn't need struct drm_i915_private for anything. > + "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"); > + 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)); > + > val = intel_de_read(i915, reg); > if (enable) > val |= XELPDP_TCSS_POWER_REQUEST;
> -----Original Message----- > From: Jadav, Raag <raag.jadav@intel.com> > Sent: Monday, 28 October 2024 16.43 > To: Sousa, Gustavo <gustavo.sousa@intel.com> > Cc: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915/xe3lpd: Power request asserting/deasserting > > On Mon, Oct 28, 2024 at 11:21:59AM -0300, Gustavo Sousa wrote: > > Quoting Raag Jadav (2024-10-28 11:02:02-03:00) > > >On Mon, Oct 28, 2024 at 02:58:35PM +0200, Mika Kahola wrote: > > > > > >... > > > > > >> +static bool wa_tcss_power_request_assert(struct drm_i915_private *i915, > > >> + bool enable) { > > >> + /* > > >> + * Limit to PTL only > > >> + * TODO: Add check for PICA IP and use that instead limiting WA for > > >> + * platform > > >> + */ > > >> + if (DISPLAY_VER(i915) != 30) > > > > > >Not sure if hardcoding constants is the best of the approach, but I > > >found similar patterns in other places, so upto you. > > > > Using version number directly makes it easier to concurrently merge > > independent patches for some display IP without having to wait some > > #define to become available. That comes with the cost of having to > > remember the version numbers (or checking somewhere) though. > > Agree. Although usually we have codenames for this, which is easier to track or > grep. > > > > > > >> + 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"); > > > > > >Is timeout considered a failure? Not sure _dbg is the right helper if it. > > > > I believe it is okay to have _dbg here, because the returned value is > > checked and a warning is raised. > > Which makes the logging redundant... > > > Althought we could make this more > > self-contained by jumping an error path inside this function. > > ... but if we'd still want it I think this will be best. > > > > > > > > >> + 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); > > > > > >Since enable it already a bool, this can be > > > > > > intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, enable); > > > > > >> + > > >> + 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 */ > > > > > >Any specific reason to do this on exit? > > >I'm assuming anyone trying to use the mailbox further down would be > > >doing this anyway since it's a prerequisite, right? > > > > This wait is part of the "DE to IOM Mailbox" flow. I believe this is > > necessary and the workaround might even not be effective without it. > > My understanding is that the wait is for the availability of mailbox which is not > equivalent to command success, but I could be wrong. I think these two are related. We could consider command to be successful if mailbox is free to use within a certain timeframe. Otherwise, I would consider that there might be something wrong with the communication or command hasn't been successfully executed. -Mika- > > Raag
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Monday, 28 October 2024 16.55 > To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Sousa, Gustavo <gustavo.sousa@intel.com>; Kahola, Mika > <mika.kahola@intel.com> > Subject: Re: [PATCH] drm/i915/xe3lpd: Power request asserting/deasserting > > On Mon, 28 Oct 2024, Mika Kahola <mika.kahola@intel.com> wrote: > > 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@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) > > +#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) > > + > > #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, > > Please use struct intel_display *display for new code. Ok I will switch to use that. Thanks! > > > + bool enable) > > +{ > > + /* > > + * Limit to PTL only > > + * TODO: Add check for PICA IP and use that instead limiting WA for > > + * platform > > + */ > > + if (DISPLAY_VER(i915) != 30) > > s/i915/display/ > > > + 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)) { > > s/i915/display/ > > > + drm_dbg_kms(&i915->drm, > > display->drm > > Ditto below, this function doesn't need struct drm_i915_private for anything. > > > + "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"); > > + 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)); > > + > > val = intel_de_read(i915, reg); > > if (enable) > > val |= XELPDP_TCSS_POWER_REQUEST; > > -- > Jani Nikula, Intel
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@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 >
> -----Original Message----- > From: Sousa, Gustavo <gustavo.sousa@intel.com> > Sent: Monday, 28 October 2024 17.51 > To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Kahola, Mika <mika.kahola@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@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 > >
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) +#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) + #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) +{ + /* + * 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"); + 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)); + val = intel_de_read(i915, reg); if (enable) val |= XELPDP_TCSS_POWER_REQUEST;
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@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(+)