[PATCH] drm/i915: skip __i915_request_skip() for already signaled requests

[PATCH] drm/i915: skip __i915_request_skip() for already signaled requests

[Sebastian Brzezinka]

After a GPU reset the HWSP is zeroed, so previously completed
requests appear incomplete. If such a request is picked up during
reset_rewind() and marked guilty, i915_request_set_error_once()
returns early (fence already signaled), leaving fence.error without
a fatal error code. The subsequent __i915_request_skip() then hits:
```
GEM_BUG_ON(!fatal_error(rq->fence.error))
```

Fixes a kernel BUG observed on Sandy Bridge (Gen6) during
heartbeat-triggered engine resets.
```
kernel BUG at drivers/gpu/drm/i915/i915_request.c:556!
RIP: __i915_request_skip+0x15e/0x1d0 [i915]
...
__i915_request_reset+0x212/0xa70 [i915]
reset_rewind+0xe4/0x280 [i915]
intel_gt_reset+0x30d/0x5b0 [i915]
heartbeat+0x516/0x530 [i915]
```

Guard __i915_request_skip() with i915_request_signaled(), if the
fence is already signaled, the ring content is committed and there
is nothing left to skip.

Cc: stable@vger.kernel.org
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/13729
Fixes: 36e191f0644b ("drm/i915: Apply i915_request_skip() on submission")
Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_reset.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
index 37272871b0f2..b728a5171e93 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -133,7 +133,8 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
 	rcu_read_lock(); /* protect the GEM context */
 	if (guilty) {
 		i915_request_set_error_once(rq, -EIO);
-		__i915_request_skip(rq);
+		if (!i915_request_signaled(rq))
+			__i915_request_skip(rq);
 		banned = mark_guilty(rq);
 	} else {
 		i915_request_set_error_once(rq, -EAGAIN);
-- 
2.53.0

Re: [PATCH] drm/i915: skip __i915_request_skip() for already signaled requests

[Krzysztof Karas]

Hi Sebastian,

On 2026-04-16 at 13:31:18 +0200, Sebastian Brzezinka wrote:
> After a GPU reset the HWSP is zeroed, so previously completed
> requests appear incomplete. If such a request is picked up during
> reset_rewind() and marked guilty, i915_request_set_error_once()
> returns early (fence already signaled), leaving fence.error without
> a fatal error code. The subsequent __i915_request_skip() then hits:
> ```
> GEM_BUG_ON(!fatal_error(rq->fence.error))
> ```
> 
> Fixes a kernel BUG observed on Sandy Bridge (Gen6) during
By "Fixes" do you mean this patch? Or are you referring to the
tag "Fixes:" below? If former would be the case, then imperative
form might be better: Fix.

In any case the patch looks sane:
Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>

> heartbeat-triggered engine resets.
> ```
> kernel BUG at drivers/gpu/drm/i915/i915_request.c:556!
> RIP: __i915_request_skip+0x15e/0x1d0 [i915]
> ...
> __i915_request_reset+0x212/0xa70 [i915]
> reset_rewind+0xe4/0x280 [i915]
> intel_gt_reset+0x30d/0x5b0 [i915]
> heartbeat+0x516/0x530 [i915]
> ```
> 
> Guard __i915_request_skip() with i915_request_signaled(), if the
> fence is already signaled, the ring content is committed and there
> is nothing left to skip.
> 
> Cc: stable@vger.kernel.org
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/13729
> Fixes: 36e191f0644b ("drm/i915: Apply i915_request_skip() on submission")
> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_reset.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 37272871b0f2..b728a5171e93 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -133,7 +133,8 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
>  	rcu_read_lock(); /* protect the GEM context */
>  	if (guilty) {
>  		i915_request_set_error_once(rq, -EIO);
> -		__i915_request_skip(rq);
> +		if (!i915_request_signaled(rq))
> +			__i915_request_skip(rq);
>  		banned = mark_guilty(rq);
>  	} else {
>  		i915_request_set_error_once(rq, -EAGAIN);
> -- 
> 2.53.0
> 

-- 
Best Regards,
Krzysztof

Re: [PATCH] drm/i915: skip __i915_request_skip() for already signaled requests

[Andi Shyti]

Hi Sebastian,

On Mon, Apr 20, 2026 at 09:18:03AM +0000, Krzysztof Karas wrote:
> On 2026-04-16 at 13:31:18 +0200, Sebastian Brzezinka wrote:
> > After a GPU reset the HWSP is zeroed, so previously completed
> > requests appear incomplete. If such a request is picked up during
> > reset_rewind() and marked guilty, i915_request_set_error_once()
> > returns early (fence already signaled), leaving fence.error without
> > a fatal error code. The subsequent __i915_request_skip() then hits:
> > ```
> > GEM_BUG_ON(!fatal_error(rq->fence.error))
> > ```
> > 
> > Fixes a kernel BUG observed on Sandy Bridge (Gen6) during
> By "Fixes" do you mean this patch? Or are you referring to the
> tag "Fixes:" below? If former would be the case, then imperative
> form might be better: Fix.

Pour parler: the imperative is used in the last paragraph:
"Guard" :-)

> 
> In any case the patch looks sane:
> Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>
> 
> > heartbeat-triggered engine resets.
> > ```
> > kernel BUG at drivers/gpu/drm/i915/i915_request.c:556!
> > RIP: __i915_request_skip+0x15e/0x1d0 [i915]
> > ...
> > __i915_request_reset+0x212/0xa70 [i915]
> > reset_rewind+0xe4/0x280 [i915]
> > intel_gt_reset+0x30d/0x5b0 [i915]
> > heartbeat+0x516/0x530 [i915]
> > ```
> > 
> > Guard __i915_request_skip() with i915_request_signaled(), if the
> > fence is already signaled, the ring content is committed and there
> > is nothing left to skip.
> > 
> > Cc: stable@vger.kernel.org
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/13729
> > Fixes: 36e191f0644b ("drm/i915: Apply i915_request_skip() on submission")
> > Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

Re: [PATCH] drm/i915: skip __i915_request_skip() for already signaled requests

[Andi Shyti]

Hi Sebastian,

On Thu, Apr 16, 2026 at 01:31:18PM +0200, Sebastian Brzezinka wrote:
> After a GPU reset the HWSP is zeroed, so previously completed
> requests appear incomplete. If such a request is picked up during
> reset_rewind() and marked guilty, i915_request_set_error_once()
> returns early (fence already signaled), leaving fence.error without
> a fatal error code. The subsequent __i915_request_skip() then hits:
> ```
> GEM_BUG_ON(!fatal_error(rq->fence.error))
> ```
> 
> Fixes a kernel BUG observed on Sandy Bridge (Gen6) during
> heartbeat-triggered engine resets.
> ```
> kernel BUG at drivers/gpu/drm/i915/i915_request.c:556!
> RIP: __i915_request_skip+0x15e/0x1d0 [i915]
> ...
> __i915_request_reset+0x212/0xa70 [i915]
> reset_rewind+0xe4/0x280 [i915]
> intel_gt_reset+0x30d/0x5b0 [i915]
> heartbeat+0x516/0x530 [i915]
> ```
> 
> Guard __i915_request_skip() with i915_request_signaled(), if the
> fence is already signaled, the ring content is committed and there
> is nothing left to skip.
> 
> Cc: stable@vger.kernel.org
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/13729
> Fixes: 36e191f0644b ("drm/i915: Apply i915_request_skip() on submission")
> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>

Merged to drm-intel-gt-next.

Thanks,
Andi

Re: [PATCH] drm/i915: skip __i915_request_skip() for already signaled requests

[Tvrtko Ursulin]

On 16/04/2026 12:31, Sebastian Brzezinka wrote:
> After a GPU reset the HWSP is zeroed, so previously completed
> requests appear incomplete. If such a request is picked up during
> reset_rewind() and marked guilty, i915_request_set_error_once()
> returns early (fence already signaled), leaving fence.error without
> a fatal error code. The subsequent __i915_request_skip() then hits:
> ```
> GEM_BUG_ON(!fatal_error(rq->fence.error))
> ```
> 
> Fixes a kernel BUG observed on Sandy Bridge (Gen6) during
> heartbeat-triggered engine resets.
> ```
> kernel BUG at drivers/gpu/drm/i915/i915_request.c:556!
> RIP: __i915_request_skip+0x15e/0x1d0 [i915]
> ...
> __i915_request_reset+0x212/0xa70 [i915]
> reset_rewind+0xe4/0x280 [i915]
> intel_gt_reset+0x30d/0x5b0 [i915]
> heartbeat+0x516/0x530 [i915]
> ```
> 
> Guard __i915_request_skip() with i915_request_signaled(), if the
> fence is already signaled, the ring content is committed and there
> is nothing left to skip.
> 
> Cc: stable@vger.kernel.org
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/13729
> Fixes: 36e191f0644b ("drm/i915: Apply i915_request_skip() on submission")
> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_reset.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 37272871b0f2..b728a5171e93 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -133,7 +133,8 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
>   	rcu_read_lock(); /* protect the GEM context */
>   	if (guilty) {
>   		i915_request_set_error_once(rq, -EIO);
> -		__i915_request_skip(rq);
> +		if (!i915_request_signaled(rq))
> +			__i915_request_skip(rq);

I spotted this patch in drm-intel-fixes today so some questions.

If the request is okay why is setting error and marking it guilty left?

1)
How confident are you of the Fixes: target? That patch is six years old 
but the Closes: issue is only from last year? Do internal Intel log have 
evidence bug was there in between those two dates? How sporadic was it? 
Were you able to verify the fix easily or with difficulty and how?

2)
Is the issue only that the order of setting the error code and the bug 
on got swapped?

Ie. before 36e191f0644b

__i915_request_reset
  -> i915_request_skip
        GEM_BUG_ON(!IS_ERR_VALUE((long)error));
        dma_fence_set_error(&rq->fence, error);

After:

__i915_request_reset
  i915_request_set_error_once
  -> i915_request_skip

If that is the case commit message should have been clearer on both 
questions.

I will hold off the drm-intel-fixes pull request until we can clarify 
the situation.

Regards,

Tvrtko

>   		banned = mark_guilty(rq);
>   	} else {
>   		i915_request_set_error_once(rq, -EAGAIN);

Re: [PATCH] drm/i915: skip __i915_request_skip() for already signaled requests

[Sebastian Brzezinka]

Hi,

On Wed May 13, 2026 at 10:47 AM CEST, Tvrtko Ursulin wrote:
>
> On 16/04/2026 12:31, Sebastian Brzezinka wrote:
>> After a GPU reset the HWSP is zeroed, so previously completed
>> requests appear incomplete. If such a request is picked up during
>> reset_rewind() and marked guilty, i915_request_set_error_once()
>> returns early (fence already signaled), leaving fence.error without
>> a fatal error code. The subsequent __i915_request_skip() then hits:
>> ```
>> GEM_BUG_ON(!fatal_error(rq->fence.error))
>> ```
>> 
>> Fixes a kernel BUG observed on Sandy Bridge (Gen6) during
>> heartbeat-triggered engine resets.
>> ```
>> kernel BUG at drivers/gpu/drm/i915/i915_request.c:556!
>> RIP: __i915_request_skip+0x15e/0x1d0 [i915]
>> ...
>> __i915_request_reset+0x212/0xa70 [i915]
>> reset_rewind+0xe4/0x280 [i915]
>> intel_gt_reset+0x30d/0x5b0 [i915]
>> heartbeat+0x516/0x530 [i915]
>> ```
>> 
>> Guard __i915_request_skip() with i915_request_signaled(), if the
>> fence is already signaled, the ring content is committed and there
>> is nothing left to skip.
>> 
>> Cc: stable@vger.kernel.org
>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/13729
>> Fixes: 36e191f0644b ("drm/i915: Apply i915_request_skip() on submission")
>> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_reset.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
>> index 37272871b0f2..b728a5171e93 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>> @@ -133,7 +133,8 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
>>   	rcu_read_lock(); /* protect the GEM context */
>>   	if (guilty) {
>>   		i915_request_set_error_once(rq, -EIO);
>> -		__i915_request_skip(rq);
>> +		if (!i915_request_signaled(rq))
>> +			__i915_request_skip(rq);
>
> I spotted this patch in drm-intel-fixes today so some questions.
>
> If the request is okay why is setting error and marking it guilty left?
The request can still be guilty even if it already looks signaled
after reset. The important point is that i915_request_set_error_once()
will return early once the request is already signaled, so it may not
actually inject the error. That leaves __i915_request_skip() with no
error to work with, which is why the guard is needed.

>
> 1)
> How confident are you of the Fixes: target? That patch is six years old 
> but the Closes: issue is only from last year? Do internal Intel log have 
> evidence bug was there in between those two dates? How sporadic was it? 
a
I’m quite confident the fix is correct, and it should not break anything
if it is missing some corner detail. This is an extremely rare issue,
specific to Sandy Bridge, but we know it was present at least as far
back as 2022 from work item 5774. The bug only shows up when the right
reset timing lines up, which is why it is so sporadic.

> Were you able to verify the fix easily or with difficulty and how?
I verified the fix mainly by code analysis. In the worst case, it should
not break anything because the change only skips __i915_request_skip()
when the request is already signaled, and  the ring content is already
committed, so there is nothing left to skip.

>
> 2)
> Is the issue only that the order of setting the error code and the bug 
> on got swapped?
>
> Ie. before 36e191f0644b
>
> __i915_request_reset
>   -> i915_request_skip
>         GEM_BUG_ON(!IS_ERR_VALUE((long)error));
>         dma_fence_set_error(&rq->fence, error);
>
> After:
>
> __i915_request_reset
>   i915_request_set_error_once
>   -> i915_request_skip
>
Yes, exactly. In the old code, i915_request_skip(rq, error) always set
the fence error first, before doing anything else:
```
GEM_BUG_ON(!IS_ERR_VALUE((long)error));
dma_fence_set_error(&rq->fence, error);
```

So even if the request was already signaled, the error was still
recorded. That is the important difference.  After 36e191f0644b, that
logic was split into two steps:
```
i915_request_set_error_once(rq, -EIO);
__i915_request_skip(rq);
```

Now i915_request_set_error_once() can return early when the request is
already signaled, which means the error may never get set at all. That
is why the new guard is needed around __i915_request_skip(). The old
code did not have this problem because the error was set unconditionally
inside i915_request_skip().

> If that is the case commit message should have been clearer on both 
> questions.
could you tell me how I should send the corrected commit message? Is it
enough to send it here, or should I send a new version to the mailing
list?

-- 
Best regards,
Sebastian

Re: [PATCH] drm/i915: skip __i915_request_skip() for already signaled requests

[Tvrtko Ursulin]

On 13/05/2026 12:38, Sebastian Brzezinka wrote:
> Hi,
> 
> On Wed May 13, 2026 at 10:47 AM CEST, Tvrtko Ursulin wrote:
>>
>> On 16/04/2026 12:31, Sebastian Brzezinka wrote:
>>> After a GPU reset the HWSP is zeroed, so previously completed
>>> requests appear incomplete. If such a request is picked up during
>>> reset_rewind() and marked guilty, i915_request_set_error_once()
>>> returns early (fence already signaled), leaving fence.error without
>>> a fatal error code. The subsequent __i915_request_skip() then hits:
>>> ```
>>> GEM_BUG_ON(!fatal_error(rq->fence.error))
>>> ```
>>>
>>> Fixes a kernel BUG observed on Sandy Bridge (Gen6) during
>>> heartbeat-triggered engine resets.
>>> ```
>>> kernel BUG at drivers/gpu/drm/i915/i915_request.c:556!
>>> RIP: __i915_request_skip+0x15e/0x1d0 [i915]
>>> ...
>>> __i915_request_reset+0x212/0xa70 [i915]
>>> reset_rewind+0xe4/0x280 [i915]
>>> intel_gt_reset+0x30d/0x5b0 [i915]
>>> heartbeat+0x516/0x530 [i915]
>>> ```
>>>
>>> Guard __i915_request_skip() with i915_request_signaled(), if the
>>> fence is already signaled, the ring content is committed and there
>>> is nothing left to skip.
>>>
>>> Cc: stable@vger.kernel.org
>>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/13729
>>> Fixes: 36e191f0644b ("drm/i915: Apply i915_request_skip() on submission")
>>> Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_reset.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
>>> index 37272871b0f2..b728a5171e93 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>>> @@ -133,7 +133,8 @@ void __i915_request_reset(struct i915_request *rq, bool guilty)
>>>    	rcu_read_lock(); /* protect the GEM context */
>>>    	if (guilty) {
>>>    		i915_request_set_error_once(rq, -EIO);
>>> -		__i915_request_skip(rq);
>>> +		if (!i915_request_signaled(rq))
>>> +			__i915_request_skip(rq);
>>
>> I spotted this patch in drm-intel-fixes today so some questions.
>>
>> If the request is okay why is setting error and marking it guilty left?
> The request can still be guilty even if it already looks signaled
> after reset. The important point is that i915_request_set_error_once()
> will return early once the request is already signaled, so it may not
> actually inject the error. That leaves __i915_request_skip() with no
> error to work with, which is why the guard is needed.
> 
>>
>> 1)
>> How confident are you of the Fixes: target? That patch is six years old
>> but the Closes: issue is only from last year? Do internal Intel log have
>> evidence bug was there in between those two dates? How sporadic was it?
> a
> I’m quite confident the fix is correct, and it should not break anything
> if it is missing some corner detail. This is an extremely rare issue,
> specific to Sandy Bridge, but we know it was present at least as far
> back as 2022 from work item 5774. The bug only shows up when the right
> reset timing lines up, which is why it is so sporadic.
> 
>> Were you able to verify the fix easily or with difficulty and how?
> I verified the fix mainly by code analysis. In the worst case, it should
> not break anything because the change only skips __i915_request_skip()
> when the request is already signaled, and  the ring content is already
> committed, so there is nothing left to skip.
> 
>>
>> 2)
>> Is the issue only that the order of setting the error code and the bug
>> on got swapped?
>>
>> Ie. before 36e191f0644b
>>
>> __i915_request_reset
>>    -> i915_request_skip
>>          GEM_BUG_ON(!IS_ERR_VALUE((long)error));
>>          dma_fence_set_error(&rq->fence, error);
>>
>> After:
>>
>> __i915_request_reset
>>    i915_request_set_error_once
>>    -> i915_request_skip
>>
> Yes, exactly. In the old code, i915_request_skip(rq, error) always set
> the fence error first, before doing anything else:
> ```
> GEM_BUG_ON(!IS_ERR_VALUE((long)error));
> dma_fence_set_error(&rq->fence, error);
> ```
> 
> So even if the request was already signaled, the error was still
> recorded. That is the important difference.  After 36e191f0644b, that
> logic was split into two steps:
> ```
> i915_request_set_error_once(rq, -EIO);
> __i915_request_skip(rq);
> ```
> 
> Now i915_request_set_error_once() can return early when the request is
> already signaled, which means the error may never get set at all. That
> is why the new guard is needed around __i915_request_skip(). The old
> code did not have this problem because the error was set unconditionally
> inside i915_request_skip().

Hmm right, I did not look into i915_request_set_error_once() so did not 
spot it already has the i915_request_signaled() check.

Would it then be nicer if the code was written as:

if (i915_request_set_error_once(rq, -EIO))
	__i915_request_skip(eq);

?

But the above is details. What worried me more is whether with the patch 
there is scope for regressions due not zapping request in a chain, 
depending on timing. TBH I don't remember exactly how the reset flow 
works, especially on Gen6 which was before my time.

Okay, lets have this as is for now and hope it is good.

Regards,

Tvrtko

> 
>> If that is the case commit message should have been clearer on both
>> questions.
> could you tell me how I should send the corrected commit message? Is it
> enough to send it here, or should I send a new version to the mailing
> list?
>