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

[Srivatsa, Anusha]

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

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