RE: [PATCH 2/2] drm/i915/display: Add compare config for MTL+ platforms
> -Original Message-
> From: Jani Nikula
> Sent: Wednesday, May 22, 2024 12:39 PM
> To: Kahola, Mika ; intel-gfx@lists.freedesktop.org
> Cc: Kahola, Mika
> Subject: Re: [PATCH 2/2] drm/i915/display: Add compare config for MTL+
> platforms
>
> On Wed, 22 May 2024, Mika Kahola 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
> > ---
> > 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, _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 &
Re: [PATCH 2/2] drm/i915/display: Add compare config for MTL+ platforms
On Wed, 22 May 2024, Mika Kahola 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
> ---
> 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?
>
> 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, _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?
> +}
> +
> +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.
> +
> + for (i = 4; i < 4; i++) {
Typo, this does nothing... but memcmp.
> + 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.
> +
> + 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;
> + if (a->use_c10 && b->use_c10)
> + return mtl_compare_hw_state_c10(>c10,
> + >c10);
> + else
> + return mtl_compare_hw_state_c20(>c20,
> + >c20);
> +}
> +
> int intel_cx0pll_calc_port_clock(struct intel_encoder *encoder,
>const struct intel_cx0pll_state *pll_state)
> {
>
[PATCH 2/2] drm/i915/display: Add compare config for MTL+ platforms
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
---
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;
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, _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];
+}
+
+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;
+ }
+
+ for (i = 4; i < 4; i++) {
+ 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;
+ }
+ }
+
+ return true;
+}
+
+bool intel_cx0pll_compare_hw_state(const struct intel_cx0pll_state *a,
+ const struct intel_cx0pll_state *b)
+{
+ if (a->use_c10 && b->use_c10)
+ return mtl_compare_hw_state_c10(>c10,
+ >c10);
+ else
+ return mtl_compare_hw_state_c20(>c20,
+ >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
