[Intel-gfx] [PATCH 2/3] drm/i915/display: Do both crawl and squash when changing cdclk

[Matt Roper]

On Mon, Nov 14, 2022 at 03:14:33PM -0800, Srivatsa, Anusha wrote:
...
> > > +static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > +			  const struct intel_cdclk_config *cdclk_config,
> > > +			  enum pipe pipe)
> > > +{
> > > +	struct intel_cdclk_config mid_cdclk_config;
> > > +	int cdclk = cdclk_config->cdclk;
> > > +	int ret;
> > 
> > You should initialize ret to 0 here since you don't set it in the new branch of
> > the if/else ladder below.
> > 
> > > +
> > > +	/* Inform power controller of upcoming frequency change. */
> > > +	if (DISPLAY_VER(dev_priv) >= 14) {
> > > +		/* DISPLAY14+ do not follow the PUnit mailbox
> > communication,
> > 
> > "Display versions 14 and above" or "Xe_LPD+ and beyond"
> > 
> > Also, this is another case where "/*" should be on its own line.
> > 
> > > +		 * skip this step.
> > > +		 */
> > > +	} else if (DISPLAY_VER(dev_priv) >= 11) {
> > > +		ret = skl_pcode_request(&dev_priv->uncore,
> > SKL_PCODE_CDCLK_CONTROL,
> > > +					SKL_CDCLK_PREPARE_FOR_CHANGE,
> > > +					SKL_CDCLK_READY_FOR_CHANGE,
> > > +					SKL_CDCLK_READY_FOR_CHANGE, 3);
> > >  	} else {
> > >  		/*
> > > -		 * The timeout isn't specified, the 2ms used here is based on
> > > -		 * experiment.
> > > -		 * FIXME: Waiting for the request completion could be
> > delayed
> > > -		 * until the next PCODE request based on BSpec.
> > > +		 * BSpec requires us to wait up to 150usec, but that leads to
> > > +		 * timeouts; the 2ms used here is based on experiment.
> > >  		 */
> > >  		ret = snb_pcode_write_timeout(&dev_priv->uncore,
> > >
> > HSW_PCODE_DE_WRITE_FREQ_REQ,
> > > -					      cdclk_config->voltage_level,
> > > -					      150, 2);
> > > +					      0x80000000, 150, 2);
> > 
> > The switch from cdclk_config->voltage_level to a magic number
> > (0x80000000) on old platforms doesn't seem to be related to the rest of this
> > patch.  Ditto for the comment update about 150usec not being enough.
> > Were those intended for a different patch?
> Well, initially both squash and crawl support for MTl was the
> intention. The change came to be a part of this patch because MTL
> doesn't go through the Punit mailbox communication like previous
> platforms and hence the churn. Thinking out loud if changing the
> commit message makes more sense or a separate patch. A separate patch
> would just remove make sure MTL does not touch the bits of code Punit
> code. And the next patch would be the actual
> squash_crawl-mid_cdclk_config patch.

Okay, it looks like part of my confusion is just that the code movement
made the diff deltas somewhat non-intuitive; due to code ordering
changes, it's actually giving a diff of two completely different code
blocks that just happen to look similar; you're not actually changing
the value here.

However I still think there might be a problem here.  For pre-MTL
platforms there are supposed to be two pcode transactions, one before
the frequency change and one after.  Before your patch, the 'before'
transaction used a hardcoded 0x80000000 and the 'after' transaction used
cdclk_config->voltage_level [1].  Your patch keeps the 'before' step at the
beginning of bxt_set_cdclk, but it seems to drop the 'after' step as far
as I can see.  I don't believe that was intentional was it?


Matt


[1]  It looks like we might need some other updates to that pcode stuff
in general for some pre-MTL platforms.  What we have today doesn't match
what I see on the bspec for TGL at least (bspec 49208).  That would be
work for a different series though; just figured I should mention it...

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation