If we unwind the active requests, and on resubmission discover that we intend to preempt the active context with itself, simply skip the ELSP submission.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/gt/intel_lrc.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 431d3b8c3371..3cfea1758fd2 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1739,11 +1739,26 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (submit) { *port = execlists_schedule_in(last, port - execlists->pending); - memset(port + 1, 0, (last_port - port) * sizeof(*port)); execlists->switch_priority_hint = switch_prio(engine, *execlists->pending); + + /* + * Skip if we ended up with exactly the same set of requests, + * e.g. trying to timeslice a pair of ordered contexts + */ + if (!memcmp(execlists->active, execlists->pending, + (port - execlists->pending + 1) * sizeof(*port))) { + do + execlists_schedule_out(fetch_and_zero(port)); + while (port-- != execlists->pending); + + goto skip_submit; + } + + memset(port + 1, 0, (last_port - port) * sizeof(*port)); execlists_submit_ports(engine); } else { +skip_submit: ring_set_paused(engine, 0); } }
Make dma_fence_enable_sw_signaling() behave like its dma_fence_add_callback() and dma_fence_default_wait() counterparts and perform the test to enable signaling under the fence->lock, along with the action to do so. This ensure that should an implementation be trying to flush the cb_list (by signaling) on retirement before freeing the fence, it can do so in a race-free manner.
See also 0fc89b6802ba ("dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal").
v2: Refactor all 3 enable_signaling paths to use a common function.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/dma-buf/dma-fence.c | 78 +++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 43 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 2c136aee3e79..b58528c1cc9d 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -273,6 +273,30 @@ void dma_fence_free(struct dma_fence *fence) } EXPORT_SYMBOL(dma_fence_free);
+static bool __dma_fence_enable_signaling(struct dma_fence *fence) +{ + bool was_set; + + lockdep_assert_held(fence->lock); + + was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, + &fence->flags); + + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) + return false; + + if (!was_set && fence->ops->enable_signaling) { + if (!fence->ops->enable_signaling(fence)) { + dma_fence_signal_locked(fence); + return false; + } + + trace_dma_fence_enable_signal(fence); + } + + return true; +} + /** * dma_fence_enable_sw_signaling - enable signaling on fence * @fence: the fence to enable @@ -285,19 +309,12 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence) { unsigned long flags;
- if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, - &fence->flags) && - !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && - fence->ops->enable_signaling) { - trace_dma_fence_enable_signal(fence); - - spin_lock_irqsave(fence->lock, flags); - - if (!fence->ops->enable_signaling(fence)) - dma_fence_signal_locked(fence); + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) + return;
- spin_unlock_irqrestore(fence->lock, flags); - } + spin_lock_irqsave(fence->lock, flags); + __dma_fence_enable_signaling(fence); + spin_unlock_irqrestore(fence->lock, flags); } EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
@@ -331,7 +348,6 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, { unsigned long flags; int ret = 0; - bool was_set;
if (WARN_ON(!fence || !func)) return -EINVAL; @@ -343,25 +359,14 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
spin_lock_irqsave(fence->lock, flags);
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, - &fence->flags); - - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) - ret = -ENOENT; - else if (!was_set && fence->ops->enable_signaling) { - trace_dma_fence_enable_signal(fence); - - if (!fence->ops->enable_signaling(fence)) { - dma_fence_signal_locked(fence); - ret = -ENOENT; - } - } - - if (!ret) { + if (__dma_fence_enable_signaling(fence)) { cb->func = func; list_add_tail(&cb->node, &fence->cb_list); - } else + } else { INIT_LIST_HEAD(&cb->node); + ret = -ENOENT; + } + spin_unlock_irqrestore(fence->lock, flags);
return ret; @@ -461,7 +466,6 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) struct default_wait_cb cb; unsigned long flags; signed long ret = timeout ? timeout : 1; - bool was_set;
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) return ret; @@ -473,21 +477,9 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) goto out; }
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, - &fence->flags); - - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) + if (!__dma_fence_enable_signaling(fence)) goto out;
- if (!was_set && fence->ops->enable_signaling) { - trace_dma_fence_enable_signal(fence); - - if (!fence->ops->enable_signaling(fence)) { - dma_fence_signal_locked(fence); - goto out; - } - } - if (!timeout) { ret = 0; goto out;
On 03/10/2019 22:00, Chris Wilson wrote:
Make dma_fence_enable_sw_signaling() behave like its dma_fence_add_callback() and dma_fence_default_wait() counterparts and perform the test to enable signaling under the fence->lock, along with the action to do so. This ensure that should an implementation be trying to flush the cb_list (by signaling) on retirement before freeing the fence, it can do so in a race-free manner.
See also 0fc89b6802ba ("dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal").
v2: Refactor all 3 enable_signaling paths to use a common function.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/dma-buf/dma-fence.c | 78 +++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 43 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 2c136aee3e79..b58528c1cc9d 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -273,6 +273,30 @@ void dma_fence_free(struct dma_fence *fence) } EXPORT_SYMBOL(dma_fence_free);
+static bool __dma_fence_enable_signaling(struct dma_fence *fence) +{
- bool was_set;
- lockdep_assert_held(fence->lock);
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&fence->flags);- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
return false;- if (!was_set && fence->ops->enable_signaling) {
if (!fence->ops->enable_signaling(fence)) {dma_fence_signal_locked(fence);return false;}trace_dma_fence_enable_signal(fence);
Tracepoint used to come before the enable_signaling call so probably best to keep it like that.
- }
- return true;
+}
- /**
- dma_fence_enable_sw_signaling - enable signaling on fence
- @fence: the fence to enable
@@ -285,19 +309,12 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence) { unsigned long flags;
- if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&fence->flags) &&!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&fence->ops->enable_signaling) {trace_dma_fence_enable_signal(fence);spin_lock_irqsave(fence->lock, flags);if (!fence->ops->enable_signaling(fence))dma_fence_signal_locked(fence);
- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
return;
spin_unlock_irqrestore(fence->lock, flags);- }
- spin_lock_irqsave(fence->lock, flags);
- __dma_fence_enable_signaling(fence);
- spin_unlock_irqrestore(fence->lock, flags); } EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
@@ -331,7 +348,6 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, { unsigned long flags; int ret = 0;
bool was_set;
if (WARN_ON(!fence || !func)) return -EINVAL;
@@ -343,25 +359,14 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
spin_lock_irqsave(fence->lock, flags);
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&fence->flags);- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
ret = -ENOENT;- else if (!was_set && fence->ops->enable_signaling) {
trace_dma_fence_enable_signal(fence);if (!fence->ops->enable_signaling(fence)) {dma_fence_signal_locked(fence);ret = -ENOENT;}- }
- if (!ret) {
- if (__dma_fence_enable_signaling(fence)) { cb->func = func; list_add_tail(&cb->node, &fence->cb_list);
- } else
} else { INIT_LIST_HEAD(&cb->node);
ret = -ENOENT;}
spin_unlock_irqrestore(fence->lock, flags);
return ret;
@@ -461,7 +466,6 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) struct default_wait_cb cb; unsigned long flags; signed long ret = timeout ? timeout : 1;
bool was_set;
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) return ret;
@@ -473,21 +477,9 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) goto out; }
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&fence->flags);- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
- if (!__dma_fence_enable_signaling(fence)) goto out;
- if (!was_set && fence->ops->enable_signaling) {
trace_dma_fence_enable_signal(fence);if (!fence->ops->enable_signaling(fence)) {dma_fence_signal_locked(fence);goto out;}- }
- if (!timeout) { ret = 0; goto out;
The rest looks correct to me.
Regards,
Tvrtko
Make dma_fence_enable_sw_signaling() behave like its dma_fence_add_callback() and dma_fence_default_wait() counterparts and perform the test to enable signaling under the fence->lock, along with the action to do so. This ensure that should an implementation be trying to flush the cb_list (by signaling) on retirement before freeing the fence, it can do so in a race-free manner.
See also 0fc89b6802ba ("dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal").
v2: Refactor all 3 enable_signaling paths to use a common function. v3: Don't argue, just keep the tracepoint in the existing spot.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com --- drivers/dma-buf/dma-fence.c | 78 +++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 43 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 2c136aee3e79..052a41e2451c 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -273,6 +273,30 @@ void dma_fence_free(struct dma_fence *fence) } EXPORT_SYMBOL(dma_fence_free);
+static bool __dma_fence_enable_signaling(struct dma_fence *fence) +{ + bool was_set; + + lockdep_assert_held(fence->lock); + + was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, + &fence->flags); + + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) + return false; + + if (!was_set && fence->ops->enable_signaling) { + trace_dma_fence_enable_signal(fence); + + if (!fence->ops->enable_signaling(fence)) { + dma_fence_signal_locked(fence); + return false; + } + } + + return true; +} + /** * dma_fence_enable_sw_signaling - enable signaling on fence * @fence: the fence to enable @@ -285,19 +309,12 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence) { unsigned long flags;
- if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, - &fence->flags) && - !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && - fence->ops->enable_signaling) { - trace_dma_fence_enable_signal(fence); - - spin_lock_irqsave(fence->lock, flags); - - if (!fence->ops->enable_signaling(fence)) - dma_fence_signal_locked(fence); + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) + return;
- spin_unlock_irqrestore(fence->lock, flags); - } + spin_lock_irqsave(fence->lock, flags); + __dma_fence_enable_signaling(fence); + spin_unlock_irqrestore(fence->lock, flags); } EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
@@ -331,7 +348,6 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, { unsigned long flags; int ret = 0; - bool was_set;
if (WARN_ON(!fence || !func)) return -EINVAL; @@ -343,25 +359,14 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
spin_lock_irqsave(fence->lock, flags);
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, - &fence->flags); - - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) - ret = -ENOENT; - else if (!was_set && fence->ops->enable_signaling) { - trace_dma_fence_enable_signal(fence); - - if (!fence->ops->enable_signaling(fence)) { - dma_fence_signal_locked(fence); - ret = -ENOENT; - } - } - - if (!ret) { + if (__dma_fence_enable_signaling(fence)) { cb->func = func; list_add_tail(&cb->node, &fence->cb_list); - } else + } else { INIT_LIST_HEAD(&cb->node); + ret = -ENOENT; + } + spin_unlock_irqrestore(fence->lock, flags);
return ret; @@ -461,7 +466,6 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) struct default_wait_cb cb; unsigned long flags; signed long ret = timeout ? timeout : 1; - bool was_set;
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) return ret; @@ -473,21 +477,9 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) goto out; }
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, - &fence->flags); - - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) + if (!__dma_fence_enable_signaling(fence)) goto out;
- if (!was_set && fence->ops->enable_signaling) { - trace_dma_fence_enable_signal(fence); - - if (!fence->ops->enable_signaling(fence)) { - dma_fence_signal_locked(fence); - goto out; - } - } - if (!timeout) { ret = 0; goto out;
On 04/10/2019 11:11, Chris Wilson wrote:
Make dma_fence_enable_sw_signaling() behave like its dma_fence_add_callback() and dma_fence_default_wait() counterparts and perform the test to enable signaling under the fence->lock, along with the action to do so. This ensure that should an implementation be trying to flush the cb_list (by signaling) on retirement before freeing the fence, it can do so in a race-free manner.
See also 0fc89b6802ba ("dma-fence: Simply wrap dma_fence_signal_locked with dma_fence_signal").
v2: Refactor all 3 enable_signaling paths to use a common function. v3: Don't argue, just keep the tracepoint in the existing spot.
I would understand the argument but a) meh and b) why change the ABI, kind of.
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com
drivers/dma-buf/dma-fence.c | 78 +++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 43 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 2c136aee3e79..052a41e2451c 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -273,6 +273,30 @@ void dma_fence_free(struct dma_fence *fence) } EXPORT_SYMBOL(dma_fence_free);
+static bool __dma_fence_enable_signaling(struct dma_fence *fence) +{
- bool was_set;
- lockdep_assert_held(fence->lock);
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&fence->flags);- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
return false;- if (!was_set && fence->ops->enable_signaling) {
trace_dma_fence_enable_signal(fence);if (!fence->ops->enable_signaling(fence)) {dma_fence_signal_locked(fence);return false;}- }
- return true;
+}
- /**
- dma_fence_enable_sw_signaling - enable signaling on fence
- @fence: the fence to enable
@@ -285,19 +309,12 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence) { unsigned long flags;
- if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&fence->flags) &&!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&fence->ops->enable_signaling) {trace_dma_fence_enable_signal(fence);spin_lock_irqsave(fence->lock, flags);if (!fence->ops->enable_signaling(fence))dma_fence_signal_locked(fence);
- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
return;
spin_unlock_irqrestore(fence->lock, flags);- }
- spin_lock_irqsave(fence->lock, flags);
- __dma_fence_enable_signaling(fence);
- spin_unlock_irqrestore(fence->lock, flags); } EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
@@ -331,7 +348,6 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, { unsigned long flags; int ret = 0;
bool was_set;
if (WARN_ON(!fence || !func)) return -EINVAL;
@@ -343,25 +359,14 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
spin_lock_irqsave(fence->lock, flags);
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&fence->flags);- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
ret = -ENOENT;- else if (!was_set && fence->ops->enable_signaling) {
trace_dma_fence_enable_signal(fence);if (!fence->ops->enable_signaling(fence)) {dma_fence_signal_locked(fence);ret = -ENOENT;}- }
- if (!ret) {
- if (__dma_fence_enable_signaling(fence)) { cb->func = func; list_add_tail(&cb->node, &fence->cb_list);
- } else
} else { INIT_LIST_HEAD(&cb->node);
ret = -ENOENT;}
spin_unlock_irqrestore(fence->lock, flags);
return ret;
@@ -461,7 +466,6 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) struct default_wait_cb cb; unsigned long flags; signed long ret = timeout ? timeout : 1;
bool was_set;
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) return ret;
@@ -473,21 +477,9 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) goto out; }
- was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
&fence->flags);- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
- if (!__dma_fence_enable_signaling(fence)) goto out;
- if (!was_set && fence->ops->enable_signaling) {
trace_dma_fence_enable_signal(fence);if (!fence->ops->enable_signaling(fence)) {dma_fence_signal_locked(fence);goto out;}- }
- if (!timeout) { ret = 0; goto out;
In preparation for rearranging the booleans into a flags field, ensure all the current users are using the inline helpers and not directly accessing the members.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_mm.c | 19 ++++++++++++------- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 4 ++-- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +- drivers/gpu/drm/i915/i915_gem.c | 12 ++++++------ drivers/gpu/drm/i915/i915_gem_evict.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 2 +- drivers/gpu/drm/i915/i915_vma.h | 4 ++-- drivers/gpu/drm/selftests/test-drm_mm.c | 14 +++++++------- drivers/gpu/drm/vc4/vc4_crtc.c | 2 +- drivers/gpu/drm/vc4/vc4_hvs.c | 2 +- drivers/gpu/drm/vc4/vc4_plane.c | 4 ++-- 11 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index 4581c5387372..99312bdc6273 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -174,7 +174,7 @@ static void drm_mm_interval_tree_add_node(struct drm_mm_node *hole_node,
node->__subtree_last = LAST(node);
- if (hole_node->allocated) { + if (drm_mm_node_allocated(hole_node)) { rb = &hole_node->rb; while (rb) { parent = rb_entry(rb, struct drm_mm_node, rb); @@ -561,6 +561,11 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, } EXPORT_SYMBOL(drm_mm_insert_node_in_range);
+static inline bool drm_mm_node_scanned_block(const struct drm_mm_node *node) +{ + return node->scanned_block; +} + /** * drm_mm_remove_node - Remove a memory node from the allocator. * @node: drm_mm_node to remove @@ -574,8 +579,8 @@ void drm_mm_remove_node(struct drm_mm_node *node) struct drm_mm *mm = node->mm; struct drm_mm_node *prev_node;
- DRM_MM_BUG_ON(!node->allocated); - DRM_MM_BUG_ON(node->scanned_block); + DRM_MM_BUG_ON(!drm_mm_node_allocated(node)); + DRM_MM_BUG_ON(drm_mm_node_scanned_block(node));
prev_node = list_prev_entry(node, node_list);
@@ -605,7 +610,7 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new) { struct drm_mm *mm = old->mm;
- DRM_MM_BUG_ON(!old->allocated); + DRM_MM_BUG_ON(!drm_mm_node_allocated(old));
*new = *old;
@@ -731,8 +736,8 @@ bool drm_mm_scan_add_block(struct drm_mm_scan *scan, u64 adj_start, adj_end;
DRM_MM_BUG_ON(node->mm != mm); - DRM_MM_BUG_ON(!node->allocated); - DRM_MM_BUG_ON(node->scanned_block); + DRM_MM_BUG_ON(!drm_mm_node_allocated(node)); + DRM_MM_BUG_ON(drm_mm_node_scanned_block(node)); node->scanned_block = true; mm->scan_active++;
@@ -818,7 +823,7 @@ bool drm_mm_scan_remove_block(struct drm_mm_scan *scan, struct drm_mm_node *prev_node;
DRM_MM_BUG_ON(node->mm != scan->mm); - DRM_MM_BUG_ON(!node->scanned_block); + DRM_MM_BUG_ON(!drm_mm_node_scanned_block(node)); node->scanned_block = false;
DRM_MM_BUG_ON(!node->mm->scan_active); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 27dbcb508055..20d8a6297985 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -968,7 +968,7 @@ static void reloc_cache_reset(struct reloc_cache *cache) intel_gt_flush_ggtt_writes(ggtt->vm.gt); io_mapping_unmap_atomic((void __iomem *)vaddr);
- if (cache->node.allocated) { + if (drm_mm_node_allocated(&cache->node)) { ggtt->vm.clear_range(&ggtt->vm, cache->node.start, cache->node.size); @@ -1061,7 +1061,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj, }
offset = cache->node.start; - if (cache->node.allocated) { + if (drm_mm_node_allocated(&cache->node)) { ggtt->vm.insert_page(&ggtt->vm, i915_gem_object_get_dma_address(obj, page), offset, I915_CACHE_NONE, 0); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c index bb878119f06c..bb4889d2346d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -387,7 +387,7 @@ static u32 uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt) { struct drm_mm_node *node = &ggtt->uc_fw;
- GEM_BUG_ON(!node->allocated); + GEM_BUG_ON(!drm_mm_node_allocated(node)); GEM_BUG_ON(upper_32_bits(node->start)); GEM_BUG_ON(upper_32_bits(node->start + node->size - 1));
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 1426e506700d..fa8e028ac0b5 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -356,7 +356,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, ret = insert_mappable_node(ggtt, &node, PAGE_SIZE); if (ret) goto out_unlock; - GEM_BUG_ON(!node.allocated); + GEM_BUG_ON(!drm_mm_node_allocated(&node)); }
mutex_unlock(&i915->drm.struct_mutex); @@ -393,7 +393,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, unsigned page_offset = offset_in_page(offset); unsigned page_length = PAGE_SIZE - page_offset; page_length = remain < page_length ? remain : page_length; - if (node.allocated) { + if (drm_mm_node_allocated(&node)) { ggtt->vm.insert_page(&ggtt->vm, i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT), node.start, I915_CACHE_NONE, 0); @@ -415,7 +415,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, i915_gem_object_unlock_fence(obj, fence); out_unpin: mutex_lock(&i915->drm.struct_mutex); - if (node.allocated) { + if (drm_mm_node_allocated(&node)) { ggtt->vm.clear_range(&ggtt->vm, node.start, node.size); remove_mappable_node(&node); } else { @@ -566,7 +566,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, ret = insert_mappable_node(ggtt, &node, PAGE_SIZE); if (ret) goto out_rpm; - GEM_BUG_ON(!node.allocated); + GEM_BUG_ON(!drm_mm_node_allocated(&node)); }
mutex_unlock(&i915->drm.struct_mutex); @@ -604,7 +604,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, unsigned int page_offset = offset_in_page(offset); unsigned int page_length = PAGE_SIZE - page_offset; page_length = remain < page_length ? remain : page_length; - if (node.allocated) { + if (drm_mm_node_allocated(&node)) { /* flush the write before we modify the GGTT */ intel_gt_flush_ggtt_writes(ggtt->vm.gt); ggtt->vm.insert_page(&ggtt->vm, @@ -636,7 +636,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, out_unpin: mutex_lock(&i915->drm.struct_mutex); intel_gt_flush_ggtt_writes(ggtt->vm.gt); - if (node.allocated) { + if (drm_mm_node_allocated(&node)) { ggtt->vm.clear_range(&ggtt->vm, node.start, node.size); remove_mappable_node(&node); } else { diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index e76c9da9992d..8c1e04f402bc 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -299,7 +299,7 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, break; }
- GEM_BUG_ON(!node->allocated); + GEM_BUG_ON(!drm_mm_node_allocated(node)); vma = container_of(node, typeof(*vma), node);
/* If we are using coloring to insert guard pages between diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 9d5b0f87c210..68c34b1a20e4 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -795,7 +795,7 @@ void i915_vma_reopen(struct i915_vma *vma)
static void __i915_vma_destroy(struct i915_vma *vma) { - GEM_BUG_ON(vma->node.allocated); + GEM_BUG_ON(drm_mm_node_allocated(&vma->node)); GEM_BUG_ON(vma->fence);
mutex_lock(&vma->vm->mutex); diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 8bcb5812c446..e49b199f7de7 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -228,7 +228,7 @@ static inline bool i915_vma_is_closed(const struct i915_vma *vma) static inline u32 i915_ggtt_offset(const struct i915_vma *vma) { GEM_BUG_ON(!i915_vma_is_ggtt(vma)); - GEM_BUG_ON(!vma->node.allocated); + GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); GEM_BUG_ON(upper_32_bits(vma->node.start)); GEM_BUG_ON(upper_32_bits(vma->node.start + vma->node.size - 1)); return lower_32_bits(vma->node.start); @@ -390,7 +390,7 @@ static inline bool i915_vma_is_bound(const struct i915_vma *vma, static inline bool i915_node_color_differs(const struct drm_mm_node *node, unsigned long color) { - return node->allocated && node->color != color; + return drm_mm_node_allocated(node) && node->color != color; }
/** diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c index 388f9844f4ba..9aabe82dcd3a 100644 --- a/drivers/gpu/drm/selftests/test-drm_mm.c +++ b/drivers/gpu/drm/selftests/test-drm_mm.c @@ -854,7 +854,7 @@ static bool assert_contiguous_in_range(struct drm_mm *mm,
if (start > 0) { node = __drm_mm_interval_first(mm, 0, start - 1); - if (node->allocated) { + if (drm_mm_node_allocated(node)) { pr_err("node before start: node=%llx+%llu, start=%llx\n", node->start, node->size, start); return false; @@ -863,7 +863,7 @@ static bool assert_contiguous_in_range(struct drm_mm *mm,
if (end < U64_MAX) { node = __drm_mm_interval_first(mm, end, U64_MAX); - if (node->allocated) { + if (drm_mm_node_allocated(node)) { pr_err("node after end: node=%llx+%llu, end=%llx\n", node->start, node->size, end); return false; @@ -1156,12 +1156,12 @@ static void show_holes(const struct drm_mm *mm, int count) struct drm_mm_node *next = list_next_entry(hole, node_list); const char *node1 = NULL, *node2 = NULL;
- if (hole->allocated) + if (drm_mm_node_allocated(hole)) node1 = kasprintf(GFP_KERNEL, "[%llx + %lld, color=%ld], ", hole->start, hole->size, hole->color);
- if (next->allocated) + if (drm_mm_node_allocated(next)) node2 = kasprintf(GFP_KERNEL, ", [%llx + %lld, color=%ld]", next->start, next->size, next->color); @@ -1900,18 +1900,18 @@ static void separate_adjacent_colors(const struct drm_mm_node *node, u64 *start, u64 *end) { - if (node->allocated && node->color != color) + if (drm_mm_node_allocated(node) && node->color != color) ++*start;
node = list_next_entry(node, node_list); - if (node->allocated && node->color != color) + if (drm_mm_node_allocated(node) && node->color != color) --*end; }
static bool colors_abutt(const struct drm_mm_node *node) { if (!drm_mm_hole_follows(node) && - list_next_entry(node, node_list)->allocated) { + drm_mm_node_allocated(list_next_entry(node, node_list))) { pr_err("colors abutt; %ld [%llx + %llx] is next to %ld [%llx + %llx]!\n", node->color, node->start, node->size, list_next_entry(node, node_list)->color, diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index f1f0a7c87771..b00e20f5ce05 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -994,7 +994,7 @@ static void vc4_crtc_destroy_state(struct drm_crtc *crtc, struct vc4_dev *vc4 = to_vc4_dev(crtc->dev); struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
- if (vc4_state->mm.allocated) { + if (drm_mm_node_allocated(&vc4_state->mm)) { unsigned long flags;
spin_lock_irqsave(&vc4->hvs->mm_lock, flags); diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index 9936b15d0bf1..5a43659da319 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -315,7 +315,7 @@ static void vc4_hvs_unbind(struct device *dev, struct device *master, struct drm_device *drm = dev_get_drvdata(master); struct vc4_dev *vc4 = drm->dev_private;
- if (vc4->hvs->mitchell_netravali_filter.allocated) + if (drm_mm_node_allocated(&vc4->hvs->mitchell_netravali_filter)) drm_mm_remove_node(&vc4->hvs->mitchell_netravali_filter);
drm_mm_takedown(&vc4->hvs->dlist_mm); diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 5e5f90810aca..4934127f0d76 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -178,7 +178,7 @@ static void vc4_plane_destroy_state(struct drm_plane *plane, struct vc4_dev *vc4 = to_vc4_dev(plane->dev); struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
- if (vc4_state->lbm.allocated) { + if (drm_mm_node_allocated(&vc4_state->lbm)) { unsigned long irqflags;
spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags); @@ -557,7 +557,7 @@ static int vc4_plane_allocate_lbm(struct drm_plane_state *state) /* Allocate the LBM memory that the HVS will use for temporary * storage due to our scaling/format conversion. */ - if (!vc4_state->lbm.allocated) { + if (!drm_mm_node_allocated(&vc4_state->lbm)) { int ret;
spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
On 03/10/2019 22:00, Chris Wilson wrote:
In preparation for rearranging the booleans into a flags field, ensure all the current users are using the inline helpers and not directly accessing the members.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/drm_mm.c | 19 ++++++++++++------- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 4 ++-- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +- drivers/gpu/drm/i915/i915_gem.c | 12 ++++++------ drivers/gpu/drm/i915/i915_gem_evict.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 2 +- drivers/gpu/drm/i915/i915_vma.h | 4 ++-- drivers/gpu/drm/selftests/test-drm_mm.c | 14 +++++++------- drivers/gpu/drm/vc4/vc4_crtc.c | 2 +- drivers/gpu/drm/vc4/vc4_hvs.c | 2 +- drivers/gpu/drm/vc4/vc4_plane.c | 4 ++-- 11 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index 4581c5387372..99312bdc6273 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -174,7 +174,7 @@ static void drm_mm_interval_tree_add_node(struct drm_mm_node *hole_node,
node->__subtree_last = LAST(node);
- if (hole_node->allocated) {
- if (drm_mm_node_allocated(hole_node)) { rb = &hole_node->rb; while (rb) { parent = rb_entry(rb, struct drm_mm_node, rb);
@@ -561,6 +561,11 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, } EXPORT_SYMBOL(drm_mm_insert_node_in_range);
+static inline bool drm_mm_node_scanned_block(const struct drm_mm_node *node) +{
- return node->scanned_block;
+}
- /**
- drm_mm_remove_node - Remove a memory node from the allocator.
- @node: drm_mm_node to remove
@@ -574,8 +579,8 @@ void drm_mm_remove_node(struct drm_mm_node *node) struct drm_mm *mm = node->mm; struct drm_mm_node *prev_node;
- DRM_MM_BUG_ON(!node->allocated);
- DRM_MM_BUG_ON(node->scanned_block);
DRM_MM_BUG_ON(!drm_mm_node_allocated(node));
DRM_MM_BUG_ON(drm_mm_node_scanned_block(node));
prev_node = list_prev_entry(node, node_list);
@@ -605,7 +610,7 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new) { struct drm_mm *mm = old->mm;
- DRM_MM_BUG_ON(!old->allocated);
DRM_MM_BUG_ON(!drm_mm_node_allocated(old));
*new = *old;
@@ -731,8 +736,8 @@ bool drm_mm_scan_add_block(struct drm_mm_scan *scan, u64 adj_start, adj_end;
DRM_MM_BUG_ON(node->mm != mm);
- DRM_MM_BUG_ON(!node->allocated);
- DRM_MM_BUG_ON(node->scanned_block);
- DRM_MM_BUG_ON(!drm_mm_node_allocated(node));
- DRM_MM_BUG_ON(drm_mm_node_scanned_block(node)); node->scanned_block = true; mm->scan_active++;
@@ -818,7 +823,7 @@ bool drm_mm_scan_remove_block(struct drm_mm_scan *scan, struct drm_mm_node *prev_node;
DRM_MM_BUG_ON(node->mm != scan->mm);
- DRM_MM_BUG_ON(!node->scanned_block);
DRM_MM_BUG_ON(!drm_mm_node_scanned_block(node)); node->scanned_block = false;
DRM_MM_BUG_ON(!node->mm->scan_active);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 27dbcb508055..20d8a6297985 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -968,7 +968,7 @@ static void reloc_cache_reset(struct reloc_cache *cache) intel_gt_flush_ggtt_writes(ggtt->vm.gt); io_mapping_unmap_atomic((void __iomem *)vaddr);
if (cache->node.allocated) {
if (drm_mm_node_allocated(&cache->node)) { ggtt->vm.clear_range(&ggtt->vm, cache->node.start, cache->node.size);@@ -1061,7 +1061,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj, }
offset = cache->node.start;
- if (cache->node.allocated) {
- if (drm_mm_node_allocated(&cache->node)) { ggtt->vm.insert_page(&ggtt->vm, i915_gem_object_get_dma_address(obj, page), offset, I915_CACHE_NONE, 0);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c index bb878119f06c..bb4889d2346d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -387,7 +387,7 @@ static u32 uc_fw_ggtt_offset(struct intel_uc_fw *uc_fw, struct i915_ggtt *ggtt) { struct drm_mm_node *node = &ggtt->uc_fw;
- GEM_BUG_ON(!node->allocated);
- GEM_BUG_ON(!drm_mm_node_allocated(node)); GEM_BUG_ON(upper_32_bits(node->start)); GEM_BUG_ON(upper_32_bits(node->start + node->size - 1));
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 1426e506700d..fa8e028ac0b5 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -356,7 +356,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, ret = insert_mappable_node(ggtt, &node, PAGE_SIZE); if (ret) goto out_unlock;
GEM_BUG_ON(!node.allocated);
GEM_BUG_ON(!drm_mm_node_allocated(&node));}
mutex_unlock(&i915->drm.struct_mutex);
@@ -393,7 +393,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, unsigned page_offset = offset_in_page(offset); unsigned page_length = PAGE_SIZE - page_offset; page_length = remain < page_length ? remain : page_length;
if (node.allocated) {
if (drm_mm_node_allocated(&node)) { ggtt->vm.insert_page(&ggtt->vm, i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT), node.start, I915_CACHE_NONE, 0);@@ -415,7 +415,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, i915_gem_object_unlock_fence(obj, fence); out_unpin: mutex_lock(&i915->drm.struct_mutex);
- if (node.allocated) {
- if (drm_mm_node_allocated(&node)) { ggtt->vm.clear_range(&ggtt->vm, node.start, node.size); remove_mappable_node(&node); } else {
@@ -566,7 +566,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, ret = insert_mappable_node(ggtt, &node, PAGE_SIZE); if (ret) goto out_rpm;
GEM_BUG_ON(!node.allocated);
GEM_BUG_ON(!drm_mm_node_allocated(&node));}
mutex_unlock(&i915->drm.struct_mutex);
@@ -604,7 +604,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, unsigned int page_offset = offset_in_page(offset); unsigned int page_length = PAGE_SIZE - page_offset; page_length = remain < page_length ? remain : page_length;
if (node.allocated) {
if (drm_mm_node_allocated(&node)) { /* flush the write before we modify the GGTT */ intel_gt_flush_ggtt_writes(ggtt->vm.gt); ggtt->vm.insert_page(&ggtt->vm,@@ -636,7 +636,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, out_unpin: mutex_lock(&i915->drm.struct_mutex); intel_gt_flush_ggtt_writes(ggtt->vm.gt);
- if (node.allocated) {
- if (drm_mm_node_allocated(&node)) { ggtt->vm.clear_range(&ggtt->vm, node.start, node.size); remove_mappable_node(&node); } else {
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index e76c9da9992d..8c1e04f402bc 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -299,7 +299,7 @@ int i915_gem_evict_for_node(struct i915_address_space *vm, break; }
GEM_BUG_ON(!node->allocated);
GEM_BUG_ON(!drm_mm_node_allocated(node));vma = container_of(node, typeof(*vma), node);
/* If we are using coloring to insert guard pages between
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 9d5b0f87c210..68c34b1a20e4 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -795,7 +795,7 @@ void i915_vma_reopen(struct i915_vma *vma)
static void __i915_vma_destroy(struct i915_vma *vma) {
- GEM_BUG_ON(vma->node.allocated);
GEM_BUG_ON(drm_mm_node_allocated(&vma->node)); GEM_BUG_ON(vma->fence);
mutex_lock(&vma->vm->mutex);
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 8bcb5812c446..e49b199f7de7 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -228,7 +228,7 @@ static inline bool i915_vma_is_closed(const struct i915_vma *vma) static inline u32 i915_ggtt_offset(const struct i915_vma *vma) { GEM_BUG_ON(!i915_vma_is_ggtt(vma));
- GEM_BUG_ON(!vma->node.allocated);
- GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); GEM_BUG_ON(upper_32_bits(vma->node.start)); GEM_BUG_ON(upper_32_bits(vma->node.start + vma->node.size - 1)); return lower_32_bits(vma->node.start);
@@ -390,7 +390,7 @@ static inline bool i915_vma_is_bound(const struct i915_vma *vma, static inline bool i915_node_color_differs(const struct drm_mm_node *node, unsigned long color) {
- return node->allocated && node->color != color;
return drm_mm_node_allocated(node) && node->color != color; }
/**
diff --git a/drivers/gpu/drm/selftests/test-drm_mm.c b/drivers/gpu/drm/selftests/test-drm_mm.c index 388f9844f4ba..9aabe82dcd3a 100644 --- a/drivers/gpu/drm/selftests/test-drm_mm.c +++ b/drivers/gpu/drm/selftests/test-drm_mm.c @@ -854,7 +854,7 @@ static bool assert_contiguous_in_range(struct drm_mm *mm,
if (start > 0) { node = __drm_mm_interval_first(mm, 0, start - 1);
if (node->allocated) {
if (drm_mm_node_allocated(node)) { pr_err("node before start: node=%llx+%llu, start=%llx\n", node->start, node->size, start); return false;@@ -863,7 +863,7 @@ static bool assert_contiguous_in_range(struct drm_mm *mm,
if (end < U64_MAX) { node = __drm_mm_interval_first(mm, end, U64_MAX);
if (node->allocated) {
if (drm_mm_node_allocated(node)) { pr_err("node after end: node=%llx+%llu, end=%llx\n", node->start, node->size, end); return false;@@ -1156,12 +1156,12 @@ static void show_holes(const struct drm_mm *mm, int count) struct drm_mm_node *next = list_next_entry(hole, node_list); const char *node1 = NULL, *node2 = NULL;
if (hole->allocated)
if (drm_mm_node_allocated(hole)) node1 = kasprintf(GFP_KERNEL, "[%llx + %lld, color=%ld], ", hole->start, hole->size, hole->color);
if (next->allocated)
if (drm_mm_node_allocated(next)) node2 = kasprintf(GFP_KERNEL, ", [%llx + %lld, color=%ld]", next->start, next->size, next->color);@@ -1900,18 +1900,18 @@ static void separate_adjacent_colors(const struct drm_mm_node *node, u64 *start, u64 *end) {
- if (node->allocated && node->color != color)
if (drm_mm_node_allocated(node) && node->color != color) ++*start;
node = list_next_entry(node, node_list);
- if (node->allocated && node->color != color)
if (drm_mm_node_allocated(node) && node->color != color) --*end; }
static bool colors_abutt(const struct drm_mm_node *node) { if (!drm_mm_hole_follows(node) &&
list_next_entry(node, node_list)->allocated) {
drm_mm_node_allocated(list_next_entry(node, node_list))) {diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index f1f0a7c87771..b00e20f5ce05 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -994,7 +994,7 @@ static void vc4_crtc_destroy_state(struct drm_crtc *crtc, struct vc4_dev *vc4 = to_vc4_dev(crtc->dev); struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
- if (vc4_state->mm.allocated) {
if (drm_mm_node_allocated(&vc4_state->mm)) { unsigned long flags;
spin_lock_irqsave(&vc4->hvs->mm_lock, flags);
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index 9936b15d0bf1..5a43659da319 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -315,7 +315,7 @@ static void vc4_hvs_unbind(struct device *dev, struct device *master, struct drm_device *drm = dev_get_drvdata(master); struct vc4_dev *vc4 = drm->dev_private;
- if (vc4->hvs->mitchell_netravali_filter.allocated)
if (drm_mm_node_allocated(&vc4->hvs->mitchell_netravali_filter)) drm_mm_remove_node(&vc4->hvs->mitchell_netravali_filter);
drm_mm_takedown(&vc4->hvs->dlist_mm);
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 5e5f90810aca..4934127f0d76 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -178,7 +178,7 @@ static void vc4_plane_destroy_state(struct drm_plane *plane, struct vc4_dev *vc4 = to_vc4_dev(plane->dev); struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
- if (vc4_state->lbm.allocated) {
if (drm_mm_node_allocated(&vc4_state->lbm)) { unsigned long irqflags;
spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
@@ -557,7 +557,7 @@ static int vc4_plane_allocate_lbm(struct drm_plane_state *state) /* Allocate the LBM memory that the HVS will use for temporary * storage due to our scaling/format conversion. */
- if (!vc4_state->lbm.allocated) {
if (!drm_mm_node_allocated(&vc4_state->lbm)) { int ret;
spin_lock_irqsave(&vc4->hvs->mm_lock, irqflags);
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
A straightforward conversion of assignment and checking of the boolean state flags (allocated, scanned) into non-atomic bitops. The caller remains responsible for all locking around the drm_mm and its nodes.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_mm.c | 18 +++++++++--------- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- drivers/gpu/drm/i915/i915_gem.c | 4 ++-- include/drm/drm_mm.h | 7 ++++--- 4 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index 99312bdc6273..a9cab5e53731 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -426,7 +426,7 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
list_add(&node->node_list, &hole->node_list); drm_mm_interval_tree_add_node(hole, node); - node->allocated = true; + __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); node->hole_size = 0;
rm_hole(hole); @@ -545,7 +545,7 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm,
list_add(&node->node_list, &hole->node_list); drm_mm_interval_tree_add_node(hole, node); - node->allocated = true; + __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
rm_hole(hole); if (adj_start > hole_start) @@ -563,7 +563,7 @@ EXPORT_SYMBOL(drm_mm_insert_node_in_range);
static inline bool drm_mm_node_scanned_block(const struct drm_mm_node *node) { - return node->scanned_block; + return test_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); }
/** @@ -589,7 +589,7 @@ void drm_mm_remove_node(struct drm_mm_node *node)
drm_mm_interval_tree_remove(node, &mm->interval_tree); list_del(&node->node_list); - node->allocated = false; + __clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
if (drm_mm_hole_follows(prev_node)) rm_hole(prev_node); @@ -627,8 +627,8 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new) &mm->holes_addr); }
- old->allocated = false; - new->allocated = true; + __clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &old->flags); + __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &new->flags); } EXPORT_SYMBOL(drm_mm_replace_node);
@@ -738,7 +738,7 @@ bool drm_mm_scan_add_block(struct drm_mm_scan *scan, DRM_MM_BUG_ON(node->mm != mm); DRM_MM_BUG_ON(!drm_mm_node_allocated(node)); DRM_MM_BUG_ON(drm_mm_node_scanned_block(node)); - node->scanned_block = true; + __set_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); mm->scan_active++;
/* Remove this block from the node_list so that we enlarge the hole @@ -824,7 +824,7 @@ bool drm_mm_scan_remove_block(struct drm_mm_scan *scan,
DRM_MM_BUG_ON(node->mm != scan->mm); DRM_MM_BUG_ON(!drm_mm_node_scanned_block(node)); - node->scanned_block = false; + __clear_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags);
DRM_MM_BUG_ON(!node->mm->scan_active); node->mm->scan_active--; @@ -922,7 +922,7 @@ void drm_mm_init(struct drm_mm *mm, u64 start, u64 size)
/* Clever trick to avoid a special case in the free hole tracking. */ INIT_LIST_HEAD(&mm->head_node.node_list); - mm->head_node.allocated = false; + mm->head_node.flags = 0; mm->head_node.mm = mm; mm->head_node.start = start + size; mm->head_node.size = -size; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 20d8a6297985..c049199a1df5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -906,7 +906,7 @@ static void reloc_cache_init(struct reloc_cache *cache, cache->use_64bit_reloc = HAS_64BIT_RELOC(i915); cache->has_fence = cache->gen < 4; cache->needs_unfenced = INTEL_INFO(i915)->unfenced_needs_alignment; - cache->node.allocated = false; + cache->node.flags = 0; cache->ce = NULL; cache->rq = NULL; cache->rq_size = 0; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index fa8e028ac0b5..7046067f70c1 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -351,7 +351,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, PIN_NOEVICT); if (!IS_ERR(vma)) { node.start = i915_ggtt_offset(vma); - node.allocated = false; + node.flags = 0; } else { ret = insert_mappable_node(ggtt, &node, PAGE_SIZE); if (ret) @@ -561,7 +561,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, PIN_NOEVICT); if (!IS_ERR(vma)) { node.start = i915_ggtt_offset(vma); - node.allocated = false; + node.flags = 0; } else { ret = insert_mappable_node(ggtt, &node, PAGE_SIZE); if (ret) diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h index 2c3bbb43c7d1..d7939c054259 100644 --- a/include/drm/drm_mm.h +++ b/include/drm/drm_mm.h @@ -168,8 +168,9 @@ struct drm_mm_node { struct rb_node rb_hole_addr; u64 __subtree_last; u64 hole_size; - bool allocated : 1; - bool scanned_block : 1; + unsigned long flags; +#define DRM_MM_NODE_ALLOCATED_BIT 0 +#define DRM_MM_NODE_SCANNED_BIT 1 #ifdef CONFIG_DRM_DEBUG_MM depot_stack_handle_t stack; #endif @@ -253,7 +254,7 @@ struct drm_mm_scan { */ static inline bool drm_mm_node_allocated(const struct drm_mm_node *node) { - return node->allocated; + return test_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); }
/**
On 03/10/2019 22:00, Chris Wilson wrote:
A straightforward conversion of assignment and checking of the boolean state flags (allocated, scanned) into non-atomic bitops. The caller remains responsible for all locking around the drm_mm and its nodes.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/drm_mm.c | 18 +++++++++--------- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- drivers/gpu/drm/i915/i915_gem.c | 4 ++-- include/drm/drm_mm.h | 7 ++++--- 4 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index 99312bdc6273..a9cab5e53731 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -426,7 +426,7 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
list_add(&node->node_list, &hole->node_list); drm_mm_interval_tree_add_node(hole, node);
- node->allocated = true;
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); node->hole_size = 0;
rm_hole(hole);
@@ -545,7 +545,7 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm,
list_add(&node->node_list, &hole->node_list); drm_mm_interval_tree_add_node(hole, node);
node->allocated = true;
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);rm_hole(hole); if (adj_start > hole_start)
@@ -563,7 +563,7 @@ EXPORT_SYMBOL(drm_mm_insert_node_in_range);
static inline bool drm_mm_node_scanned_block(const struct drm_mm_node *node) {
- return node->scanned_block;
return test_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); }
/**
@@ -589,7 +589,7 @@ void drm_mm_remove_node(struct drm_mm_node *node)
drm_mm_interval_tree_remove(node, &mm->interval_tree); list_del(&node->node_list);
- node->allocated = false;
__clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
if (drm_mm_hole_follows(prev_node)) rm_hole(prev_node);
@@ -627,8 +627,8 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new) &mm->holes_addr); }
- old->allocated = false;
- new->allocated = true;
- __clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &old->flags);
- __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &new->flags); } EXPORT_SYMBOL(drm_mm_replace_node);
@@ -738,7 +738,7 @@ bool drm_mm_scan_add_block(struct drm_mm_scan *scan, DRM_MM_BUG_ON(node->mm != mm); DRM_MM_BUG_ON(!drm_mm_node_allocated(node)); DRM_MM_BUG_ON(drm_mm_node_scanned_block(node));
- node->scanned_block = true;
__set_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); mm->scan_active++;
/* Remove this block from the node_list so that we enlarge the hole
@@ -824,7 +824,7 @@ bool drm_mm_scan_remove_block(struct drm_mm_scan *scan,
DRM_MM_BUG_ON(node->mm != scan->mm); DRM_MM_BUG_ON(!drm_mm_node_scanned_block(node));
- node->scanned_block = false;
__clear_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags);
DRM_MM_BUG_ON(!node->mm->scan_active); node->mm->scan_active--;
@@ -922,7 +922,7 @@ void drm_mm_init(struct drm_mm *mm, u64 start, u64 size)
/* Clever trick to avoid a special case in the free hole tracking. */ INIT_LIST_HEAD(&mm->head_node.node_list);
- mm->head_node.allocated = false;
- mm->head_node.flags = 0; mm->head_node.mm = mm; mm->head_node.start = start + size; mm->head_node.size = -size;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 20d8a6297985..c049199a1df5 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -906,7 +906,7 @@ static void reloc_cache_init(struct reloc_cache *cache, cache->use_64bit_reloc = HAS_64BIT_RELOC(i915); cache->has_fence = cache->gen < 4; cache->needs_unfenced = INTEL_INFO(i915)->unfenced_needs_alignment;
- cache->node.allocated = false;
- cache->node.flags = 0; cache->ce = NULL; cache->rq = NULL; cache->rq_size = 0;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index fa8e028ac0b5..7046067f70c1 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -351,7 +351,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, PIN_NOEVICT); if (!IS_ERR(vma)) { node.start = i915_ggtt_offset(vma);
node.allocated = false;
node.flags = 0;@@ -561,7 +561,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, PIN_NOEVICT); if (!IS_ERR(vma)) { node.start = i915_ggtt_offset(vma);
node.allocated = false;
node.flags = 0;diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h index 2c3bbb43c7d1..d7939c054259 100644 --- a/include/drm/drm_mm.h +++ b/include/drm/drm_mm.h @@ -168,8 +168,9 @@ struct drm_mm_node { struct rb_node rb_hole_addr; u64 __subtree_last; u64 hole_size;
- bool allocated : 1;
- bool scanned_block : 1;
- unsigned long flags;
+#define DRM_MM_NODE_ALLOCATED_BIT 0 +#define DRM_MM_NODE_SCANNED_BIT 1 #ifdef CONFIG_DRM_DEBUG_MM depot_stack_handle_t stack; #endif @@ -253,7 +254,7 @@ struct drm_mm_scan { */ static inline bool drm_mm_node_allocated(const struct drm_mm_node *node) {
- return node->allocated;
return test_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); }
/**
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
A few callers need to serialise the destruction of their drm_mm_node and ensure it is removed from the drm_mm before freeing. However, to be completely sure that any access from another thread is complete before we free the struct, we require the RELEASE semantics of clear_bit_unlock().
This allows the conditional locking such as
Thread A Thread B mutex_lock(mm_lock); if (drm_mm_node_allocated(node)) { drm_mm_node_remove(node); mutex_lock(mm_lock); mutex_unlock(mm_lock); drm_mm_node_remove(node); mutex_unlock(mm_lock); } kfree(node);
to serialise correctly without any lingering accesses from A to the freed node. Allocation / insertion of the node is assumed never to race with removal or eviction scanning.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_mm.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index a9cab5e53731..2a6e34663146 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -424,9 +424,9 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
node->mm = mm;
+ __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); list_add(&node->node_list, &hole->node_list); drm_mm_interval_tree_add_node(hole, node); - __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); node->hole_size = 0;
rm_hole(hole); @@ -543,9 +543,9 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, node->color = color; node->hole_size = 0;
+ __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); list_add(&node->node_list, &hole->node_list); drm_mm_interval_tree_add_node(hole, node); - __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
rm_hole(hole); if (adj_start > hole_start) @@ -589,11 +589,12 @@ void drm_mm_remove_node(struct drm_mm_node *node)
drm_mm_interval_tree_remove(node, &mm->interval_tree); list_del(&node->node_list); - __clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
if (drm_mm_hole_follows(prev_node)) rm_hole(prev_node); add_hole(prev_node); + + clear_bit_unlock(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); } EXPORT_SYMBOL(drm_mm_remove_node);
@@ -614,6 +615,7 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new)
*new = *old;
+ __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &new->flags); list_replace(&old->node_list, &new->node_list); rb_replace_node_cached(&old->rb, &new->rb, &mm->interval_tree);
@@ -627,8 +629,7 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new) &mm->holes_addr); }
- __clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &old->flags); - __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &new->flags); + clear_bit_unlock(DRM_MM_NODE_ALLOCATED_BIT, &old->flags); } EXPORT_SYMBOL(drm_mm_replace_node);
On 03/10/2019 22:01, Chris Wilson wrote:
A few callers need to serialise the destruction of their drm_mm_node and ensure it is removed from the drm_mm before freeing. However, to be completely sure that any access from another thread is complete before we free the struct, we require the RELEASE semantics of clear_bit_unlock().
This allows the conditional locking such as
Thread A Thread B mutex_lock(mm_lock); if (drm_mm_node_allocated(node)) { drm_mm_node_remove(node); mutex_lock(mm_lock); mutex_unlock(mm_lock); drm_mm_node_remove(node); mutex_unlock(mm_lock); } kfree(node);
My understanding is that release semantics on node allocated mean 1 -> 0 transition is guaranteed to be seen only when thread A drm_mm_node_remove is fully done with the removal. But if it is in the middle of removal, node is still seen as allocated outside and thread B can enter the if-body, wait for the lock, and then drm_mm_node_remove will attempt a double removal. So I think another drm_mm_node_allocated under the lock is needed.
But the fkree part looks correct. There can be no false negatives with the release semantics on the allocated bit.
Regards,
Tvrtko
to serialise correctly without any lingering accesses from A to the freed node. Allocation / insertion of the node is assumed never to race with removal or eviction scanning.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/drm_mm.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index a9cab5e53731..2a6e34663146 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -424,9 +424,9 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
node->mm = mm;
- __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); list_add(&node->node_list, &hole->node_list); drm_mm_interval_tree_add_node(hole, node);
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); node->hole_size = 0;
rm_hole(hole);
@@ -543,9 +543,9 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, node->color = color; node->hole_size = 0;
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);rm_hole(hole); if (adj_start > hole_start)
@@ -589,11 +589,12 @@ void drm_mm_remove_node(struct drm_mm_node *node)
drm_mm_interval_tree_remove(node, &mm->interval_tree); list_del(&node->node_list);
__clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
if (drm_mm_hole_follows(prev_node)) rm_hole(prev_node); add_hole(prev_node);
- clear_bit_unlock(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); } EXPORT_SYMBOL(drm_mm_remove_node);
@@ -614,6 +615,7 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new)
*new = *old;
- __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &new->flags); list_replace(&old->node_list, &new->node_list); rb_replace_node_cached(&old->rb, &new->rb, &mm->interval_tree);
@@ -627,8 +629,7 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new) &mm->holes_addr); }
- __clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &old->flags);
- __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &new->flags);
- clear_bit_unlock(DRM_MM_NODE_ALLOCATED_BIT, &old->flags); } EXPORT_SYMBOL(drm_mm_replace_node);
Quoting Tvrtko Ursulin (2019-10-04 10:15:20)
On 03/10/2019 22:01, Chris Wilson wrote:
A few callers need to serialise the destruction of their drm_mm_node and ensure it is removed from the drm_mm before freeing. However, to be completely sure that any access from another thread is complete before we free the struct, we require the RELEASE semantics of clear_bit_unlock().
This allows the conditional locking such as
Thread A Thread B mutex_lock(mm_lock); if (drm_mm_node_allocated(node)) { drm_mm_node_remove(node); mutex_lock(mm_lock); mutex_unlock(mm_lock); drm_mm_node_remove(node); mutex_unlock(mm_lock); } kfree(node);
My understanding is that release semantics on node allocated mean 1 -> 0 transition is guaranteed to be seen only when thread A drm_mm_node_remove is fully done with the removal. But if it is in the middle of removal, node is still seen as allocated outside and thread B can enter the if-body, wait for the lock, and then drm_mm_node_remove will attempt a double removal. So I think another drm_mm_node_allocated under the lock is needed.
Yes. Check after the lock is indeed required in this scenario. And drm_mm_node_remove() insists the caller doesn't try a double remove. -Chris
Quoting Chris Wilson (2019-10-04 12:07:10)
Quoting Tvrtko Ursulin (2019-10-04 10:15:20)
On 03/10/2019 22:01, Chris Wilson wrote:
A few callers need to serialise the destruction of their drm_mm_node and ensure it is removed from the drm_mm before freeing. However, to be completely sure that any access from another thread is complete before we free the struct, we require the RELEASE semantics of clear_bit_unlock().
This allows the conditional locking such as
Thread A Thread B mutex_lock(mm_lock); if (drm_mm_node_allocated(node)) { drm_mm_node_remove(node); mutex_lock(mm_lock); mutex_unlock(mm_lock); drm_mm_node_remove(node); mutex_unlock(mm_lock); } kfree(node);
My understanding is that release semantics on node allocated mean 1 -> 0 transition is guaranteed to be seen only when thread A drm_mm_node_remove is fully done with the removal. But if it is in the middle of removal, node is still seen as allocated outside and thread B can enter the if-body, wait for the lock, and then drm_mm_node_remove will attempt a double removal. So I think another drm_mm_node_allocated under the lock is needed.
Yes. Check after the lock is indeed required in this scenario. And drm_mm_node_remove() insists the caller doesn't try a double remove.
I had to go back and double check the vma code, and that's fine. (We hit this case where one thread is evicting and another thread is destroying the object. And for us we do the check under the lock inside __i915_vma_unbind() on the destroy path.) -Chris
On 04/10/2019 12:17, Chris Wilson wrote:
Quoting Chris Wilson (2019-10-04 12:07:10)
Quoting Tvrtko Ursulin (2019-10-04 10:15:20)
On 03/10/2019 22:01, Chris Wilson wrote:
A few callers need to serialise the destruction of their drm_mm_node and ensure it is removed from the drm_mm before freeing. However, to be completely sure that any access from another thread is complete before we free the struct, we require the RELEASE semantics of clear_bit_unlock().
This allows the conditional locking such as
Thread A Thread B mutex_lock(mm_lock); if (drm_mm_node_allocated(node)) { drm_mm_node_remove(node); mutex_lock(mm_lock); mutex_unlock(mm_lock); drm_mm_node_remove(node); mutex_unlock(mm_lock); } kfree(node);
My understanding is that release semantics on node allocated mean 1 -> 0 transition is guaranteed to be seen only when thread A drm_mm_node_remove is fully done with the removal. But if it is in the middle of removal, node is still seen as allocated outside and thread B can enter the if-body, wait for the lock, and then drm_mm_node_remove will attempt a double removal. So I think another drm_mm_node_allocated under the lock is needed.
Yes. Check after the lock is indeed required in this scenario. And drm_mm_node_remove() insists the caller doesn't try a double remove.
I had to go back and double check the vma code, and that's fine. (We hit this case where one thread is evicting and another thread is destroying the object. And for us we do the check under the lock inside __i915_vma_unbind() on the destroy path.)
So I think if you amend the commit message to contain the repeated check under the lock patch looks good to me. With that:
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
On Fri, Oct 04, 2019 at 01:01:36PM +0100, Tvrtko Ursulin wrote:
On 04/10/2019 12:17, Chris Wilson wrote:
Quoting Chris Wilson (2019-10-04 12:07:10)
Quoting Tvrtko Ursulin (2019-10-04 10:15:20)
On 03/10/2019 22:01, Chris Wilson wrote:
A few callers need to serialise the destruction of their drm_mm_node and ensure it is removed from the drm_mm before freeing. However, to be completely sure that any access from another thread is complete before we free the struct, we require the RELEASE semantics of clear_bit_unlock().
This allows the conditional locking such as
Thread A Thread B mutex_lock(mm_lock); if (drm_mm_node_allocated(node)) { drm_mm_node_remove(node); mutex_lock(mm_lock); mutex_unlock(mm_lock); drm_mm_node_remove(node); mutex_unlock(mm_lock); } kfree(node);
My understanding is that release semantics on node allocated mean 1 -> 0 transition is guaranteed to be seen only when thread A drm_mm_node_remove is fully done with the removal. But if it is in the middle of removal, node is still seen as allocated outside and thread B can enter the if-body, wait for the lock, and then drm_mm_node_remove will attempt a double removal. So I think another drm_mm_node_allocated under the lock is needed.
Yes. Check after the lock is indeed required in this scenario. And drm_mm_node_remove() insists the caller doesn't try a double remove.
I had to go back and double check the vma code, and that's fine. (We hit this case where one thread is evicting and another thread is destroying the object. And for us we do the check under the lock inside __i915_vma_unbind() on the destroy path.)
So I think if you amend the commit message to contain the repeated check under the lock patch looks good to me. With that:
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
I think a follow-up patch to update the kerneldoc to mention that we're guaranteeing this now is missing here (best with the above fixed example). Plus maybe a oneline code comment for the ALLOCATED_BIT. -Daniel
On 03/10/2019 22:00, Chris Wilson wrote:
If we unwind the active requests, and on resubmission discover that we intend to preempt the active context with itself, simply skip the ELSP submission.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/i915/gt/intel_lrc.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 431d3b8c3371..3cfea1758fd2 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1739,11 +1739,26 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (submit) { *port = execlists_schedule_in(last, port - execlists->pending);
memset(port + 1, 0, (last_port - port) * sizeof(*port));
/** Skip if we ended up with exactly the same set of requests,* e.g. trying to timeslice a pair of ordered contexts*/if (!memcmp(execlists->active, execlists->pending,(port - execlists->pending + 1) * sizeof(*port))) {doexeclists_schedule_out(fetch_and_zero(port));while (port-- != execlists->pending);goto skip_submit;}memset(port + 1, 0, (last_port - port) * sizeof(*port));+skip_submit: ring_set_paused(engine, 0); } }
A little bit of tracepoint noise with in/out but looks correct after I read the new code. And I wonder if trace.pl still works after last couple months of refactors.. :)
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Tvrtko Ursulin