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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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. 

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.

> 
> > 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@intel.com>
> 
> >the assumption and calculate the correct divider from the other
> >parameters.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@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