> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Monday, November 14, 2022 4:43 PM
> To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> > > Sent: Monday, November 14, 2022 4:01 PM
> > > To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
> > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ville Syrjälä
> > > <ville.syrjala@xxxxxxxxxxxxxxx>
> > > 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
