On Thu, 2016-10-13 at 21:44 +0300, Ville Syrjälä wrote:
> On Thu, Oct 13, 2016 at 11:04:19AM -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.
> >
> > This should fix https://bugs.freedesktop.org/show_bug.cgi?id=97907
> >
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 47
> > +++++++++++++++++++++++++++++++++---
> > 1 file changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index cfcb03f..6a05183 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6603,14 +6603,43 @@ static int valleyview_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > return 0;
> > }
> >
> > +static bool cdclk_min_for_dp_audio(struct drm_atomic_state *state)
> > +{
> > +
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_crtc *crtc;
> > + int i;
> > +
> > + /* 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."
> > + */
> > +
> > + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > + struct intel_crtc_state *pipe_config =
> > + to_intel_crtc_state(crtc_state);
> > +
> > + return (intel_crtc_has_dp_encoder(pipe_config) &&
> > + pipe_config->has_audio &&
> > + pipe_config->port_clock == 540000 &&
> > + pipe_config->lane_count == 4);
> > + }
>
> That's not good enough on account of crtcs not part of the state
> potentially needing the workaround as well. However, since we only do
> this when there's a modeset, I think we'll be covered by the
> connection_mutex, and so we should be able to peek at the current state
> of all crtcs without grabbing the corresponding crtc locks.
>
Please correct me if I am wrong. Won't the first modeset that has all
the conditions met (DP + HBR2 + 4 lanes + audio) include the crtc
driving the display which triggered the modeset?
Since, the new cdclk freq that will be set is common for all the crtcs,
we don't need the workaround for crtcs that are not in state.
> So I think we'd be OK with something like:
>
> WARN_ON(!locked(connection_mutex));
>
> for_each_intel_crtc() {
> /*
> * Peeking at the current state is safe since
> * we can only get here while holding the
> * connection_mutex.
> */
> crtc_state = intel_get_existing_crtc_state();
> if (!crtc_state)
> crtc_state = to_intel_crtc_state(crtc->base.state);
>
> ...
> }
>
> The other option would be to track the min cdclk for each pipe in the
> top level state I suppose. We already do that for the pixel rate
> actually so that we can calculate the cdclk to begin with. Hmm. Maybe
> we should just switch to tracking the min cdclk per pipe instead of the
> pixel rate. Or did we need to the pixel rate itself for something else,
> Maarten?
>
> Or we could perhaps replace the pixel rate/pixclk tracking with the peek
> approach entirely. Not quite sure. Would have to read the entire thing
> through.
>
I thought of this, but the work around applies for only three platforms
(potentially just two) as of now. Does it warrant a driver wide change?
I have to check if mincdclk is useful elsewhere.
> > +
> > + return false;
> > +}
> > +
> > static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > {
> > int max_pixclk = ilk_max_pixel_rate(state);
> > + int cdclk, min_cdclk = 0;
> > struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> >
> > - intel_state->cdclk = intel_state->dev_cdclk =
> > - bxt_calc_cdclk(max_pixclk);
> > + if (cdclk_min_for_dp_audio(state))
> > + min_cdclk = bxt_calc_cdclk(432000);
> > +
> > + cdclk = bxt_calc_cdclk(max_pixclk);
> > + intel_state->cdclk = intel_state->dev_cdclk = max(min_cdclk, cdclk);
> >
> > if (!intel_state->active_crtcs)
> > intel_state->dev_cdclk = bxt_calc_cdclk(0);
> > @@ -10374,7 +10403,10 @@ static int broadwell_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 = ilk_max_pixel_rate(state);
> > - int cdclk;
> > + int cdclk, min_cdclk = 0;
> > +
> > + if (cdclk_min_for_dp_audio(state))
> > + min_cdclk = broadwell_calc_cdclk(432000);
> >
> > /*
> > * FIXME should also account for plane ratio
> > @@ -10382,6 +10414,8 @@ static int broadwell_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > */
> > cdclk = broadwell_calc_cdclk(max_pixclk);
> >
> > + cdclk = max(min_cdclk, cdclk);
> > +
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > cdclk, dev_priv->max_cdclk_freq);
> > @@ -10411,7 +10445,10 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > const int max_pixclk = ilk_max_pixel_rate(state);
> > int vco = intel_state->cdclk_pll_vco;
> > - int cdclk;
> > + int cdclk, min_cdclk = 0;
> > +
> > + if (cdclk_min_for_dp_audio(state))
> > + min_cdclk = skl_calc_cdclk(432000, vco);
> >
> > /*
> > * FIXME should also account for plane ratio
> > @@ -10419,6 +10456,8 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > */
> > cdclk = skl_calc_cdclk(max_pixclk, vco);
> >
> > + cdclk = max(min_cdclk, cdclk);
> > +
> > /*
> > * FIXME move the cdclk caclulation to
> > * compute_config() so we can fail gracegully.
> > --
> > 2.7.4
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
