diff mbox series

[01/13] drm/i915/cdclk: Fix CDCLK programming order when pipes are active

Message ID 20240327174544.983-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Implemnt vblank sycnhronized mbus joining changes | expand

Commit Message

Ville Syrjälä March 27, 2024, 5:45 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we always reprogram CDCLK from the
intel_set_cdclk_pre_plane_update() when using squahs/crawl.
The code only works correctly for the cd2x update or full
modeset cases, and it was simply never updated to deal with
squash/crawl.

If the CDCLK frequency is increasing we must reprogram it
before we do anything else that might depend on the new
higher frequency, and conversely we must not decrease
the frequency until everything that might still depend
on the old higher frequency has been dealt with.

Since cdclk_state->pipe is only relevant when doing a cd2x
update we can't use it to determine the correct sequence
during squash/crawl. To that end introduce cdclk_state->disable_pipes
which simply indicates that we must perform the update
while the pipes are disable (ie. during
intel_set_cdclk_pre_plane_update()). Otherwise we use the
same old vs. new CDCLK frequency comparsiong as for cd2x
updates.

The only remaining problem case is when the voltage_level
needs to increase due to a DDI port, but the CDCLK frequency
is decreasing (and not all pipes are being disabled). The
current approach will not bump the voltage level up until
after the port has already been enabled, which is too late.
But we'll take care of that case separately.

v2: Don't break the "must disable pipes case"

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++++------
 drivers/gpu/drm/i915/display/intel_cdclk.h |  3 +++
 2 files changed, 12 insertions(+), 6 deletions(-)

Comments

Murthy, Arun R March 28, 2024, 9:16 a.m. UTC | #1
> -----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 01/13] drm/i915/cdclk: Fix CDCLK programming order when
> pipes are active
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we always reprogram CDCLK from the
> intel_set_cdclk_pre_plane_update() when using squahs/crawl.
Typo squashs->squash

> The code only works correctly for the cd2x update or full modeset cases, and it
> was simply never updated to deal with squash/crawl.
> 
> If the CDCLK frequency is increasing we must reprogram it before we do
> anything else that might depend on the new higher frequency, and conversely
> we must not decrease the frequency until everything that might still depend on
> the old higher frequency has been dealt with.
> 
> Since cdclk_state->pipe is only relevant when doing a cd2x update we can't use
> it to determine the correct sequence during squash/crawl. To that end
> introduce cdclk_state->disable_pipes which simply indicates that we must
> perform the update while the pipes are disable (ie. during
> intel_set_cdclk_pre_plane_update()). Otherwise we use the same old vs. new
> CDCLK frequency comparsiong as for cd2x updates.
> 
> The only remaining problem case is when the voltage_level needs to increase
> due to a DDI port, but the CDCLK frequency is decreasing (and not all pipes are
> being disabled). The current approach will not bump the voltage level up until
> after the port has already been enabled, which is too late.
> But we'll take care of that case separately.
> 
> v2: Don't break the "must disable pipes case"
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++++------
> drivers/gpu/drm/i915/display/intel_cdclk.h |  3 +++
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 31aaa9780dfc..619529dba095 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2600,7 +2600,6 @@ 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);
> -	enum pipe pipe = new_cdclk_state->pipe;
Looks like this cdclk_state->pipe is not more used in the driver and can it be removed?

Thanks and Regards,
Arun R Murthy
--------------------
> 
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual))
> @@ -2609,11 +2608,12 @@ intel_set_cdclk_pre_plane_update(struct
> intel_atomic_state *state)
>  	if (IS_DG2(i915))
>  		intel_cdclk_pcode_pre_notify(state);
> 
> -	if (pipe == INVALID_PIPE ||
> +	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);
> 
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		intel_set_cdclk(i915, &new_cdclk_state->actual,
> +				new_cdclk_state->pipe);
>  	}
>  }
> 
> @@ -2632,7 +2632,6 @@ intel_set_cdclk_post_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);
> -	enum pipe pipe = new_cdclk_state->pipe;
> 
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual))
> @@ -2641,11 +2640,12 @@ intel_set_cdclk_post_plane_update(struct
> intel_atomic_state *state)
>  	if (IS_DG2(i915))
>  		intel_cdclk_pcode_post_notify(state);
> 
> -	if (pipe != INVALID_PIPE &&
> +	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);
> 
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		intel_set_cdclk(i915, &new_cdclk_state->actual,
> +				new_cdclk_state->pipe);
>  	}
>  }
> 
> @@ -3124,6 +3124,7 @@ static struct intel_global_state
> *intel_cdclk_duplicate_state(struct intel_globa
>  		return NULL;
> 
>  	cdclk_state->pipe = INVALID_PIPE;
> +	cdclk_state->disable_pipes = false;
> 
>  	return &cdclk_state->base;
>  }
> @@ -3316,6 +3317,8 @@ int intel_modeset_calc_cdclk(struct
> intel_atomic_state *state)
>  		if (ret)
>  			return ret;
> 
> +		new_cdclk_state->disable_pipes = true;
> +
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "Modeset required for cdclk change\n");
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h
> b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index bc8f86e292d8..2843fc091086 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -53,6 +53,9 @@ struct intel_cdclk_state {
> 
>  	/* bitmask of active pipes */
>  	u8 active_pipes;
> +
> +	/* update cdclk with pipes disabled */
> +	bool disable_pipes;
>  };
> 
>  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
> --
> 2.43.2
Shankar, Uma March 28, 2024, 11:35 a.m. UTC | #2
> -----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 01/13] drm/i915/cdclk: Fix CDCLK programming order when
> pipes are active
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we always reprogram CDCLK from the
> intel_set_cdclk_pre_plane_update() when using squahs/crawl.

Nitpick: Typo in squash

Change Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> The code only works correctly for the cd2x update or full modeset cases, and it
> was simply never updated to deal with squash/crawl.
> 
> If the CDCLK frequency is increasing we must reprogram it before we do anything
> else that might depend on the new higher frequency, and conversely we must not
> decrease the frequency until everything that might still depend on the old higher
> frequency has been dealt with.
> 
> Since cdclk_state->pipe is only relevant when doing a cd2x update we can't use it
> to determine the correct sequence during squash/crawl. To that end introduce
> cdclk_state->disable_pipes which simply indicates that we must perform the
> update while the pipes are disable (ie. during
> intel_set_cdclk_pre_plane_update()). Otherwise we use the same old vs. new
> CDCLK frequency comparsiong as for cd2x updates.
> 
> The only remaining problem case is when the voltage_level needs to increase due
> to a DDI port, but the CDCLK frequency is decreasing (and not all pipes are being
> disabled). The current approach will not bump the voltage level up until after the
> port has already been enabled, which is too late.
> But we'll take care of that case separately.
> 
> v2: Don't break the "must disable pipes case"
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++++------
> drivers/gpu/drm/i915/display/intel_cdclk.h |  3 +++
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 31aaa9780dfc..619529dba095 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -2600,7 +2600,6 @@ 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);
> -	enum pipe pipe = new_cdclk_state->pipe;
> 
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual))
> @@ -2609,11 +2608,12 @@ intel_set_cdclk_pre_plane_update(struct
> intel_atomic_state *state)
>  	if (IS_DG2(i915))
>  		intel_cdclk_pcode_pre_notify(state);
> 
> -	if (pipe == INVALID_PIPE ||
> +	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);
> 
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		intel_set_cdclk(i915, &new_cdclk_state->actual,
> +				new_cdclk_state->pipe);
>  	}
>  }
> 
> @@ -2632,7 +2632,6 @@ intel_set_cdclk_post_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);
> -	enum pipe pipe = new_cdclk_state->pipe;
> 
>  	if (!intel_cdclk_changed(&old_cdclk_state->actual,
>  				 &new_cdclk_state->actual))
> @@ -2641,11 +2640,12 @@ intel_set_cdclk_post_plane_update(struct
> intel_atomic_state *state)
>  	if (IS_DG2(i915))
>  		intel_cdclk_pcode_post_notify(state);
> 
> -	if (pipe != INVALID_PIPE &&
> +	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);
> 
> -		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> +		intel_set_cdclk(i915, &new_cdclk_state->actual,
> +				new_cdclk_state->pipe);
>  	}
>  }
> 
> @@ -3124,6 +3124,7 @@ static struct intel_global_state
> *intel_cdclk_duplicate_state(struct intel_globa
>  		return NULL;
> 
>  	cdclk_state->pipe = INVALID_PIPE;
> +	cdclk_state->disable_pipes = false;
> 
>  	return &cdclk_state->base;
>  }
> @@ -3316,6 +3317,8 @@ int intel_modeset_calc_cdclk(struct
> intel_atomic_state *state)
>  		if (ret)
>  			return ret;
> 
> +		new_cdclk_state->disable_pipes = true;
> +
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "Modeset required for cdclk change\n");
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h
> b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index bc8f86e292d8..2843fc091086 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -53,6 +53,9 @@ struct intel_cdclk_state {
> 
>  	/* bitmask of active pipes */
>  	u8 active_pipes;
> +
> +	/* update cdclk with pipes disabled */
> +	bool disable_pipes;
>  };
> 
>  int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
> --
> 2.43.2
Ville Syrjälä March 28, 2024, 12:32 p.m. UTC | #3
On Thu, Mar 28, 2024 at 09:16:06AM +0000, Murthy, Arun R wrote:
> 
> > -----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 01/13] drm/i915/cdclk: Fix CDCLK programming order when
> > pipes are active
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we always reprogram CDCLK from the
> > intel_set_cdclk_pre_plane_update() when using squahs/crawl.
> Typo squashs->squash
> 
> > The code only works correctly for the cd2x update or full modeset cases, and it
> > was simply never updated to deal with squash/crawl.
> > 
> > If the CDCLK frequency is increasing we must reprogram it before we do
> > anything else that might depend on the new higher frequency, and conversely
> > we must not decrease the frequency until everything that might still depend on
> > the old higher frequency has been dealt with.
> > 
> > Since cdclk_state->pipe is only relevant when doing a cd2x update we can't use
> > it to determine the correct sequence during squash/crawl. To that end
> > introduce cdclk_state->disable_pipes which simply indicates that we must
> > perform the update while the pipes are disable (ie. during
> > intel_set_cdclk_pre_plane_update()). Otherwise we use the same old vs. new
> > CDCLK frequency comparsiong as for cd2x updates.
> > 
> > The only remaining problem case is when the voltage_level needs to increase
> > due to a DDI port, but the CDCLK frequency is decreasing (and not all pipes are
> > being disabled). The current approach will not bump the voltage level up until
> > after the port has already been enabled, which is too late.
> > But we'll take care of that case separately.
> > 
> > v2: Don't break the "must disable pipes case"
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++++------
> > drivers/gpu/drm/i915/display/intel_cdclk.h |  3 +++
> >  2 files changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 31aaa9780dfc..619529dba095 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2600,7 +2600,6 @@ 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);
> > -	enum pipe pipe = new_cdclk_state->pipe;
> Looks like this cdclk_state->pipe is not more used in the driver and can it be removed?

It is still used for its primary purpose (cd2x update pipe select).

The only thing changing here is that we no longer use it as a
canary to indicate whether we need to do the cdclk programming
with pipes off or not.
Gustavo Sousa March 29, 2024, 3:29 p.m. UTC | #4
Quoting Ville Syrjala (2024-03-27 14:45:32-03:00)
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>Currently we always reprogram CDCLK from the
>intel_set_cdclk_pre_plane_update() when using squahs/crawl.
>The code only works correctly for the cd2x update or full
>modeset cases, and it was simply never updated to deal with
>squash/crawl.
>
>If the CDCLK frequency is increasing we must reprogram it
>before we do anything else that might depend on the new
>higher frequency, and conversely we must not decrease
>the frequency until everything that might still depend
>on the old higher frequency has been dealt with.
>
>Since cdclk_state->pipe is only relevant when doing a cd2x
>update we can't use it to determine the correct sequence
>during squash/crawl. To that end introduce cdclk_state->disable_pipes
>which simply indicates that we must perform the update
>while the pipes are disable (ie. during
>intel_set_cdclk_pre_plane_update()). Otherwise we use the
>same old vs. new CDCLK frequency comparsiong as for cd2x
>updates.
>
>The only remaining problem case is when the voltage_level
>needs to increase due to a DDI port, but the CDCLK frequency
>is decreasing (and not all pipes are being disabled). The
>current approach will not bump the voltage level up until
>after the port has already been enabled, which is too late.
>But we'll take care of that case separately.

Yep. Maybe that's another reason to have that logic detached from the
cdclk sequence in the future?

Another one mentioned in an earlier discussion[1] would be the case
where voltage level changes without changes to CDCLK.

[1] https://lore.kernel.org/intel-gfx/Zc0dygncPPX_pqIi@intel.com/

>
>v2: Don't break the "must disable pipes case"
>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

>---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 15 +++++++++------
> drivers/gpu/drm/i915/display/intel_cdclk.h |  3 +++
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>index 31aaa9780dfc..619529dba095 100644
>--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>@@ -2600,7 +2600,6 @@ 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);
>-        enum pipe pipe = new_cdclk_state->pipe;
> 
>         if (!intel_cdclk_changed(&old_cdclk_state->actual,
>                                  &new_cdclk_state->actual))
>@@ -2609,11 +2608,12 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>         if (IS_DG2(i915))
>                 intel_cdclk_pcode_pre_notify(state);
> 
>-        if (pipe == INVALID_PIPE ||
>+        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);
> 
>-                intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
>+                intel_set_cdclk(i915, &new_cdclk_state->actual,
>+                                new_cdclk_state->pipe);
>         }
> }
> 
>@@ -2632,7 +2632,6 @@ intel_set_cdclk_post_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);
>-        enum pipe pipe = new_cdclk_state->pipe;
> 
>         if (!intel_cdclk_changed(&old_cdclk_state->actual,
>                                  &new_cdclk_state->actual))
>@@ -2641,11 +2640,12 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>         if (IS_DG2(i915))
>                 intel_cdclk_pcode_post_notify(state);
> 
>-        if (pipe != INVALID_PIPE &&
>+        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);
> 
>-                intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
>+                intel_set_cdclk(i915, &new_cdclk_state->actual,
>+                                new_cdclk_state->pipe);
>         }
> }
> 
>@@ -3124,6 +3124,7 @@ static struct intel_global_state *intel_cdclk_duplicate_state(struct intel_globa
>                 return NULL;
> 
>         cdclk_state->pipe = INVALID_PIPE;
>+        cdclk_state->disable_pipes = false;
> 
>         return &cdclk_state->base;
> }
>@@ -3316,6 +3317,8 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
>                 if (ret)
>                         return ret;
> 
>+                new_cdclk_state->disable_pipes = true;
>+
>                 drm_dbg_kms(&dev_priv->drm,
>                             "Modeset required for cdclk change\n");
>         }
>diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
>index bc8f86e292d8..2843fc091086 100644
>--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
>+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
>@@ -53,6 +53,9 @@ struct intel_cdclk_state {
> 
>         /* bitmask of active pipes */
>         u8 active_pipes;
>+
>+        /* update cdclk with pipes disabled */
>+        bool disable_pipes;
> };
> 
> int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
>-- 
>2.43.2
>
Ville Syrjälä April 3, 2024, 3:51 p.m. UTC | #5
On Fri, Mar 29, 2024 at 12:29:09PM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjala (2024-03-27 14:45:32-03:00)
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Currently we always reprogram CDCLK from the
> >intel_set_cdclk_pre_plane_update() when using squahs/crawl.
> >The code only works correctly for the cd2x update or full
> >modeset cases, and it was simply never updated to deal with
> >squash/crawl.
> >
> >If the CDCLK frequency is increasing we must reprogram it
> >before we do anything else that might depend on the new
> >higher frequency, and conversely we must not decrease
> >the frequency until everything that might still depend
> >on the old higher frequency has been dealt with.
> >
> >Since cdclk_state->pipe is only relevant when doing a cd2x
> >update we can't use it to determine the correct sequence
> >during squash/crawl. To that end introduce cdclk_state->disable_pipes
> >which simply indicates that we must perform the update
> >while the pipes are disable (ie. during
> >intel_set_cdclk_pre_plane_update()). Otherwise we use the
> >same old vs. new CDCLK frequency comparsiong as for cd2x
> >updates.
> >
> >The only remaining problem case is when the voltage_level
> >needs to increase due to a DDI port, but the CDCLK frequency
> >is decreasing (and not all pipes are being disabled). The
> >current approach will not bump the voltage level up until
> >after the port has already been enabled, which is too late.
> >But we'll take care of that case separately.
> 
> Yep. Maybe that's another reason to have that logic detached from the
> cdclk sequence in the future?

Perhaps.

The cdclk sequence is typically specified as
1. request max voltage
2. change cdclk
3. request final voltage
 
We don't actually know whether step 1 has any other side effects
beyond changing the voltage. Eg. it might also do some other magic
to prepare the hardware/firmware for the cdclk change, and so we
might not be able to decouple it from the cdclk sequence 100%.
One solution could be to bump the voltage to the max in the pre
plane voltage hook whenever cdclk is also changing.

We would definitely end up spreading the voltage requests further
out from the actual cdclk programming (they'd have to be the
outermost pre/post plane hooks), which may or may not have some
other side effects as well.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 31aaa9780dfc..619529dba095 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -2600,7 +2600,6 @@  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);
-	enum pipe pipe = new_cdclk_state->pipe;
 
 	if (!intel_cdclk_changed(&old_cdclk_state->actual,
 				 &new_cdclk_state->actual))
@@ -2609,11 +2608,12 @@  intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
 	if (IS_DG2(i915))
 		intel_cdclk_pcode_pre_notify(state);
 
-	if (pipe == INVALID_PIPE ||
+	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);
 
-		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
+		intel_set_cdclk(i915, &new_cdclk_state->actual,
+				new_cdclk_state->pipe);
 	}
 }
 
@@ -2632,7 +2632,6 @@  intel_set_cdclk_post_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);
-	enum pipe pipe = new_cdclk_state->pipe;
 
 	if (!intel_cdclk_changed(&old_cdclk_state->actual,
 				 &new_cdclk_state->actual))
@@ -2641,11 +2640,12 @@  intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
 	if (IS_DG2(i915))
 		intel_cdclk_pcode_post_notify(state);
 
-	if (pipe != INVALID_PIPE &&
+	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);
 
-		intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
+		intel_set_cdclk(i915, &new_cdclk_state->actual,
+				new_cdclk_state->pipe);
 	}
 }
 
@@ -3124,6 +3124,7 @@  static struct intel_global_state *intel_cdclk_duplicate_state(struct intel_globa
 		return NULL;
 
 	cdclk_state->pipe = INVALID_PIPE;
+	cdclk_state->disable_pipes = false;
 
 	return &cdclk_state->base;
 }
@@ -3316,6 +3317,8 @@  int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
 		if (ret)
 			return ret;
 
+		new_cdclk_state->disable_pipes = true;
+
 		drm_dbg_kms(&dev_priv->drm,
 			    "Modeset required for cdclk change\n");
 	}
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index bc8f86e292d8..2843fc091086 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -53,6 +53,9 @@  struct intel_cdclk_state {
 
 	/* bitmask of active pipes */
 	u8 active_pipes;
+
+	/* update cdclk with pipes disabled */
+	bool disable_pipes;
 };
 
 int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);