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

[Raag Jadav]

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