| Message ID | 1488337038-2906-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-02-28 at 18:57 -0800, Dhinakaran Pandiyan wrote: > 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@intel.com> > --- > 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; > > - /* 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); > + > + /* 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); Looks like intel_min_cdclk() returns a valid cdclk for the platform, so wouldn't cdclk = *_calc_cdclk(max_pixclk); if (cdclk < min_cdclk) cdclk = min_cdclk; be simpler? > 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); > } With the idea above, this would become if (IS_GEMINILAKE(dev_priv)) cdclk = glk_calc_cdclk(max_pixclk); else cdclk = bxt_calc_cdclk(max_pixclk); if (cdclk < min_cdclk) cdclk = min_cdclk; if (IS_GEMINILAKE(dev_priv)) vco = glk_de_pll_vco(dev_priv, cdclk); else vco = bxt_de_pll_vco(dev_priv, cdclk); Not great, but maybe still better than changing the signature of glk_calc_cdclk()? Also, this conflicts with Paulo's cdclk clean up [1]. [1] https://patchwork.freedesktop.org/series/19974/ Ander > > @@ -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);
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@intel.com> > --- > 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? > > - /* 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. 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);
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@intel.com> > > --- > > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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; - /* 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); + + /* 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);
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@intel.com> --- drivers/gpu/drm/i915/intel_cdclk.c | 83 ++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 31 deletions(-)