> -----Original Message-----
> From: Jadav, Raag <raag.jadav@xxxxxxxxx>
> Sent: Monday, 28 October 2024 16.43
> To: Sousa, Gustavo <gustavo.sousa@xxxxxxxxx>
> Cc: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/i915/xe3lpd: Power request asserting/deasserting
>
> On Mon, Oct 28, 2024 at 11:21:59AM -0300, Gustavo Sousa wrote:
> > Quoting Raag Jadav (2024-10-28 11:02:02-03:00)
> > >On Mon, Oct 28, 2024 at 02:58:35PM +0200, Mika Kahola wrote:
> > >
> > >...
> > >
> > >> +static bool wa_tcss_power_request_assert(struct drm_i915_private *i915,
> > >> + bool enable) {
> > >> + /*
> > >> + * Limit to PTL only
> > >> + * TODO: Add check for PICA IP and use that instead limiting WA for
> > >> + * platform
> > >> + */
> > >> + if (DISPLAY_VER(i915) != 30)
> > >
> > >Not sure if hardcoding constants is the best of the approach, but I
> > >found similar patterns in other places, so upto you.
> >
> > Using version number directly makes it easier to concurrently merge
> > independent patches for some display IP without having to wait some
> > #define to become available. That comes with the cost of having to
> > remember the version numbers (or checking somewhere) though.
>
> Agree. Although usually we have codenames for this, which is easier to track or
> grep.
>
> > >
> > >> + return true;
> > >> +
> > >> + /* check if mailbox is running busy */
> > >> + if (intel_de_wait_for_clear(i915, TCSS_DISP_MAILBOX_IN_CMD,
> > >> + TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY, 10)) {
> > >> + drm_dbg_kms(&i915->drm,
> > >> + "Power request assert WA timeout
> > >> + waiting for TCSS mailbox run/busy bit to clear\n");
> > >
> > >Is timeout considered a failure? Not sure _dbg is the right helper if it.
> >
> > I believe it is okay to have _dbg here, because the returned value is
> > checked and a warning is raised.
>
> Which makes the logging redundant...
>
> > Althought we could make this more
> > self-contained by jumping an error path inside this function.
>
> ... but if we'd still want it I think this will be best.
>
> >
> > >
> > >> + return false;
> > >> + }
> > >> +
> > >> + if (enable)
> > >> + intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, 1);
> > >> + else
> > >> + intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA,
> > >> + 0);
> > >
> > >Since enable it already a bool, this can be
> > >
> > > intel_de_write(i915, TCSS_DISP_MAILBOX_IN_DATA, enable);
> > >
> > >> +
> > >> + intel_de_write(i915, TCSS_DISP_MAILBOX_IN_CMD,
> > >> + TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY |
> > >> + TCSS_DISP_MAILBOX_IN_CMD_DATA(0x1));
> > >> +
> > >> + /* wait to clear mailbox running busy bit before
> > >> + continuing */
> > >
> > >Any specific reason to do this on exit?
> > >I'm assuming anyone trying to use the mailbox further down would be
> > >doing this anyway since it's a prerequisite, right?
> >
> > This wait is part of the "DE to IOM Mailbox" flow. I believe this is
> > necessary and the workaround might even not be effective without it.
>
> My understanding is that the wait is for the availability of mailbox which is not
> equivalent to command success, but I could be wrong.
I think these two are related. We could consider command to be successful if mailbox is free to use within a certain timeframe. Otherwise, I would consider that there might be something wrong with the communication or command hasn't been successfully executed.
-Mika-
>
> Raag
