On Fri, Nov 14, 2025 at 1:47 PM Ian Forbes <[email protected]> wrote: > > This hashtable is only used under a lock in vmw_execbuf_process and needs > to be cleared before vmw_execbuf_process returns otherwise bad things > happen because the nodes that are stored in the table come from an arena > allocator that is cleared at the end of the function. > > Rather than wasting time cleaning up the hashtable move it onto the stack > so we don't have to do any cleanup. > > Signed-off-by: Ian Forbes <[email protected]> > --- > > v2: > - remove sw_context from the validation context > > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 -- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 - > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 7 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +- > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 6 +- > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 98 ++++------------------ > drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 18 ++-- > 8 files changed, 32 insertions(+), 115 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > index abceffea3683..3ac9eb8a2e21 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > @@ -814,13 +814,6 @@ static void vmw_write_driver_id(struct vmw_private *dev) > } > } > > -static void vmw_sw_context_init(struct vmw_private *dev_priv) > -{ > - struct vmw_sw_context *sw_context = &dev_priv->ctx; > - > - hash_init(sw_context->res_ht); > -} > - > static void vmw_sw_context_fini(struct vmw_private *dev_priv) > { > struct vmw_sw_context *sw_context = &dev_priv->ctx; > @@ -836,8 +829,6 @@ static int vmw_driver_load(struct vmw_private *dev_priv, > u32 pci_id) > enum vmw_res_type i; > bool refuse_dma = false; > > - vmw_sw_context_init(dev_priv); > - > mutex_init(&dev_priv->cmdbuf_mutex); > mutex_init(&dev_priv->binding_mutex); > spin_lock_init(&dev_priv->resource_lock); > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h > index 62db3f3f3aa0..f7c760d72b85 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h > @@ -79,7 +79,6 @@ > #define VMW_RES_STREAM ttm_driver_type2 > #define VMW_RES_FENCE ttm_driver_type3 > #define VMW_RES_SHADER ttm_driver_type4 > -#define VMW_RES_HT_ORDER 12 > > #define MKSSTAT_CAPACITY_LOG2 5U > #define MKSSTAT_CAPACITY (1U << MKSSTAT_CAPACITY_LOG2) > @@ -348,7 +347,6 @@ struct vmw_ctx_validation_info; > > /** > * struct vmw_sw_context - Command submission context > - * @res_ht: Pointer hash table used to find validation duplicates > * @kernel: Whether the command buffer originates from kernel code rather > * than from user-space > * @fp: If @kernel is false, points to the file of the client. Otherwise > @@ -378,7 +376,6 @@ struct vmw_ctx_validation_info; > * @ctx: The validation context > */ > struct vmw_sw_context{ > - DECLARE_HASHTABLE(res_ht, VMW_RES_HT_ORDER); > bool kernel; > struct vmw_fpriv *fp; > struct drm_file *filp; > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > index 3057f8baa7d2..3a211baa8b88 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > @@ -4094,7 +4094,7 @@ int vmw_execbuf_process(struct drm_file *file_priv, > int ret; > int32_t out_fence_fd = -1; > struct sync_file *sync_file = NULL; > - DECLARE_VAL_CONTEXT(val_ctx, sw_context, 1); > + DECLARE_VAL_CONTEXT(val_ctx, 1); > > if (flags & DRM_VMW_EXECBUF_FLAG_EXPORT_FENCE_FD) { > out_fence_fd = get_unused_fd_flags(O_CLOEXEC); > @@ -4184,8 +4184,6 @@ int vmw_execbuf_process(struct drm_file *file_priv, > if (unlikely(ret != 0)) > goto out_err; > > - vmw_validation_drop_ht(&val_ctx); > - > ret = mutex_lock_interruptible(&dev_priv->binding_mutex); > if (unlikely(ret != 0)) { > ret = -ERESTARTSYS; > @@ -4294,7 +4292,6 @@ int vmw_execbuf_process(struct drm_file *file_priv, > __vmw_execbuf_release_pinned_bo(dev_priv, NULL); > out_unlock: > vmw_cmdbuf_res_revert(&sw_context->staged_cmd_res); > - vmw_validation_drop_ht(&val_ctx); > WARN_ON(!list_empty(&sw_context->ctx_list)); > mutex_unlock(&dev_priv->cmdbuf_mutex); > > @@ -4363,7 +4360,7 @@ void __vmw_execbuf_release_pinned_bo(struct vmw_private > *dev_priv, > { > int ret = 0; > struct vmw_fence_obj *lfence = NULL; > - DECLARE_VAL_CONTEXT(val_ctx, NULL, 0); > + DECLARE_VAL_CONTEXT(val_ctx, 0); > > if (dev_priv->pinned_bo == NULL) > goto out_unlock; > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > index 07f2a5ead34b..4446f25e526d 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c > @@ -1739,7 +1739,7 @@ int vmw_du_helper_plane_update(struct > vmw_du_update_plane *update) > struct drm_atomic_helper_damage_iter iter; > struct drm_rect clip; > struct drm_rect bb; > - DECLARE_VAL_CONTEXT(val_ctx, NULL, 0); > + DECLARE_VAL_CONTEXT(val_ctx, 0); > uint32_t reserved_size = 0; > uint32_t submit_size = 0; > uint32_t curr_size = 0; > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c > index 5f5f5a94301f..4e85ba5206ae 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c > @@ -1104,7 +1104,7 @@ int vmw_kms_sou_do_surface_dirty(struct vmw_private > *dev_priv, > struct vmw_framebuffer_surface *vfbs = > container_of(framebuffer, typeof(*vfbs), base); > struct vmw_kms_sou_surface_dirty sdirty; > - DECLARE_VAL_CONTEXT(val_ctx, NULL, 0); > + DECLARE_VAL_CONTEXT(val_ctx, 0); > int ret; > > if (!srf) > @@ -1219,7 +1219,7 @@ int vmw_kms_sou_do_bo_dirty(struct vmw_private > *dev_priv, > container_of(framebuffer, struct vmw_framebuffer_bo, > base)->buffer; > struct vmw_kms_dirty dirty; > - DECLARE_VAL_CONTEXT(val_ctx, NULL, 0); > + DECLARE_VAL_CONTEXT(val_ctx, 0); > int ret; > > vmw_bo_placement_set(buf, VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM, > @@ -1327,7 +1327,7 @@ int vmw_kms_sou_readback(struct vmw_private *dev_priv, > struct vmw_bo *buf = > container_of(vfb, struct vmw_framebuffer_bo, base)->buffer; > struct vmw_kms_dirty dirty; > - DECLARE_VAL_CONTEXT(val_ctx, NULL, 0); > + DECLARE_VAL_CONTEXT(val_ctx, 0); > int ret; > > vmw_bo_placement_set(buf, VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM, > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c > index 37cb742ba1d9..faacfef7baa5 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c > @@ -566,7 +566,7 @@ int vmw_kms_stdu_readback(struct vmw_private *dev_priv, > container_of(vfb, struct vmw_framebuffer_bo, base)->buffer; > struct vmw_stdu_dirty ddirty; > int ret; > - DECLARE_VAL_CONTEXT(val_ctx, NULL, 0); > + DECLARE_VAL_CONTEXT(val_ctx, 0); > > /* > * The GMR domain might seem confusing because it might seem like it > should > @@ -733,7 +733,7 @@ int vmw_kms_stdu_surface_dirty(struct vmw_private > *dev_priv, > struct vmw_framebuffer_surface *vfbs = > container_of(framebuffer, typeof(*vfbs), base); > struct vmw_stdu_dirty sdirty; > - DECLARE_VAL_CONTEXT(val_ctx, NULL, 0); > + DECLARE_VAL_CONTEXT(val_ctx, 0); > int ret; > > if (!srf) > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > index 35dc94c3db39..508809712398 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > @@ -162,24 +162,13 @@ vmw_validation_find_bo_dup(struct > vmw_validation_context *ctx, > if (!ctx->merge_dups) > return NULL; > > - if (ctx->sw_context) { > - struct vmwgfx_hash_item *hash; > - unsigned long key = (unsigned long) vbo; > - > - hash_for_each_possible_rcu(ctx->sw_context->res_ht, hash, > head, key) { > - if (hash->key == key) { > - bo_node = container_of(hash, > typeof(*bo_node), hash); > - break; > - } > - } > - } else { > - struct vmw_validation_bo_node *entry; > + struct vmwgfx_hash_item *hash; > + unsigned long key = (unsigned long)vbo; > > - list_for_each_entry(entry, &ctx->bo_list, base.head) { > - if (entry->base.bo == &vbo->tbo) { > - bo_node = entry; > - break; > - } > + hash_for_each_possible(ctx->res_ht, hash, head, key) { > + if (hash->key == key) { > + bo_node = container_of(hash, typeof(*bo_node), hash); > + break; > } > } > > @@ -204,35 +193,15 @@ vmw_validation_find_res_dup(struct > vmw_validation_context *ctx, > if (!ctx->merge_dups) > return NULL; > > - if (ctx->sw_context) { > - struct vmwgfx_hash_item *hash; > - unsigned long key = (unsigned long) res; > - > - hash_for_each_possible_rcu(ctx->sw_context->res_ht, hash, > head, key) { > - if (hash->key == key) { > - res_node = container_of(hash, > typeof(*res_node), hash); > - break; > - } > - } > - } else { > - struct vmw_validation_res_node *entry; > - > - list_for_each_entry(entry, &ctx->resource_ctx_list, head) { > - if (entry->res == res) { > - res_node = entry; > - goto out; > - } > - } > + struct vmwgfx_hash_item *hash; > + unsigned long key = (unsigned long)res; > > - list_for_each_entry(entry, &ctx->resource_list, head) { > - if (entry->res == res) { > - res_node = entry; > - break; > - } > + hash_for_each_possible(ctx->res_ht, hash, head, key) { > + if (hash->key == key) { > + res_node = container_of(hash, typeof(*res_node), > hash); > + break; > } > - > } > -out: > return res_node; > } > > @@ -256,10 +225,9 @@ int vmw_validation_add_bo(struct vmw_validation_context > *ctx, > if (!bo_node) > return -ENOMEM; > > - if (ctx->sw_context) { > + if (ctx->merge_dups) { > bo_node->hash.key = (unsigned long) vbo; > - hash_add_rcu(ctx->sw_context->res_ht, > &bo_node->hash.head, > - bo_node->hash.key); > + hash_add(ctx->res_ht, &bo_node->hash.head, > bo_node->hash.key); > } > val_buf = &bo_node->base; > vmw_bo_reference(vbo); > @@ -303,13 +271,13 @@ int vmw_validation_add_resource(struct > vmw_validation_context *ctx, > return -ENOMEM; > } > > - if (ctx->sw_context) { > + if (ctx->merge_dups) { > node->hash.key = (unsigned long) res; > - hash_add_rcu(ctx->sw_context->res_ht, &node->hash.head, > node->hash.key); > + hash_add(ctx->res_ht, &node->hash.head, node->hash.key); > } > node->res = vmw_resource_reference_unless_doomed(res); > if (!node->res) { > - hash_del_rcu(&node->hash.head); > + hash_del(&node->hash.head); > return -ESRCH;
This bit looks suspicious. It looks like if ctx->merge_dups is false and the resource is doomed then we'll try to remove an invalid node (because it has not been added to the hash). Other than that, it looks great. z
smime.p7s
Description: S/MIME Cryptographic Signature
