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

[Gustavo Sousa]

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.

>
>> +                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. Althought we could make this more
self-contained by jumping an error path inside this function.

>
>> +                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.

--
Gustavo Sousa

>
>> +        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");
>
>Ditto.
>
>> +                return false;
>> +        }
>> +
>> +        return true;
>> +}
>
>Raag