Hi, Christian,
Still working through some backlog and this series appears to have slipped under the radar.
Still I think a cover letter would benefit reviewers...
On Mon, 2021-07-19 at 13:51 +0200, Christian König wrote:
Keep track for which BO a resource was allocated. This is necessary to move the LRU handling into the resources.
So is this needed, when looking up a resource from the LRU, to find the bo the resource is currently attached to?
A bit problematic is i915 since it tries to use the resource interface without a BO which is illegal from the conceptional
s/conceptional/conceptual/ ?
point of view.
v2: Document that this is a weak reference and add a workaround for i915
Hmm. As pointed out in my previous review the weak pointer should really be cleared under a lookup lock to avoid use-after-free bugs. I don't see that happening here. Doesn't this series aim for a future direction of early destruction of bos and ditching the ghost objects? In that case IMHO you need to allow for a NULL bo pointer and any bo information needed at resource destruction needs to be copied on the resource... (That would also ofc help with the i915 problem).
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/i915/intel_region_ttm.c | 5 +++++ drivers/gpu/drm/ttm/ttm_bo_util.c | 7 +++++-- drivers/gpu/drm/ttm/ttm_resource.c | 1 + include/drm/ttm/ttm_resource.h | 2 ++ 4 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c index 27fe0668d094..980b10a7debf 100644 --- a/drivers/gpu/drm/i915/intel_region_ttm.c +++ b/drivers/gpu/drm/i915/intel_region_ttm.c @@ -88,6 +88,7 @@ intel_region_ttm_node_reserve(struct intel_memory_region *mem, place.fpfn = offset >> PAGE_SHIFT; place.lpfn = place.fpfn + (size >> PAGE_SHIFT); mock_bo.base.size = size; + mock_bo.bdev = &mem->i915->bdev; ret = man->func->alloc(man, &mock_bo, &place, &res); if (ret == -ENOSPC) ret = -ENXIO; @@ -104,7 +105,11 @@ void intel_region_ttm_node_free(struct intel_memory_region *mem, struct ttm_resource *res) { struct ttm_resource_manager *man = mem->region_private; + struct ttm_buffer_object mock_bo = {}; + mock_bo.base.size = res->num_pages * PAGE_SIZE; + mock_bo.bdev = &mem->i915->bdev; + res->bo = &mock_bo; man->func->free(man, res); } diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index 2f57f824e6db..a1570aa8ff56 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -239,6 +239,11 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, if (bo->type != ttm_bo_type_sg) fbo->base.base.resv = &fbo->base.base._resv; + if (fbo->base.resource) { + fbo->base.resource->bo = &fbo->base;
This is scary: What if someone else (LRU lookup) just dereferenced the previous resource->bo pointer? I think this needs proper weak reference locking and lifetime handling, as mentioned above.
+ bo->resource = NULL; + }
/Thomas