[Intel-gfx] [PATCH 35/39] drm/i915: Encode fence specific waitqueue behaviour into the wait.flags

[Thomas Hellström (Intel)]

On 9/3/20 1:32 PM, Chris Wilson wrote:
> Quoting Thomas Hellström (Intel) (2020-09-03 10:50:45)
>> On 9/2/20 4:02 PM, Thomas Hellström (Intel) wrote:
>>> Hi, Chris,
>>>
>>> On 8/26/20 3:28 PM, Chris Wilson wrote:
>>>> Use the wait_queue_entry.flags to denote the special fence behaviour
>>>> (flattening continuations along fence chains, and for propagating
>>>> errors) rather than trying to detect ordinary waiters by their
>>>> functions.
>>>>
>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_sw_fence.c | 25 +++++++++++++++----------
>>>>    1 file changed, 15 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
>>>> b/drivers/gpu/drm/i915/i915_sw_fence.c
>>>> index 4cd2038cbe35..4e557d1c4644 100644
>>>> --- a/drivers/gpu/drm/i915/i915_sw_fence.c
>>>> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
>>>> @@ -18,10 +18,15 @@
>>>>    #define I915_SW_FENCE_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
>>>>    #endif
>>>>    -#define I915_SW_FENCE_FLAG_ALLOC BIT(3) /* after WQ_FLAG_* for
>>>> safety */
>>>> -
>>>>    static DEFINE_SPINLOCK(i915_sw_fence_lock);
>>>>    +#define WQ_FLAG_BITS \
>>>> +    BITS_PER_TYPE(typeof_member(struct wait_queue_entry, flags))
>>>> +
>>>> +/* after WQ_FLAG_* for safety */
>>>> +#define I915_SW_FENCE_FLAG_FENCE BIT(WQ_FLAG_BITS - 1)
>>>> +#define I915_SW_FENCE_FLAG_ALLOC BIT(WQ_FLAG_BITS - 2)
>>> Not sure if sharing the flags field with the wait.c implementation is
>>> fully OK either. Is the @key parameter to the wake function useable? I
>>> mean rather than passing just a list head could we pass something like
>>>
>>> struct i915_sw_fence_key {
>>>      bool no_recursion; /* Makes the wait function just put its entry
>>> on @continuation and return */
>>>      int error;
>>>      struct list_head continuation;
>>> }
> That would mean wait_event-esque routines do not work on a fence.

OK, that is a no-go then.

>
>> internal wake function instead of the autoremove_wake_function remove
>> the fragility. Would that be possible?
> autoremove is the function used by display for its wait_event loop over
> multiple sources.

Hmm. I don't think I follow. I meant instead of

if (pos->func == autoremove_wake_function)
    ...
else
    do_i915_specific_stuff;

we use

if (pos->func != i915_sw_fence_wake)
     ...
else
    do_i915_specific_stuff;

/Thomas




> -Chris