On Monday, July 10, 2017 10:33:46 PM PDT ville.syrjala@xxxxxxxxxxxxxxx wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Make the min_pixclk thing less confusing by changing it to track > the minimum acceptable cdclk frequency instead. This means moving > the application of the guardbands to a slightly higher level from > the low level platform specific calc_cdclk() functions. > > The immediate benefit is elimination of the confusing 2x factors > on GLK/CNL+ in the audio workarounds (which stems from the fact > that the pipes produce two pixels per clock). > > v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage > handling v3: Squash with the CNL cdclk limits patch (DK) > Looks good to me, I only have some bikesheds and nitpicks below. Will leave it to you to decide if you want to address them. Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.h | 12 ++- > drivers/gpu/drm/i915/intel_cdclk.c | 202 > ++++++++++++++++++----------------- drivers/gpu/drm/i915/intel_display.c | > 21 ++-- > drivers/gpu/drm/i915/intel_drv.h | 4 +- > 4 files changed, 125 insertions(+), 114 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h index 81cd21ecfa7d..c80176424d84 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -563,6 +563,15 @@ struct i915_hotplug { > (__i)++) \ > for_each_if (plane_state) > > +#define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state, > __i) \ + for ((__i) = 0; \ > + (__i) < (__state)->base.dev->mode_config.num_crtc && \ > + ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \ > + (new_crtc_state) = > to_intel_crtc_state((__state)->base.crtcs[__i].new_state), 1); \ + > (__i)++) \ > + for_each_if (crtc) > + > + I am not sure how useful adding this macro is. The loop looks concise with this macro, but we end up doing unnecessary to_intel_crtc() conversions in the loop. Looks like all we need is a to_intel_crtc_state(). > struct drm_i915_private; > struct i915_mm_struct; > struct i915_mmu_object; > @@ -2268,7 +2277,8 @@ struct drm_i915_private { > struct mutex dpll_lock; > > unsigned int active_crtcs; > - unsigned int min_pixclk[I915_MAX_PIPES]; > + /* minimum acceptable cdclk for each pipe */ > + int min_cdclk[I915_MAX_PIPES]; > > int dpio_phy_iosf_port[I915_NUM_PHYS_VLV]; > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > b/drivers/gpu/drm/i915/intel_cdclk.c index 1241e5891b29..50f153dbea14 > 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -417,24 +417,21 @@ static void hsw_get_cdclk(struct drm_i915_private > *dev_priv, cdclk_state->cdclk = 540000; > } > > -static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, > - int max_pixclk) > +static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk) > { > int freq_320 = (dev_priv->hpll_freq << 1) % 320000 != 0 ? > 333333 : 320000; > - int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90; > > /* > * We seem to get an unstable or solid color picture at 200MHz. > * Not sure what's wrong. For now use 200MHz only when all pipes > * are off. > */ > - if (!IS_CHERRYVIEW(dev_priv) && > - max_pixclk > freq_320*limit/100) > + if (IS_VALLEYVIEW(dev_priv) && min_cdclk > freq_320) > return 400000; > - else if (max_pixclk > 266667*limit/100) > + else if (min_cdclk > 266667) > return freq_320; > - else if (max_pixclk > 0) > + else if (min_cdclk > 0) > return 266667; > else > return 200000; > @@ -612,13 +609,13 @@ static void chv_set_cdclk(struct drm_i915_private > *dev_priv, intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A); > } > > -static int bdw_calc_cdclk(int max_pixclk) > +static int bdw_calc_cdclk(int min_cdclk) > { > - if (max_pixclk > 540000) > + if (min_cdclk > 540000) > return 675000; > - else if (max_pixclk > 450000) > + else if (min_cdclk > 450000) > return 540000; > - else if (max_pixclk > 337500) > + else if (min_cdclk > 337500) > return 450000; > else > return 337500; > @@ -724,23 +721,23 @@ static void bdw_set_cdclk(struct drm_i915_private > *dev_priv, cdclk, dev_priv->cdclk.hw.cdclk); > } > > -static int skl_calc_cdclk(int max_pixclk, int vco) > +static int skl_calc_cdclk(int min_cdclk, int vco) > { > if (vco == 8640000) { > - if (max_pixclk > 540000) > + if (min_cdclk > 540000) > return 617143; > - else if (max_pixclk > 432000) > + else if (min_cdclk > 432000) > return 540000; > - else if (max_pixclk > 308571) > + else if (min_cdclk > 308571) > return 432000; > else > return 308571; > } else { > - if (max_pixclk > 540000) > + if (min_cdclk > 540000) > return 675000; > - else if (max_pixclk > 450000) > + else if (min_cdclk > 450000) > return 540000; > - else if (max_pixclk > 337500) > + else if (min_cdclk > 337500) > return 450000; > else > return 337500; > @@ -1075,31 +1072,25 @@ void skl_uninit_cdclk(struct drm_i915_private > *dev_priv) skl_set_cdclk(dev_priv, &cdclk_state); > } > > -static int bxt_calc_cdclk(int max_pixclk) > +static int bxt_calc_cdclk(int min_cdclk) > { > - if (max_pixclk > 576000) > + if (min_cdclk > 576000) > return 624000; > - else if (max_pixclk > 384000) > + else if (min_cdclk > 384000) > return 576000; > - else if (max_pixclk > 288000) > + else if (min_cdclk > 288000) > return 384000; > - else if (max_pixclk > 144000) > + else if (min_cdclk > 144000) > return 288000; > else > return 144000; > } > > -static int glk_calc_cdclk(int max_pixclk) > +static int glk_calc_cdclk(int min_cdclk) > { > - /* > - * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk > - * as a temporary workaround. Use a higher cdclk instead. (Note that > - * intel_compute_max_dotclk() limits the max pixel clock to 99% of max > - * cdclk.) > - */ > - if (max_pixclk > DIV_ROUND_UP(2 * 158400 * 99, 100)) > + if (min_cdclk > 158400) > return 316800; > - else if (max_pixclk > DIV_ROUND_UP(2 * 79200 * 99, 100)) > + else if (min_cdclk > 79200) > return 158400; > else > return 79200; > @@ -1420,11 +1411,11 @@ void bxt_uninit_cdclk(struct drm_i915_private > *dev_priv) bxt_set_cdclk(dev_priv, &cdclk_state); > } > > -static int cnl_calc_cdclk(int max_pixclk) > +static int cnl_calc_cdclk(int min_cdclk) > { > - if (max_pixclk > 336000) > + if (min_cdclk > 336000) > return 528000; > - else if (max_pixclk > 168000) > + else if (min_cdclk > 168000) > return 336000; > else > return 168000; > @@ -1732,98 +1723,106 @@ void intel_set_cdclk(struct drm_i915_private > *dev_priv, dev_priv->display.set_cdclk(dev_priv, cdclk_state); > } > > -static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state > *crtc_state, - int pixel_rate) > +static int intel_min_cdclk(struct drm_i915_private *dev_priv, The names have started to sound similar. intel_min_cdclk intel_crtc_compute_min_cdclk intel_compute_min_cdclk I can't think of anything better. How about something like intel_pixel_rate_to_cdclk() ? > + int pixel_rate) > +{ > + if (INTEL_GEN(dev_priv) >= 10) > + /* > + * FIXME: Switch to DIV_ROUND_UP(pixel_rate, 2) > + * once DDI clock voltage requirements are > + * handled correctly. > + */ > + return pixel_rate; > + else if (IS_GEMINILAKE(dev_priv)) > + /* > + * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk > + * as a temporary workaround. Use a higher cdclk instead. (Note that > + * intel_compute_max_dotclk() limits the max pixel clock to 99% of max > + * cdclk.) > + */ > + return DIV_ROUND_UP(pixel_rate * 100, 2 * 99); > + else if (IS_GEN9(dev_priv) || > + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) I don't see why IS_HASWELL() is needed here. The callers seem to need a .modeset_calc_cdclk() hook and Haswell does not have one. > + return pixel_rate; > + else if (IS_CHERRYVIEW(dev_priv)) > + return DIV_ROUND_UP(pixel_rate * 100, 95); > + else > + return DIV_ROUND_UP(pixel_rate * 100, 90); Type out IS_VALLEYVIEW() explicitly for documentation? > +} > + > +int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = > to_i915(crtc_state->base.crtc->dev); > + int min_cdclk; > + > + if (!crtc_state->base.enable) > + return 0; > + > + min_cdclk = intel_min_cdclk(dev_priv, crtc_state->pixel_rate); > > /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */ > if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled) > - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95); > + min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95); > > /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz, > * audio enabled, port width x4, and link rate HBR2 (5.4 GHz), or else > * there may be audio corruption or screen corruption." This cdclk > - * restriction for GLK is 316.8 MHz and since GLK can output two > - * pixels per clock, the pixel rate becomes 2 * 316.8 MHz. > + * restriction for GLK is 316.8 MHz. > */ > if (intel_crtc_has_dp_encoder(crtc_state) && > crtc_state->has_audio && > crtc_state->port_clock >= 540000 && > crtc_state->lane_count == 4) { > - if (IS_CANNONLAKE(dev_priv)) > - pixel_rate = max(316800, pixel_rate); > - else if (IS_GEMINILAKE(dev_priv)) > - pixel_rate = max(2 * 316800, pixel_rate); > - else > - pixel_rate = max(432000, pixel_rate); > + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) { > + /* Display WA #1145: glk,cnl */ > + min_cdclk = max(316800, min_cdclk); > + } else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) { > + /* Display WA #1144: skl,bxt */ > + min_cdclk = max(432000, min_cdclk); > + } > } > > /* According to BSpec, "The CD clock frequency must be at least twice > * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default. > - * The check for GLK has to be adjusted as the platform can output > - * two pixels per clock. > */ > - if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) { > - if (IS_GEMINILAKE(dev_priv)) > - pixel_rate = max(2 * 2 * 96000, pixel_rate); > - else > - pixel_rate = max(2 * 96000, pixel_rate); > - } > + if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) > + min_cdclk = max(2 * 96000, min_cdclk); > > - return pixel_rate; > + return min_cdclk; > } > > -/* compute the max rate for new configuration */ > -static int intel_max_pixel_rate(struct drm_atomic_state *state) > +static int intel_compute_min_cdclk(struct drm_atomic_state *state) > { > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct drm_i915_private *dev_priv = to_i915(state->dev); > - struct drm_crtc *crtc; > - struct drm_crtc_state *cstate; > + struct intel_crtc *crtc; > struct intel_crtc_state *crtc_state; > - unsigned int max_pixel_rate = 0, i; > + int min_cdclk = 0, i; > enum pipe pipe; > > - memcpy(intel_state->min_pixclk, dev_priv->min_pixclk, > - sizeof(intel_state->min_pixclk)); > - > - for_each_new_crtc_in_state(state, crtc, cstate, i) { > - int pixel_rate; > - > - crtc_state = to_intel_crtc_state(cstate); > - if (!crtc_state->base.enable) { > - intel_state->min_pixclk[i] = 0; > - continue; > - } > + memcpy(intel_state->min_cdclk, dev_priv->min_cdclk, > + sizeof(intel_state->min_cdclk)); > > - pixel_rate = crtc_state->pixel_rate; > - > - if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) > - pixel_rate = > - bdw_adjust_min_pipe_pixel_rate(crtc_state, > - pixel_rate); > - > - intel_state->min_pixclk[i] = pixel_rate; > - } > + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) > + intel_state->min_cdclk[i] = > + intel_crtc_compute_min_cdclk(crtc_state); > Like I wrote above, we could just reuse for_each_new_crtc_in_state() > for_each_pipe(dev_priv, pipe) > - max_pixel_rate = max(intel_state->min_pixclk[pipe], > - max_pixel_rate); > + min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk); > > - return max_pixel_rate; > + return min_cdclk; > } > > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->dev); > - int max_pixclk = intel_max_pixel_rate(state); > + int min_cdclk = intel_compute_min_cdclk(state); > struct intel_atomic_state *intel_state = > to_intel_atomic_state(state); > int cdclk; > > - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk); > + cdclk = vlv_calc_cdclk(dev_priv, min_cdclk); > > if (cdclk > dev_priv->max_cdclk_freq) { > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > @@ -1849,14 +1848,14 @@ static int bdw_modeset_calc_cdclk(struct > drm_atomic_state *state) { > struct drm_i915_private *dev_priv = to_i915(state->dev); > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > - int max_pixclk = intel_max_pixel_rate(state); > + int min_cdclk = intel_compute_min_cdclk(state); > int cdclk; > > /* > * FIXME should also account for plane ratio > * once 64bpp pixel formats are supported. > */ > - cdclk = bdw_calc_cdclk(max_pixclk); > + cdclk = bdw_calc_cdclk(min_cdclk); > > if (cdclk > dev_priv->max_cdclk_freq) { > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > @@ -1882,7 +1881,7 @@ static int skl_modeset_calc_cdclk(struct > drm_atomic_state *state) { > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct drm_i915_private *dev_priv = to_i915(state->dev); > - const int max_pixclk = intel_max_pixel_rate(state); > + int min_cdclk = intel_compute_min_cdclk(state); > int cdclk, vco; > > vco = intel_state->cdclk.logical.vco; > @@ -1893,7 +1892,7 @@ static int skl_modeset_calc_cdclk(struct > drm_atomic_state *state) * FIXME should also account for plane ratio > * once 64bpp pixel formats are supported. > */ > - cdclk = skl_calc_cdclk(max_pixclk, vco); > + cdclk = skl_calc_cdclk(min_cdclk, vco); > > if (cdclk > dev_priv->max_cdclk_freq) { > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n", > @@ -1920,16 +1919,16 @@ static int skl_modeset_calc_cdclk(struct > drm_atomic_state *state) static int bxt_modeset_calc_cdclk(struct > drm_atomic_state *state) { > struct drm_i915_private *dev_priv = to_i915(state->dev); > - int max_pixclk = intel_max_pixel_rate(state); > + int min_cdclk = intel_compute_min_cdclk(state); > struct intel_atomic_state *intel_state = > to_intel_atomic_state(state); > int cdclk, vco; > > if (IS_GEMINILAKE(dev_priv)) { > - cdclk = glk_calc_cdclk(max_pixclk); > + cdclk = glk_calc_cdclk(min_cdclk); > vco = glk_de_pll_vco(dev_priv, cdclk); > } else { > - cdclk = bxt_calc_cdclk(max_pixclk); > + cdclk = bxt_calc_cdclk(min_cdclk); > vco = bxt_de_pll_vco(dev_priv, cdclk); > } > > @@ -1966,10 +1965,10 @@ static int cnl_modeset_calc_cdclk(struct > drm_atomic_state *state) struct drm_i915_private *dev_priv = > to_i915(state->dev); > struct intel_atomic_state *intel_state = > to_intel_atomic_state(state); > - int max_pixclk = intel_max_pixel_rate(state); > + int min_cdclk = intel_compute_min_cdclk(state); > int cdclk, vco; > > - cdclk = cnl_calc_cdclk(max_pixclk); > + cdclk = cnl_calc_cdclk(min_cdclk); > vco = cnl_cdclk_pll_vco(dev_priv, cdclk); > > if (cdclk > dev_priv->max_cdclk_freq) { > @@ -1999,14 +1998,21 @@ static int intel_compute_max_dotclk(struct > drm_i915_private *dev_priv) { > int max_cdclk_freq = dev_priv->max_cdclk_freq; > > - if (IS_GEMINILAKE(dev_priv)) > + if (INTEL_GEN(dev_priv) >= 10) > + /* > + * FIXME: Allow '2 * max_cdclk_freq' > + * once DDI clock voltage requirements are > + * handled correctly. > + */ > + return max_cdclk_freq; > + else if (IS_GEMINILAKE(dev_priv)) > /* > * FIXME: Limiting to 99% as a temporary workaround. See > - * glk_calc_cdclk() for details. > + * intel_min_cdclk() for details. > */ > return 2 * max_cdclk_freq * 99 / 100; > - else if (INTEL_INFO(dev_priv)->gen >= 9 || > - IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > + else if (IS_GEN9(dev_priv) || > + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv)) > return max_cdclk_freq; > else if (IS_CHERRYVIEW(dev_priv)) > return max_cdclk_freq*95/100; > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c index 2144adc5b1d5..b47535f5d95d > 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5920,7 +5920,7 @@ static void intel_crtc_disable_noatomic(struct > drm_crtc *crtc, intel_crtc->enabled_power_domains = 0; > > dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe); > - dev_priv->min_pixclk[intel_crtc->pipe] = 0; > + dev_priv->min_cdclk[intel_crtc->pipe] = 0; > } > > /* > @@ -13292,8 +13292,8 @@ static int intel_atomic_commit(struct drm_device > *dev, intel_atomic_track_fbs(state); > > if (intel_state->modeset) { > - memcpy(dev_priv->min_pixclk, intel_state->min_pixclk, > - sizeof(intel_state->min_pixclk)); > + memcpy(dev_priv->min_cdclk, intel_state->min_cdclk, > + sizeof(intel_state->min_cdclk)); > dev_priv->active_crtcs = intel_state->active_crtcs; > dev_priv->cdclk.logical = intel_state->cdclk.logical; > dev_priv->cdclk.actual = intel_state->cdclk.actual; > @@ -15569,7 +15569,7 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) for_each_intel_crtc(dev, crtc) { > struct intel_crtc_state *crtc_state = > to_intel_crtc_state(crtc->base.state); > - int pixclk = 0; > + int min_cdclk = 0; > > memset(&crtc->base.mode, 0, sizeof(crtc->base.mode)); > if (crtc_state->base.active) { > @@ -15590,22 +15590,15 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) > > intel_crtc_compute_pixel_rate(crtc_state); > > - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv) || > - IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > - pixclk = crtc_state->pixel_rate; > - else > - WARN_ON(dev_priv->display.modeset_calc_cdclk); > - > - /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */ > - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled) > - pixclk = DIV_ROUND_UP(pixclk * 100, 95); > + if (dev_priv->display.modeset_calc_cdclk) > + min_cdclk = intel_crtc_compute_min_cdclk(crtc_state); Hmm. So we were not applying the audio workarounds here. Wonder why it did not cause any trouble. I believe, the .modeset_calc_cdclk() functions are not called when this hw_state is committed. Is that right? > > drm_calc_timestamping_constants(&crtc->base, > &crtc_state->base.adjusted_mode); > update_scanline_offset(crtc); > } > > - dev_priv->min_pixclk[crtc->pipe] = pixclk; > + dev_priv->min_cdclk[crtc->pipe] = min_cdclk; > > intel_pipe_config_sanity_check(dev_priv, crtc_state); > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h index d17a32437f07..8cc1b86b799a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -383,7 +383,8 @@ struct intel_atomic_state { > unsigned int active_pipe_changes; > > unsigned int active_crtcs; > - unsigned int min_pixclk[I915_MAX_PIPES]; > + /* minimum acceptable cdclk for each pipe */ > + int min_cdclk[I915_MAX_PIPES]; > > struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS]; > > @@ -1308,6 +1309,7 @@ void intel_audio_init(struct drm_i915_private > *dev_priv); void intel_audio_deinit(struct drm_i915_private *dev_priv); > > /* intel_cdclk.c */ > +int intel_crtc_compute_min_cdclk(const struct intel_crtc_state > *crtc_state); void skl_init_cdclk(struct drm_i915_private *dev_priv); > void skl_uninit_cdclk(struct drm_i915_private *dev_priv); > void cnl_init_cdclk(struct drm_i915_private *dev_priv); _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx