| Message ID | 20200826132811.17577-35-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/39] drm/i915/gem: Avoid implicit vmap for highmem on x86-32 | expand |
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@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; } /Thomas > + > enum { > DEBUG_FENCE_IDLE = 0, > DEBUG_FENCE_NOTIFY, > @@ -154,10 +159,10 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence, > spin_lock_irqsave_nested(&x->lock, flags, 1 + !!continuation); > if (continuation) { > list_for_each_entry_safe(pos, next, &x->head, entry) { > - if (pos->func == autoremove_wake_function) > - pos->func(pos, TASK_NORMAL, 0, continuation); > - else > + if (pos->flags & I915_SW_FENCE_FLAG_FENCE) > list_move_tail(&pos->entry, continuation); > + else > + pos->func(pos, TASK_NORMAL, 0, continuation); > } > } else { > LIST_HEAD(extra); > @@ -166,9 +171,9 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence, > list_for_each_entry_safe(pos, next, &x->head, entry) { > int wake_flags; > > - wake_flags = fence->error; > - if (pos->func == autoremove_wake_function) > - wake_flags = 0; > + wake_flags = 0; > + if (pos->flags & I915_SW_FENCE_FLAG_FENCE) > + wake_flags = fence->error; > > pos->func(pos, TASK_NORMAL, wake_flags, &extra); > } > @@ -332,8 +337,8 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > struct i915_sw_fence *signaler, > wait_queue_entry_t *wq, gfp_t gfp) > { > + unsigned int pending; > unsigned long flags; > - int pending; > > debug_fence_assert(fence); > might_sleep_if(gfpflags_allow_blocking(gfp)); > @@ -349,7 +354,7 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > if (unlikely(i915_sw_fence_check_if_after(fence, signaler))) > return -EINVAL; > > - pending = 0; > + pending = I915_SW_FENCE_FLAG_FENCE; > if (!wq) { > wq = kmalloc(sizeof(*wq), gfp); > if (!wq) {
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@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; > } > > /Thomas > > Actually, after doing some thinking, wouldn't just comparing against the internal wake function instead of the autoremove_wake_function remove the fragility. Would that be possible? /Thomas
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@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. > 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. -Chris
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@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
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) + enum { DEBUG_FENCE_IDLE = 0, DEBUG_FENCE_NOTIFY, @@ -154,10 +159,10 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence, spin_lock_irqsave_nested(&x->lock, flags, 1 + !!continuation); if (continuation) { list_for_each_entry_safe(pos, next, &x->head, entry) { - if (pos->func == autoremove_wake_function) - pos->func(pos, TASK_NORMAL, 0, continuation); - else + if (pos->flags & I915_SW_FENCE_FLAG_FENCE) list_move_tail(&pos->entry, continuation); + else + pos->func(pos, TASK_NORMAL, 0, continuation); } } else { LIST_HEAD(extra); @@ -166,9 +171,9 @@ static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence, list_for_each_entry_safe(pos, next, &x->head, entry) { int wake_flags; - wake_flags = fence->error; - if (pos->func == autoremove_wake_function) - wake_flags = 0; + wake_flags = 0; + if (pos->flags & I915_SW_FENCE_FLAG_FENCE) + wake_flags = fence->error; pos->func(pos, TASK_NORMAL, wake_flags, &extra); } @@ -332,8 +337,8 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, struct i915_sw_fence *signaler, wait_queue_entry_t *wq, gfp_t gfp) { + unsigned int pending; unsigned long flags; - int pending; debug_fence_assert(fence); might_sleep_if(gfpflags_allow_blocking(gfp)); @@ -349,7 +354,7 @@ static int __i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, if (unlikely(i915_sw_fence_check_if_after(fence, signaler))) return -EINVAL; - pending = 0; + pending = I915_SW_FENCE_FLAG_FENCE; if (!wq) { wq = kmalloc(sizeof(*wq), gfp); if (!wq) {
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@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_sw_fence.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)