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
