| Message ID | 20200525075347.582-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] drm/i915/gt: Stop cross-polluting PIN_GLOBAL with PIN_USER with no-ppgtt | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Since the worker may rearm, we currently are only guaranteed to flush > the work if we cancel the timer. If the work was running at the time we > try and cancel it, we will wait for it to complete, but it may leave > items in the pool and requeue the work. If we rearrange the immediate > discard of the pool then cancel the work, we know that the work cannot > rearm and so our flush will be final. > > <0> [314.146044] i915_mod-1321 2.... 299799443us : intel_gt_fini_buffer_pool: intel_gt_fini_buffer_pool:227 GEM_BUG_ON(!list_empty(&pool->cache_list[n])) > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c > index 1495054a4305..418ae184cecf 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c > @@ -212,8 +212,9 @@ void intel_gt_flush_buffer_pool(struct intel_gt *gt) > { > struct intel_gt_buffer_pool *pool = >->buffer_pool; > > - if (cancel_delayed_work_sync(&pool->work)) > + do { > pool_free_imm(pool); > + } while (cancel_delayed_work_sync(&pool->work)); Yeah changing of order makes sense. If you want a guarantee that the finit goes as you expect, you could add two cancel_delayed_work_sync and assert that the final one return false. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > } > > void intel_gt_fini_buffer_pool(struct intel_gt *gt) > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Mika Kuoppala (2020-05-25 14:51:18) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Since the worker may rearm, we currently are only guaranteed to flush > > the work if we cancel the timer. If the work was running at the time we > > try and cancel it, we will wait for it to complete, but it may leave > > items in the pool and requeue the work. If we rearrange the immediate > > discard of the pool then cancel the work, we know that the work cannot > > rearm and so our flush will be final. > > > > <0> [314.146044] i915_mod-1321 2.... 299799443us : intel_gt_fini_buffer_pool: intel_gt_fini_buffer_pool:227 GEM_BUG_ON(!list_empty(&pool->cache_list[n])) > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c > > index 1495054a4305..418ae184cecf 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c > > @@ -212,8 +212,9 @@ void intel_gt_flush_buffer_pool(struct intel_gt *gt) > > { > > struct intel_gt_buffer_pool *pool = >->buffer_pool; > > > > - if (cancel_delayed_work_sync(&pool->work)) > > + do { > > pool_free_imm(pool); > > + } while (cancel_delayed_work_sync(&pool->work)); > > Yeah changing of order makes sense. If you want > a guarantee that the finit goes as you expect, you > could add two cancel_delayed_work_sync and assert > that the final one return false. We have an assert that the lists are empty after this function returns. That's enough to keep me [un]happy :) -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c index 1495054a4305..418ae184cecf 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c @@ -212,8 +212,9 @@ void intel_gt_flush_buffer_pool(struct intel_gt *gt) { struct intel_gt_buffer_pool *pool = >->buffer_pool; - if (cancel_delayed_work_sync(&pool->work)) + do { pool_free_imm(pool); + } while (cancel_delayed_work_sync(&pool->work)); } void intel_gt_fini_buffer_pool(struct intel_gt *gt)
Since the worker may rearm, we currently are only guaranteed to flush the work if we cancel the timer. If the work was running at the time we try and cancel it, we will wait for it to complete, but it may leave items in the pool and requeue the work. If we rearrange the immediate discard of the pool then cancel the work, we know that the work cannot rearm and so our flush will be final. <0> [314.146044] i915_mod-1321 2.... 299799443us : intel_gt_fini_buffer_pool: intel_gt_fini_buffer_pool:227 GEM_BUG_ON(!list_empty(&pool->cache_list[n])) Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/intel_gt_buffer_pool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)