[Intel-gfx] [PATCH v2 2/2] drm/i915: Implement cdclk restrictions based on Azalia BCLK

[Ville Syrjälä]

On Wed, Mar 15, 2017 at 06:03:58PM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-03-15 at 11:32 +0200, Jani Nikula wrote:
> > On Tue, 14 Mar 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan at intel.com> wrote:
> > > On Tue, 2017-03-14 at 17:47 -0300, Paulo Zanoni wrote:
> > >> Em Ter, 2017-03-07 às 16:12 -0800, Dhinakaran Pandiyan escreveu:
> > >> > According to BSpec, "The CD clock frequency must be at least twice
> > >> > the
> > >> > frequency of the Azalia BCLK." and BCLK is configured to 96 MHz by
> > >> > default. This check is needed because BXT and GLK support cdclk
> > >> > frequencies less than 192 MHz.
> > >> > 
> > >> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > >> > ---
> > >> >  drivers/gpu/drm/i915/intel_cdclk.c | 12 ++++++++++++
> > >> >  1 file changed, 12 insertions(+)
> > >> > 
> > >> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > >> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > >> > index e8c1181..7b1ac1d 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > >> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > >> > @@ -1458,6 +1458,18 @@ static int
> > >> > bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
> > >> >  			pixel_rate = max(432000, pixel_rate);
> > >> >  	}
> > >> >  
> > >> > +	/* According to BSpec, "The CD clock frequency must be at
> > >> > least twice
> > >> > +	 * the frequency of the Azalia BCLK." and BCLK is 96 MHz by
> > >> > default.
> > >> > +	 * The check for GLK has to be adjusted as the platform can
> > >> > output
> > >> > +	 * two pixels per clock.
> > >> > +	 */
> > >> > +	if (crtc_state->has_audio) {
> > >> > +		if (IS_GEMINILAKE(dev_priv))
> > >> > +			pixel_rate = max(2 * 2 * 96000, pixel_rate);
> > >> > +		if (IS_BROXTON(dev_priv))
> > >> 
> > >> SKL also documents this in the page that explains the cdclk freq change
> > >> sequences. The funny thing is that the minimum CDCLK for SKL seems to
> > >> be 308.57, so that's still bigger than 96*2... Anyway, having this for
> > >> completeness would probably be good, just in case I'm missing some
> > >> detail that's important here.
> > >> 
> > >> I'd like to see the SKL addition, but I can live without it in case you
> > >> have some better argument, so if you don't send a new version, here's
> > >> it:
> > >> 
> > >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > >> 
> > >> 
> > >
> > > I did not include SKL as the lowest cdclk freq. it supports was higher
> > > than 2 * 96MHz. But, I agree it's good to include it for clarity. I'll
> > > send another version.
> > >
> > >
> > >> Perhaps it would also be interesting to have some sort of macro to
> > >> identify the platform(s) that need the magic *2 calculation. A more
> > >> confusion-proof version of this function would look like this:
> > >> 
> > >> if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> > >> 	if (HAS_2_PIXELS_PER_CLOCK(dev_priv))
> > 
> > I didn't check the spec (where's the fun in that?!) about the
> > terminology it uses, but isn't that just double data rate, or DDR?
> > 
> > BR,
> > Jani.
> > 
> > 
> 
> I found no reference to "DDR" in the spec., which thankfully avoids
> confusing this with the memory technology.

What this was called back in the gen2/3 days is "double wide pipe". We
could perhaps just keep using that name. Although the gen2/3 thing was
something you could enable/disable on demand.

-- 
Ville Syrjälä
Intel OTC