On Mon, Mar 11, 2024 at 06:13:29PM -0300, Gustavo Sousa wrote:
> 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@xxxxxxxxx>
> >> ---
> >> 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
Yes, right. We lock global state if we modify it and/or serialize commit
if we want to write the hardware.
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
>
> >
> >
> >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
> >>
