> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Wednesday, May 22, 2024 12:39 PM
> To: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Kahola, Mika <mika.kahola@xxxxxxxxx>
> Subject: Re: [PATCH 2/2] drm/i915/display: Add compare config for MTL+ platforms
>
> On Wed, 22 May 2024, Mika Kahola <mika.kahola@xxxxxxxxx> wrote:
> > Currently, we may bump into pll mismatch errors during the state
> > verification stage. This happens when we try to use fastset instead of
> > full modeset. Hence, we would need to add a check for pipe
> > configuration to ensure that the sw and the hw configuration will
> > match. In case of hw and sw mismatch, we would need to disable fastset
> > and use full modeset instead.
> >
> > Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 74
> > +++++++++++++++++++ drivers/gpu/drm/i915/display/intel_cx0_phy.h |
> > 2 + drivers/gpu/drm/i915/display/intel_display.c | 39 ++++++++++
> > drivers/gpu/drm/i915/display/intel_dpll_mgr.h | 1 +
> > 4 files changed, 116 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index c9e5bb6ecfd7..f549753ab1cf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -2038,6 +2038,7 @@ static int intel_c10pll_calc_state(struct intel_crtc_state
> *crtc_state,
> > if (crtc_state->port_clock == tables[i]->clock) {
> > crtc_state->dpll_hw_state.cx0pll.c10 = *tables[i];
> > intel_c10pll_update_pll(crtc_state, encoder);
> > + crtc_state->dpll_hw_state.cx0pll.use_c10 = true;
>
> The readout doesn't set use_c10 anywhere, does it?
No, it is just used to select which C10 or C20 sw and hw configs are compared.
>
> >
> > return 0;
> > }
> > @@ -2277,6 +2278,7 @@ static int intel_c20pll_calc_state(struct intel_crtc_state
> *crtc_state,
> > for (i = 0; tables[i]; i++) {
> > if (crtc_state->port_clock == tables[i]->clock) {
> > crtc_state->dpll_hw_state.cx0pll.c20 = *tables[i];
> > + crtc_state->dpll_hw_state.cx0pll.use_c10 = false;
> > return 0;
> > }
> > }
> > @@ -3272,6 +3274,78 @@ void intel_cx0pll_readout_hw_state(struct
> intel_encoder *encoder,
> > intel_c20pll_readout_hw_state(encoder, &pll_state->c20); }
> >
> > +static bool mtl_compare_hw_state_c10(const struct intel_c10pll_state *a,
> > + const struct intel_c10pll_state *b) {
> > + return a->clock == b->clock ||
> > + a->tx == b->tx ||
> > + a->cmn == b->cmn ||
> > + a->pll[0] == b->pll[0] ||
> > + a->pll[1] == b->pll[1] ||
> > + a->pll[2] == b->pll[2] ||
> > + a->pll[3] == b->pll[3] ||
> > + a->pll[4] == b->pll[4] ||
> > + a->pll[5] == b->pll[5] ||
> > + a->pll[6] == b->pll[6] ||
> > + a->pll[7] == b->pll[7] ||
> > + a->pll[8] == b->pll[8] ||
> > + a->pll[9] == b->pll[9] ||
> > + a->pll[10] == b->pll[10] ||
> > + a->pll[11] == b->pll[11] ||
> > + a->pll[12] == b->pll[12] ||
> > + a->pll[13] == b->pll[13] ||
> > + a->pll[14] == b->pll[14] ||
> > + a->pll[15] == b->pll[15] ||
> > + a->pll[16] == b->pll[16] ||
> > + a->pll[17] == b->pll[17] ||
> > + a->pll[18] == b->pll[18] ||
> > + a->pll[19] == b->pll[19];
>
> How about memcmp(a->pll, b->pll, sizeof(a->pll)) == 0 instead?
Yes, this is possible. I tried to mimic the comparison check done for some other platforms.
>
>
> > +}
> > +
> > +static bool mtl_compare_hw_state_c20(const struct intel_c20pll_state *a,
> > + const struct intel_c20pll_state *b) {
> > + int i;
> > +
> > + if (a->clock != b->clock)
> > + return false;
> > +
> > + for (i = 0; i < 3; i++) {
> > + if (a->tx[i] != b->tx[i])
> > + return false;
> > + }
>
> memcmp with sizeof, so we don't have to hardcode the sizes.
Yes.
>
> > +
> > + for (i = 4; i < 4; i++) {
>
> Typo, this does nothing... but memcmp.
Yep.
>
> > + if (a->cmn[i] != b->cmn[i])
> > + return false;
> > + }
> > +
> > + if (a->tx[0] & C20_PHY_USE_MPLLB) {
> > + for (i = 0; i < ARRAY_SIZE(a->mpllb); i++) {
> > + if (a->mpllb[i] != b->mpllb[i])
> > + return false;
> > + }
> > + } else {
> > + for (i = 0; i < ARRAY_SIZE(a->mplla); i++) {
> > + if (a->mplla[i] != b->mplla[i])
> > + return false;
> > + }
> > + }
>
> And memcmp.
That one too.
>
> > +
> > + return true;
> > +}
> > +
> > +bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
> > + const struct intel_cx0pll_state *b) {
>
> Maybe this for starters?
>
> if (a->use_c10 != b->use_c10)
> return false;
Ok, let's do the check first before doing anything else.
>
> > + if (a->use_c10 && b->use_c10)
> > + return mtl_compare_hw_state_c10(&a->c10,
> > + &b->c10);
> > + else
> > + return mtl_compare_hw_state_c20(&a->c20,
> > + &b->c20);
> > +}
> > +
> > int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder,
> > const struct intel_cx0pll_state *pll_state) { diff --git
> > a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > index 3e03af3e006c..180821df1834 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.h
> > @@ -39,6 +39,8 @@ void intel_c10pll_dump_hw_state(struct drm_i915_private
> *dev_priv,
> > const struct intel_c10pll_state *hw_state); void
> > intel_cx0pll_state_verify(struct intel_atomic_state *state,
> > struct intel_crtc *crtc);
> > +bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
> > + const struct intel_cx0pll_state *b);
> > void intel_c20pll_dump_hw_state(struct drm_i915_private *i915,
> > const struct intel_c20pll_state *hw_state); void
> > intel_cx0_phy_set_signal_levels(struct intel_encoder *encoder, diff
> > --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index cce1420fb541..17b43b2ae0e9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -66,6 +66,7 @@
> > #include "intel_crtc.h"
> > #include "intel_crtc_state_dump.h"
> > #include "intel_cursor_regs.h"
> > +#include "intel_cx0_phy.h"
> > #include "intel_ddi.h"
> > #include "intel_de.h"
> > #include "intel_display_driver.h"
> > @@ -5002,6 +5003,30 @@ pipe_config_pll_mismatch(struct drm_printer *p, bool
> fastset,
> > intel_dpll_dump_hw_state(i915, p, b); }
> >
> > +static void
> > +pipe_config_cx0pll_mismatch(struct drm_printer *p, bool fastset,
> > + const struct intel_crtc *crtc,
> > + const char *name,
> > + const struct intel_cx0pll_state *a,
> > + const struct intel_cx0pll_state *b) {
> > + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +
> > + pipe_config_mismatch(p, fastset, crtc, name, " "); /* stupid
> > +-Werror=format-zero-length */
>
> Instead of working around something and adding comments like that, maybe actually
> use it for something useful?
>
> Something like, idk, "%s", a->c10 ? "c10" : "c20"
Ah, stupid copy and paste. I should have fixed this but missed.
>
> > +
> > + if (a->use_c10) {
> > + drm_printf(p, "expected:\n");
> > + intel_c10pll_dump_hw_state(i915, &a->c10);
> > + drm_printf(p, "found:\n");
> > + intel_c10pll_dump_hw_state(i915, &b->c10);
> > + } else {
> > + drm_printf(p, "expected:\n");
> > + intel_c20pll_dump_hw_state(i915, &a->c20);
> > + drm_printf(p, "found:\n");
> > + intel_c20pll_dump_hw_state(i915, &b->c20);
> > + }
> > + drm_printf(p, "found:\n");
> > + intel_c10pll_dump_hw_state(i915, &b->c10);
> > + } else {
> > + drm_printf(p, "expected:\n");
> > + intel_c20pll_dump_hw_state(i915, &a->c20);
> > + drm_printf(p, "found:\n");
> > + intel_c20pll_dump_hw_state(i915, &b->c20);
> > + }
>
> I think I'd add a intel_cx0pll_dump_hw_state() to avoid looking into the details like
> this at high level code. This becomes cleaner too.
True. I rephrase this part. Anyway, there was a BAT issue that I need to solve that I wasn't able to trigger while I was testing this patch.
Thanks for the review and comments!
-Mika-
>
> > +}
> > +
> > bool
> > intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > const struct intel_crtc_state *pipe_config, @@ -5105,6
> +5130,16
> > @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > } \
> > } while (0)
> >
> > +#define PIPE_CONF_CHECK_PLL_CX0(name) do { \
> > + if (!intel_cx0pll_compare_hw_state(¤t_config->name, \
> > + &pipe_config->name)) { \
> > + pipe_config_cx0pll_mismatch(&p, fastset, crtc, __stringify(name), \
> > + ¤t_config->name, \
> > + &pipe_config->name); \
> > + ret = false; \
> > + } \
> > +} while (0)
> > +
> > #define PIPE_CONF_CHECK_TIMINGS(name) do { \
> > PIPE_CONF_CHECK_I(name.crtc_hdisplay); \
> > PIPE_CONF_CHECK_I(name.crtc_htotal); \ @@ -5337,6 +5372,10 @@
> > intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > if (dev_priv->display.dpll.mgr || HAS_GMCH(dev_priv))
> > PIPE_CONF_CHECK_PLL(dpll_hw_state);
> >
> > + /* FIXME convert MTL+ platforms over to dpll_mgr */
> > + if (DISPLAY_VER(dev_priv) >= 14)
> > + PIPE_CONF_CHECK_PLL_CX0(dpll_hw_state.cx0pll);
> > +
> > PIPE_CONF_CHECK_X(dsi_pll.ctrl);
> > PIPE_CONF_CHECK_X(dsi_pll.div);
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > index f09e513ce05b..36baed75b89a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.h
> > @@ -264,6 +264,7 @@ struct intel_cx0pll_state {
> > struct intel_c20pll_state c20;
> > };
> > bool ssc_enabled;
> > + bool use_c10;
> > };
> >
> > struct intel_dpll_hw_state {
>
> --
> Jani Nikula, Intel
