[Intel-gfx] [PATCH v2 2/2] drm/i915/dp: BDW cdclk fix for DP audio

[Daniel Vetter]

On Tue, Oct 25, 2016 at 06:19:59PM +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2016-10-25 at 12:14 +0300, Jani Nikula wrote:
> > On Tue, 25 Oct 2016, Jani Nikula <jani.nikula at intel.com> wrote:
> > > On Tue, 25 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com> wrote:
> > >> According to BSpec, cdclk has to be not less than 432 MHz with DP audio
> > >> enabled, port width x4, and link rate HBR2 (5.4 GHz)
> > >>
> > >> Having a lower cdclk triggers pipe underruns, which then lead to displays
> > >> continuously cycling off and on. This is essential for DP MST audio as the
> > >> link is trained at HBR2 and 4 lanes by default.
> > >>
> > >> v2: Restrict fix to BDW
> > >>     Retain the set cdclk across modesets (Ville)
> > >
> > > Cc: stable at vger.kernel.org
> > >
> > >>
> > >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_display.c | 28 +++++++++++++++++++++++++---
> > >>  1 file changed, 25 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >> index a94f7d1..8c59651 100644
> > >> --- a/drivers/gpu/drm/i915/intel_display.c
> > >> +++ b/drivers/gpu/drm/i915/intel_display.c
> > >> @@ -10260,6 +10260,18 @@ static void bxt_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> > >>  	bxt_set_cdclk(to_i915(dev), req_cdclk);
> > >>  }
> > >>  
> > >> +static unsigned int bdw_dp_audio_cdclk(struct intel_crtc_state *crtc_state)
> > >> +{
> > >> +
> > >> +	if (intel_crtc_has_dp_encoder(crtc_state) &&
> > >> +	    crtc_state->has_audio &&
> > >> +	    crtc_state->port_clock >= 540000 &&
> > >> +	    crtc_state->lane_count == 4)
> > >> +		return 432000;
> > >
> > > Where does 432000 come from? 450000 or even (337500 + 1). See below.
> > >
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >>  /* compute the max rate for new configuration */
> > >>  static int ilk_max_pixel_rate(struct drm_atomic_state *state)
> > >>  {
> > >> @@ -10275,7 +10287,7 @@ static int ilk_max_pixel_rate(struct drm_atomic_state *state)
> > >>  	       sizeof(intel_state->min_pixclk));
> > >>  
> > >>  	for_each_crtc_in_state(state, crtc, cstate, i) {
> > >> -		int pixel_rate;
> > >> +		unsigned int pixel_rate;
> > >>  
> > >>  		crtc_state = to_intel_crtc_state(cstate);
> > >>  		if (!crtc_state->base.enable) {
> > >> @@ -10285,9 +10297,19 @@ static int ilk_max_pixel_rate(struct drm_atomic_state *state)
> > >>  
> > >>  		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
> > >>  
> > >> +		if (IS_BROADWELL(dev_priv)) {
> > >>  		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> > >> -		if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)432
> > >> -			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> > >> +			if (crtc_state->ips_enabled)
> > >> +				pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> > >> +
> > >> +		/* BSpec says "Do not use DisplayPort with CDCLK less than
> > >> +		 * 432 MHz, audio enabled, port width x4, and link rate
> > >
> > > For me the spec says "Do not use DisplayPort with CDCLK 337.5 MHz", not
> > > "less than 432 MHz".
> > 
> > Right, so the spec for *Skylake* mentions 432 MHz. Now, we need this fix
> > for both Broadwell and Skylake, where's the Skylake part?
> > 
> > BR,
> > Jani.
> > 
> > 
> 
> I believe you are looking at CDCLK_CTL that refers to pre-production SKL
> SKU's. See the description for DP_TP_CTL instead. The information seems
> to be scattered a bit

Please add a detailed citation (mentioning the Bspec hierarchy and all
that) and quote the workaround text in full in the commit message. Because
next month when someone else looks at this code and runs git blame they
will have no chance to find the Bspec notice.

This should be standard practice for any hw workarounds and things like
that which are done due to Bspec late in the platform cycle (personally
I'd say past beta milestone, which we are way past for skl ...).
-Daniel

> 
> 
> > >
> > >> +		 * HBR2 (5.4 GHz), or else there may be audio corruption or
> > >> +		 * screen corruption."
> > >> +		 */
> > >> +			pixel_rate = max(pixel_rate,
> > >> +					 bdw_dp_audio_cdclk(crtc_state));
> > >> +		}
> > >
> > > I'd add a new function
> > >
> > > static int bwd_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> > > 					  int pixel_rate)
> > >
> > > and do both the IPS adjustment and the audio adjustment there, returning
> > > the original pixel_rate if adjustment is not needed. Move the comments
> > > there as well.
> > >
> > > It would be called as
> > >
> > > 	if (IS_BROADWELL(dev_priv))
> > >         	pixel_rate = bwd_adjust_min_pipe_pixel_rate(crtc_state, pixel_rate);
> > >
> > > here.
> > >
> > >
> > > BR,
> > > Jani.
> > >
> 
> Will do.
> 
> -DK
> > >>  
> > >>  		intel_state->min_pixclk[i] = pixel_rate;
> > >>  	}
> > 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch