| Message ID | 20240327174544.983-3-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Implemnt vblank sycnhronized mbus joining changes | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville > Syrjala > Sent: Wednesday, March 27, 2024 11:16 PM > To: intel-gfx@lists.freedesktop.org > Subject: [PATCH 02/13] drm/i915/cdclk: Fix voltage_level programming edge case > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently we only consider the relationship of the old and new CDCLK frequencies > when determining whether to do the repgramming from > intel_set_cdclk_pre_plane_update() > or intel_set_cdclk_post_plane_update(). > > It is technically possible to have a situation where the CDCLK frequency is > decreasing, but the voltage_level is increasing due a DDI port. In this case we > should bump the voltage level already in intel_set_cdclk_pre_plane_update() > (so that the voltage_level will have been increased by the time the port gets > enabled), while leaving the CDCLK frequency unchanged (as active planes/etc. > may still depend on it). > We can then reduce the CDCLK frequency to its final value from > intel_set_cdclk_post_plane_update(). > > In order to handle that correctly we shall construct a suitable amalgam of the old > and new cdclk states in intel_set_cdclk_pre_plane_update(). > > And we can simply call intel_set_cdclk() unconditionally in both places as it will > not do anything if nothing actually changes vs. the current hw state. > > v2: Handle cdclk_state->disable_pipes Looks Good to me. Reviewed-by: Uma Shankar <uma.shankar@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 27 +++++++++++++--------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c > b/drivers/gpu/drm/i915/display/intel_cdclk.c > index 619529dba095..504c5cbbcfff 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -2600,6 +2600,7 @@ intel_set_cdclk_pre_plane_update(struct > intel_atomic_state *state) > intel_atomic_get_old_cdclk_state(state); > const struct intel_cdclk_state *new_cdclk_state = > intel_atomic_get_new_cdclk_state(state); > + struct intel_cdclk_config cdclk_config; > > if (!intel_cdclk_changed(&old_cdclk_state->actual, > &new_cdclk_state->actual)) > @@ -2608,13 +2609,21 @@ intel_set_cdclk_pre_plane_update(struct > intel_atomic_state *state) > if (IS_DG2(i915)) > intel_cdclk_pcode_pre_notify(state); > > - if (new_cdclk_state->disable_pipes || > - old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) { > - drm_WARN_ON(&i915->drm, !new_cdclk_state- > >base.changed); > + if (new_cdclk_state->disable_pipes) { > + cdclk_config = new_cdclk_state->actual; > + } else { > + if (new_cdclk_state->actual.cdclk >= old_cdclk_state- > >actual.cdclk) > + cdclk_config = new_cdclk_state->actual; > + else > + cdclk_config = old_cdclk_state->actual; > > - intel_set_cdclk(i915, &new_cdclk_state->actual, > - new_cdclk_state->pipe); > + cdclk_config.voltage_level = max(new_cdclk_state- > >actual.voltage_level, > + old_cdclk_state- > >actual.voltage_level); > } > + > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); > + > + intel_set_cdclk(i915, &cdclk_config, new_cdclk_state->pipe); > } > > /** > @@ -2640,13 +2649,9 @@ intel_set_cdclk_post_plane_update(struct > intel_atomic_state *state) > if (IS_DG2(i915)) > intel_cdclk_pcode_post_notify(state); > > - if (!new_cdclk_state->disable_pipes && > - old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) { > - drm_WARN_ON(&i915->drm, !new_cdclk_state- > >base.changed); > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); > > - intel_set_cdclk(i915, &new_cdclk_state->actual, > - new_cdclk_state->pipe); > - } > + intel_set_cdclk(i915, &new_cdclk_state->actual, > +new_cdclk_state->pipe); > } > > static int intel_pixel_rate_to_cdclk(const struct intel_crtc_state *crtc_state) > -- > 2.43.2
Quoting Ville Syrjala (2024-03-27 14:45:33-03:00) >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >Currently we only consider the relationship of the >old and new CDCLK frequencies when determining whether >to do the repgramming from intel_set_cdclk_pre_plane_update() >or intel_set_cdclk_post_plane_update(). > >It is technically possible to have a situation where the >CDCLK frequency is decreasing, but the voltage_level is >increasing due a DDI port. In this case we should bump >the voltage level already in intel_set_cdclk_pre_plane_update() >(so that the voltage_level will have been increased by the >time the port gets enabled), while leaving the CDCLK frequency >unchanged (as active planes/etc. may still depend on it). >We can then reduce the CDCLK frequency to its final value >from intel_set_cdclk_post_plane_update(). > >In order to handle that correctly we shall construct a >suitable amalgam of the old and new cdclk states in >intel_set_cdclk_pre_plane_update(). > >And we can simply call intel_set_cdclk() unconditionally >in both places as it will not do anything if nothing actually >changes vs. the current hw state. > >v2: Handle cdclk_state->disable_pipes > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >--- > drivers/gpu/drm/i915/display/intel_cdclk.c | 27 +++++++++++++--------- > 1 file changed, 16 insertions(+), 11 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c >index 619529dba095..504c5cbbcfff 100644 >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >@@ -2600,6 +2600,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state) > intel_atomic_get_old_cdclk_state(state); > const struct intel_cdclk_state *new_cdclk_state = > intel_atomic_get_new_cdclk_state(state); >+ struct intel_cdclk_config cdclk_config; > > if (!intel_cdclk_changed(&old_cdclk_state->actual, > &new_cdclk_state->actual)) >@@ -2608,13 +2609,21 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state) > if (IS_DG2(i915)) > intel_cdclk_pcode_pre_notify(state); > >- if (new_cdclk_state->disable_pipes || >- old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) { >- drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); >+ if (new_cdclk_state->disable_pipes) { >+ cdclk_config = new_cdclk_state->actual; >+ } else { >+ if (new_cdclk_state->actual.cdclk >= old_cdclk_state->actual.cdclk) >+ cdclk_config = new_cdclk_state->actual; >+ else >+ cdclk_config = old_cdclk_state->actual; > >- intel_set_cdclk(i915, &new_cdclk_state->actual, >- new_cdclk_state->pipe); >+ cdclk_config.voltage_level = max(new_cdclk_state->actual.voltage_level, >+ old_cdclk_state->actual.voltage_level); > } >+ >+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); >+ >+ intel_set_cdclk(i915, &cdclk_config, new_cdclk_state->pipe); Not sure if there could be unwanted side effects with passing new_cdclk_state->pipe when using old_cdclk_state->actual. Because voltage_level might have changed, parts of the cdclk change sequence end up being exercised even when cdclk_config == old_cdclk_state->actual. Well, even if those side effects might be harmless, I wonder if it would be better if we used INVALID_PIPE when using old_cdclk_state->actual. -- Gustavo Sousa > } > > /** >@@ -2640,13 +2649,9 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state) > if (IS_DG2(i915)) > intel_cdclk_pcode_post_notify(state); > >- if (!new_cdclk_state->disable_pipes && >- old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) { >- drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); >+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); > >- intel_set_cdclk(i915, &new_cdclk_state->actual, >- new_cdclk_state->pipe); >- } >+ intel_set_cdclk(i915, &new_cdclk_state->actual, new_cdclk_state->pipe); > } > > static int intel_pixel_rate_to_cdclk(const struct intel_crtc_state *crtc_state) >-- >2.43.2 >
On Fri, Mar 29, 2024 at 02:04:55PM -0300, Gustavo Sousa wrote: > Quoting Ville Syrjala (2024-03-27 14:45:33-03:00) > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >Currently we only consider the relationship of the > >old and new CDCLK frequencies when determining whether > >to do the repgramming from intel_set_cdclk_pre_plane_update() > >or intel_set_cdclk_post_plane_update(). > > > >It is technically possible to have a situation where the > >CDCLK frequency is decreasing, but the voltage_level is > >increasing due a DDI port. In this case we should bump > >the voltage level already in intel_set_cdclk_pre_plane_update() > >(so that the voltage_level will have been increased by the > >time the port gets enabled), while leaving the CDCLK frequency > >unchanged (as active planes/etc. may still depend on it). > >We can then reduce the CDCLK frequency to its final value > >from intel_set_cdclk_post_plane_update(). > > > >In order to handle that correctly we shall construct a > >suitable amalgam of the old and new cdclk states in > >intel_set_cdclk_pre_plane_update(). > > > >And we can simply call intel_set_cdclk() unconditionally > >in both places as it will not do anything if nothing actually > >changes vs. the current hw state. > > > >v2: Handle cdclk_state->disable_pipes > > > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >--- > > drivers/gpu/drm/i915/display/intel_cdclk.c | 27 +++++++++++++--------- > > 1 file changed, 16 insertions(+), 11 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > >index 619529dba095..504c5cbbcfff 100644 > >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c > >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > >@@ -2600,6 +2600,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state) > > intel_atomic_get_old_cdclk_state(state); > > const struct intel_cdclk_state *new_cdclk_state = > > intel_atomic_get_new_cdclk_state(state); > >+ struct intel_cdclk_config cdclk_config; > > > > if (!intel_cdclk_changed(&old_cdclk_state->actual, > > &new_cdclk_state->actual)) > >@@ -2608,13 +2609,21 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state) > > if (IS_DG2(i915)) > > intel_cdclk_pcode_pre_notify(state); > > > >- if (new_cdclk_state->disable_pipes || > >- old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) { > >- drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); > >+ if (new_cdclk_state->disable_pipes) { > >+ cdclk_config = new_cdclk_state->actual; > >+ } else { > >+ if (new_cdclk_state->actual.cdclk >= old_cdclk_state->actual.cdclk) > >+ cdclk_config = new_cdclk_state->actual; > >+ else > >+ cdclk_config = old_cdclk_state->actual; > > > >- intel_set_cdclk(i915, &new_cdclk_state->actual, > >- new_cdclk_state->pipe); > >+ cdclk_config.voltage_level = max(new_cdclk_state->actual.voltage_level, > >+ old_cdclk_state->actual.voltage_level); > > } > >+ > >+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); > >+ > >+ intel_set_cdclk(i915, &cdclk_config, new_cdclk_state->pipe); > > Not sure if there could be unwanted side effects with passing > new_cdclk_state->pipe when using old_cdclk_state->actual. Because > voltage_level might have changed, parts of the cdclk change sequence end > up being exercised even when cdclk_config == old_cdclk_state->actual. > > Well, even if those side effects might be harmless, I wonder if it would > be better if we used INVALID_PIPE when using old_cdclk_state->actual. Yeah. I doubt there should be any really bad side effects, but probably a good idea to sidestep the whole question by passing in INVALID_PIPE.
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index 619529dba095..504c5cbbcfff 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -2600,6 +2600,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state) intel_atomic_get_old_cdclk_state(state); const struct intel_cdclk_state *new_cdclk_state = intel_atomic_get_new_cdclk_state(state); + struct intel_cdclk_config cdclk_config; if (!intel_cdclk_changed(&old_cdclk_state->actual, &new_cdclk_state->actual)) @@ -2608,13 +2609,21 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state) if (IS_DG2(i915)) intel_cdclk_pcode_pre_notify(state); - if (new_cdclk_state->disable_pipes || - old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) { - drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); + if (new_cdclk_state->disable_pipes) { + cdclk_config = new_cdclk_state->actual; + } else { + if (new_cdclk_state->actual.cdclk >= old_cdclk_state->actual.cdclk) + cdclk_config = new_cdclk_state->actual; + else + cdclk_config = old_cdclk_state->actual; - intel_set_cdclk(i915, &new_cdclk_state->actual, - new_cdclk_state->pipe); + cdclk_config.voltage_level = max(new_cdclk_state->actual.voltage_level, + old_cdclk_state->actual.voltage_level); } + + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); + + intel_set_cdclk(i915, &cdclk_config, new_cdclk_state->pipe); } /** @@ -2640,13 +2649,9 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state) if (IS_DG2(i915)) intel_cdclk_pcode_post_notify(state); - if (!new_cdclk_state->disable_pipes && - old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) { - drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed); - intel_set_cdclk(i915, &new_cdclk_state->actual, - new_cdclk_state->pipe); - } + intel_set_cdclk(i915, &new_cdclk_state->actual, new_cdclk_state->pipe); } static int intel_pixel_rate_to_cdclk(const struct intel_crtc_state *crtc_state)