Re: [PATCH] drm/i915/cx0: Add step for programming msgbus timer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Kahola, Mika (2023-09-12 09:26:59-03:00)
>> -----Original Message-----
>> From: Sousa, Gustavo <gustavo.sousa@xxxxxxxxx>
>> Sent: Monday, September 11, 2023 7:16 PM
>> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Kahola, Mika <mika.kahola@xxxxxxxxx>; Taylor, Clinton A <clinton.a.taylor@xxxxxxxxx>
>> Subject: [PATCH] drm/i915/cx0: Add step for programming msgbus timer
>> 
>> There was a recent update in the BSpec adding an extra step to the PLL enable sequence, which is for programming the msgbus
>> timer. Since we also touch PHY registers during hw readout, let's do the programming when starting a transaction rather than only
>> when doing the PLL enable sequence.
>> 
>> The BSpec isn't clear about whether the programming should be done for owned PHY lanes or only PHY lane 0. Let's program the
>> timer for owned PHY lanes for now. We can revisit this once we get clarification from the BSpec.
>> 
>> This might be the missing step that was causing the timeouts that we have recently seen during C20 SRAM register programming
>> sequences. With this in place, we shouldn't need the logic to bump the timer thresholds, since now we have a documented value
>> that should be set peform programming the registers. As such, let's also remove intel_cx0_bus_check_and_bump_timer(), but
>> keep the part that checks if hardware really detected a timeout, which might be useful debugging information.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/display/intel_cx0_phy.c  | 88 +++++++++----------  .../gpu/drm/i915/display/intel_cx0_phy_regs.h |  2 +-
>>  2 files changed, 42 insertions(+), 48 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> index e6d3027c821d..1b0a868845b7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> @@ -29,8 +29,6 @@
>>  #define INTEL_CX0_LANE1                BIT(1)
>>  #define INTEL_CX0_BOTH_LANES        (INTEL_CX0_LANE1 | INTEL_CX0_LANE0)
>> 
>> -#define INTEL_CX0_MSGBUS_TIMER_BUMPED_VAL        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) @@ -73,19 +71,39 @@ assert_dc_off(struct
>> drm_i915_private *i915)
>>          drm_WARN_ON(&i915->drm, !enabled);
>>  }
>> 
>> +static void intel_cx0_program_msgbus_timer(struct intel_encoder
>> +*encoder) {
>> +        int lane;
>> +        struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> +        u8 owned_lane_mask = intel_cx0_get_owned_lane_mask(i915, encoder);
>> +
>> +        for_each_cx0_lane_in_mask(owned_lane_mask, lane)
>> +                intel_de_rmw(i915,
>> +                             XELPDP_PORT_MSGBUS_TIMER(encoder->port, lane),
>> +                             XELPDP_PORT_MSGBUS_TIMER_VAL_MASK,
>> +                             XELPDP_PORT_MSGBUS_TIMER_VAL);
>> +}
>> +
>>  /*
>>   * Prepare HW for CX0 phy transactions.
>>   *
>>   * It is required that PSR and DC5/6 are disabled before any CX0 message
>>   * bus transaction is executed.
>> + *
>> + * We also do the msgbus timer programming here to ensure that the
>> + timer
>> + * is already programmed before any access to the msgbus.
>>   */
>>  static intel_wakeref_t intel_cx0_phy_transaction_begin(struct intel_encoder *encoder)  {
>> +        intel_wakeref_t wakeref;
>>          struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>>          struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>> 
>>          intel_psr_pause(intel_dp);
>> -        return intel_display_power_get(i915, POWER_DOMAIN_DC_OFF);
>> +        wakeref = intel_display_power_get(i915, POWER_DOMAIN_DC_OFF);
>> +        intel_cx0_program_msgbus_timer(encoder);
>> +
>> +        return wakeref;
>>  }
>> 
>>  static void intel_cx0_phy_transaction_end(struct intel_encoder *encoder, intel_wakeref_t wakeref) @@ -121,42 +139,6 @@
>> 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_BUMPED_VAL)
>> -                return;
>> -
>> -        val &= ~XELPDP_PORT_MSGBUS_TIMER_VAL_MASK;
>> -        val |= XELPDP_PORT_MSGBUS_TIMER_VAL(INTEL_CX0_MSGBUS_TIMER_BUMPED_VAL);
>> -
>> -        drm_dbg_kms(&i915->drm,
>> -                    "PHY %c lane %d: increasing msgbus timer threshold to %#x\n",
>> -                    phy_name(phy), lane, INTEL_CX0_MSGBUS_TIMER_BUMPED_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)
>>  {
>> @@ -170,7 +152,13 @@ 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);
>> +
>> +                if (!(intel_de_read(i915, XELPDP_PORT_MSGBUS_TIMER(port, lane)) &
>> +                      XELPDP_PORT_MSGBUS_TIMER_TIMED_OUT))
>> +                        drm_warn(&i915->drm,
>> +                                 "PHY %c Hardware did not detect a timeout\n",
>> +                                 phy_name(phy));
>> +
>
>drm_warn() seems a bit exaggerated here as we are informing only about not detecting timeout.

Okay. I'll send a new version changing it to use drm_dbg_kms(). Thanks!

>Dbg message here might be sufficient. This is something that we could leave out as well. We already
>have information about the timeouts if these happen.

I think querying the hardware might give us helpful debugging info in future
issues. A scenario that comes to mind is that we might *think* a timeout
happened, but the hardware might disagree, which could be indicative that the
hardware is somehow misconfigured or that we somehow are not waiting enough.

--
Gustavo Sousa

>
>-Mika-
>
>>                  intel_cx0_bus_reset(i915, port, lane);
>>                  return -ETIMEDOUT;
>>          }
>> @@ -2737,39 +2725,45 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
>>          intel_cx0_powerdown_change_sequence(i915, encoder->port, INTEL_CX0_BOTH_LANES,
>>                                              CX0_P2_STATE_READY);
>> 
>> -        /* 4. Program PHY internal PLL internal registers. */
>> +        /*
>> +         * 4. Program PORT_MSGBUS_TIMER register's Message Bus Timer field to 0xA000.
>> +         *    (This is done inside intel_cx0_phy_transaction_begin(), since we would need
>> +         *    the right timer thresholds for readouts too.)
>> +         */
>> +
>> +        /* 5. Program PHY internal PLL internal registers. */
>>          if (intel_is_c10phy(i915, phy))
>>                  intel_c10_pll_program(i915, crtc_state, encoder);
>>          else
>>                  intel_c20_pll_program(i915, crtc_state, encoder);
>> 
>>          /*
>> -         * 5. Program the enabled and disabled owned PHY lane
>> +         * 6. Program the enabled and disabled owned PHY lane
>>           * transmitters over message bus
>>           */
>>          intel_cx0_program_phy_lane(i915, encoder, crtc_state->lane_count, lane_reversal);
>> 
>>          /*
>> -         * 6. Follow the Display Voltage Frequency Switching - Sequence
>> +         * 7. Follow the Display Voltage Frequency Switching - Sequence
>>           * Before Frequency Change. We handle this step in bxt_set_cdclk().
>>           */
>> 
>>          /*
>> -         * 7. Program DDI_CLK_VALFREQ to match intended DDI
>> +         * 8. Program DDI_CLK_VALFREQ to match intended DDI
>>           * clock frequency.
>>           */
>>          intel_de_write(i915, DDI_CLK_VALFREQ(encoder->port),
>>                         crtc_state->port_clock);
>> 
>>          /*
>> -         * 8. Set PORT_CLOCK_CTL register PCLK PLL Request
>> +         * 9. Set PORT_CLOCK_CTL register PCLK PLL Request
>>           * LN<Lane for maxPCLK> to "1" to enable PLL.
>>           */
>>          intel_de_rmw(i915, XELPDP_PORT_CLOCK_CTL(encoder->port),
>>                       intel_cx0_get_pclk_pll_request(INTEL_CX0_BOTH_LANES),
>>                       intel_cx0_get_pclk_pll_request(maxpclk_lane));
>> 
>> -        /* 9. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> == "1". */
>> +        /* 10. Poll on PORT_CLOCK_CTL PCLK PLL Ack LN<Lane for maxPCLK> ==
>> +"1". */
>>          if (__intel_de_wait_for_register(i915, XELPDP_PORT_CLOCK_CTL(encoder->port),
>>                                           intel_cx0_get_pclk_pll_ack(INTEL_CX0_BOTH_LANES),
>>                                           intel_cx0_get_pclk_pll_ack(maxpclk_lane),
>> @@ -2778,7 +2772,7 @@ static void intel_cx0pll_enable(struct intel_encoder *encoder,
>>                           phy_name(phy), XELPDP_PCLK_PLL_ENABLE_TIMEOUT_US);
>> 
>>          /*
>> -         * 10. Follow the Display Voltage Frequency Switching Sequence After
>> +         * 11. Follow the Display Voltage Frequency Switching Sequence After
>>           * Frequency Change. We handle this step in bxt_set_cdclk().
>>           */
>> 
>> 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 b2db4cc366d6..adf8f4ce0d49 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy_regs.h
>> @@ -121,7 +121,7 @@
>> 
>> _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_MSGBUS_TIMER_VAL(val)                REG_FIELD_PREP(XELPDP_PORT_MSGBUS_TIMER_VAL_MASK,
>> val)
>> +#define   XELPDP_PORT_MSGBUS_TIMER_VAL                        REG_FIELD_PREP(XELPDP_PORT_MSGBUS_TIMER_VAL_MASK,
>> 0xa000)
>> 
>>  #define _XELPDP_PORT_CLOCK_CTL_A                        0x640E0
>>  #define _XELPDP_PORT_CLOCK_CTL_B                        0x641E0
>> --
>> 2.42.0
>




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux