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@xxxxxxx>
> ---
> 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
