Re: [PATCH 3/4] drm/i915/cdclk: Remove the hardcoded divider from cdclk_compute_crawl_and_squash_midpoint()

From: Gustavo Sousa <gustavo.sousa@intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 3/4] drm/i915/cdclk: Remove the hardcoded divider from cdclk_compute_crawl_and_squash_midpoint()
Date: Fri, 16 Feb 2024 09:16:41 -0300	[thread overview]
Message-ID: <170808580111.10917.5861102353295435557@gjsousa-mobl2> (raw)
In-Reply-To: <20240207013334.29606-4-ville.syrjala@linux.intel.com>

Quoting Ville Syrjala (2024-02-06 22:33:33-03:00)
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>cdclk_compute_crawl_and_squash_midpoint() was still assuming
>that cd2x divider == 1 (ie. full divider == 2). Remove that
>assumption by computing the dividers properly.
>
>We'll also toss in a WARN in case the divider someone ends

s/someone/somehow/ ?

>up different between the old and new cdclk configs. That should
>never happen given we have div==2 in all the cdclk table entries
>for the affected platforms.
>
>If in the future we need a config where the divider also needs
>to be changed then we likely need to add an extra step into the
>cdclk programming sequence to make sure things stay within
>legal limits throughout the process.
>
>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 | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>index a0013e37d53c..ca00586fdbc8 100644
>--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>@@ -1846,7 +1846,7 @@ static bool cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i91
>                                                     struct intel_cdclk_config *mid_cdclk_config)
> {
>         u16 old_waveform, new_waveform, mid_waveform;
>-        int div = 2;
>+        int old_div, new_div, mid_div;
> 
>         /* Return if PLL is in an unknown state, force a complete disable and re-enable. */
>         if (cdclk_pll_is_unknown(old_cdclk_config->vco))
>@@ -1865,6 +1865,18 @@ static bool cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i91
>             old_waveform == new_waveform)
>                 return false;
> 
>+        old_div = cdclk_divider(old_cdclk_config->cdclk,
>+                                old_cdclk_config->vco, old_waveform);
>+        new_div = cdclk_divider(new_cdclk_config->cdclk,
>+                                new_cdclk_config->vco, new_waveform);
>+
>+        /*
>+         * Should not happen currently. We might need more midpoint
>+         * transitions if we need to also change the cd2x divider.
>+         */
>+        if (drm_WARN_ON(&i915->drm, old_div != new_div))
>+                return false;
>+
>         *mid_cdclk_config = *new_cdclk_config;
> 
>         /*
>@@ -1877,15 +1889,17 @@ static bool cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i91
> 
>         if (cdclk_squash_divider(new_waveform) > cdclk_squash_divider(old_waveform)) {
>                 mid_cdclk_config->vco = old_cdclk_config->vco;
>+                mid_div = old_div;
>                 mid_waveform = new_waveform;
>         } else {
>                 mid_cdclk_config->vco = new_cdclk_config->vco;
>+                mid_div = new_div;
>                 mid_waveform = old_waveform;
>         }
> 
>         mid_cdclk_config->cdclk = DIV_ROUND_CLOSEST(cdclk_squash_divider(mid_waveform) *
>                                                     mid_cdclk_config->vco,
>-                                                    cdclk_squash_len * div);
>+                                                    cdclk_squash_len * mid_div);
> 
>         /* make sure the mid clock came out sane */
> 
>-- 
>2.43.0
>