[Intel-gfx] [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"

Pandiyan, Dhinakaran dhinakaran.pandiyan at intel.com
Tue Jul 11 20:35:55 UTC 2017




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 at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > 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 at intel.com>
> > 
> > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  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);
> > 
> 


More information about the Intel-gfx mailing list