Quoting Ville Syrjälä (2019-07-18 19:04:56) > On Thu, Jul 18, 2019 at 06:11:08PM +0100, Chris Wilson wrote: > > Quoting Ville Syrjala (2019-07-17 12:45:36) > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > On VLV/CHV there is some kind of linkage between the cdclk frequency > > > and the DP link frequency. The spec says: > > > "For DP audio configuration, cdclk frequency shall be set to > > > meet the following requirements: > > > DP Link Frequency(MHz) | Cdclk frequency(MHz) > > > 270 | 320 or higher > > > 162 | 200 or higher" > > > > > > I suspect that would more accurately be expressed as > > > "cdclk >= DP link clock", and in any case we can express it like > > > that in the code because of the limited set of cdclk and link > > > frequencies we support. > > > > > > Without this we can end up in a situation where the cdclk > > > is too low and enabling DP audio will kill the pipe. Happens > > > eg. with 2560x1440 modes where the 266MHz cdclk is sufficient > > > to pump the pixels (241.5 MHz dotclock) but is too low for > > > the DP audio due to the link frequency being 270 MHz. > > > > > > Cc: stable@xxxxxxxxxxxxxxx > > > Tested-by: Stefan Gottwald <gottwald@xxxxxxxx> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111149 > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > --- > > > drivers/gpu/drm/i915/display/intel_cdclk.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > > > index d0581a1ac243..93b0d190c184 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > > > @@ -2262,6 +2262,17 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) > > > if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) > > > min_cdclk = max(2 * 96000, min_cdclk); > > > > > > + /* > > > + * "For DP audio configuration, cdclk frequency shall be set to > > > + * meet the following requirements: > > > + * DP Link Frequency(MHz) | Cdclk frequency(MHz) > > > + * 270 | 320 or higher > > > + * 162 | 200 or higher" > > > + */ > > > + if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && > > > + intel_crtc_has_dp_encoder(crtc_state) && crtc_state->has_audio) > > > + min_cdclk = max(crtc_state->port_clock, min_cdclk); > > > > I tracked port_clock down to being the dp link clock (162 or 270) so > > that part of the story checks out. > > > > Judging by the rest of the function, I buy that the cdclk and link clock > > may be inscrutably tied together, and accept the test result that the > > cdclk must be at least the link clock with audio enabled. > > > > It may be that a corner case does require a higher frequency (rather > > than just bumping from 266 to 270), but for here and now > > Yeah there could be some extra headroom required. But our cdclk > can only be 200, 266, 320 or 400 MHz (and 200 won't actually get used > due to inexplicable display failure when try to use it). So in practice > we going to actually get bumped 162->266 and 270->320 here. I should > have expressed that better in the commit message. Also, I didn't find an easy way to confirm the limited set of cdlck. Hmm, actually I was looking at min_cdclk and didn't thin to compare that against any table. Ah, adding a see vlv_calc_cdclk() might have helped me put together the other side. Certainly adding more of a hint that min_cdclk isn't the final clock would help :) -Chris
