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

[Raag Jadav <raag.jadav@intel.com>]

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.

Raag