| Message ID | 20240723121750.2086-5-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] drm/amdgpu: use GEM references instead of TTMs | expand |
On Tue, Jul 23, 2024 at 02:17:47PM +0200, Christian König wrote: > This reverts commit 64ad2abfe9a628ce79859d072704bd1ef7682044. > > To me it looks like this functionality was never actually used. At least > I can't find any protection in vmw_bo_free(). Just to double-check I've done the audit of all callers, and they all look like they're holding a full reference indeed. The somewhat annoying case was vmw_sw_context->cur_query_bo because it seems to not be refcounted itself. But that's either dev_priv->pinned_bo or dev_priv->dummy_query_bo, both of which are refcounted, so we're good. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Christian König <christian.koenig@amd.com> > Cc: Zack Rusin <zack.rusin@broadcom.com> > Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > index e7625b3f71e0..e11837e484aa 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > @@ -262,7 +262,8 @@ int vmw_validation_add_bo(struct vmw_validation_context *ctx, > bo_node->hash.key); > } > val_buf = &bo_node->base; > - val_buf->bo = ttm_bo_get_unless_zero(&vbo->tbo); > + vmw_bo_reference(vbo); > + val_buf->bo = &vbo->tbo; > if (!val_buf->bo) > return -ESRCH; > val_buf->num_shared = 0; > @@ -656,7 +657,7 @@ void vmw_validation_unref_lists(struct vmw_validation_context *ctx) > struct vmw_validation_res_node *val; > > list_for_each_entry(entry, &ctx->bo_list, base.head) { > - ttm_bo_put(entry->base.bo); > + drm_gem_object_put(&entry->base.bo->base); > entry->base.bo = NULL; > } > > -- > 2.34.1 >
On Tue, 2024-07-23 at 14:17 +0200, Christian König wrote: > This reverts commit 64ad2abfe9a628ce79859d072704bd1ef7682044. > > To me it looks like this functionality was never actually used. At > least > I can't find any protection in vmw_bo_free(). > > Signed-off-by: Christian König <christian.koenig@amd.com> > Cc: Zack Rusin <zack.rusin@broadcom.com> > Cc: Broadcom internal kernel review list > <bcm-kernel-feedback-list@broadcom.com> IIRC the reference-free lookups were used to avoid the extensive referencing and unreferencing during the command stream parsing by means of rcu protection, so when vmw_validation_add() was called the bo pointer might have been only rcu-protected. From a brief look this looks like it's been changed with the gem rewrite and if so, this patch should probably be safe. /Thomas > --- > drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > index e7625b3f71e0..e11837e484aa 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > @@ -262,7 +262,8 @@ int vmw_validation_add_bo(struct > vmw_validation_context *ctx, > bo_node->hash.key); > } > val_buf = &bo_node->base; > - val_buf->bo = ttm_bo_get_unless_zero(&vbo->tbo); > + vmw_bo_reference(vbo); > + val_buf->bo = &vbo->tbo; > if (!val_buf->bo) > return -ESRCH; > val_buf->num_shared = 0; > @@ -656,7 +657,7 @@ void vmw_validation_unref_lists(struct > vmw_validation_context *ctx) > struct vmw_validation_res_node *val; > > list_for_each_entry(entry, &ctx->bo_list, base.head) { > - ttm_bo_put(entry->base.bo); > + drm_gem_object_put(&entry->base.bo->base); > entry->base.bo = NULL; > } >
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c index e7625b3f71e0..e11837e484aa 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c @@ -262,7 +262,8 @@ int vmw_validation_add_bo(struct vmw_validation_context *ctx, bo_node->hash.key); } val_buf = &bo_node->base; - val_buf->bo = ttm_bo_get_unless_zero(&vbo->tbo); + vmw_bo_reference(vbo); + val_buf->bo = &vbo->tbo; if (!val_buf->bo) return -ESRCH; val_buf->num_shared = 0; @@ -656,7 +657,7 @@ void vmw_validation_unref_lists(struct vmw_validation_context *ctx) struct vmw_validation_res_node *val; list_for_each_entry(entry, &ctx->bo_list, base.head) { - ttm_bo_put(entry->base.bo); + drm_gem_object_put(&entry->base.bo->base); entry->base.bo = NULL; }
This reverts commit 64ad2abfe9a628ce79859d072704bd1ef7682044. To me it looks like this functionality was never actually used. At least I can't find any protection in vmw_bo_free(). Signed-off-by: Christian König <christian.koenig@amd.com> Cc: Zack Rusin <zack.rusin@broadcom.com> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com> --- drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)