* [PATCH] drm/i915/display: C20 clock state verification
@ 2023-12-15 8:00 Mika Kahola
2023-12-15 8:53 ` Imre Deak
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Mika Kahola @ 2023-12-15 8:00 UTC (permalink / raw)
To: intel-gfx
Add clock state verification for C20. Since we
are usign either A or B contexts, which are
selected based on clock rate, we first need to
calculate hw clock and use that clock to select
which context we are using.
Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
index 775c1c4a8978..6757e9f941e4 100644
--- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
+++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
@@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
bool use_mplla;
int i;
+ int hw_clock = intel_c20pll_calc_port_clock(encoder, mpll_hw_state);
- use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
+ use_mplla = intel_c20_use_mplla(hw_clock);
if (use_mplla) {
for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
I915_STATE_WARN(i915, mpll_hw_state->mplla[i] != mpll_sw_state->mplla[i],
@@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
crtc->base.base.id, crtc->base.name, i,
mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
}
+
+ I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
+ "[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
+ crtc->base.base.id, crtc->base.name,
+ mpll_sw_state->clock, hw_clock);
}
void intel_cx0pll_state_verify(struct intel_atomic_state *state,
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/display: C20 clock state verification
2023-12-15 8:00 [PATCH] drm/i915/display: C20 clock state verification Mika Kahola
@ 2023-12-15 8:53 ` Imre Deak
2023-12-15 9:01 ` Imre Deak
2023-12-15 9:10 ` Ville Syrjälä
2023-12-15 12:49 ` ✓ Fi.CI.BAT: success for " Patchwork
2023-12-15 23:27 ` ✓ Fi.CI.IGT: " Patchwork
2 siblings, 2 replies; 10+ messages in thread
From: Imre Deak @ 2023-12-15 8:53 UTC (permalink / raw)
To: Mika Kahola; +Cc: intel-gfx
On Fri, Dec 15, 2023 at 10:00:57AM +0200, Mika Kahola wrote:
> Add clock state verification for C20. Since we
> are usign either A or B contexts, which are
> selected based on clock rate, we first need to
> calculate hw clock and use that clock to select
> which context we are using.
Could the description be clearer that the patch _fixes_ the context
selection? (Also the subject line should always say _what_ the patch
does.)
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index 775c1c4a8978..6757e9f941e4 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
> bool use_mplla;
> int i;
> + int hw_clock = intel_c20pll_calc_port_clock(encoder, mpll_hw_state);
>
> - use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
> + use_mplla = intel_c20_use_mplla(hw_clock);
It's mpll_hw_state->tx[0] C20_PHY_USE_MPLLB which tells the HW which
context to use, so I think it's better to use the same condition here.
> if (use_mplla) {
> for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
> I915_STATE_WARN(i915, mpll_hw_state->mplla[i] != mpll_sw_state->mplla[i],
> @@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> crtc->base.base.id, crtc->base.name, i,
> mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
> }
> +
> + I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
> + "[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
> + crtc->base.base.id, crtc->base.name,
> + mpll_sw_state->clock, hw_clock);
I think the intel_crtc_state::port_clock SW/HW state verification is
equivalent to the above, but it's good to verify it here as well. I
would store hw_clock to mpll_hw_state->clock in
intel_c20pll_readout_hw_state() though.
> }
>
> void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/display: C20 clock state verification
2023-12-15 8:53 ` Imre Deak
@ 2023-12-15 9:01 ` Imre Deak
2023-12-15 14:00 ` Kahola, Mika
2023-12-15 9:10 ` Ville Syrjälä
1 sibling, 1 reply; 10+ messages in thread
From: Imre Deak @ 2023-12-15 9:01 UTC (permalink / raw)
To: Mika Kahola, intel-gfx
On Fri, Dec 15, 2023 at 10:53:36AM +0200, Imre Deak wrote:
> On Fri, Dec 15, 2023 at 10:00:57AM +0200, Mika Kahola wrote:
> > Add clock state verification for C20. Since we
> > are usign either A or B contexts, which are
> > selected based on clock rate, we first need to
> > calculate hw clock and use that clock to select
> > which context we are using.
>
> Could the description be clearer that the patch _fixes_ the context
> selection? (Also the subject line should always say _what_ the patch
> does.)
>
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index 775c1c4a8978..6757e9f941e4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> > const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
> > bool use_mplla;
> > int i;
> > + int hw_clock = intel_c20pll_calc_port_clock(encoder, mpll_hw_state);
> >
> > - use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
> > + use_mplla = intel_c20_use_mplla(hw_clock);
>
> It's mpll_hw_state->tx[0] C20_PHY_USE_MPLLB which tells the HW which
> context to use, so I think it's better to use the same condition here.
You could also add a check intel_c20_use_mplla(clock) matches the above
flag.
>
> > if (use_mplla) {
> > for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
> > I915_STATE_WARN(i915, mpll_hw_state->mplla[i] != mpll_sw_state->mplla[i],
> > @@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> > crtc->base.base.id, crtc->base.name, i,
> > mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
> > }
> > +
> > + I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
> > + "[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
> > + crtc->base.base.id, crtc->base.name,
> > + mpll_sw_state->clock, hw_clock);
>
> I think the intel_crtc_state::port_clock SW/HW state verification is
> equivalent to the above, but it's good to verify it here as well. I
> would store hw_clock to mpll_hw_state->clock in
> intel_c20pll_readout_hw_state() though.
>
> > }
> >
> > void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/display: C20 clock state verification
2023-12-15 8:53 ` Imre Deak
2023-12-15 9:01 ` Imre Deak
@ 2023-12-15 9:10 ` Ville Syrjälä
2023-12-15 9:17 ` Imre Deak
1 sibling, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2023-12-15 9:10 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
On Fri, Dec 15, 2023 at 10:53:31AM +0200, Imre Deak wrote:
> On Fri, Dec 15, 2023 at 10:00:57AM +0200, Mika Kahola wrote:
> > Add clock state verification for C20. Since we
> > are usign either A or B contexts, which are
> > selected based on clock rate, we first need to
> > calculate hw clock and use that clock to select
> > which context we are using.
>
> Could the description be clearer that the patch _fixes_ the context
> selection? (Also the subject line should always say _what_ the patch
> does.)
>
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index 775c1c4a8978..6757e9f941e4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> > const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
> > bool use_mplla;
> > int i;
> > + int hw_clock = intel_c20pll_calc_port_clock(encoder, mpll_hw_state);
> >
> > - use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
> > + use_mplla = intel_c20_use_mplla(hw_clock);
>
> It's mpll_hw_state->tx[0] C20_PHY_USE_MPLLB which tells the HW which
> context to use, so I think it's better to use the same condition here.
Yes, one should never assume anything about how the register values
were calculated/etc. That would pretty much defeat the whole purpose
of doing readout and state check.
BTW I don't see even being set anywhere C20_PHY_USE_MPLLB.
Do we not use it or is it encoded in that ugly hex soup in
intel_c20_compute_hdmi_tmds_pll()? Instead of repeating the
mistakes of the VLV PHY code someone should convert that into
human readable form...
>
> > if (use_mplla) {
> > for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
> > I915_STATE_WARN(i915, mpll_hw_state->mplla[i] != mpll_sw_state->mplla[i],
> > @@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> > crtc->base.base.id, crtc->base.name, i,
> > mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
> > }
> > +
> > + I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
> > + "[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
> > + crtc->base.base.id, crtc->base.name,
> > + mpll_sw_state->clock, hw_clock);
>
> I think the intel_crtc_state::port_clock SW/HW state verification is
> equivalent to the above, but it's good to verify it here as well. I
> would store hw_clock to mpll_hw_state->clock in
> intel_c20pll_readout_hw_state() though.
>
> > }
> >
> > void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> > --
> > 2.34.1
> >
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/display: C20 clock state verification
2023-12-15 9:10 ` Ville Syrjälä
@ 2023-12-15 9:17 ` Imre Deak
0 siblings, 0 replies; 10+ messages in thread
From: Imre Deak @ 2023-12-15 9:17 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Fri, Dec 15, 2023 at 11:10:52AM +0200, Ville Syrjälä wrote:
> On Fri, Dec 15, 2023 at 10:53:31AM +0200, Imre Deak wrote:
> > On Fri, Dec 15, 2023 at 10:00:57AM +0200, Mika Kahola wrote:
> > > Add clock state verification for C20. Since we
> > > are usign either A or B contexts, which are
> > > selected based on clock rate, we first need to
> > > calculate hw clock and use that clock to select
> > > which context we are using.
> >
> > Could the description be clearer that the patch _fixes_ the context
> > selection? (Also the subject line should always say _what_ the patch
> > does.)
> >
> > >
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > index 775c1c4a8978..6757e9f941e4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > @@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> > > const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
> > > bool use_mplla;
> > > int i;
> > > + int hw_clock = intel_c20pll_calc_port_clock(encoder, mpll_hw_state);
> > >
> > > - use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
> > > + use_mplla = intel_c20_use_mplla(hw_clock);
> >
> > It's mpll_hw_state->tx[0] C20_PHY_USE_MPLLB which tells the HW which
> > context to use, so I think it's better to use the same condition here.
>
> Yes, one should never assume anything about how the register values
> were calculated/etc. That would pretty much defeat the whole purpose
> of doing readout and state check.
>
> BTW I don't see even being set anywhere C20_PHY_USE_MPLLB.
> Do we not use it or is it encoded in that ugly hex soup in
> intel_c20_compute_hdmi_tmds_pll()?
Yes, the pll_state->tx[0] line there. For DP, it's in the
intel_c20pll_state predefined tables.
> Instead of repeating the mistakes of the VLV PHY code someone should
> convert that into human readable form...
>
> >
> > > if (use_mplla) {
> > > for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
> > > I915_STATE_WARN(i915, mpll_hw_state->mplla[i] != mpll_sw_state->mplla[i],
> > > @@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> > > crtc->base.base.id, crtc->base.name, i,
> > > mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
> > > }
> > > +
> > > + I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
> > > + "[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
> > > + crtc->base.base.id, crtc->base.name,
> > > + mpll_sw_state->clock, hw_clock);
> >
> > I think the intel_crtc_state::port_clock SW/HW state verification is
> > equivalent to the above, but it's good to verify it here as well. I
> > would store hw_clock to mpll_hw_state->clock in
> > intel_c20pll_readout_hw_state() though.
> >
> > > }
> > >
> > > void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> > > --
> > > 2.34.1
> > >
>
> --
> Ville Syrjälä
> Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/display: C20 clock state verification
2023-12-15 8:00 [PATCH] drm/i915/display: C20 clock state verification Mika Kahola
2023-12-15 8:53 ` Imre Deak
@ 2023-12-15 12:49 ` Patchwork
2023-12-15 23:27 ` ✓ Fi.CI.IGT: " Patchwork
2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-12-15 12:49 UTC (permalink / raw)
To: Mika Kahola; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 2787 bytes --]
== Series Details ==
Series: drm/i915/display: C20 clock state verification
URL : https://patchwork.freedesktop.org/series/127858/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_14027 -> Patchwork_127858v1
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/index.html
Participating hosts (37 -> 36)
------------------------------
Additional (1): bat-adlp-11
Missing (2): fi-snb-2520m fi-pnv-d510
Known issues
------------
Here are the changes found in Patchwork_127858v1 that come from known issues:
### CI changes ###
#### Issues hit ####
* boot:
- bat-adlp-11: NOTRUN -> [FAIL][1] ([i915#8293])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/bat-adlp-11/boot.html
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live@execlists:
- fi-bsw-nick: [PASS][2] -> [ABORT][3] ([i915#7911])
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/fi-bsw-nick/igt@i915_selftest@live@execlists.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/fi-bsw-nick/igt@i915_selftest@live@execlists.html
* igt@i915_selftest@live@workarounds:
- bat-adlm-1: [PASS][4] -> [INCOMPLETE][5] ([i915#9413])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/bat-adlm-1/igt@i915_selftest@live@workarounds.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/bat-adlm-1/igt@i915_selftest@live@workarounds.html
* igt@kms_pm_rpm@basic-rte:
- bat-rpls-2: [PASS][6] -> [ABORT][7] ([i915#8668])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/bat-rpls-2/igt@kms_pm_rpm@basic-rte.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/bat-rpls-2/igt@kms_pm_rpm@basic-rte.html
[i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
[i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
[i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668
[i915#9413]: https://gitlab.freedesktop.org/drm/intel/issues/9413
Build changes
-------------
* Linux: CI_DRM_14027 -> Patchwork_127858v1
CI-20190529: 20190529
CI_DRM_14027: 961ef418705ac5808345e883acd91f8ce167e00b @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7643: ced22f8bf4263ff395dc852c86b682e62a7a1c1b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_127858v1: 961ef418705ac5808345e883acd91f8ce167e00b @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
80be4c9afed8 drm/i915/display: C20 clock state verification
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/index.html
[-- Attachment #2: Type: text/html, Size: 3479 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] drm/i915/display: C20 clock state verification
2023-12-15 9:01 ` Imre Deak
@ 2023-12-15 14:00 ` Kahola, Mika
2023-12-15 15:10 ` Jani Nikula
0 siblings, 1 reply; 10+ messages in thread
From: Kahola, Mika @ 2023-12-15 14:00 UTC (permalink / raw)
To: Deak, Imre, intel-gfx@lists.freedesktop.org
> -----Original Message-----
> From: Deak, Imre <imre.deak@intel.com>
> Sent: Friday, December 15, 2023 11:02 AM
> To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/display: C20 clock state verification
>
> On Fri, Dec 15, 2023 at 10:53:36AM +0200, Imre Deak wrote:
> > On Fri, Dec 15, 2023 at 10:00:57AM +0200, Mika Kahola wrote:
> > > Add clock state verification for C20. Since we are usign either A or
> > > B contexts, which are selected based on clock rate, we first need to
> > > calculate hw clock and use that clock to select which context we are
> > > using.
> >
> > Could the description be clearer that the patch _fixes_ the context
> > selection? (Also the subject line should always say _what_ the patch
> > does.)
OK, should I add the fixes tag as well? I will reword the commit message to better describe what's going on in this patch.
> >
> > >
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > index 775c1c4a8978..6757e9f941e4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > > @@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> > > const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
> > > bool use_mplla;
> > > int i;
> > > + int hw_clock = intel_c20pll_calc_port_clock(encoder,
> > > +mpll_hw_state);
> > >
> > > - use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
> > > + use_mplla = intel_c20_use_mplla(hw_clock);
> >
> > It's mpll_hw_state->tx[0] C20_PHY_USE_MPLLB which tells the HW which
> > context to use, so I think it's better to use the same condition here.
Maybe I will ditch the use_mplla completely and go directly with
if (mpll_hw_state->tx]0] & C20_PHY_USE_MPLLB) { .. }
instead?
>
> You could also add a check intel_c20_use_mplla(clock) matches the above flag.
>
> >
> > > if (use_mplla) {
> > > for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
> > > I915_STATE_WARN(i915, mpll_hw_state->mplla[i] !=
> > > mpll_sw_state->mplla[i], @@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> > > crtc->base.base.id, crtc->base.name, i,
> > > mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
> > > }
> > > +
> > > + I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
> > > + "[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
> > > + crtc->base.base.id, crtc->base.name,
> > > + mpll_sw_state->clock, hw_clock);
> >
> > I think the intel_crtc_state::port_clock SW/HW state verification is
> > equivalent to the above, but it's good to verify it here as well. I
> > would store hw_clock to mpll_hw_state->clock in
> > intel_c20pll_readout_hw_state() though.
Well, clock is part of pll structure anyway, so it might as well be checked here too.
I will store hw clock too in intel_c20pll_readout_hw_state()
Thanks!
Mika
> >
> > > }
> > >
> > > void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> > > --
> > > 2.34.1
> > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] drm/i915/display: C20 clock state verification
2023-12-15 14:00 ` Kahola, Mika
@ 2023-12-15 15:10 ` Jani Nikula
2023-12-20 8:05 ` Kahola, Mika
0 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2023-12-15 15:10 UTC (permalink / raw)
To: Kahola, Mika, Deak, Imre, intel-gfx@lists.freedesktop.org
On Fri, 15 Dec 2023, "Kahola, Mika" <mika.kahola@intel.com> wrote:
>> -----Original Message-----
>> From: Deak, Imre <imre.deak@intel.com>
>> Sent: Friday, December 15, 2023 11:02 AM
>> To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/i915/display: C20 clock state verification
>>
>> On Fri, Dec 15, 2023 at 10:53:36AM +0200, Imre Deak wrote:
>> > On Fri, Dec 15, 2023 at 10:00:57AM +0200, Mika Kahola wrote:
>> > > Add clock state verification for C20. Since we are usign either A or
>> > > B contexts, which are selected based on clock rate, we first need to
>> > > calculate hw clock and use that clock to select which context we are
>> > > using.
>> >
>> > Could the description be clearer that the patch _fixes_ the context
>> > selection? (Also the subject line should always say _what_ the patch
>> > does.)
> OK, should I add the fixes tag as well? I will reword the commit message to better describe what's going on in this patch.
>
>> >
>> > >
>> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
>> > > ---
>> > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
>> > > 1 file changed, 7 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> > > index 775c1c4a8978..6757e9f941e4 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
>> > > @@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
>> > > const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
>> > > bool use_mplla;
>> > > int i;
>> > > + int hw_clock = intel_c20pll_calc_port_clock(encoder,
>> > > +mpll_hw_state);
>> > >
>> > > - use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
>> > > + use_mplla = intel_c20_use_mplla(hw_clock);
>> >
>> > It's mpll_hw_state->tx[0] C20_PHY_USE_MPLLB which tells the HW which
>> > context to use, so I think it's better to use the same condition here.
>
> Maybe I will ditch the use_mplla completely and go directly with
>
> if (mpll_hw_state->tx]0] & C20_PHY_USE_MPLLB) { .. }
>
> instead?
You should first verify that the hw and sw states for use_mplla agree.
If they don't, it doesn't matter which one you use.
BR,
Jani.
>
>>
>> You could also add a check intel_c20_use_mplla(clock) matches the above flag.
>>
>> >
>> > > if (use_mplla) {
>> > > for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
>> > > I915_STATE_WARN(i915, mpll_hw_state->mplla[i] !=
>> > > mpll_sw_state->mplla[i], @@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
>> > > crtc->base.base.id, crtc->base.name, i,
>> > > mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
>> > > }
>> > > +
>> > > + I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
>> > > + "[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
>> > > + crtc->base.base.id, crtc->base.name,
>> > > + mpll_sw_state->clock, hw_clock);
>> >
>> > I think the intel_crtc_state::port_clock SW/HW state verification is
>> > equivalent to the above, but it's good to verify it here as well. I
>> > would store hw_clock to mpll_hw_state->clock in
>> > intel_c20pll_readout_hw_state() though.
> Well, clock is part of pll structure anyway, so it might as well be checked here too.
>
> I will store hw clock too in intel_c20pll_readout_hw_state()
>
> Thanks!
> Mika
>
>> >
>> > > }
>> > >
>> > > void intel_cx0pll_state_verify(struct intel_atomic_state *state,
>> > > --
>> > > 2.34.1
>> > >
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915/display: C20 clock state verification
2023-12-15 8:00 [PATCH] drm/i915/display: C20 clock state verification Mika Kahola
2023-12-15 8:53 ` Imre Deak
2023-12-15 12:49 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2023-12-15 23:27 ` Patchwork
2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-12-15 23:27 UTC (permalink / raw)
To: Kahola, Mika; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 22769 bytes --]
== Series Details ==
Series: drm/i915/display: C20 clock state verification
URL : https://patchwork.freedesktop.org/series/127858/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_14027_full -> Patchwork_127858v1_full
====================================================
Summary
-------
**WARNING**
Minor unknown changes coming with Patchwork_127858v1_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_127858v1_full, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
to document this new failure mode, which will reduce false positives in CI.
Participating hosts (8 -> 8)
------------------------------
Additional (1): shard-snb-0
Missing (1): shard-glk-0
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_127858v1_full:
### IGT changes ###
#### Warnings ####
* igt@gem_ctx_freq@sysfs@gt0:
- shard-glk: [ABORT][1] ([i915#9847]) -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk1/igt@gem_ctx_freq@sysfs@gt0.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk8/igt@gem_ctx_freq@sysfs@gt0.html
* igt@i915_pm_rps@engine-order:
- shard-snb: [INCOMPLETE][3] ([i915#9847]) -> [INCOMPLETE][4]
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-snb7/igt@i915_pm_rps@engine-order.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-snb4/igt@i915_pm_rps@engine-order.html
Known issues
------------
Here are the changes found in Patchwork_127858v1_full that come from known issues:
### CI changes ###
#### Possible fixes ####
* boot:
- shard-glk: ([PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [FAIL][11], [PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], [PASS][24], [PASS][25], [PASS][26], [PASS][27], [PASS][28], [PASS][29]) ([i915#8293]) -> ([PASS][30], [PASS][31], [PASS][32], [PASS][33], [PASS][34], [PASS][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [PASS][47], [PASS][48], [PASS][49], [PASS][50], [PASS][51], [PASS][52], [PASS][53], [PASS][54])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk9/boot.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk9/boot.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk9/boot.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk9/boot.html
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk8/boot.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk8/boot.html
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk7/boot.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk6/boot.html
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk6/boot.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk5/boot.html
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk4/boot.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk4/boot.html
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk4/boot.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk4/boot.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk3/boot.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk3/boot.html
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk3/boot.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk3/boot.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk2/boot.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk2/boot.html
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk2/boot.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk2/boot.html
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk1/boot.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk1/boot.html
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk1/boot.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk9/boot.html
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk9/boot.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk8/boot.html
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk8/boot.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk8/boot.html
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk8/boot.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk7/boot.html
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk7/boot.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk7/boot.html
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk7/boot.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk6/boot.html
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk5/boot.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk5/boot.html
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk5/boot.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk4/boot.html
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk4/boot.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk4/boot.html
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk4/boot.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk3/boot.html
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk3/boot.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk3/boot.html
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk2/boot.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk2/boot.html
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk2/boot.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk1/boot.html
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_param@set-priority-not-supported:
- shard-mtlp: NOTRUN -> [SKIP][55] ([fdo#109314])
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-8/igt@gem_ctx_param@set-priority-not-supported.html
* igt@gem_exec_balancer@indices:
- shard-glk: NOTRUN -> [INCOMPLETE][56] ([i915#9856]) +1 other test incomplete
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk4/igt@gem_exec_balancer@indices.html
* igt@gem_exec_balancer@nop:
- shard-dg2: NOTRUN -> [INCOMPLETE][57] ([i915#9856]) +1 other test incomplete
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-6/igt@gem_exec_balancer@nop.html
- shard-rkl: NOTRUN -> [ABORT][58] ([i915#9855] / [i915#9856])
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-5/igt@gem_exec_balancer@nop.html
- shard-mtlp: NOTRUN -> [ABORT][59] ([i915#9855] / [i915#9856])
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-8/igt@gem_exec_balancer@nop.html
* igt@gem_exec_fair@basic-none-share@rcs0:
- shard-tglu: [PASS][60] -> [FAIL][61] ([i915#2842])
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-tglu-10/igt@gem_exec_fair@basic-none-share@rcs0.html
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-tglu-4/igt@gem_exec_fair@basic-none-share@rcs0.html
- shard-glk: [PASS][62] -> [FAIL][63] ([i915#2842])
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk2/igt@gem_exec_fair@basic-none-share@rcs0.html
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk3/igt@gem_exec_fair@basic-none-share@rcs0.html
* igt@gem_exec_reloc@basic-cpu-read-noreloc:
- shard-mtlp: NOTRUN -> [SKIP][64] ([i915#3281])
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-8/igt@gem_exec_reloc@basic-cpu-read-noreloc.html
* igt@gem_exec_whisper@basic-fds-forked:
- shard-mtlp: [PASS][65] -> [ABORT][66] ([i915#9857])
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-mtlp-2/igt@gem_exec_whisper@basic-fds-forked.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-2/igt@gem_exec_whisper@basic-fds-forked.html
* igt@gem_render_copy@yf-tiled-ccs-to-linear:
- shard-mtlp: NOTRUN -> [SKIP][67] ([i915#8428])
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-8/igt@gem_render_copy@yf-tiled-ccs-to-linear.html
* igt@i915_pm_rps@reset:
- shard-snb: [PASS][68] -> [INCOMPLETE][69] ([i915#7790])
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-snb4/igt@i915_pm_rps@reset.html
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-snb5/igt@i915_pm_rps@reset.html
* igt@kms_big_fb@linear-8bpp-rotate-90:
- shard-dg2: NOTRUN -> [SKIP][70] ([fdo#111614])
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-7/igt@kms_big_fb@linear-8bpp-rotate-90.html
* igt@kms_ccs@pipe-a-bad-aux-stride-y-tiled-gen12-mc-ccs:
- shard-dg2: NOTRUN -> [SKIP][71] ([i915#5354]) +1 other test skip
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-7/igt@kms_ccs@pipe-a-bad-aux-stride-y-tiled-gen12-mc-ccs.html
* igt@kms_ccs@pipe-c-crc-primary-basic-y-tiled-gen12-rc-ccs:
- shard-glk: NOTRUN -> [SKIP][72] ([fdo#109271])
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk6/igt@kms_ccs@pipe-c-crc-primary-basic-y-tiled-gen12-rc-ccs.html
* igt@kms_ccs@pipe-d-bad-aux-stride-y-tiled-gen12-mc-ccs:
- shard-mtlp: NOTRUN -> [SKIP][73] ([i915#5354] / [i915#6095])
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-8/igt@kms_ccs@pipe-d-bad-aux-stride-y-tiled-gen12-mc-ccs.html
* igt@kms_chamelium_edid@hdmi-mode-timings:
- shard-mtlp: NOTRUN -> [SKIP][74] ([i915#7828])
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-8/igt@kms_chamelium_edid@hdmi-mode-timings.html
* igt@kms_cursor_crc@cursor-sliding-32x10:
- shard-dg2: NOTRUN -> [SKIP][75] ([i915#3555])
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-7/igt@kms_cursor_crc@cursor-sliding-32x10.html
* igt@kms_cursor_legacy@cursora-vs-flipb-atomic-transitions-varying-size:
- shard-snb: [PASS][76] -> [SKIP][77] ([fdo#109271])
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-snb7/igt@kms_cursor_legacy@cursora-vs-flipb-atomic-transitions-varying-size.html
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-snb4/igt@kms_cursor_legacy@cursora-vs-flipb-atomic-transitions-varying-size.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
- shard-dg2: NOTRUN -> [SKIP][78] ([i915#8708])
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc.html
* igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-rotation@pipe-b-hdmi-a-2:
- shard-rkl: NOTRUN -> [SKIP][79] ([i915#5176] / [i915#9423]) +1 other test skip
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-1/igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-rotation@pipe-b-hdmi-a-2.html
* igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-rotation@pipe-c-hdmi-a-4:
- shard-dg1: NOTRUN -> [SKIP][80] ([i915#5176] / [i915#9423]) +3 other tests skip
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg1-16/igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-rotation@pipe-c-hdmi-a-4.html
* igt@kms_plane_scaling@planes-downscale-factor-0-25-unity-scaling@pipe-b-hdmi-a-4:
- shard-dg1: NOTRUN -> [SKIP][81] ([i915#5235]) +3 other tests skip
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg1-19/igt@kms_plane_scaling@planes-downscale-factor-0-25-unity-scaling@pipe-b-hdmi-a-4.html
* igt@kms_psr2_su@page_flip-nv12:
- shard-dg2: NOTRUN -> [SKIP][82] ([i915#9683])
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-7/igt@kms_psr2_su@page_flip-nv12.html
* igt@perf@gen12-group-exclusive-stream-sample-oa:
- shard-glk: NOTRUN -> [ABORT][83] ([i915#9847])
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk1/igt@perf@gen12-group-exclusive-stream-sample-oa.html
* igt@perf@invalid-oa-metric-set-id:
- shard-dg1: NOTRUN -> [ABORT][84] ([i915#9847])
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg1-13/igt@perf@invalid-oa-metric-set-id.html
* igt@perf@polling-parameterized:
- shard-dg2: NOTRUN -> [ABORT][85] ([i915#9847]) +3 other tests abort
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-7/igt@perf@polling-parameterized.html
* igt@perf_pmu@faulting-read:
- shard-snb: NOTRUN -> [INCOMPLETE][86] ([i915#9853])
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-snb1/igt@perf_pmu@faulting-read.html
* igt@syncobj_timeline@invalid-wait-zero-handles:
- shard-mtlp: NOTRUN -> [FAIL][87] ([i915#9781])
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-8/igt@syncobj_timeline@invalid-wait-zero-handles.html
* igt@v3d/v3d_submit_cl@multiple-job-submission:
- shard-mtlp: NOTRUN -> [SKIP][88] ([i915#2575])
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-mtlp-8/igt@v3d/v3d_submit_cl@multiple-job-submission.html
#### Possible fixes ####
* igt@drm_fdinfo@virtual-idle:
- shard-rkl: [FAIL][89] ([i915#7742]) -> [PASS][90]
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-rkl-6/igt@drm_fdinfo@virtual-idle.html
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-5/igt@drm_fdinfo@virtual-idle.html
* igt@kms_pm_rpm@dpms-lpsp:
- shard-dg2: [SKIP][91] ([i915#9519]) -> [PASS][92]
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-dg2-3/igt@kms_pm_rpm@dpms-lpsp.html
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-10/igt@kms_pm_rpm@dpms-lpsp.html
* igt@kms_pm_rpm@dpms-non-lpsp:
- shard-rkl: [SKIP][93] ([i915#9519]) -> [PASS][94]
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-rkl-7/igt@kms_pm_rpm@dpms-non-lpsp.html
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-1/igt@kms_pm_rpm@dpms-non-lpsp.html
#### Warnings ####
* igt@gem_ctx_freq@sysfs@gt0:
- shard-rkl: [INCOMPLETE][95] -> [ABORT][96] ([i915#9847])
[95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-rkl-4/igt@gem_ctx_freq@sysfs@gt0.html
[96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-5/igt@gem_ctx_freq@sysfs@gt0.html
- shard-dg1: [INCOMPLETE][97] -> [INCOMPLETE][98] ([i915#9855])
[97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-dg1-18/igt@gem_ctx_freq@sysfs@gt0.html
[98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg1-16/igt@gem_ctx_freq@sysfs@gt0.html
* igt@gem_exec_balancer@full-late:
- shard-rkl: [ABORT][99] ([i915#9855] / [i915#9856]) -> [INCOMPLETE][100] ([i915#9856])
[99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-rkl-5/igt@gem_exec_balancer@full-late.html
[100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-7/igt@gem_exec_balancer@full-late.html
* igt@gem_exec_balancer@nop:
- shard-dg1: [ABORT][101] ([i915#9855] / [i915#9856]) -> [INCOMPLETE][102] ([i915#9856])
[101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-dg1-13/igt@gem_exec_balancer@nop.html
[102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg1-19/igt@gem_exec_balancer@nop.html
* igt@gem_exec_balancer@smoke:
- shard-dg2: [ABORT][103] ([i915#9856]) -> [INCOMPLETE][104] ([i915#9856])
[103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-dg2-7/igt@gem_exec_balancer@smoke.html
[104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-6/igt@gem_exec_balancer@smoke.html
* igt@i915_pm_rps@engine-order:
- shard-glk: [INCOMPLETE][105] -> [INCOMPLETE][106] ([i915#9847])
[105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk3/igt@i915_pm_rps@engine-order.html
[106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk7/igt@i915_pm_rps@engine-order.html
* igt@kms_fbcon_fbt@psr:
- shard-rkl: [SKIP][107] ([i915#3955]) -> [SKIP][108] ([fdo#110189] / [i915#3955])
[107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-rkl-7/igt@kms_fbcon_fbt@psr.html
[108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-1/igt@kms_fbcon_fbt@psr.html
* igt@perf_pmu@busy-check-all@rcs0:
- shard-rkl: [INCOMPLETE][109] ([i915#9853]) -> [INCOMPLETE][110] ([i915#9847] / [i915#9853])
[109]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-rkl-1/igt@perf_pmu@busy-check-all@rcs0.html
[110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-7/igt@perf_pmu@busy-check-all@rcs0.html
* igt@perf_pmu@module-unload:
- shard-rkl: [ABORT][111] ([i915#9847] / [i915#9853]) -> [INCOMPLETE][112] ([i915#9853])
[111]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-rkl-5/igt@perf_pmu@module-unload.html
[112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-rkl-7/igt@perf_pmu@module-unload.html
* igt@perf_pmu@most-busy-idle-check-all@rcs0:
- shard-dg2: [ABORT][113] ([i915#9847] / [i915#9853]) -> [INCOMPLETE][114] ([i915#9853])
[113]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-dg2-3/igt@perf_pmu@most-busy-idle-check-all@rcs0.html
[114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg2-10/igt@perf_pmu@most-busy-idle-check-all@rcs0.html
* igt@perf_pmu@semaphore-wait@rcs0:
- shard-dg1: [INCOMPLETE][115] ([i915#9853]) -> [ABORT][116] ([i915#9847] / [i915#9853])
[115]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-dg1-19/igt@perf_pmu@semaphore-wait@rcs0.html
[116]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-dg1-13/igt@perf_pmu@semaphore-wait@rcs0.html
- shard-glk: [ABORT][117] ([i915#9847] / [i915#9853]) -> [INCOMPLETE][118] ([i915#9853])
[117]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14027/shard-glk1/igt@perf_pmu@semaphore-wait@rcs0.html
[118]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/shard-glk3/igt@perf_pmu@semaphore-wait@rcs0.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
[i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
[i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
[i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
[i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
[i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
[i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
[i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
[i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
[i915#7790]: https://gitlab.freedesktop.org/drm/intel/issues/7790
[i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
[i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
[i915#8428]: https://gitlab.freedesktop.org/drm/intel/issues/8428
[i915#8708]: https://gitlab.freedesktop.org/drm/intel/issues/8708
[i915#9423]: https://gitlab.freedesktop.org/drm/intel/issues/9423
[i915#9519]: https://gitlab.freedesktop.org/drm/intel/issues/9519
[i915#9673]: https://gitlab.freedesktop.org/drm/intel/issues/9673
[i915#9683]: https://gitlab.freedesktop.org/drm/intel/issues/9683
[i915#9732]: https://gitlab.freedesktop.org/drm/intel/issues/9732
[i915#9781]: https://gitlab.freedesktop.org/drm/intel/issues/9781
[i915#9847]: https://gitlab.freedesktop.org/drm/intel/issues/9847
[i915#9853]: https://gitlab.freedesktop.org/drm/intel/issues/9853
[i915#9855]: https://gitlab.freedesktop.org/drm/intel/issues/9855
[i915#9856]: https://gitlab.freedesktop.org/drm/intel/issues/9856
[i915#9857]: https://gitlab.freedesktop.org/drm/intel/issues/9857
Build changes
-------------
* Linux: CI_DRM_14027 -> Patchwork_127858v1
* Piglit: None -> piglit_4509
CI-20190529: 20190529
CI_DRM_14027: 961ef418705ac5808345e883acd91f8ce167e00b @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7643: ced22f8bf4263ff395dc852c86b682e62a7a1c1b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_127858v1: 961ef418705ac5808345e883acd91f8ce167e00b @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127858v1/index.html
[-- Attachment #2: Type: text/html, Size: 27401 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] drm/i915/display: C20 clock state verification
2023-12-15 15:10 ` Jani Nikula
@ 2023-12-20 8:05 ` Kahola, Mika
0 siblings, 0 replies; 10+ messages in thread
From: Kahola, Mika @ 2023-12-20 8:05 UTC (permalink / raw)
To: Jani Nikula, Deak, Imre, intel-gfx@lists.freedesktop.org
> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Friday, December 15, 2023 5:10 PM
> To: Kahola, Mika <mika.kahola@intel.com>; Deak, Imre <imre.deak@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/i915/display: C20 clock state verification
>
> On Fri, 15 Dec 2023, "Kahola, Mika" <mika.kahola@intel.com> wrote:
> >> -----Original Message-----
> >> From: Deak, Imre <imre.deak@intel.com>
> >> Sent: Friday, December 15, 2023 11:02 AM
> >> To: Kahola, Mika <mika.kahola@intel.com>;
> >> intel-gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH] drm/i915/display: C20 clock state verification
> >>
> >> On Fri, Dec 15, 2023 at 10:53:36AM +0200, Imre Deak wrote:
> >> > On Fri, Dec 15, 2023 at 10:00:57AM +0200, Mika Kahola wrote:
> >> > > Add clock state verification for C20. Since we are usign either A
> >> > > or B contexts, which are selected based on clock rate, we first
> >> > > need to calculate hw clock and use that clock to select which
> >> > > context we are using.
> >> >
> >> > Could the description be clearer that the patch _fixes_ the context
> >> > selection? (Also the subject line should always say _what_ the
> >> > patch
> >> > does.)
> > OK, should I add the fixes tag as well? I will reword the commit message to better describe what's going on in this patch.
> >
> >> >
> >> > >
> >> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> > > ---
> >> > > drivers/gpu/drm/i915/display/intel_cx0_phy.c | 8 +++++++-
> >> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> > > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> > > index 775c1c4a8978..6757e9f941e4 100644
> >> > > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> > > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> >> > > @@ -3079,8 +3079,9 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state *state,
> >> > > const struct intel_c20pll_state *mpll_sw_state = &state->cx0pll_state.c20;
> >> > > bool use_mplla;
> >> > > int i;
> >> > > + int hw_clock = intel_c20pll_calc_port_clock(encoder,
> >> > > +mpll_hw_state);
> >> > >
> >> > > - use_mplla = intel_c20_use_mplla(mpll_hw_state->clock);
> >> > > + use_mplla = intel_c20_use_mplla(hw_clock);
> >> >
> >> > It's mpll_hw_state->tx[0] C20_PHY_USE_MPLLB which tells the HW
> >> > which context to use, so I think it's better to use the same condition here.
> >
> > Maybe I will ditch the use_mplla completely and go directly with
> >
> > if (mpll_hw_state->tx]0] & C20_PHY_USE_MPLLB) { .. }
> >
> > instead?
>
> You should first verify that the hw and sw states for use_mplla agree.
>
> If they don't, it doesn't matter which one you use.
Right, I will compare these two context selection and throw a note if these two doesn't match.
Thanks!
-Mika-
>
> BR,
> Jani.
>
>
> >
> >>
> >> You could also add a check intel_c20_use_mplla(clock) matches the above flag.
> >>
> >> >
> >> > > if (use_mplla) {
> >> > > for (i = 0; i < ARRAY_SIZE(mpll_sw_state->mplla); i++) {
> >> > > I915_STATE_WARN(i915, mpll_hw_state->mplla[i] !=
> >> > > mpll_sw_state->mplla[i], @@ -3110,6 +3111,11 @@ static void intel_c20pll_state_verify(const struct intel_crtc_state
> *state,
> >> > > crtc->base.base.id, crtc->base.name, i,
> >> > > mpll_sw_state->cmn[i], mpll_hw_state->cmn[i]);
> >> > > }
> >> > > +
> >> > > + I915_STATE_WARN(i915, hw_clock != mpll_sw_state->clock,
> >> > > + "[CRTC:%d:%s] mismatch in C20: Register CLOCK (expected %d, found %d)",
> >> > > + crtc->base.base.id, crtc->base.name,
> >> > > + mpll_sw_state->clock, hw_clock);
> >> >
> >> > I think the intel_crtc_state::port_clock SW/HW state verification
> >> > is equivalent to the above, but it's good to verify it here as
> >> > well. I would store hw_clock to mpll_hw_state->clock in
> >> > intel_c20pll_readout_hw_state() though.
> > Well, clock is part of pll structure anyway, so it might as well be checked here too.
> >
> > I will store hw clock too in intel_c20pll_readout_hw_state()
> >
> > Thanks!
> > Mika
> >
> >> >
> >> > > }
> >> > >
> >> > > void intel_cx0pll_state_verify(struct intel_atomic_state *state,
> >> > > --
> >> > > 2.34.1
> >> > >
>
> --
> Jani Nikula, Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-20 8:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-15 8:00 [PATCH] drm/i915/display: C20 clock state verification Mika Kahola
2023-12-15 8:53 ` Imre Deak
2023-12-15 9:01 ` Imre Deak
2023-12-15 14:00 ` Kahola, Mika
2023-12-15 15:10 ` Jani Nikula
2023-12-20 8:05 ` Kahola, Mika
2023-12-15 9:10 ` Ville Syrjälä
2023-12-15 9:17 ` Imre Deak
2023-12-15 12:49 ` ✓ Fi.CI.BAT: success for " Patchwork
2023-12-15 23:27 ` ✓ Fi.CI.IGT: " Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox