[Intel-xe] [Intel-gfx] [PATCH v2 24/27] drm/i915/lnl: Introduce MDCLK_CDCLK ratio to DBuf
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Mon Sep 11 08:06:42 UTC 2023
On Fri, Sep 08, 2023 at 03:43:03PM -0700, Matt Roper wrote:
> On Thu, Sep 07, 2023 at 08:37:54AM -0700, Lucas De Marchi wrote:
> > From: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> >
> > When we change MDCLK/CDCLK the BSpec now instructs us to write a ratio
> > between MDCLK/CDCLK to MBUS CTL and DBUF CTL registers during that
> > change.
> >
> > Previsouly DBuf state and CDCLK were not anyhow coupled together. Now
> > at compute stage when we know which CDCLK/MDCLK we are going to use, we
> > need to update the DBuf state with that ratio, being properly encoded,
> > so that it gets written to those registers, once DBuf state is being
> > update. The criteria for updating DBuf state is also a CDCLK/MDCLK ratio
> > change now.
> >
> > v2:
> > - Remove condition check for display version 20 since it's compatible
> > with previous versions (Matt Roper)
> > - Squash the serialization of global state when mdclk_cdclk_ratio
> > changes
>
> I'm not sure I follow the serialization change here; can you add some
> more explanation of that to the commit message? If the mdclk:cdclk
> ratio changes then that means we're changing the cdclk (either on its
> own if only the squashing waveform is changing, or along with the mdclk
> if the PLL is also getting reprogrammed). In either case we should
> already be serializing in intel_cdclk_need_serialize() due to the cdclk
> change, right? Is the new check added here actually important somehow?
I agree, in theory whenever mdclk:cdclk ratio changes, we also probably
would be changing cdclk as well, so it is just based on our rule:
"whenever global data changes, we lock the global state", "whenever
hw needs to be written, we serialize the global state".
Calling this twice, just adds all crtcs into state, if it wasn't already
there, so shouldn't be harmful(anyway we have other places, where we might
call this again) however we can remove it also.
I have written this patch, somewhile ago, at least now I can't recall any
other reasons or cases when mdclk:cdclk ratio might change..
However I see that there is that other function called get_mbus_mdclk_cdclk_ratio
and its result is used for writing DBUF_CTL_S* regs and it seems to depend not
only on mdclk:cdclk ratio, but also if mbus_join is enabled or not.
Could be that we actually need to make sure, we serialize in that case?
Stan
>
>
> Matt
>
> >
> > Bspec: 68864, 69482, 69445
> > Cc: Mika Kahola <mika.kahola at intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 28 +++++++++++++++++++
> > drivers/gpu/drm/i915/display/skl_watermark.c | 28 ++++++++++++++++---
> > drivers/gpu/drm/i915/display/skl_watermark.h | 1 +
> > .../gpu/drm/i915/display/skl_watermark_regs.h | 2 ++
> > 4 files changed, 55 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index abe845906c7c..677a50c28dae 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -39,6 +39,7 @@
> > #include "intel_pcode.h"
> > #include "intel_psr.h"
> > #include "intel_vdsc.h"
> > +#include "skl_watermark.h"
> > #include "vlv_sideband.h"
> >
> > /**
> > @@ -1800,6 +1801,16 @@ static bool cdclk_pll_is_unknown(unsigned int vco)
> > return vco == ~0;
> > }
> >
> > +/* Return the MBUS_CTL's encoding of the mdclk/cdclk ratio */
> > +static int get_mdclk_cdclk_ratio(struct drm_i915_private *i915,
> > + const struct intel_cdclk_config *cdclk_config)
> > +{
> > + if (DISPLAY_VER(i915) >= 20)
> > + return DIV_ROUND_UP(cdclk_config->vco, cdclk_config->cdclk) - 1;
> > +
> > + return 1;
> > +}
> > +
> > static int cdclk_squash_divider(u16 waveform)
> > {
> > return hweight16(waveform ?: 0xffff);
> > @@ -2735,6 +2746,8 @@ static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
> > struct intel_crtc_state *crtc_state;
> > int min_cdclk, i;
> > enum pipe pipe;
> > + struct intel_dbuf_state *new_dbuf_state;
> > + struct intel_dbuf_state *old_dbuf_state;
> >
> > for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> > int ret;
> > @@ -2768,6 +2781,21 @@ static int intel_compute_min_cdclk(struct intel_cdclk_state *cdclk_state)
> > }
> > }
> >
> > + new_dbuf_state = intel_atomic_get_new_dbuf_state(state);
> > + old_dbuf_state = intel_atomic_get_old_dbuf_state(state);
> > + if (new_dbuf_state && old_dbuf_state) {
> > + new_dbuf_state->mdclk_cdclk_ratio =
> > + get_mdclk_cdclk_ratio(dev_priv, &cdclk_state->actual);
> > +
> > + if (new_dbuf_state->mdclk_cdclk_ratio != old_dbuf_state->mdclk_cdclk_ratio) {
> > + int ret;
> > +
> > + ret = intel_atomic_serialize_global_state(&new_dbuf_state->base);
> > + if (ret)
> > + return ret;
> > + }
> > + }
> > +
> > min_cdclk = max(cdclk_state->force_min_cdclk,
> > cdclk_state->bw_min_cdclk);
> > for_each_pipe(dev_priv, pipe)
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 64a122d3c9c0..1fefb02876c8 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -3472,6 +3472,16 @@ int intel_dbuf_init(struct drm_i915_private *i915)
> > return 0;
> > }
> >
> > +static int get_mbus_mdclk_cdclk_ratio(struct drm_i915_private *i915,
> > + int mdclk_cdclk_ratio,
> > + int mbus_joined)
> > +{
> > + if (mbus_joined)
> > + return (mdclk_cdclk_ratio << 1) + 1;
> > +
> > + return mdclk_cdclk_ratio;
> > +}
> > +
> > /*
> > * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before
> > * update the request state of all DBUS slices.
> > @@ -3483,10 +3493,16 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
> > enum dbuf_slice slice;
> > const struct intel_dbuf_state *dbuf_state =
> > intel_atomic_get_new_dbuf_state(state);
> > + int tracker_state_service;
> >
> > if (!HAS_MBUS_JOINING(i915))
> > return;
> >
> > + tracker_state_service =
> > + get_mbus_mdclk_cdclk_ratio(i915,
> > + dbuf_state->mdclk_cdclk_ratio,
> > + dbuf_state->joined_mbus);
> > +
> > /*
> > * TODO: Implement vblank synchronized MBUS joining changes.
> > * Must be properly coordinated with dbuf reprogramming.
> > @@ -3494,13 +3510,15 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state)
> > if (dbuf_state->joined_mbus) {
> > mbus_ctl = MBUS_HASHING_MODE_1x4 | MBUS_JOIN |
> > MBUS_JOIN_PIPE_SELECT_NONE;
> > - dbuf_min_tracker_val = DBUF_MIN_TRACKER_STATE_SERVICE(3);
> > } else {
> > mbus_ctl = MBUS_HASHING_MODE_2x2 |
> > MBUS_JOIN_PIPE_SELECT_NONE;
> > - dbuf_min_tracker_val = DBUF_MIN_TRACKER_STATE_SERVICE(1);
> > }
> >
> > + dbuf_min_tracker_val = DBUF_MIN_TRACKER_STATE_SERVICE(tracker_state_service);
> > +
> > + mbus_ctl |= MBUS_TRANS_THROTTLE_MIN_SELECT(dbuf_state->mdclk_cdclk_ratio);
> > +
> > intel_de_rmw(i915, MBUS_CTL,
> > MBUS_HASHING_MODE_MASK | MBUS_JOIN |
> > MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl);
> > @@ -3521,7 +3539,8 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> >
> > if (!new_dbuf_state ||
> > (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices &&
> > - new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus))
> > + new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus &&
> > + new_dbuf_state->mdclk_cdclk_ratio == old_dbuf_state->mdclk_cdclk_ratio))
> > return;
> >
> > WARN_ON(!new_dbuf_state->base.changed);
> > @@ -3542,7 +3561,8 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state)
> >
> > if (!new_dbuf_state ||
> > (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices &&
> > - new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus))
> > + new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus &&
> > + new_dbuf_state->mdclk_cdclk_ratio == old_dbuf_state->mdclk_cdclk_ratio))
> > return;
> >
> > WARN_ON(!new_dbuf_state->base.changed);
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.h b/drivers/gpu/drm/i915/display/skl_watermark.h
> > index f91a3d4ddc07..54db5c7d517e 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.h
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.h
> > @@ -56,6 +56,7 @@ struct intel_dbuf_state {
> > u8 slices[I915_MAX_PIPES];
> > u8 enabled_slices;
> > u8 active_pipes;
> > + u8 mdclk_cdclk_ratio;
> > bool joined_mbus;
> > };
> >
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark_regs.h b/drivers/gpu/drm/i915/display/skl_watermark_regs.h
> > index 628c5920ad49..4c820f1d351d 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark_regs.h
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark_regs.h
> > @@ -38,6 +38,8 @@
> > #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_TRANS_THROTTLE_MIN_MASK REG_GENMASK(15, 13)
> > +#define MBUS_TRANS_THROTTLE_MIN_SELECT(ratio) REG_FIELD_PREP(MBUS_TRANS_THROTTLE_MIN_MASK, ratio)
> > #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)
> >
> > --
> > 2.40.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
More information about the Intel-xe
mailing list