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
