[PATCH] drm/i915/xe3lpd: Power request asserting/deasserting

[Kahola, Mika]

> -----Original Message-----
> From: Jadav, Raag <raag.jadav at intel.com>
> Sent: Monday, 28 October 2024 16.43
> To: Sousa, Gustavo <gustavo.sousa at intel.com>
> Cc: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at lists.freedesktop.org
> 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