| Message ID | 20190915184539.16724-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dma-fence: Serialise signal enabling (dma_fence_enable_sw_signaling) | expand |
>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf >Of Chris Wilson >Sent: Sunday, September 15, 2019 2:46 PM >To: dri-devel@lists.freedesktop.org >Cc: intel-gfx@lists.freedesktop.org >Subject: [PATCH 2/2] drm/mm: Pack allocated/scanned boolean into a bitfield > >The ulterior motive to switching the booleans over to bitops is to >allow use of the allocated flag as a bitlock. > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >--- > drivers/gpu/drm/drm_mm.c | 36 +++++++++++-------- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++-- > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +- > drivers/gpu/drm/i915/i915_gem.c | 16 ++++----- > 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 +-- > include/drm/drm_mm.h | 7 ++-- > 12 files changed, 52 insertions(+), 45 deletions(-) > >diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c >index 4581c5387372..211967006cec 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); >@@ -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); Maybe a silly question(s), but you appear to be mixing atomic bit ops (clear_ and test_) with non-atomic bit ops __xxx_bit(). Should that be a concern? Should that be mention in the commit? Mike > list_add(&node->node_list, &hole->node_list); > drm_mm_interval_tree_add_node(hole, node); >- node->allocated = true; > 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); >- node->allocated = true; > > rm_hole(hole); > if (adj_start > hole_start) >@@ -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 test_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); >+} >+ > /** > * 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); > >@@ -584,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); >- node->allocated = false; > > 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); > >@@ -605,7 +611,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; > >@@ -622,8 +628,7 @@ 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_unlock(DRM_MM_NODE_ALLOCATED_BIT, &old->flags); > } > EXPORT_SYMBOL(drm_mm_replace_node); > >@@ -731,9 +736,9 @@ 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); >- node->scanned_block = true; >+ DRM_MM_BUG_ON(!drm_mm_node_allocated(node)); >+ DRM_MM_BUG_ON(drm_mm_node_scanned_block(node)); >+ __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 >@@ -818,8 +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); >- node->scanned_block = false; >+ DRM_MM_BUG_ON(!drm_mm_node_scanned_block(node)); > > DRM_MM_BUG_ON(!node->mm->scan_active); > node->mm->scan_active--; >@@ -837,6 +841,8 @@ bool drm_mm_scan_remove_block(struct >drm_mm_scan *scan, > list_next_entry(node, node_list)); > list_add(&node->node_list, &prev_node->node_list); > >+ __clear_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); >+ > return (node->start + node->size > scan->hit_start && > node->start < scan->hit_end); > } >@@ -917,7 +923,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 27dbcb508055..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; >@@ -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 296a82603be0..07fc6f28abcd 100644 >--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >@@ -401,7 +401,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 2da9544fa9a4..b8f7fe311b0a 100644 >--- a/drivers/gpu/drm/i915/i915_gem.c >+++ b/drivers/gpu/drm/i915/i915_gem.c >@@ -351,12 +351,12 @@ 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) > 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 { >@@ -561,12 +561,12 @@ 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) > 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 411047d6a909..d10614091bb9 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); >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); > } > > /** >-- >2.23.0 > >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoting Ruhl, Michael J (2019-09-16 20:45:14) > >-----Original Message----- > >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf > >Of Chris Wilson > >Sent: Sunday, September 15, 2019 2:46 PM > >@@ -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); > > Maybe a silly question(s), but you appear to be mixing atomic bit ops > (clear_ and test_) with non-atomic bit ops __xxx_bit(). > > Should that be a concern? Should that be mention in the commit? Generally yes, but this is inside an allocation function so the new node cannot be accessed concurrently yet (and manipulation of the drm_mm itself requires external serialisation). The concern is with blocks like > >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); where we are testing the bit prior to taking the lock to serialise removal before free. To avoid the cost of serialising here we have to be sure that any other thread has completely stopped using the drm_mm_node when it is marked as released. -Chris
On 15/09/2019 19:45, Chris Wilson wrote: > The ulterior motive to switching the booleans over to bitops is to > allow use of the allocated flag as a bitlock. Locked bit usage applies only to DRM_MM_NODE_ALLOCATED_BIT? Any value in extracting the conversion to calling drm_mm_node_allocated ahead of the main patch? Maybe introducing drm_mm_node_scanned_block helper in a separate patch as well. Then it's really easy to focus on the real change in a last patch which just switches the implementation over. But it's not too critical to do this. I don't think patch is that big or that it has any potential for regression on it's own to be important it is split into smaller chunks. So I think this is "would be nice" category of suggestions. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/drm_mm.c | 36 +++++++++++-------- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++-- > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +- > drivers/gpu/drm/i915/i915_gem.c | 16 ++++----- > 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 +-- > include/drm/drm_mm.h | 7 ++-- > 12 files changed, 52 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c > index 4581c5387372..211967006cec 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); > @@ -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); > - node->allocated = true; > 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); > - node->allocated = true; > > rm_hole(hole); > if (adj_start > hole_start) > @@ -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 test_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); > +} > + > /** > * 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); > > @@ -584,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); > - node->allocated = false; > > 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); > > @@ -605,7 +611,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; > > @@ -622,8 +628,7 @@ 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_unlock(DRM_MM_NODE_ALLOCATED_BIT, &old->flags); > } > EXPORT_SYMBOL(drm_mm_replace_node); > > @@ -731,9 +736,9 @@ 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); > - node->scanned_block = true; > + DRM_MM_BUG_ON(!drm_mm_node_allocated(node)); > + DRM_MM_BUG_ON(drm_mm_node_scanned_block(node)); > + __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 > @@ -818,8 +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); > - node->scanned_block = false; > + DRM_MM_BUG_ON(!drm_mm_node_scanned_block(node)); > > DRM_MM_BUG_ON(!node->mm->scan_active); > node->mm->scan_active--; > @@ -837,6 +841,8 @@ bool drm_mm_scan_remove_block(struct drm_mm_scan *scan, > list_next_entry(node, node_list)); > list_add(&node->node_list, &prev_node->node_list); > > + __clear_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); > + > return (node->start + node->size > scan->hit_start && > node->start < scan->hit_end); > } > @@ -917,7 +923,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 27dbcb508055..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; > @@ -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 296a82603be0..07fc6f28abcd 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > @@ -401,7 +401,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 2da9544fa9a4..b8f7fe311b0a 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -351,12 +351,12 @@ 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) > 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 { > @@ -561,12 +561,12 @@ 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) > 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 411047d6a909..d10614091bb9 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); > 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 Document here the rules for manipulating this bit would be good. > +#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); > } > > /** > Regards, Tvrtko
>-----Original Message----- >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf >Of Chris Wilson >Sent: Thursday, October 3, 2019 3:08 AM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; dri- >devel@lists.freedesktop.org >Cc: intel-gfx@lists.freedesktop.org >Subject: RE: [PATCH 2/2] drm/mm: Pack allocated/scanned boolean into a >bitfield > >Quoting Ruhl, Michael J (2019-09-16 20:45:14) >> >-----Original Message----- >> >From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On >Behalf >> >Of Chris Wilson >> >Sent: Sunday, September 15, 2019 2:46 PM >> >@@ -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); >> >> Maybe a silly question(s), but you appear to be mixing atomic bit ops >> (clear_ and test_) with non-atomic bit ops __xxx_bit(). >> >> Should that be a concern? Should that be mention in the commit? > >Generally yes, but this is inside an allocation function so the new node >cannot be accessed concurrently yet (and manipulation of the drm_mm >itself requires external serialisation). Got it. Thanks for the clarification. Mike >The concern is with blocks like > >> >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); > >where we are testing the bit prior to taking the lock to serialise >removal before free. To avoid the cost of serialising here we have to be >sure that any other thread has completely stopped using the drm_mm_node >when it is marked as released. >-Chris >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index 4581c5387372..211967006cec 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); @@ -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); - node->allocated = true; 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); - node->allocated = true; rm_hole(hole); if (adj_start > hole_start) @@ -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 test_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); +} + /** * 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); @@ -584,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); - node->allocated = false; 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); @@ -605,7 +611,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; @@ -622,8 +628,7 @@ 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_unlock(DRM_MM_NODE_ALLOCATED_BIT, &old->flags); } EXPORT_SYMBOL(drm_mm_replace_node); @@ -731,9 +736,9 @@ 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); - node->scanned_block = true; + DRM_MM_BUG_ON(!drm_mm_node_allocated(node)); + DRM_MM_BUG_ON(drm_mm_node_scanned_block(node)); + __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 @@ -818,8 +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); - node->scanned_block = false; + DRM_MM_BUG_ON(!drm_mm_node_scanned_block(node)); DRM_MM_BUG_ON(!node->mm->scan_active); node->mm->scan_active--; @@ -837,6 +841,8 @@ bool drm_mm_scan_remove_block(struct drm_mm_scan *scan, list_next_entry(node, node_list)); list_add(&node->node_list, &prev_node->node_list); + __clear_bit(DRM_MM_NODE_SCANNED_BIT, &node->flags); + return (node->start + node->size > scan->hit_start && node->start < scan->hit_end); } @@ -917,7 +923,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 27dbcb508055..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; @@ -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 296a82603be0..07fc6f28abcd 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -401,7 +401,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 2da9544fa9a4..b8f7fe311b0a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -351,12 +351,12 @@ 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) 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 { @@ -561,12 +561,12 @@ 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) 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 411047d6a909..d10614091bb9 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); 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); } /**
The ulterior motive to switching the booleans over to bitops is to allow use of the allocated flag as a bitlock. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/drm_mm.c | 36 +++++++++++-------- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++-- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +- drivers/gpu/drm/i915/i915_gem.c | 16 ++++----- 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 +-- include/drm/drm_mm.h | 7 ++-- 12 files changed, 52 insertions(+), 45 deletions(-)