Re: [PATCH 1/2] drm/i915/glk: Apply cdclk workaround for DP audio

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2017-03-03 at 14:55 -0300, Paulo Zanoni wrote:
> Em Ter, 2017-02-28 às 18:57 -0800, Dhinakaran Pandiyan escreveu:
> > Implement GLK cdclk restriction for DP audio, similar to what's
> > implemented
> > for BDW and other GEN9 platforms. The cdclk restriction has been
> > refactored out of max. pixel clock computation as the 1:1
> > relationship
> > between pixel clock and cdclk frequency does not hold for GLK.
> > 
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 83 ++++++++++++++++++++++++--
> > ------------
> >  1 file changed, 52 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index d643c0c..8fc0f72 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1069,11 +1069,11 @@ static int bxt_calc_cdclk(int max_pixclk)
> >  		return 144000;
> >  }
> >  
> > -static int glk_calc_cdclk(int max_pixclk)
> > +static int glk_calc_cdclk(int max_pixclk, int min_cdclk)
> >  {
> > -	if (max_pixclk > 2 * 158400)
> > +	if (max_pixclk > 2 * 158400 || min_cdclk > 158400)
> >  		return 316800;
> > -	else if (max_pixclk > 2 * 79200)
> > +	else if (max_pixclk > 2 * 79200 || min_cdclk > 79200)
> >  		return 158400;
> >  	else
> >  		return 79200;
> > @@ -1367,7 +1367,7 @@ void bxt_init_cdclk(struct drm_i915_private
> > *dev_priv)
> >  	 *   Need to make this change after VBT has changes for BXT.
> >  	 */
> >  	if (IS_GEMINILAKE(dev_priv)) {
> > -		cdclk_state.cdclk = glk_calc_cdclk(0);
> > +		cdclk_state.cdclk = glk_calc_cdclk(0, 0);
> >  		cdclk_state.vco = glk_de_pll_vco(dev_priv,
> > cdclk_state.cdclk);
> >  	} else {
> >  		cdclk_state.cdclk = bxt_calc_cdclk(0);
> > @@ -1432,28 +1432,37 @@ 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_atomic_state *state)
> >  {
> > -	struct drm_i915_private *dev_priv =
> > -		to_i915(crtc_state->base.crtc->dev);
> > +	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *cstate;
> > +	int i;
> > +	int min_cdclk = 0;
> >  
> > -	/* 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);
> > +	for_each_crtc_in_state(state, crtc, cstate, i) {
> > +		struct intel_crtc_state *crtc_state;
> 
> Here we're just looking at the CRTCs in the state. Notice that
> max_pixel_rate caches the values for all the CRTCs so we can account
> for all of them instead of just the ones that are in the current atomic
> commit.
> 
> What if some other CRTC not in the current state raised the minimum
> clock to 432/316? Don't we end up risking not noticing it and going
> with a lower clock here? Imagine two very-low-res monitors with audio
> enabled. First we do the modeset on the audio monitor, then we enable
> the other monitor. Won't the second modeset end up wrongly reducing the
> clock below 432/316?
> 
> In other words: why doesn't min_cdclk need to do the same caching
> scheme that max_cdclk needs?
> 

You are right, I wonder if intel_atomic_state.cdclk would be a good
place to cache that.

> >  
> > -	/* 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."
> > -	 */
> > -	if (intel_crtc_has_dp_encoder(crtc_state) &&
> > -	    crtc_state->has_audio &&
> > -	    crtc_state->port_clock >= 540000 &&
> > -	    crtc_state->lane_count == 4)
> > -		pixel_rate = max(432000, pixel_rate);
> > +		crtc_state = to_intel_crtc_state(cstate);
> 
> Can you please clarify why it's not possible to simply add a new check
> for GLK in the old function? That would have been simpler.
> 

The old function, bdw_adjust_min_pipe_pixel_rate(), assumes that cdclk
is same as the pixel rate and returns min. cdclk to
intel_max_pixel_rate() when audio workarounds have to applied. This
cdclk value is passed on to glk_calc_cdclk(), which needs pixel rate as
input and is compared against 2x cdclk to arrive at a valid cdclk. So,
passing in cdclk to glk_calc_cdclk(), instead of pixel rate is a bug. 
The new function I am adding separates the pixel rate computation and
min cdclk restriction. 


The other option I can think of is to change intel_max_pixel_rate() to
return both the min cdclk and max pixel rate.


-DK

> If we still want to go with this approach, I'd suggest splitting your
> commit in two separate commits: one reworking the current code (and
> addressing the issue I pointed above), and another adding the GLK
> stuff.
> 
> 
> > +
> > +		/* According to BSpec, "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." for BDW and GEN9. The cdclk
> > restriction
> > +		 * for GLK is at 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_GEMINILAKE(dev_priv))
> > +				min_cdclk = 316800;
> > +			else if (IS_BROADWELL(dev_priv) ||
> > IS_GEN9(dev_priv))
> > +				min_cdclk = 432000;
> > +		}
> > +	}
> >  
> > -	return pixel_rate;
> > +	return min_cdclk;
> >  }
> >  
> >  /* compute the max rate for new configuration */
> > @@ -1481,10 +1490,9 @@ static int intel_max_pixel_rate(struct
> > drm_atomic_state *state)
> >  
> >  		pixel_rate = crtc_state->pixel_rate;
> >  
> > -		if (IS_BROADWELL(dev_priv) || IS_GEN9(dev_priv))
> > -			pixel_rate =
> > -				bdw_adjust_min_pipe_pixel_rate(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);
> >  
> >  		intel_state->min_pixclk[i] = pixel_rate;
> >  	}
> > @@ -1531,13 +1539,17 @@ 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_min_cdclk(state);
> >  	int cdclk;
> >  
> >  	/*
> >  	 * FIXME should also account for plane ratio
> >  	 * once 64bpp pixel formats are supported.
> >  	 */
> > -	cdclk = bdw_calc_cdclk(max_pixclk);
> > +	if (min_cdclk > max_pixclk)
> > +		cdclk = bdw_calc_cdclk(min_cdclk);
> > +	else
> > +		cdclk = bdw_calc_cdclk(max_pixclk);
> >  
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> > (%d kHz)\n",
> > @@ -1564,6 +1576,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_min_cdclk(state);
> >  	int cdclk, vco;
> >  
> >  	vco = intel_state->cdclk.logical.vco;
> > @@ -1574,7 +1587,10 @@ 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);
> > +	if (min_cdclk > max_pixclk)
> > +		cdclk = skl_calc_cdclk(min_cdclk, vco);
> > +	else
> > +		cdclk = skl_calc_cdclk(max_pixclk, vco);
> >  
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max
> > (%d kHz)\n",
> > @@ -1604,13 +1620,18 @@ static int bxt_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	int max_pixclk = intel_max_pixel_rate(state);
> >  	struct intel_atomic_state *intel_state =
> >  		to_intel_atomic_state(state);
> > +	int min_cdclk = intel_min_cdclk(state);
> >  	int cdclk, vco;
> >  
> >  	if (IS_GEMINILAKE(dev_priv)) {
> > -		cdclk = glk_calc_cdclk(max_pixclk);
> > +		cdclk = glk_calc_cdclk(max_pixclk, min_cdclk);
> >  		vco = glk_de_pll_vco(dev_priv, cdclk);
> >  	} else {
> > -		cdclk = bxt_calc_cdclk(max_pixclk);
> > +		if (min_cdclk > max_pixclk)
> > +			cdclk = bxt_calc_cdclk(min_cdclk);
> > +		else
> > +			cdclk = bxt_calc_cdclk(max_pixclk);
> > +
> >  		vco = bxt_de_pll_vco(dev_priv, cdclk);
> >  	}
> >  
> > @@ -1625,7 +1646,7 @@ static int bxt_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  
> >  	if (!intel_state->active_crtcs) {
> >  		if (IS_GEMINILAKE(dev_priv)) {
> > -			cdclk = glk_calc_cdclk(0);
> > +			cdclk = glk_calc_cdclk(0, 0);
> >  			vco = glk_de_pll_vco(dev_priv, cdclk);
> >  		} else {
> >  			cdclk = bxt_calc_cdclk(0);
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux