Quoting Sripada, Radhakrishna (2023-08-28 17:30:19-03:00)
>Hi Gustavo,
Hi, RK.
Thanks for the feedback! Please, see my replies below.
>
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Gustavo
>> Sousa
>> Sent: Monday, August 28, 2023 10:34 AM
>> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Subject: [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.
Hm... Okay. I thought that would already be implied to someone familiar with the
code. What about:
s/when accessing/when going through the sequence to access/
?
>
>> 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@xxxxxxxxx>
>> ---
>> 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?
Yes. I remember getting a warning without it. Let me remove it to remember...
...done! I think it complains because the arguments are of different types:
In file included from drivers/gpu/drm/i915/display/intel_cx0_phy.c:8:
drivers/gpu/drm/i915/display/intel_cx0_phy.c: In function ‘intel_cx0_bus_check_and_bump_timer’:
./include/linux/minmax.h:20:35: error: comparison of distinct pointer types lacks a cast [-Werror]
20 | (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
| ^~
./include/linux/minmax.h:26:18: note: in expansion of macro ‘__typecheck’
26 | (__typecheck(x, y) && __no_side_effects(x, y))
| ^~~~~~~~~~~
./include/linux/minmax.h:36:31: note: in expansion of macro ‘__safe_cmp’
36 | __builtin_choose_expr(__safe_cmp(x, y), \
| ^~~~~~~~~~
./include/linux/minmax.h:67:25: note: in expansion of macro ‘__careful_cmp’
67 | #define min(x, y) __careful_cmp(x, y, <)
| ^~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_cx0_phy.c:152:21: note: in expansion of macro ‘min’
152 | timer_val = min(2 * timer_val, INTEL_CX0_MSGBUS_TIMER_VAL_MAX);
|
>
>> + 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.
I think it makes sense have definitions using REG_FIELD_PREP() in header files
and use those definitions in .c files for fields whose values are are
enumerated.
Now, for fields without enumeration, like this one in discussion, I think it is
fine to use REG_FIELD_PREP() directly. The MASK is already exposed anyway...
Besides, it seems we already have lines of code in *.c files using the pattern
above:
git grep REG_FIELD_PREP drm-tip/drm-tip -- ':/drivers/gpu/drm/i915/**/*.c'
Let me know what you think. I could add a XELPDP_PORT_MSGBUS_TIMER_VAL() macro
if you think it is necessary. My personal take is that using REG_FIELD_PREP()
for this case is fine.
--
Gustavo Sousa
>
>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
>
