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

From: Gustavo Sousa <gustavo.sousa@intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 01/13] drm/i915/cdclk: Fix CDCLK programming order when pipes are active
Date: Fri, 29 Mar 2024 12:29:09 -0300	[thread overview]
Message-ID: <171172614955.2604.11177523422567223748@gjsousa-mobl2> (raw)
In-Reply-To: <20240327174544.983-2-ville.syrjala@linux.intel.com>

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
>