Re: [PATCH] drm/amd/display: fall back to software I2C on hardware engine failure

Re: [PATCH] drm/amd/display: fall back to software I2C on hardware engine failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Pre-existing resource leak in acquire_i2c_hw_engine leaves DDC pin permanently open on setup failure.
- [High] TOCTOU race on shared I2C hardware buffer flag `i2c_hw_buffer_in_use` bypasses global mutual exclusion.
--

commit 715340ad0a0bb4f4ee8ee30d4c2c692856bedacf
Author: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>

drm/amd/display: fall back to software I2C on hardware engine failure

This commit implements a fallback mechanism to the bit-banging software I2C
engine when the hardware engine fails to complete longer transfers. It retries
the transfer on the software engine instead of outright failing, which resolves
EDID read failures on DCE6/Oland hardware.

> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c.c b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c.c
> index f5261e8d76785..238c17e6f51db 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c.c
> @@ -72,9 +72,16 @@ bool dce_i2c_submit_command(
>  
>  	dce_i2c_hw = acquire_i2c_hw_engine(pool, ddc);

[Severity: High]
This isn't a bug introduced by this patch, but does acquire_i2c_hw_engine()
leak the DDC pin on setup failure?

In acquire_i2c_hw_engine(), dal_ddc_open() is called successfully, but if
the hardware engine is busy (e.g., locked by DMCU), setup_engine() returns
false. The error path then calls release_engine() without calling
dal_ddc_close():

    if (!setup_engine(dce_i2c_hw)) {
        release_engine(dce_i2c_hw);
        return NULL;
    }

This leaves the pin leaked in the opened state. As a result, subsequent calls
to dal_ddc_open() will return GPIO_RESULT_ALREADY_OPENED, which breaks all
future I2C transfers (and display hotplug/EDID detection) on that port until
reboot. Is there a missing dal_ddc_close() here?

[Severity: High]
This is a pre-existing issue, but does acquire_i2c_hw_engine() have a TOCTOU
race on the shared pool->i2c_hw_buffer_in_use flag?

The function checks and sets pool->i2c_hw_buffer_in_use without a global
lock:

    if (pool->i2c_hw_buffer_in_use || !is_engine_available(dce_i2c_hw))
        return NULL;
    ...
    pool->i2c_hw_buffer_in_use = true;

Because I2C adapter locks in amdgpu_dm_i2c_xfer() are per-connector,
concurrent I2C transfers on different display connectors can execute
acquire_i2c_hw_engine() simultaneously. Both threads can read
pool->i2c_hw_buffer_in_use as false, proceed to configure their respective
hardware engines concurrently, and set the flag to true. Can this bypass
global mutual exclusion and cause hardware timeouts or corrupted I2C
transactions?

> -	if (dce_i2c_hw)
> -		return dce_i2c_submit_command_hw(pool, ddc, cmd, dce_i2c_hw);
> +	if (dce_i2c_hw && dce_i2c_submit_command_hw(pool, ddc, cmd, dce_i2c_hw))
> +		return true;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826170549.21985-1-neptune@imm0nv1nhtv.is-a.dev?part=1

[PATCH 1/2] drm/amd/display: skip receiver power control without AUX

[NepNep7601]

Passive DP to TMDS dongles do not provide a DP receiver and use native
GPIO I2C rather than AUX. dpcd_write_rx_power_ctrl() nevertheless tries
to write DP_SET_POWER, causing the DP helpers to retry a transaction
that cannot succeed 32 times before giving up.

Skip receiver power control when the link is not using AUX mode.

On an Oland GPU with a passive DP to HDMI to DVI chain, this reduced
boot-time "DP AUX transfer fail" messages from 32 to 0. The EDID
remained 256 bytes and the display continued to use its native
1600x900 mode.

Assisted-by: Claude:claude-opus-5
Signed-off-by: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>
---
 .../gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c  | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
index 49521ac4b0e8..7991531f6ef4 100644
--- a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
+++ b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
@@ -50,6 +50,15 @@ void dpcd_write_rx_power_ctrl(struct dc_link *link, bool on)
 	if (link->sync_lt_in_progress)
 		return;
 
+	/*
+	 * A passive DP to TMDS dongle presents no DP receiver, so there is
+	 * nothing to power up or down. The write can only fail, and the DP
+	 * helpers retry it 32 times before giving up, which adds tens of
+	 * milliseconds to link bring up.
+	 */
+	if (!link->aux_mode)
+		return;
+
 	core_link_write_dpcd(link, DP_SET_POWER, &state,
 						 sizeof(state));
 
-- 
2.47.3

[PATCH 2/2] drm/amd/display: close DDC on I2C engine setup failure

[NepNep7601]

acquire_i2c_hw_engine() opens the DDC pins before setting up the
hardware engine. If setup_engine() fails, the error path releases the
engine but leaves the DDC pins open and the engine's DDC pointer set.

Subsequent attempts to open the pins then return
GPIO_RESULT_ALREADY_OPENED, preventing further I2C transfers, EDID
reads and hotplug detection on that port until reboot.

Mirror the normal teardown path by closing the DDC pins and clearing
the pointer after releasing the engine.

Assisted-by: Codex:gpt-5
Signed-off-by: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>
---
 drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
index 05892ab4529f..e7a05494abab 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
@@ -485,6 +485,8 @@ struct dce_i2c_hw *acquire_i2c_hw_engine(
 
 	if (!setup_engine(dce_i2c_hw)) {
 		release_engine(dce_i2c_hw);
+		dal_ddc_close(dce_i2c_hw->ddc);
+		dce_i2c_hw->ddc = NULL;
 		return NULL;
 	}
 
-- 
2.47.3

Re: [PATCH 2/2] drm/amd/display: close DDC on I2C engine setup failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless read-modify-write of the shared `pool->i2c_hw_buffer_in_use` flag allows concurrent access to hardware I2C resources.
--

commit 12dc016e9bb9b2e123aaf512011f08b0b29e4bd1
Author: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>

drm/amd/display: close DDC on I2C engine setup failure

acquire_i2c_hw_engine() opens the DDC pins before setting up the
hardware engine. If setup_engine() fails, the error path releases the
engine but leaves the DDC pins open and the engine's DDC pointer set.

> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> index 05892ab4529f6..e7a05494abab0 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> @@ -485,6 +485,8 @@ struct dce_i2c_hw *acquire_i2c_hw_engine(
>  
>  	if (!setup_engine(dce_i2c_hw)) {
>  		release_engine(dce_i2c_hw);
> +		dal_ddc_close(dce_i2c_hw->ddc);
> +		dce_i2c_hw->ddc = NULL;
>  		return NULL;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does this lockless read-modify-write of the
shared pool->i2c_hw_buffer_in_use flag allow concurrent access to hardware I2C
resources?

In drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c:acquire_i2c_hw_engine(),
there is an initial lockless check:

    if (pool->i2c_hw_buffer_in_use || !is_engine_available(dce_i2c_hw))
        return NULL;

And shortly after this diff hunk, it sets the flag:

    pool->i2c_hw_buffer_in_use = true;

Since amdgpu_dm_i2c_xfer() is called by the Linux I2C core, it serializes
operations per-adapter (per port). However, different DDC ports have different
adapters and can run concurrently. If they call dc_submit_i2c() ->
dce_i2c_submit_command() -> acquire_i2c_hw_engine() without taking a
driver-wide lock to serialize access across the single shared resource_pool,
both threads can read false and bypass the exclusion.

This could lead to hardware I2C data corruption or engine lockups.
Furthermore, one thread could clear the flag prematurely in
dce_i2c_submit_command_hw() while the other is still active, corrupting the
state machine and allowing a third thread to incorrectly enter. Userspace can
trigger this trivially by opening multiple /dev/i2c-* nodes for different AMD
display ports and issuing I2C transactions simultaneously.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826193602.6441-1-neptune@imm0nv1nhtv.is-a.dev?part=2

Re: [PATCH 1/2] drm/amd/display: skip receiver power control without AUX

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added early return in `dpcd_write_rx_power_ctrl()` incorrectly relies on `link->aux_mode` before it is initialized, causing the DP receiver power-up sequence to be skipped for all newly detected DP monitors and active dongles.
--

commit 6bcb07f6c6cfc4188f9856f76b47789a74cb855e
Author: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>

drm/amd/display: skip receiver power control without AUX
    
Passive DP to TMDS dongles do not provide a DP receiver and use native
GPIO I2C rather than AUX. dpcd_write_rx_power_ctrl() nevertheless tries
to write DP_SET_POWER, causing the DP helpers to retry a transaction
that cannot succeed 32 times before giving up. Skip receiver power
control when the link is not using AUX mode.

> diff --git a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
> index 49521ac4b0e85..7991531f6ef44 100644
> --- a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
> +++ b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
> @@ -50,6 +50,15 @@ void dpcd_write_rx_power_ctrl(struct dc_link *link, bool on)
>  	if (link->sync_lt_in_progress)
>  		return;
>  
> +	/*
> +	 * A passive DP to TMDS dongle presents no DP receiver, so there is
> +	 * nothing to power up or down. The write can only fail, and the DP
> +	 * helpers retry it 32 times before giving up, which adds tens of
> +	 * milliseconds to link bring up.
> +	 */
> +	if (!link->aux_mode)
> +		return;

[Severity: High]
Does this early return skip the DP receiver power-up sequence for all newly
detected DP monitors and active dongles?

During system boot or display hotplug, the dc_link struct is zero-allocated,
so link->aux_mode defaults to false. The detection sequence starts with
detect_link_and_local_sink(), which calls detect_dp() -> retrieve_link_cap()
-> dp_wa_power_up_0010FA(), eventually calling dpcd_write_rx_power_ctrl().

Since link->aux_mode is still false at this point, the early return triggers
and skips the DP_SET_POWER D0 write. This breaks detection for active dongles
(such as the 0010FA) that depend on being powered up before a DPCD read.

Looking at detect_link_and_local_sink() in link_detection.c, link->aux_mode
is actually initialized after detect_dp() has already executed and failed:

drivers/gpu/drm/amd/display/dc/link/link_detection.c:detect_link_and_local_sink() {
    ...
    if (!detect_dp(link, &sink_caps, reason)) {
    ...
    link->aux_mode =
            link_is_in_aux_transaction_mode(link->ddc);
    ...
}

Should we ensure link->aux_mode is initialized earlier in the detection flow,
or use a different condition to skip the power control write?

> +
>  	core_link_write_dpcd(link, DP_SET_POWER, &state,
>  						 sizeof(state));
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826193602.6441-1-neptune@imm0nv1nhtv.is-a.dev?part=1

[PATCH v2 1/2] drm/amd/display: skip receiver power control without AUX

[NepNep7601]

Passive DP to TMDS dongles do not provide a DP receiver and use native
GPIO I2C rather than AUX. The normal link PHY and stream blanking paths
nevertheless try to write DP_SET_POWER, causing the DP helpers to retry
a transaction that cannot succeed 32 times before giving up.

Gate the receiver-power calls in dp_enable_link_phy(),
dp_disable_link_phy() and link_blank_dp_stream() on the post-detection
aux_mode state. Keep dpcd_write_rx_power_ctrl() unchanged because early
DP detection calls it before aux_mode is initialized, and active dongles
may require the receiver power-up before DPCD reads.

On an Oland GPU with a passive DP to HDMI to DVI chain, this reduced
boot-time "DP AUX transfer fail" messages from 32 to 0. The EDID
remained 256 bytes and the display continued to use its native
1600x900 mode.

Assisted-by: Claude:claude-opus-5
Assisted-by: Codex:gpt-5
Signed-off-by: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>
---

Notes (amdgpu-followups-v2):
    v2:
    - Move the aux_mode check out of dpcd_write_rx_power_ctrl() so early
      detection can still power active dongles before reading DPCD.
    - Gate receiver-power writes at the normal PHY and stream-blanking call
      sites.
    - Retest the revised placement on the affected Oland system.
    
    v1: https://lore.kernel.org/r/20260826193602.6441-1-neptune@imm0nv1nhtv.is-a.dev

 drivers/gpu/drm/amd/display/dc/link/link_dpms.c        |  6 ++++--
 .../drm/amd/display/dc/link/protocols/link_dp_phy.c    | 10 ++++++----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/link/link_dpms.c b/drivers/gpu/drm/amd/display/dc/link/link_dpms.c
index 48b086d15ab0..81d63e1aab53 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_dpms.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_dpms.c
@@ -146,8 +146,10 @@ void link_blank_dp_stream(struct dc_link *link, bool hw_init)
 				}
 		}
 
-		if (((!dc->is_switch_in_progress_dest) && ((!link->wa_flags.dp_keep_receiver_powered) || hw_init)) &&
-			(link->type != dc_connection_none))
+		if (link->aux_mode &&
+		    !dc->is_switch_in_progress_dest &&
+		    (!link->wa_flags.dp_keep_receiver_powered || hw_init) &&
+		    link->type != dc_connection_none)
 			dpcd_write_rx_power_ctrl(link, false);
 	}
 }
diff --git a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
index 49521ac4b0e8..24f09bdcab51 100644
--- a/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
+++ b/drivers/gpu/drm/amd/display/dc/link/protocols/link_dp_phy.c
@@ -65,7 +65,8 @@ void dp_enable_link_phy(
 	link->cur_link_settings = *link_settings;
 	link->dc->hwss.enable_dp_link_output(link, link_res, signal,
 			clock_source, link_settings);
-	dpcd_write_rx_power_ctrl(link, true);
+	if (link->aux_mode)
+		dpcd_write_rx_power_ctrl(link, true);
 }
 
 void dp_disable_link_phy(struct dc_link *link,
@@ -74,9 +75,10 @@ void dp_disable_link_phy(struct dc_link *link,
 {
 	struct dc  *dc = link->ctx->dc;
 
-	if (!link->wa_flags.dp_keep_receiver_powered &&
-			!link->skip_implict_edp_power_control &&
-			link->type != dc_connection_none)
+	if (link->aux_mode &&
+	    !link->wa_flags.dp_keep_receiver_powered &&
+	    !link->skip_implict_edp_power_control &&
+	    link->type != dc_connection_none)
 		dpcd_write_rx_power_ctrl(link, false);
 
 	dc->hwss.disable_link_output(link, link_res, signal);
-- 
2.47.3

[PATCH v2 2/2] drm/amd/display: close DDC on I2C engine setup failure

[NepNep7601]

acquire_i2c_hw_engine() opens the DDC pins before setting up the
hardware engine. If setup_engine() fails, the error path releases the
engine but leaves the DDC pins open and the engine's DDC pointer set.

Subsequent attempts to open the pins then return
GPIO_RESULT_ALREADY_OPENED, preventing further I2C transfers, EDID
reads and hotplug detection on that port until reboot.

Mirror the normal teardown path by closing the DDC pins and clearing
the pointer after releasing the engine.

Assisted-by: Codex:gpt-5
Signed-off-by: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>
---
 drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
index 05892ab4529f..e7a05494abab 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
@@ -485,6 +485,8 @@ struct dce_i2c_hw *acquire_i2c_hw_engine(
 
 	if (!setup_engine(dce_i2c_hw)) {
 		release_engine(dce_i2c_hw);
+		dal_ddc_close(dce_i2c_hw->ddc);
+		dce_i2c_hw->ddc = NULL;
 		return NULL;
 	}
 
-- 
2.47.3

Re: [PATCH v2 2/2] drm/amd/display: close DDC on I2C engine setup failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless access to the shared `pool->i2c_hw_buffer_in_use` flag allows concurrent HW I2C engine usage across different display connectors.
--

commit e10eae8a87c1382752a4ccbab3d8554652f1a66a
Author: NepNep7601 <neptune@imm0nv1nhtv.is-a.dev>

drm/amd/display: close DDC on I2C engine setup failure

This commit ensures the DDC pins are closed and the DDC pointer is cleared if
I2C hardware engine setup fails. This prevents the port from becoming
permanently blocked due to the pins remaining open.

> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> index 05892ab4529f6..e7a05494abab0 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_i2c_hw.c
> @@ -485,6 +485,8 @@ struct dce_i2c_hw *acquire_i2c_hw_engine(
>  
>  	if (!setup_engine(dce_i2c_hw)) {
>  		release_engine(dce_i2c_hw);
> +		dal_ddc_close(dce_i2c_hw->ddc);
> +		dce_i2c_hw->ddc = NULL;
>  		return NULL;
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for concurrent
I2C transfers to bypass the pool lock and corrupt the hardware buffer?

Earlier in acquire_i2c_hw_engine(), there is a lockless check of
the shared pool state:

	if (pool->i2c_hw_buffer_in_use || !is_engine_available(dce_i2c_hw))
		return NULL;

If two different I2C adapters initiate transfers concurrently, they are
serialized locally by their per-adapter I2C bus lock, but they can enter
acquire_i2c_hw_engine() simultaneously because they share the same GPU
resource pool.

Both threads could read pool->i2c_hw_buffer_in_use as false at the same
time, proceed to acquire their respective hardware engines, and then both
update the state locklessly at the end of this function:

	pool->i2c_hw_buffer_in_use = true;

Could this race condition be triggered by concurrent I2C accesses via
userspace tools (like ddcutil) or simultaneous hotplug events, leading to
concurrent access and I2C payload corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826170549.21985-1-neptune@imm0nv1nhtv.is-a.dev?part=2