[Intel-gfx] [CI] drm/i915: Filter wake_flags passed to default_wake_function

[Intel-gfx] [CI] drm/i915: Filter wake_flags passed to default_wake_function

[Chris Wilson]

The flags passed to the wait_entry.func are passed onwards to
try_to_wake_up(), which has a very particular interpretation for its
wake_flags. In particular, beyond the published WF_SYNC, it has a few
internal flags as well. Since we passed the fence->error down the chain
via the flags argument, these ended up in the default_wake_function
confusing the kernel/sched.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2110
Fixes: ef4688497512 ("drm/i915: Propagate fence errors")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: <stable@vger.kernel.org> # v5.4+
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_sw_fence.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
index 295b9829e2da..4cd2038cbe35 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -164,9 +164,13 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
 
 		do {
 			list_for_each_entry_safe(pos, next, &x->head, entry) {
-				pos->func(pos,
-					  TASK_NORMAL, fence->error,
-					  &extra);
+				int wake_flags;
+
+				wake_flags = fence->error;
+				if (pos->func == autoremove_wake_function)
+					wake_flags = 0;
+
+				pos->func(pos, TASK_NORMAL, wake_flags, &extra);
 			}
 
 			if (list_empty(&extra))
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Filter wake_flags passed to default_wake_function (rev2)

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 429 bytes --]

== Series Details ==

Series: drm/i915: Filter wake_flags passed to default_wake_function (rev2)
URL   : https://patchwork.freedesktop.org/series/79824/
State : failure

== Summary ==

Applying: drm/i915: Filter wake_flags passed to default_wake_function
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_sw_fence.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.



[-- Attachment #1.2: Type: text/html, Size: 923 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [CI] drm/i915: Filter wake_flags passed to default_wake_function

[Dave Airlie]

On Wed, 29 Jul 2020 at 01:21, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> The flags passed to the wait_entry.func are passed onwards to
> try_to_wake_up(), which has a very particular interpretation for its
> wake_flags. In particular, beyond the published WF_SYNC, it has a few
> internal flags as well. Since we passed the fence->error down the chain
> via the flags argument, these ended up in the default_wake_function
> confusing the kernel/sched.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2110
> Fixes: ef4688497512 ("drm/i915: Propagate fence errors")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: <stable@vger.kernel.org> # v5.4+
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_sw_fence.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> index 295b9829e2da..4cd2038cbe35 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -164,9 +164,13 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
>
>                 do {
>                         list_for_each_entry_safe(pos, next, &x->head, entry) {
> -                               pos->func(pos,
> -                                         TASK_NORMAL, fence->error,
> -                                         &extra);
> +                               int wake_flags;
> +
> +                               wake_flags = fence->error;
> +                               if (pos->func == autoremove_wake_function)
> +                                       wake_flags = 0;
> +
> +                               pos->func(pos, TASK_NORMAL, wake_flags, &extra);
>                         }
>
>                         if (list_empty(&extra))

This seems to be heading for my tree at the moment, there is only one
place in the kernel where someone compares pos->func with
autoremove_wake_function, and it's in this file.

This seems horribly brittle, can we at least make this better in -next
even if we have to have this fix in fixes?

I also have to question the whole raison d'etre for i915_sw_fence,
it's initial commit says it was meant to be a core kernel struct, but
I haven't seen any effort on behalf of i915 team to make that happen,
I expect when that is attempted the whole thing will get shredded for
layering violations like the above.

Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx