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

[Ville Syrjälä]

On Wed, Mar 15, 2017 at 03:39:58PM -0300, Paulo Zanoni wrote:
> 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...

Dunno. I'll keep calling it double wide most likely. One less name for
me to remember ;)

> 
> 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.

We'd have to duplicate boatloads of code if we wanted to match the bspec
register names all the time (+ I have a feeling they keep renaming them
even after the silicon has shipped). And duplicated code just means more
bugs which are also harder to find since it reduces the testing coverage
for each copy of the same code.

Thus we really want to be using the old register names at least when the
code as a whole can be shared. And I think sometimes using the old name
and sometimes the new name would lead to even more confusion. I think
there are a few cases where that is probably happening already, and
people might not even realize because they no longer associate the old
name with the new platform since most other parts of the code are using
the new name.

So I still think trying to follow the latest and greatest naming scheme
of the day would just lead to chaos. IMO the best way to fix this would
be to convinve the hardware folks to stop renaming things all the time.

-- 
Ville Syrjälä
Intel OTC