On Tue, 2017-07-11 at 16:00 +0300, Ville Syrjälä wrote:
> On Tue, Jul 11, 2017 at 02:47:42AM -0700, Dhinakaran Pandiyan wrote:
> > 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.
>
> Those are free.
>
> > Looks like all we need is a to_intel_crtc_state().
>
> I want to move away from the drm_ types as much as possible. Ideally
> I'd rather not see those in the i915 code at all, but that's slightly
> unrealistic thanks to the function pointers we hook into the
> core/helpers. I've been pondering if we could automagically generate
> some kind of wrappers for those that would give us the intel_ types
> directly, but not sure if that's doable.
>
> >
> > > 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() ?
>
> Sure, I can respin with that.
>
> >
> >
> > > + 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.
>
> I still have the HSW code tucked away in a branch ;)
>
> >
> >
> > > + 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?
>
> That 90% would apply to all remaining platforms. Not that we currently
> implement cdclk frequency scaling for those, but we could (at least for
> some gmch platforms).
>
> Even if we never add the cdclk code fo HSW/GMCH platforms I think keeping
> this in sync with the max_dotclock() thing makes it easier to cross
> check things.
>
> >
> > > +}
> > > +
> > > +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()
>
> As mentioned that doesn't agree with the direction where we want to go.
>
> >
> > > 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 guess usually there's a modeset happening at some point.
>
> >
> > I believe, the .modeset_calc_cdclk() functions are not called when this
> > hw_state is committed. Is that right?
>
> This is the state readout for init/resume. So not sure which commit
> you're referring to here?
>
I was referring to the commit that immediately follows the readout in
__intel_display_resume. But, I read the code again and it answered my
question :)
> >
> >
> > >
> > > 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
