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

[Paulo Zanoni]

Em Qua, 2017-03-15 às 20:16 +0200, Ville Syrjälä escreveu:
> 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@
> > > 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 int
> > > > > > el.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.

At least in the docs I can find, this feature for the new platform is
always referred to as "two pixels per clock". AFAIR the gen 2/3 thing
is indeed "double wide" and I don't think it makes sense to call the
new thing "double wide", even if it were the exact same thing. IMHO
naming a feature of Gen 9 after something that's only present in Gen
2/3, even if it's the same, only adds confusion to the driver: now
you're requiring that people working on new stuff actually have
knowledge about the super old stuff for basically zero reason. You can
just use the new name in the new context...

This is the same reason why I'm not a big fan of using older #defines
for registers that changed name but are still mostly the same on newer
platforms: it requires that people reading the code actually know about
all the different platforms, while having an additional #define
wouldn't really hurt. But, anyway, that's just MHO.

>