[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:43 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 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.

I meant add the if-else for Punit mailbox communication to the existing drm-tip as one patch and add mid_cdclk_config on top of this change as a separate patch. 

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