Re: [PATCH 6/8] drm/i915/xe2lpd: Support MDCLK:CDCLK ratio changes

From: Gustavo Sousa <gustavo.sousa@intel.com>
To: "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>
Cc: <intel-gfx@lists.freedesktop.org>,
	<intel-xe@lists.freedesktop.org>,
	"Lucas De Marchi" <lucas.demarchi@intel.com>,
	<vinod.govindapillai@intel.com>,
	"Matt Roper" <matthew.d.roper@intel.com>
Subject: Re: [PATCH 6/8] drm/i915/xe2lpd: Support MDCLK:CDCLK ratio changes
Date: Mon, 11 Mar 2024 18:13:29 -0300	[thread overview]
Message-ID: <171019160954.2042.4426543267974940607@gjsousa-mobl2> (raw)
In-Reply-To: <Ze9xEIf2wBsl5iOY@intel.com>

Quoting Lisovskiy, Stanislav (2024-03-11 18:01:04-03:00)
>On Mon, Mar 04, 2024 at 03:30:25PM -0300, Gustavo Sousa wrote:
>> Commit 394b4b7df9f7 ("drm/i915/lnl: Add CDCLK table") and commit
>> 3d3696c0fed1 ("drm/i915/lnl: Start using CDCLK through PLL") started
>> adding support for CDCLK programming support for Xe2LPD. One final piece
>> is missing, which is the programming necessary for changed in the ratio
>> between MDCLK and CDCLK. Let's do that now.
>> 
>> BSpec instructs us to update MBUS_CTL and DBUF_CTL_S* registers when the
>> ratio between MDCLK and CDCLK changes. The updates must be done before
>> changing the CDCLK when decreasing the frequency; or after it when
>> increasing the frequency.
>> 
>> Ratio-related updates to MBUS_CTL also depend on the state of MBus
>> joining, so they are performed by either CDCLK change sequence or by
>> changes in MBus joining. Since one might happen independently of the
>> other, we need to make sure that both logics see the necessary state
>> values when programming that register. MBus joining logic needs to know
>> the MDCLK:CDCLK ratio and that's already provided via mdclk_cdclk_ratio
>> field of struct intel_dbuf_state.
>> 
>> For the CDCLK logic, we need to have something similar: we need to
>> propagate the status of MBus joining to struct intel_cdclk_state. Do
>> that by adding the field joined_mbus to struct intel_cdclk_config.
>> (Preferably, that field would be added to intel_cdclk_state, however
>> currently only intel_cdclk_config is passed down to the functions that
>> do the register programming. We might revisit this decision if we find
>> that refactoring the code to pass the whole intel_cdclk_state is worth
>> it.)
>> 
>> Bspec: 68864, 68868, 69090, 69482
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_cdclk.c    | 31 ++++++++++++++
>>  drivers/gpu/drm/i915/display/intel_cdclk.h    |  3 ++
>>  drivers/gpu/drm/i915/display/skl_watermark.c  | 40 +++++++++++++++----
>>  drivers/gpu/drm/i915/display/skl_watermark.h  |  1 +
>>  .../gpu/drm/i915/display/skl_watermark_regs.h | 18 +++++----
>>  5 files changed, 77 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> index 04a6e9806254..12753589072d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> @@ -40,6 +40,7 @@
>>  #include "intel_psr.h"
>>  #include "intel_vdsc.h"
>>  #include "skl_watermark.h"
>> +#include "skl_watermark_regs.h"
>>  #include "vlv_sideband.h"
>>  
>>  /**
>> @@ -1683,6 +1684,8 @@ static void bxt_get_cdclk(struct drm_i915_private *dev_priv,
>>          }
>>  
>>   out:
>> +        if (DISPLAY_VER(dev_priv) >= 20)
>> +                cdclk_config->joined_mbus = intel_de_read(dev_priv, MBUS_CTL) & MBUS_JOIN;
>>          /*
>>           * Can't read this out :( Let's assume it's
>>           * at least what the CDCLK frequency requires.
>> @@ -1908,6 +1911,14 @@ u8 intel_mdclk_cdclk_ratio(struct drm_i915_private *i915,
>>          }
>>  }
>>  
>> +static void xe2lpd_mdclk_cdclk_ratio_program(struct drm_i915_private *i915,
>> +                                             const struct intel_cdclk_config *cdclk_config)
>> +{
>> +        intel_dbuf_mdclk_cdclk_ratio_update(i915,
>> +                                            intel_mdclk_cdclk_ratio(i915, cdclk_config),
>> +                                            cdclk_config->joined_mbus);
>> +}
>> +
>>  static bool cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i915,
>>                                                      const struct intel_cdclk_config *old_cdclk_config,
>>                                                      const struct intel_cdclk_config *new_cdclk_config,
>> @@ -2089,6 +2100,9 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>>                  return;
>>          }
>>  
>> +        if (DISPLAY_VER(dev_priv) >= 20 && cdclk < dev_priv->display.cdclk.hw.cdclk)
>> +                xe2lpd_mdclk_cdclk_ratio_program(dev_priv, cdclk_config);
>> +
>>          if (cdclk_compute_crawl_and_squash_midpoint(dev_priv, &dev_priv->display.cdclk.hw,
>>                                                      cdclk_config, &mid_cdclk_config)) {
>>                  _bxt_set_cdclk(dev_priv, &mid_cdclk_config, pipe);
>> @@ -2097,6 +2111,9 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>>                  _bxt_set_cdclk(dev_priv, cdclk_config, pipe);
>>          }
>>  
>> +        if (DISPLAY_VER(dev_priv) >= 20 && cdclk > dev_priv->display.cdclk.hw.cdclk)
>> +                xe2lpd_mdclk_cdclk_ratio_program(dev_priv, cdclk_config);
>> +
>>          if (DISPLAY_VER(dev_priv) >= 14)
>>                  /*
>>                   * NOOP - No Pcode communication needed for
>> @@ -3179,6 +3196,20 @@ int intel_cdclk_atomic_check(struct intel_atomic_state *state,
>>          return 0;
>>  }
>>  
>> +int intel_cdclk_state_set_joined_mbus(struct intel_atomic_state *state, bool joined_mbus)
>> +{
>> +        struct intel_cdclk_state *cdclk_state;
>> +
>> +        cdclk_state = intel_atomic_get_cdclk_state(state);
>> +        if (IS_ERR(cdclk_state))
>> +                return PTR_ERR(cdclk_state);
>> +
>> +        cdclk_state->actual.joined_mbus = joined_mbus;
>> +        cdclk_state->logical.joined_mbus = joined_mbus;
>> +
>> +        return intel_atomic_lock_global_state(&cdclk_state->base);
>
>Shouldn't we actually lock global state already before we modify
>the cdclk_state?

I don't think so. The cdclk_state that we have here is already a copy
that belongs only to this commit. I think it is actually critical that
we have the write locked at the swap time, which is done only after the
atomic checks.

As such, I think locking after we modify the copy should be fine. In
fact, we already have places in the code using the same pattern.

--
Gustavo Sousa

>
>
>Stan
>
>> +}
>> +
>>  int intel_cdclk_init(struct drm_i915_private *dev_priv)
>>  {
>>          struct intel_cdclk_state *cdclk_state;
>> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
>> index 8e6e302bd599..e358234bfe26 100644
>> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
>> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
>> @@ -18,6 +18,8 @@ struct intel_crtc_state;
>>  struct intel_cdclk_config {
>>          unsigned int cdclk, vco, ref, bypass;
>>          u8 voltage_level;
>> +        /* This field is only valid for Xe2LPD and above. */
>> +        bool joined_mbus;
>>  };
>>  
>>  struct intel_cdclk_state {
>> @@ -74,6 +76,7 @@ void intel_cdclk_get_cdclk(struct drm_i915_private *dev_priv,
>>                             struct intel_cdclk_config *cdclk_config);
>>  int intel_cdclk_atomic_check(struct intel_atomic_state *state,
>>                               bool *need_cdclk_calc);
>> +int intel_cdclk_state_set_joined_mbus(struct intel_atomic_state *state, bool joined_mbus);
>>  struct intel_cdclk_state *
>>  intel_atomic_get_cdclk_state(struct intel_atomic_state *state);
>>  
>> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
>> index 4410e21888ad..bc341abcab2f 100644
>> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
>> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
>> @@ -12,6 +12,7 @@
>>  #include "intel_atomic.h"
>>  #include "intel_atomic_plane.h"
>>  #include "intel_bw.h"
>> +#include "intel_cdclk.h"
>>  #include "intel_crtc.h"
>>  #include "intel_de.h"
>>  #include "intel_display.h"
>> @@ -2601,10 +2602,17 @@ skl_compute_ddb(struct intel_atomic_state *state)
>>                          return ret;
>>          }
>>  
>> -        if (HAS_MBUS_JOINING(i915))
>> +        if (HAS_MBUS_JOINING(i915)) {
>>                  new_dbuf_state->joined_mbus =
>>                          adlp_check_mbus_joined(new_dbuf_state->active_pipes);
>>  
>> +                if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus) {
>> +                        ret = intel_cdclk_state_set_joined_mbus(state, new_dbuf_state->joined_mbus);
>> +                        if (ret)
>> +                                return ret;
>> +                }
>> +        }
>> +
>>          for_each_intel_crtc(&i915->drm, crtc) {
>>                  enum pipe pipe = crtc->pipe;
>>  
>> @@ -3545,12 +3553,14 @@ int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state, u8
>>          return intel_atomic_lock_global_state(&dbuf_state->base);
>>  }
>>  
>> -static void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915,
>> -                                                u8 ratio,
>> -                                                bool joined_mbus)
>> +void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio, bool joined_mbus)
>>  {
>>          enum dbuf_slice slice;
>>  
>> +        if (DISPLAY_VER(i915) >= 20)
>> +                intel_de_rmw(i915, MBUS_CTL, MBUS_TRANSLATION_THROTTLE_MIN_MASK,
>> +                             MBUS_TRANSLATION_THROTTLE_MIN(ratio - 1));
>> +
>>          if (joined_mbus)
>>                  ratio *= 2;
>>  
>> @@ -3568,7 +3578,9 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
>>  {
>>          struct drm_i915_private *i915 = to_i915(state->base.dev);
>>          u32 mbus_ctl;
>> -        const struct intel_dbuf_state *dbuf_state =
>> +        const struct intel_dbuf_state *old_dbuf_state =
>> +                intel_atomic_get_old_dbuf_state(state);
>> +        const struct intel_dbuf_state *new_dbuf_state =
>>                  intel_atomic_get_new_dbuf_state(state);
>>  
>>          if (!HAS_MBUS_JOINING(i915))
>> @@ -3578,7 +3590,7 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
>>           * TODO: Implement vblank synchronized MBUS joining changes.
>>           * Must be properly coordinated with dbuf reprogramming.
>>           */
>> -        if (dbuf_state->joined_mbus)
>> +        if (new_dbuf_state->joined_mbus)
>>                  mbus_ctl = MBUS_HASHING_MODE_1x4 | MBUS_JOIN |
>>                          MBUS_JOIN_PIPE_SELECT_NONE;
>>          else
>> @@ -3589,8 +3601,20 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
>>                       MBUS_HASHING_MODE_MASK | MBUS_JOIN |
>>                       MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl);
>>  
>> -        intel_dbuf_mdclk_cdclk_ratio_update(i915, dbuf_state->mdclk_cdclk_ratio,
>> -                                            dbuf_state->joined_mbus);
>> +        if (DISPLAY_VER(i915) >= 20 &&
>> +            old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) {
>> +                /*
>> +                 * For Xe2LPD and beyond, when there is a change in the ratio
>> +                 * between MDCLK and CDCLK, updates to related registers need to
>> +                 * happen at a specific point in the CDCLK change sequence. In
>> +                 * that case, we defer to the call to
>> +                 * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
>> +                 */
>> +                return;
>> +        }
>> +
>> +        intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
>> +                                            new_dbuf_state->joined_mbus);
>>  }
>>  
>>  void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
>> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
>> index fed4d12df584..3a90741cab06 100644
>> --- a/drivers/gpu/drm/i915/display/skl_watermark.h
>> +++ b/drivers/gpu/drm/i915/display/skl_watermark.h
>> @@ -76,6 +76,7 @@ int intel_dbuf_state_set_mdclk_cdclk_ratio(struct intel_atomic_state *state, u8
>>  
>>  void intel_dbuf_pre_plane_update(struct intel_atomic_state *state);
>>  void intel_dbuf_post_plane_update(struct intel_atomic_state *state);
>> +void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio, bool joined_mbus);
>>  void intel_mbus_dbox_update(struct intel_atomic_state *state);
>>  
>>  #endif /* __SKL_WATERMARK_H__ */
>> diff --git a/drivers/gpu/drm/i915/display/skl_watermark_regs.h b/drivers/gpu/drm/i915/display/skl_watermark_regs.h
>> index 20b30c9a6613..269163fa3350 100644
>> --- a/drivers/gpu/drm/i915/display/skl_watermark_regs.h
>> +++ b/drivers/gpu/drm/i915/display/skl_watermark_regs.h
>> @@ -32,14 +32,16 @@
>>  #define MBUS_BBOX_CTL_S1                _MMIO(0x45040)
>>  #define MBUS_BBOX_CTL_S2                _MMIO(0x45044)
>>  
>> -#define MBUS_CTL                        _MMIO(0x4438C)
>> -#define MBUS_JOIN                        REG_BIT(31)
>> -#define MBUS_HASHING_MODE_MASK                REG_BIT(30)
>> -#define MBUS_HASHING_MODE_2x2                REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 0)
>> -#define MBUS_HASHING_MODE_1x4                REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 1)
>> -#define MBUS_JOIN_PIPE_SELECT_MASK        REG_GENMASK(28, 26)
>> -#define MBUS_JOIN_PIPE_SELECT(pipe)        REG_FIELD_PREP(MBUS_JOIN_PIPE_SELECT_MASK, pipe)
>> -#define MBUS_JOIN_PIPE_SELECT_NONE        MBUS_JOIN_PIPE_SELECT(7)
>> +#define MBUS_CTL                                _MMIO(0x4438C)
>> +#define   MBUS_JOIN                                REG_BIT(31)
>> +#define   MBUS_HASHING_MODE_MASK                REG_BIT(30)
>> +#define   MBUS_HASHING_MODE_2x2                        REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 0)
>> +#define   MBUS_HASHING_MODE_1x4                        REG_FIELD_PREP(MBUS_HASHING_MODE_MASK, 1)
>> +#define   MBUS_JOIN_PIPE_SELECT_MASK                REG_GENMASK(28, 26)
>> +#define   MBUS_JOIN_PIPE_SELECT(pipe)                REG_FIELD_PREP(MBUS_JOIN_PIPE_SELECT_MASK, pipe)
>> +#define   MBUS_JOIN_PIPE_SELECT_NONE                MBUS_JOIN_PIPE_SELECT(7)
>> +#define   MBUS_TRANSLATION_THROTTLE_MIN_MASK        REG_GENMASK(15, 13)
>> +#define   MBUS_TRANSLATION_THROTTLE_MIN(val)        REG_FIELD_PREP(MBUS_TRANSLATION_THROTTLE_MIN_MASK, val)
>>  
>>  /* Watermark register definitions for SKL */
>>  #define _CUR_WM_A_0                0x70140
>> -- 
>> 2.44.0
>>