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 Mon, Dec 04, 2023 at 05:46:28PM -0300, Gustavo Sousa wrote:
> 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@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?

Looks like we've at least attempted to document this:

struct intel_cdclk_vals {
	u32 cdclk;
	u16 refclk;
	u16 waveform;
	u8 divider;     /* CD2X divider * 2 */
	u8 ratio;
};

> 
> >
> >> 
> >> > 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>
> 
> 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!

I can try to tweak it a bit anyway for extra clarity.

And thanks for the review.

> 
> --
> Gustavo Sousa
> 
> >> 
> >> >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

-- 
Ville Syrjälä
Intel