[Intel-gfx] [PATCH] drm/i915: Make sure cdclk is high enough for DP audio on VLV/CHV

[Chris Wilson]

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 at linux.intel.com>
> > > 
> > > 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 at vger.kernel.org
> > > Tested-by: Stefan Gottwald <gottwald at igel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111149
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  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