| Message ID | 1477369117-24627-3-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 24, 2016 at 09:18:37PM -0700, Dhinakaran Pandiyan 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) > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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) > +{ > + Useless blank line. > + if (intel_crtc_has_dp_encoder(crtc_state) && > + crtc_state->has_audio && > + crtc_state->port_clock >= 540000 && > + crtc_state->lane_count == 4) > + return 432000; > + > + 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) > - 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 > + * HBR2 (5.4 GHz), or else there may be audio corruption or > + * screen corruption." > + */ Indentation of the comments is wrong. > + pixel_rate = max(pixel_rate, > + bdw_dp_audio_cdclk(crtc_state)); > + } > > intel_state->min_pixclk[i] = pixel_rate; Otherwise I suppose this ought to work. So with the formatting stuff fixed this is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> The whole min_pixclk vs. pixel_rate vs. cdclk thing is a bit of a mess though. So it could use a thorough cleaning to make it less confusing. I'm tinking we might just want to start tracking a minimum acceptable cdclk per pipe. Probably the main thing would be to pull in the 5%/10% guardband handling here for all platforms. > } > -- > 2.7.4
On Tue, 25 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@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@vger.kernel.org > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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) > - 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". > + * 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. > > intel_state->min_pixclk[i] = pixel_rate; > }
On Tue, 25 Oct 2016, Jani Nikula <jani.nikula@intel.com> wrote: > On Tue, 25 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@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@vger.kernel.org > >> >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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) >> - 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. > >> + * 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. > >> >> intel_state->min_pixclk[i] = pixel_rate; >> }
On Tue, 2016-10-25 at 12:14 +0300, Jani Nikula wrote: > On Tue, 25 Oct 2016, Jani Nikula <jani.nikula@intel.com> wrote: > > On Tue, 25 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@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@vger.kernel.org > > > >> > >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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 > > > >> + * 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; > >> } >
On Tue, 2016-10-25 at 11:46 +0300, Ville Syrjälä wrote: > On Mon, Oct 24, 2016 at 09:18:37PM -0700, Dhinakaran Pandiyan 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) > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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) > > +{ > > + > > Useless blank line. > > > + if (intel_crtc_has_dp_encoder(crtc_state) && > > + crtc_state->has_audio && > > + crtc_state->port_clock >= 540000 && > > + crtc_state->lane_count == 4) > > + return 432000; > > + > > + 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) > > - 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 > > + * HBR2 (5.4 GHz), or else there may be audio corruption or > > + * screen corruption." > > + */ > > Indentation of the comments is wrong. > > > + pixel_rate = max(pixel_rate, > > + bdw_dp_audio_cdclk(crtc_state)); > > + } > > > > intel_state->min_pixclk[i] = pixel_rate; > > Otherwise I suppose this ought to work. > > So with the formatting stuff fixed this is > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The whole min_pixclk vs. pixel_rate vs. cdclk thing is a bit of a mess > though. So it could use a thorough cleaning to make it less confusing. > I'm tinking we might just want to start tracking a minimum acceptable > cdclk per pipe. Probably the main thing would be to pull in the > 5%/10% guardband handling here for all platforms. > Yeah, definitely needs some work. As you wrote in the previous review, we can get rid of tracking min_pixclk and track min_cdclk instead. I could not find any other use for min_pixclk other than computing cdclk. Thanks for the review. -DK > > } > > -- > > 2.7.4 >
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@intel.com> wrote: > > > On Tue, 25 Oct 2016, Dhinakaran Pandiyan <dhinakaran.pandiyan@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@vger.kernel.org > > > > > >> > > >> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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; + + 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) - 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 + * 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)); + } intel_state->min_pixclk[i] = pixel_rate; }
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) Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)