[PATCH AUTOSEL 6.19] drm/amd/display: Fix mismatched unlock for DMUB HW lock in HWSS fast path

[Sasha Levin <sashal@kernel.org>]

From: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

[ Upstream commit af3303970da5ce5bfe6dffdd07f38f42aad603e0 ]

[Why]
The evaluation for whether we need to use the DMUB HW lock isn't the
same as whether we need to unlock which results in a hang when the
fast path is used for ASIC without FAMS support.

[How]
Store a flag that indicates whether we should use the lock and use
that same flag to specify whether unlocking is needed.

Reviewed-by: Swapnil Patel <swapnil.patel@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Signed-off-by: Chenyu Chen <chen-yu.chen@amd.com>
Tested-by: Daniel Wheeler <daniel.wheeler@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have a complete understanding. Let me summarize my analysis.

---

## Detailed Analysis

### 1. Commit Message Analysis

The commit message is clear and well-structured (AMD's `[Why]`/`[How]`
format):

- **[Why]:** The evaluation for whether to **acquire** the DMUB HW lock
  is different from the evaluation for whether to **release** it, which
  causes a **hang** on ASICs without FAMS support.
- **[How]:** Store the lock requirement decision in a single variable
  and reuse it for both lock and unlock.
- **Indicators:** "Fix", "hang" — these are strong backport signals.
- **Tags:** `Reviewed-by`, `Tested-by` — this has been properly reviewed
  and tested.

### 2. Code Change Analysis — The Bug Mechanism

The bug is in `hwss_build_fast_sequence()` in `dc_hw_sequencer.c`. This
function builds a sequence of hardware programming blocks that are
executed during fast display updates (page flips, cursor updates, etc.).

**The lock acquisition (line ~765-772 of buggy code):**

```765:772:drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
        if (dc->hwss.dmub_hw_control_lock_fast) {
block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.dc =
dc;
block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.lock
= true;
                block_sequence[*num_steps].params.dmub_hw_control_lock_f
ast_params.is_required =
                        dc_state_is_fams2_in_use(dc, context) ||
                        dmub_hw_lock_mgr_does_link_require_lock(dc,
stream->link);
                block_sequence[*num_steps].func =
DMUB_HW_CONTROL_LOCK_FAST;
```

**The lock release (line ~906-911 of buggy code):**

```906:911:drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
        if (dc->hwss.dmub_hw_control_lock_fast) {
block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.dc =
dc;
block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.lock
= false;
                block_sequence[*num_steps].params.dmub_hw_control_lock_f
ast_params.is_required = dc_state_is_fams2_in_use(dc, context);
                block_sequence[*num_steps].func =
DMUB_HW_CONTROL_LOCK_FAST;
                (*num_steps)++;
```

The critical mismatch:
- **Lock:** `is_required = dc_state_is_fams2_in_use(dc, context) ||
  dmub_hw_lock_mgr_does_link_require_lock(dc, stream->link)`
- **Unlock:** `is_required = dc_state_is_fams2_in_use(dc, context)` —
  **MISSING the `dmub_hw_lock_mgr_does_link_require_lock` check!**

Looking at `dcn401_dmub_hw_control_lock_fast()`, when `is_required` is
`true`, it sends a DMUB inbox0 command to acquire/release a hardware
lock. When `is_required` is `false`, it does nothing.

**The hang scenario:**
1. FAMS2 is NOT in use (`dc_state_is_fams2_in_use()` returns `false`)
2. The link requires lock due to PSR or Replay being enabled
   (`dmub_hw_lock_mgr_does_link_require_lock()` returns `true`)
3. Lock path: `is_required = false || true = true` → **lock IS
   acquired**
4. Unlock path: `is_required = false` → unlock is **NOT performed**
5. The hardware lock is now permanently held → **system hangs**

This is triggered specifically on ASICs that use PSR (Panel Self
Refresh) or Replay but do NOT have FAMS2 enabled, which is a real-world
configuration (many AMD laptops with eDP panels use PSR).

### 3. Bug Origin

The bug was introduced by commit `7d041982fe11` ("drm/amd/display:
Extend inbox0 lock to run Replay/PSR"), which added the
`dmub_hw_lock_mgr_does_link_require_lock()` call to the **lock** path
but forgot to update the **unlock** path. That commit first appeared in
`v6.19-rc1`.

### 4. The Fix

The fix is minimal and surgical — it introduces a local boolean variable
`is_dmub_lock_required` that stores the lock decision once, then reuses
it for both lock and unlock:

```c
bool is_dmub_lock_required = false;
// ...
if (dc->hwss.dmub_hw_control_lock_fast) {
    is_dmub_lock_required = dc_state_is_fams2_in_use(dc, context) ||
                            dmub_hw_lock_mgr_does_link_require_lock(dc,
stream->link);
    // ... lock with is_dmub_lock_required ...
}
// ... later ...
if (dc->hwss.dmub_hw_control_lock_fast) {
    // ... unlock with is_dmub_lock_required (same value!) ...
}
```

This guarantees the lock and unlock always use the same `is_required`
value.

### 5. Scope and Risk Assessment

- **Files changed:** 1 file (`dc_hw_sequencer.c`)
- **Lines changed:** ~6 lines net (variable declaration + 3 modified
  lines)
- **Risk:** Extremely low — the fix is purely ensuring consistent
  behavior between lock/unlock. No new logic, no behavioral changes for
  correctly-working paths.
- **Subsystem:** AMD display driver (amdgpu/dc), which is widely used on
  AMD GPUs

### 6. User Impact

- **Severity:** **System hang** — this is a hard hang when the DMUB
  hardware lock is held permanently. The GPU display hardware becomes
  unresponsive, likely requiring a reboot.
- **Who is affected:** Users with AMD GPUs using PSR or Replay (eDP
  laptop panels) on ASICs without FAMS2 support. This is a common laptop
  configuration.
- **Trigger:** Normal display fast-path updates (page flips) — this is a
  hot path exercised during normal display operation.

### 7. Stability Indicators

- `Reviewed-by: Swapnil Patel` — peer reviewed
- `Tested-by: Daniel Wheeler` — tested by AMD's QA
- The author (Nicholas Kazlauskas) is a key AMD display developer
- The fix is obviously correct by inspection — lock/unlock must use the
  same condition

### 8. Dependencies

The fix depends on commit `7d041982fe11` ("Extend inbox0 lock to run
Replay/PSR") being present. That commit introduced the bug and is in
`v6.19-rc1` and later. The fix only needs to go to stable trees that
contain the buggy commit. The `hwss_build_fast_sequence` function itself
has existed since v6.10 (commit `5324e2b205a2`), but the specific bug
was only introduced by the inbox0 extension in 6.19-rc1.

Since the buggy commit went into `v6.19-rc1`, the fix would primarily
target the 6.19.y stable tree. If the buggy commit was also backported
to earlier stable trees, the fix should follow it.

### 9. Classification

This is a clear **bug fix** for a **system hang** caused by **mismatched
lock/unlock logic**. It:
- Fixes a real bug (hardware lock never released → hang)
- Is obviously correct (use the same condition for lock and unlock)
- Is small and surgical (6 lines changed in 1 file)
- Does not introduce any new features or APIs
- Has been reviewed and tested
- Fixes a severe user-visible issue (system hang)

**YES** — this commit should be backported to stable kernel trees.

**YES**

 drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
index e2763b60482a0..052d573408c3e 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_hw_sequencer.c
@@ -741,6 +741,7 @@ void hwss_build_fast_sequence(struct dc *dc,
 	struct dce_hwseq *hws = dc->hwseq;
 	struct pipe_ctx *current_pipe = NULL;
 	struct pipe_ctx *current_mpc_pipe = NULL;
+	bool is_dmub_lock_required = false;
 	unsigned int i = 0;
 
 	*num_steps = 0; // Initialize to 0
@@ -763,11 +764,12 @@ void hwss_build_fast_sequence(struct dc *dc,
 		(*num_steps)++;
 	}
 	if (dc->hwss.dmub_hw_control_lock_fast) {
+		is_dmub_lock_required = dc_state_is_fams2_in_use(dc, context) ||
+					dmub_hw_lock_mgr_does_link_require_lock(dc, stream->link);
+
 		block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.dc = dc;
 		block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.lock = true;
-		block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.is_required =
-			dc_state_is_fams2_in_use(dc, context) ||
-			dmub_hw_lock_mgr_does_link_require_lock(dc, stream->link);
+		block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.is_required = is_dmub_lock_required;
 		block_sequence[*num_steps].func = DMUB_HW_CONTROL_LOCK_FAST;
 		(*num_steps)++;
 	}
@@ -906,7 +908,7 @@ void hwss_build_fast_sequence(struct dc *dc,
 	if (dc->hwss.dmub_hw_control_lock_fast) {
 		block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.dc = dc;
 		block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.lock = false;
-		block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.is_required = dc_state_is_fams2_in_use(dc, context);
+		block_sequence[*num_steps].params.dmub_hw_control_lock_fast_params.is_required = is_dmub_lock_required;
 		block_sequence[*num_steps].func = DMUB_HW_CONTROL_LOCK_FAST;
 		(*num_steps)++;
 	}
-- 
2.51.0