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

[Dave Airlie <airlied@gmail.com>]

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