[Intel-gfx] [PATCH 3/8] drm/i915/cdclk: Remove the assumption that cd2x div==2 when using squashing

[Gustavo Sousa]

Quoting Ville Syrjälä (2023-12-04 17:05:46-03:00)
>On Fri, Dec 01, 2023 at 05:13:38PM -0300, Gustavo Sousa wrote:
>> Quoting Ville Syrjala (2023-11-28 08:51:33-03:00)
>> >From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >
>> >Currently we have a hardcoded assumption that the cd2x divider
>> >is always 2 when squahsing is used.
>> 
>> It seems the code was actually making the assumption that the
>> divider is always 1. With the current code, when squashing is used, we
>> have that bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) is equivalent to
>> bxt_cdclk_cd2x_div_sel(dev_priv, vco / 2, vco), meaning that the
>> returned value will always be BXT_CDCLK_CD2X_DIV_SEL_1.
>
>The real cd2x divider is half of our 'divider' because we
>specify the full vco->cdclk divider instead of the vco->cd2xclk
>divider. 

Got it.

>
>Alternatively you can think of it as being the cd2x divider
>specified in .1 binary fixed point format.
>
>But yeah, saying "cd2x divider is 2" probably isn't the best
>way to put this.

Yeah, maybe because of my little time with the driver code, but I had
interpreted it as the one used right after the PLL output (I'm taking
the diagram from MTL specs as reference).

Did I miss some comment in the code explaning that? Should one be added?

>
>> 
>> > While that is true for all
>> >current platforms it might not hold in the future. So eliminate
>> 
>> Not sure about the other platforms, but for MTL, I have checked the
>> BSpec table and also did some calculations by hand, and it seems like
>> the cd2x divider is actually always 1.
>> 
>> Unless I'm missing something in my analysis above, I believe s/2/1/ in
>> the commit message is necessary. With that,
>> 
>> Reviewed-by: Gustavo Sousa <gustavo.sousa at intel.com>

Considering that the above understanding about the divider is the common
sense, the r-b stands without the tweaks in the commit messages. Thanks
for the clarification!

--
Gustavo Sousa

>> 
>> >the assumption and calculate the correct divider from the other
>> >parameters.
>> >
>> >Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> >---
>> > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 ++----
>> > 1 file changed, 2 insertions(+), 4 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >index 87d5e5b67c4e..d45071675629 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >@@ -1899,10 +1899,8 @@ static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
>> > 
>> >         waveform = cdclk_squash_waveform(dev_priv, cdclk);
>> > 
>> >-        if (waveform)
>> >-                clock = vco / 2;
>> >-        else
>> >-                clock = cdclk;
>> >+        clock = DIV_ROUND_CLOSEST(cdclk * cdclk_squash_len,
>> >+                                  cdclk_squash_divider(waveform));
>> 
>> Nit: maybe take the opportunity to rename "clock" to "unsquashed" for
>> clarity?
>> 
>> > 
>> >         if (HAS_CDCLK_SQUASH(dev_priv))
>> >                 dg2_cdclk_squash_program(dev_priv, waveform);
>> >-- 
>> >2.41.0
>> >
>
>-- 
>Ville Syrjälä
>Intel