[Intel-gfx] [PATCH] drm/i915/cx0: Check and increase msgbus timeout threshold

[Sripada, Radhakrishna]

Hi Gustavo,

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Gustavo
> Sousa
> Sent: Monday, August 28, 2023 10:34 AM
> To: intel-gfx at lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH] drm/i915/cx0: Check and increase msgbus timeout
> threshold
> 
> We have experienced timeout issues when accessing C20 SRAM registers.
This wording is misleading. It seems to imply that we directly access SRAM
registers via msg bus but the reads/writes go through intermediate registers
and it is this read/write that is failing. Adding extra clarity here would be helpful.
 
> Experimentation showed that bumping the message bus timer threshold
> helped on getting display Type-C connection on the C20 PHY to work.
> 
> While the timeout is still under investigation with the HW team, having
> logic to allow forward progress (with the proper warnings) seems useful.
> Thus, let's bump the threshold when a timeout is detected.
> 
> The maximum value of 0x200 pclk cycles was somewhat arbitrary - 2x the
> default value. That value was successfully tested on real hardware that
> was displaying timeouts otherwise. 
> 
> BSpec: 65156
> Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 41 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_cx0_phy_regs.h | 12 ++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index dd489b50ad60..55d2333032e3 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -5,6 +5,7 @@
> 
>  #include <linux/log2.h>
>  #include <linux/math64.h>
> +#include <linux/minmax.h>
>  #include "i915_reg.h"
>  #include "intel_cx0_phy.h"
>  #include "intel_cx0_phy_regs.h"
> @@ -29,6 +30,8 @@
>  #define INTEL_CX0_LANE1		BIT(1)
>  #define INTEL_CX0_BOTH_LANES	(INTEL_CX0_LANE1 |
> INTEL_CX0_LANE0)
> 
> +#define INTEL_CX0_MSGBUS_TIMER_VAL_MAX	0x200
> +
>  bool intel_is_c10phy(struct drm_i915_private *i915, enum phy phy)
>  {
>  	if (DISPLAY_VER_FULL(i915) == IP_VER(14, 0) && phy < PHY_C)
> @@ -119,6 +122,43 @@ static void intel_cx0_bus_reset(struct drm_i915_private
> *i915, enum port port, i
>  	intel_clear_response_ready_flag(i915, port, lane);
>  }
> 
> +/*
> + * Check if there was a timeout detected by the hardware in the message bus
> + * and bump the threshold if so.
> + */
> +static void intel_cx0_bus_check_and_bump_timer(struct drm_i915_private
> *i915,
> +					       enum port port, int lane)
> +{
> +	enum phy phy = intel_port_to_phy(i915, port);
> +	i915_reg_t reg;
> +	u32 val;
> +	u32 timer_val;
> +
> +	reg = XELPDP_PORT_MSGBUS_TIMER(port, lane);
> +	val = intel_de_read(i915, reg);
> +
> +	if (!(val & XELPDP_PORT_MSGBUS_TIMER_TIMED_OUT)) {
> +		drm_warn(&i915->drm,
> +			 "PHY %c lane %d: hardware did not detect a
> timeout\n",
> +			 phy_name(phy), lane);
> +		return;
> +	}
> +
> +	timer_val =
> REG_FIELD_GET(XELPDP_PORT_MSGBUS_TIMER_VAL_MASK, val);
> +
> +	if (timer_val == INTEL_CX0_MSGBUS_TIMER_VAL_MAX)
> +		return;
> +
> +	timer_val = min(2 * timer_val,
> (u32)INTEL_CX0_MSGBUS_TIMER_VAL_MAX);
     ^ is this cast necessary?

> +	val &= ~XELPDP_PORT_MSGBUS_TIMER_VAL_MASK;
> +	val |= REG_FIELD_PREP(XELPDP_PORT_MSGBUS_TIMER_VAL_MASK,
> timer_val);
We do not use REG_FIELD_PREP in the function. Instead we use it in
register definition in intel_cx0_phy_regs.h.

Thanks,
Radhakrishna Sripada

> +
> +	drm_dbg_kms(&i915->drm,
> +		    "PHY %c lane %d: increasing msgbus timer threshold to
> %#x\n",
> +		    phy_name(phy), lane, timer_val);
> +	intel_de_write(i915, reg, val);
> +}
> +
>  static int intel_cx0_wait_for_ack(struct drm_i915_private *i915, enum port port,
>  				  int command, int lane, u32 *val)
>  {
> @@ -132,6 +172,7 @@ static int intel_cx0_wait_for_ack(struct
> drm_i915_private *i915, enum port port,
>  					 XELPDP_MSGBUS_TIMEOUT_SLOW,
> val)) {
>  		drm_dbg_kms(&i915->drm, "PHY %c Timeout waiting for
> message ACK. Status: 0x%x\n",
>  			    phy_name(phy), *val);
> +		intel_cx0_bus_check_and_bump_timer(i915, port, lane);
>  		intel_cx0_bus_reset(i915, port, lane);
>  		return -ETIMEDOUT;
>  	}
> 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 cb5d1be2ba19..17cc3385aabb 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
> @@ -110,6 +110,18 @@
>  #define   CX0_P4PG_STATE_DISABLE			0xC
>  #define   CX0_P2_STATE_RESET				0x2
> 
> +#define _XELPDP_PORT_MSGBUS_TIMER_LN0_A
> 	0x640d8
> +#define _XELPDP_PORT_MSGBUS_TIMER_LN0_B
> 	0x641d8
> +#define _XELPDP_PORT_MSGBUS_TIMER_LN0_USBC1		0x16f258
> +#define _XELPDP_PORT_MSGBUS_TIMER_LN0_USBC2		0x16f458
> +#define XELPDP_PORT_MSGBUS_TIMER(port, lane)
> 	_MMIO(_PICK_EVEN_2RANGES(port, PORT_TC1, \
> +
> 	 _XELPDP_PORT_MSGBUS_TIMER_LN0_A, \
> +
> 	 _XELPDP_PORT_MSGBUS_TIMER_LN0_B, \
> +
> 	 _XELPDP_PORT_MSGBUS_TIMER_LN0_USBC1, \
> +
> 	 _XELPDP_PORT_MSGBUS_TIMER_LN0_USBC2) + (lane) * 4)
> +#define   XELPDP_PORT_MSGBUS_TIMER_TIMED_OUT		REG_BIT(31)
> +#define   XELPDP_PORT_MSGBUS_TIMER_VAL_MASK
> 	REG_GENMASK(23, 0)
> +
>  #define _XELPDP_PORT_CLOCK_CTL_A			0x640E0
>  #define _XELPDP_PORT_CLOCK_CTL_B			0x641E0
>  #define _XELPDP_PORT_CLOCK_CTL_USBC1			0x16F260
> --
> 2.41.0