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

[Matt Roper]

On Mon, Nov 14, 2022 at 04:07:13PM -0800, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: Roper, Matthew D <matthew.d.roper at intel.com>
> > Sent: Monday, November 14, 2022 4:01 PM
> > To: Srivatsa, Anusha <anusha.srivatsa at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org; Ville Syrjälä
> > <ville.syrjala at linux.intel.com>
> > Subject: Re: [PATCH 2/3] drm/i915/display: Do both crawl and squash when
> > changing cdclk
> > 
> > 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?
> 
> That was not the intention here. I think the Pcode thing needs to be a
> separate patch? Might make reviewing easy. 

The pcode handling in the current code is what we consider correct-ish
(although as noted in [1] below, not 100% correct).  So I'm not sure
what you mean by a separate patch for the pcode thing.  Do you mean
refactoring out the _bxt_set_cdclk() function in an initial patch
without the two-step midpoint stuff (since I think that refactoring is
the point you accidentally deleted the pcode hunk of code)?  You can do
that if you want, although it's also probably fine to just update this
patch to not delete that code too.


Matt

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

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