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