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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 28 Aug 2023, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>>> +#define INTEL_CX0_MSGBUS_TIMER_VAL_MAX        0x200

Either make this 0x200U (for unsigned)...

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

...or use min_t() instead of adding the cast here.

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Graphics Center