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
