| Message ID | 20230907153757.2249452-23-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable Lunar Lake display | expand |
On Thu, Sep 07, 2023 at 08:37:52AM -0700, Lucas De Marchi wrote: > From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > 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@intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > 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. 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 >
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@intel.com> > > > > 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@intel.com> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > --- > > 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
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); + 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)) {