Re: [PATCH RFC] drm/i915/cx0: request TCSS power for the non-TC C20 PHY on PTL port B

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915/tc: Race condition on global TCSS mailbox registers
- [Medium] i915/cx0: missing wait for TCSS power state to clear on disable
--

commit 61a233a7f9a55965dffb355729a4d07449153a73
Author: Burak Göncü <burakgon12@gmail.com>

drm/i915/cx0: request TCSS power for the non-TC C20 PHY on PTL port B

This commit requests TCSS power for the non-TC C20 PHY on PTL port B before
programming the PLL, waits for the acknowledgment, and releases the request
once the PLL is disabled. This mirrors what the TC PHY code does for TC ports.

Link: https://github.com/therealarnold666/zenbook-duo26-Ubuntu26.04

> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index dbebd72108484..010c7db063518 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
[ ... ]
> +static void intel_cx0_request_tcss_power(struct intel_encoder *encoder, bool enable)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +
> +	intel_tc_request_tcss_power(encoder, enable);
> +
> +	if (enable &&
> +	    intel_de_wait_for_set_ms(display, XELPDP_PORT_BUF_CTL1(display, encoder->port),
> +				     XELPDP_TCSS_POWER_STATE, 5))
> +		drm_warn(display->drm, "Port %c: TCSS power request not acknowledged\n",
> +			 port_name(encoder->port));
> +}

[Severity: Medium]
When releasing the power request during intel_cx0pll_disable() where enable
is false, does this skip the wait-for-clear logic?

The wait condition here strictly requires enable to be true. Tearing down
power domains, such as POWER_DOMAIN_DC_OFF in intel_cx0_phy_transaction_end(),
while the PHY is still asynchronously powering down could cause hardware hangs
or failures on the next enable.

The corresponding TC PHY code in xelpdp_tc_phy_enable_tcss_power() appears
to correctly wait for the power state to clear via
xelpdp_tc_phy_wait_for_tcss_power(). Should a similar wait for clear be
added here when enable is false?

[ ... ]
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c
> index f2d86b29c4154..2a8665d4cd0b4 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
[ ... ]
> +void intel_tc_request_tcss_power(struct intel_encoder *encoder, bool enable)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	intel_reg_t reg = XELPDP_PORT_BUF_CTL1(display, encoder->port);
> +
> +	if (!!(intel_de_read(display, reg) & XELPDP_TCSS_POWER_REQUEST) == enable)
> +		return;
> +
> +	if (DISPLAY_VER(display) == 30)
> +		xelpdp_tc_power_request_wa(display, enable);

[Severity: High]
Could this introduce a race condition on the global TCSS mailbox registers?

The TCSS mailbox registers, TCSS_DISP_MAILBOX_IN_CMD and
TCSS_DISP_MAILBOX_IN_DATA, are a single set of global MMIO registers per
display engine.

The xelpdp_tc_power_request_wa() function executes a multi-step
read-modify-write sequence on these registers:

intel_tc.c:xelpdp_tc_power_request_wa() {
	...
	intel_de_write(display, TCSS_DISP_MAILBOX_IN_DATA, enable ? 1 : 0);
	intel_de_write(display, TCSS_DISP_MAILBOX_IN_CMD,
		       TCSS_DISP_MAILBOX_IN_CMD_RUN_BUSY |
		       TCSS_DISP_MAILBOX_IN_CMD_DATA(0x1));
	...
}

This sequence currently lacks a global lock to serialize access.

By introducing a new caller here, the global mailbox is exposed to concurrent
modeset paths that are only protected by display->dpll.lock. Because the
existing TC port hotplug and disconnect work items only hold the per-port
tc->lock, concurrent execution could interleave the DATA and CMD writes.

For example, if a TC port disconnects while Port B is executing a modeset,
or if two TC ports disconnect simultaneously, the mailbox command payload
could be corrupted, potentially leading to hardware handshake failures or
invalid power states. Is there a need for a global lock here to serialize
these mailbox writes?

> +
> +	intel_de_rmw(display, reg, XELPDP_TCSS_POWER_REQUEST,
> +		     enable ? XELPDP_TCSS_POWER_REQUEST : 0);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-ptl-port-b-tcss-v1-1-240670342cd3@gmail.com?part=1