Re: [PATCH 5/6] drm/i915/reset: Handle the display vs. GPU reset deadlock using a custom dma-fence

Christian König
2026-04-09T12:17:25Z

On 4/9/26 13:19, Ville Syrjälä wrote:
> On Thu, Apr 09, 2026 at 12:46:11PM +0200, Christian König wrote:
>> On 4/9/26 01:34, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> The old display vs. GPU reset deadlock is back more or less.
>>> The old (working) solution to the problem was originally
>>> introduced in commit 9db529aac938 ("drm/i915: More surgically
>>> unbreak the modeset vs reset deadlock"), but it got nuked with
>>> commit d59cf7bb73f3 ("drm/i915/display: Use dma_fence interfaces
>>> instead of i915_sw_fence").
>>>
>>> Apparently no one looked hard enough to see that things didn't
>>> work quite properly anymore. What is still saving us for the most
>>> part is that we have a timeout on the fence wait
>>> (CONFIG_DRM_I915_FENCE_TIMEOUT, 10 seconds by default). But
>>> people are perhaps trying to get rid of that so we may need
>>> another solution, and 10 seconds is a bit slow.
>>
>> Yeah agree that approach with the timeout is usually a big no no.
>>
>>> Re-solve the problem yet again with a custom dma-fence that gets
>>> signaled just prior to a GPU reset, and have the atomic commit wait
>>> for either that or the real fence using dma_fence_wait_any_timeout().
>>
>> Hui? I don't fully understand what the source of the problem is, but of hand that approach of solving it doesn't sound like a good idea either.
>>
>>> Whichever signals first will let the commit proceed. We create a new
>>> "reset fence" whenever someone needs one, and keep it until the next
>>> GPU reset has completed. After that the next guy will again get a
>>> fresh unsignaled "reset fence".
>>
>> And that sounds even worse. A dma_fence which waits for the next GPU reset without triggering it itself would be an indefinite dma_fence which is not allowed.
>
> This is a purely internal thing to the i915 display code. This fence
> is never shared with anyone. And we never wait for just this fence,
> the wait always happens alongside a real fence using
> dma_fence_wait_any_timeout().
>
>>
>> I think something is missing in my picture how that is supposed to work.
>
> The problem stems from the fact that on old platforms a GPU reset
> also resets the display hardware,

Which is true for at least AMD GPUs and I think pretty much everybody else as well, but that wasn't so much of a problem so far.

> and to do that safely we need:
> 1. shut down display
> 2. perform the GPU reset
> 3. restore the display hardware to its orignal state

Mhm, I've recently talked with Michel about it and we confirmed that this is perfectly possible without issues. Adding Michel as well.

> We just do that with essentially with a normal atomic commit.

I think that is the source of the problem.

I'm not an expert on that topic but amdgpu and tons of other drivers seem to just use drm_atomic_helper_shutdown() for that.

What is i915 doing differently?

> But a
> previous atomic commit may already be waiting for a fence, which
> won't signal until the GPU reset happens, and the GPU reset is now
> waiting for that previous atomic commit to finish so that it can do
> its own atomic commit. In order to break the deadlock we need to
> abort the fence waits in the atomic commit, and that's what this
> "reset fence" achieves.

As far as I can see that approach looks strongly like a no-go.

You essentially have a lock inversion here and it is documented that it should *never* be resolved by a timeout, the approach you take now is not much better.

Regards,
Christian.

>
>>
>> Thanks,
>> Christian.
>>
>>>
>>> Cc: Simona Vetter <simona.vetter@ffwll.ch>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Jouni Högander <jouni.hogander@intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/display/intel_display.c | 34 +++++---
>>> .../gpu/drm/i915/display/intel_display_core.h | 6 ++
>>> .../drm/i915/display/intel_display_driver.c | 5 ++
>>> .../drm/i915/display/intel_display_reset.c | 77 +++++++++++++++++++
>>> .../drm/i915/display/intel_display_reset.h | 4 +
>>> drivers/gpu/drm/xe/Makefile | 1 +
>>> 6 files changed, 117 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 58a654ca0d20..83ccf13c4b16 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -72,6 +72,7 @@
>>> #include "intel_display_driver.h"
>>> #include "intel_display_power.h"
>>> #include "intel_display_regs.h"
>>> +#include "intel_display_reset.h"
>>> #include "intel_display_rpm.h"
>>> #include "intel_display_types.h"
>>> #include "intel_display_utils.h"
>>> @@ -7149,22 +7150,35 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
>>>
>>> static void intel_atomic_commit_fence_wait(struct intel_atomic_state *intel_state)
>>> {
>>> - struct drm_plane *plane;
>>> + struct intel_display *display = to_intel_display(intel_state);
>>> struct drm_plane_state *new_plane_state;
>>> - long ret;
>>> + struct dma_fence *reset_fence;
>>> + struct drm_plane *plane;
>>> int i;
>>>
>>> + reset_fence = intel_display_reset_fence_get(display);
>>> +
>>> for_each_new_plane_in_state(&intel_state->base, plane, new_plane_state, i) {
>>> - if (new_plane_state->fence) {
>>> - ret = dma_fence_wait_timeout(new_plane_state->fence, false,
>>> - i915_fence_timeout());
>>> - if (ret <= 0)
>>> - break;
>>> + struct dma_fence *fences[2] = {
>>> + [0] = new_plane_state->fence,
>>> + [1] = reset_fence,
>>> + };
>>> + long ret;
>>>
>>> - dma_fence_put(new_plane_state->fence);
>>> - new_plane_state->fence = NULL;
>>> - }
>>> + if (!new_plane_state->fence)
>>> + continue;
>>> +
>>> + ret = dma_fence_wait_any_timeout(fences, reset_fence ? 2 : 1, false,
>>> + i915_fence_timeout(), NULL);
>>> + if (ret <= 0)
>>> + break;
>>> +
>>> + dma_fence_put(new_plane_state->fence);
>>> + new_plane_state->fence = NULL;
>>> }
>>> +
>>> + if (reset_fence)
>>> + dma_fence_put(reset_fence);
>>> }
>>>
>>> static void intel_atomic_dsb_wait_commit(struct intel_crtc_state *crtc_state)
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
>>> index 9e77003addd0..6687b658c51d 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>>> @@ -556,6 +556,12 @@ struct intel_display {
>>> unsigned long mask;
>>> } quirks;
>>>
>>> + struct {
>>> + /* protects reset.fence */
>>> + struct mutex mutex;
>>> + struct dma_fence *fence;
>>> + } reset;
>>> +
>>> struct {
>>> /* restore state for suspend/resume and display reset */
>>> struct drm_atomic_state *modeset_state;
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> index 23bfecc983e8..fcd31722c731 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>>> @@ -34,6 +34,7 @@
>>> #include "intel_display_driver.h"
>>> #include "intel_display_irq.h"
>>> #include "intel_display_power.h"
>>> +#include "intel_display_reset.h"
>>> #include "intel_display_types.h"
>>> #include "intel_display_utils.h"
>>> #include "intel_display_wa.h"
>>> @@ -257,6 +258,8 @@ int intel_display_driver_probe_noirq(struct intel_display *display)
>>>
>>> intel_mode_config_init(display);
>>>
>>> + intel_display_reset_fence_init(display);
>>> +
>>> ret = intel_cdclk_init(display);
>>> if (ret)
>>> goto cleanup_wq_unordered;
>>> @@ -584,6 +587,8 @@ void intel_display_driver_remove(struct intel_display *display)
>>> if (!HAS_DISPLAY(display))
>>> return;
>>>
>>> + intel_display_reset_fence_discard(display);
>>> +
>>> flush_workqueue(display->wq.flip);
>>> flush_workqueue(display->wq.modeset);
>>> flush_workqueue(display->wq.cleanup);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_reset.c b/drivers/gpu/drm/i915/display/intel_display_reset.c
>>> index ca15dc18ef0f..80dd2ea8a0c2 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_reset.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_reset.c
>>> @@ -3,6 +3,8 @@
>>> * Copyright © 2023 Intel Corporation
>>> */
>>>
>>> +#include <linux/dma-fence.h>
>>> +
>>> #include <drm/drm_atomic_helper.h>
>>> #include <drm/drm_print.h>
>>>
>>> @@ -16,6 +18,72 @@
>>> #include "intel_hotplug.h"
>>> #include "intel_pps.h"
>>>
>>> +static const char *intel_display_reset_fence_get_driver_name(struct dma_fence *fence)
>>> +{
>>> + return "intel_display";
>>> +}
>>> +
>>> +static const char *intel_display_reset_fence_get_timeline_name(struct dma_fence *fence)
>>> +{
>>> + return "reset";
>>> +}
>>> +
>>> +static const struct dma_fence_ops intel_display_reset_fence_ops = {
>>> + .get_driver_name = intel_display_reset_fence_get_driver_name,
>>> + .get_timeline_name = intel_display_reset_fence_get_timeline_name,
>>> +};
>>> +
>>> +static void intel_display_reset_create(struct intel_display *display)
>>> +{
>>> + struct dma_fence *fence;
>>> +
>>> + fence = kzalloc_obj(*fence);
>>> + if (!fence)
>>> + return;
>>> +
>>> + dma_fence_init(fence, &intel_display_reset_fence_ops, NULL, 0, 0);
>>> +
>>> + display->reset.fence = fence;
>>> +}
>>> +
>>> +struct dma_fence *intel_display_reset_fence_get(struct intel_display *display)
>>> +{
>>> + struct dma_fence *fence;
>>> +
>>> + mutex_lock(&display->reset.mutex);
>>> +
>>> + if (!display->reset.fence)
>>> + intel_display_reset_create(display);
>>> +
>>> + fence = display->reset.fence;
>>> + if (fence)
>>> + dma_fence_get(fence);
>>> +
>>> + mutex_unlock(&display->reset.mutex);
>>> +
>>> + return fence;
>>> +}
>>> +
>>> +void intel_display_reset_fence_discard(struct intel_display *display)
>>> +{
>>> + struct dma_fence *fence;
>>> +
>>> + mutex_lock(&display->reset.mutex);
>>> +
>>> + fence = display->reset.fence;
>>> + if (fence)
>>> + dma_fence_put(fence);
>>> +
>>> + display->reset.fence = NULL;
>>> +
>>> + mutex_unlock(&display->reset.mutex);
>>> +}
>>> +
>>> +void intel_display_reset_fence_init(struct intel_display *display)
>>> +{
>>> + mutex_init(&display->reset.mutex);
>>> +}
>>> +
>>> bool intel_display_reset_supported(struct intel_display *display)
>>> {
>>> return HAS_DISPLAY(display);
>>> @@ -31,8 +99,15 @@ void intel_display_reset_prepare(struct intel_display *display)
>>> {
>>> struct drm_modeset_acquire_ctx *ctx = &display->restore.reset_ctx;
>>> struct drm_atomic_state *state;
>>> + struct dma_fence *reset_fence;
>>> int ret;
>>>
>>> + reset_fence = intel_display_reset_fence_get(display);
>>> + if (reset_fence) {
>>> + dma_fence_signal(reset_fence);
>>> + dma_fence_put(reset_fence);
>>> + }
>>> +
>>> /*
>>> * Need mode_config.mutex so that we don't
>>> * trample ongoing ->detect() and whatnot.
>>> @@ -110,6 +185,8 @@ void intel_display_reset_finish(struct intel_display *display, bool test_only)
>>>
>>> drm_atomic_state_put(state);
>>> unlock:
>>> + intel_display_reset_fence_discard(display);
>>> +
>>> drm_modeset_drop_locks(ctx);
>>> drm_modeset_acquire_fini(ctx);
>>> mutex_unlock(&display->drm->mode_config.mutex);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_reset.h b/drivers/gpu/drm/i915/display/intel_display_reset.h
>>> index a8aa7729d33f..c36a075c6b4d 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display_reset.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_reset.h
>>> @@ -10,6 +10,10 @@
>>>
>>> struct intel_display;
>>>
>>> +struct dma_fence *intel_display_reset_fence_get(struct intel_display *display);
>>> +void intel_display_reset_fence_discard(struct intel_display *display);
>>> +void intel_display_reset_fence_init(struct intel_display *display);
>>> +
>>> bool intel_display_reset_supported(struct intel_display *display);
>>> bool intel_display_reset_test(struct intel_display *display);
>>> void intel_display_reset_prepare(struct intel_display *display);
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 110fef511fe2..1a85dfe457f0 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -262,6 +262,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>>> i915-display/intel_display_power.o \
>>> i915-display/intel_display_power_map.o \
>>> i915-display/intel_display_power_well.o \
>>> + i915-display/intel_display_reset.o \
>>> i915-display/intel_display_rpm.o \
>>> i915-display/intel_display_rps.o \
>>> i915-display/intel_display_trace.o \
>