On Thu, Sep 07, 2023 at 02:52:06PM -0700, Matt Roper wrote:
> On Thu, Sep 07, 2023 at 08:37:52AM -0700, Lucas De Marchi wrote:
> > From: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> >
> > Add a new Lunar Lake CDCLK table from BSpec and also a helper function
> > in order to be able to find lowest possible CDCLK.
> >
> > v2:
> > - Remove mdclk from the table as it's not needed (Matt Roper)
> > - Update waveform values to the latest from spec (Matt Roper)
> > - Rename functions and calculation to match by pixel rate (Lucas)
> >
> > Bspec: 68861
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 55 +++++++++++++++++++++-
> > 1 file changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index cfd01050f7f1..7307af2a4af5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1382,6 +1382,31 @@ static const struct intel_cdclk_vals mtl_cdclk_table[] = {
> > {}
> > };
> >
> > +static const struct intel_cdclk_vals lnl_cdclk_table[] = {
> > + { .refclk = 38400, .cdclk = 153600, .divider = 2, .ratio = 16, .waveform = 0xaaaa },
> > + { .refclk = 38400, .cdclk = 172800, .divider = 2, .ratio = 16, .waveform = 0xad5a },
> > + { .refclk = 38400, .cdclk = 192000, .divider = 2, .ratio = 16, .waveform = 0xb6b6 },
> > + { .refclk = 38400, .cdclk = 211200, .divider = 2, .ratio = 16, .waveform = 0xdbb6 },
> > + { .refclk = 38400, .cdclk = 230400, .divider = 2, .ratio = 16, .waveform = 0xeeee },
> > + { .refclk = 38400, .cdclk = 249600, .divider = 2, .ratio = 16, .waveform = 0xf7de },
> > + { .refclk = 38400, .cdclk = 268800, .divider = 2, .ratio = 16, .waveform = 0xfefe },
> > + { .refclk = 38400, .cdclk = 288000, .divider = 2, .ratio = 16, .waveform = 0xfffe },
> > + { .refclk = 38400, .cdclk = 307200, .divider = 2, .ratio = 16, .waveform = 0xffff },
> > + { .refclk = 38400, .cdclk = 330000, .divider = 2, .ratio = 25, .waveform = 0xdbb6 },
> > + { .refclk = 38400, .cdclk = 360000, .divider = 2, .ratio = 25, .waveform = 0xeeee },
> > + { .refclk = 38400, .cdclk = 390000, .divider = 2, .ratio = 25, .waveform = 0xf7de },
> > + { .refclk = 38400, .cdclk = 420000, .divider = 2, .ratio = 25, .waveform = 0xfefe },
> > + { .refclk = 38400, .cdclk = 450000, .divider = 2, .ratio = 25, .waveform = 0xfffe },
> > + { .refclk = 38400, .cdclk = 480000, .divider = 2, .ratio = 25, .waveform = 0xffff },
> > + { .refclk = 38400, .cdclk = 487200, .divider = 2, .ratio = 29, .waveform = 0xfefe },
> > + { .refclk = 38400, .cdclk = 522000, .divider = 2, .ratio = 29, .waveform = 0xfffe },
> > + { .refclk = 38400, .cdclk = 556800, .divider = 2, .ratio = 29, .waveform = 0xffff },
> > + { .refclk = 38400, .cdclk = 571200, .divider = 2, .ratio = 34, .waveform = 0xfefe },
> > + { .refclk = 38400, .cdclk = 612000, .divider = 2, .ratio = 34, .waveform = 0xfffe },
> > + { .refclk = 38400, .cdclk = 652800, .divider = 2, .ratio = 34, .waveform = 0xffff },
> > + {}
> > +};
> > +
> > static int bxt_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
> > {
> > const struct intel_cdclk_vals *table = dev_priv->display.cdclk.table;
> > @@ -2504,12 +2529,35 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> > }
> > }
> >
> > +static int
> > +xe2lpd_cdclk_match_by_pixel_rate(struct drm_i915_private *i915, int pixel_rate)
> > +{
> > + const struct intel_cdclk_vals *table = i915->display.cdclk.table;
> > + int i;
> > +
> > + for (i = 0; table[i].refclk; i++) {
> > + if (table[i].refclk != i915->display.cdclk.hw.ref)
> > + continue;
> > +
> > + if (table[i].refclk * table[i].ratio >= pixel_rate)
> > + return table[i].cdclk;
> > + }
> > +
> > + drm_WARN(&i915->drm, 1,
> > + "Cannot satisfy pixel rate %d with refclk %u\n",
> > + pixel_rate, i915->display.cdclk.hw.ref);
> > +
> > + return 0;
> > +}
> > +
> > static int intel_pixel_rate_to_cdclk(const struct intel_crtc_state *crtc_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> > int pixel_rate = crtc_state->pixel_rate;
> >
> > - if (DISPLAY_VER(dev_priv) >= 10)
> > + if (DISPLAY_VER(dev_priv) >= 20)
> > + return xe2lpd_cdclk_match_by_pixel_rate(dev_priv, pixel_rate);
>
> Is this actually what we want to be doing? Generally this function
> returns a minimum possible value cdclk value that could support the
> pixel rate (e.g., pixel_rate / 2 on modern platforms), without caring
> (yet) about the set of cdclk values that the platform is capable of
> generating. We then do some additional CRTC-level or device-level
> adjustments to that minimum in intel_crtc_compute_min_cdclk and
> intel_compute_min_cdclk, and only at the very end of the process (in
> bxt_calc_cdclk) do we go into the table to find the lowest possible
> clock greater than or equal to the adjusted minimum we had calculated.
>
> From what I can see, the minimum cdclk (as far as this specific function
> is concerned) should still be half the pixel rate on Xe2 (bspec 68858:
> "Pipe maximum pixel rate = 2 * CDCLK frequency * Pipe Ratio"). The only
> thing that really changes with all the mdclk stuff is that cdclk and
> mdclk are no longer fundamentally locked together in hardware; that's
> why the table of possible cdclk settings in the bspec now has additional
> rows where the cdclk value ranges anywhere from mdclk/4 to mdclk/2
> (whereas on previous platforms every single row corresponded to an
> mdclk/2 value).
>
> tl;dr: I don't think we want/need this hunk of the patch.
>
BTW, completely unrelated to this LNL-specific patch, but since I'm
looking at this area of the code again, I'm questioning whether the
general logic in bxt_cdclk_cd2x_div_sel is correct for platforms that
have squashing (DG2, MTL, LNL, etc.). Now that we can reach multiple
effective cdclk values with a single PLL ratio / VCO setting, it seems
like this logic can no longer going to properly derive the CD2X divider
from VCO. And we seem to be relying on that function to program
the divider in CDCLK_CTL rather than using the value that was provided
in the table.
If I remember correctly there were some unexplained underruns seen on
DG2 before we hacked the cdclk to a higher value than expected; maybe
this is the explanation for that? Sometimes you'd get lucky and wind up
with the right divider in the end, other times you'd select it
incorrectly and wind up with something that didn't match the value
specified in the table.
Matt
>
> Matt
>
> > + else if (DISPLAY_VER(dev_priv) >= 10)
> > return DIV_ROUND_UP(pixel_rate, 2);
> > else if (DISPLAY_VER(dev_priv) == 9 ||
> > IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> > @@ -3591,7 +3639,10 @@ static const struct intel_cdclk_funcs i830_cdclk_funcs = {
> > */
> > void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
> > {
> > - if (DISPLAY_VER(dev_priv) >= 14) {
> > + if (DISPLAY_VER(dev_priv) >= 20) {
> > + dev_priv->display.funcs.cdclk = &mtl_cdclk_funcs;
> > + dev_priv->display.cdclk.table = lnl_cdclk_table;
> > + } else if (DISPLAY_VER(dev_priv) >= 14) {
> > dev_priv->display.funcs.cdclk = &mtl_cdclk_funcs;
> > dev_priv->display.cdclk.table = mtl_cdclk_table;
> > } else if (IS_DG2(dev_priv)) {
> > --
> > 2.40.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
