Hi,
On 30/08/2022 08:33, Christian König wrote:
Hi guys,
can we get an rb/acked-by for this i915 change?
Basically we are just making sure that the driver doesn't crash when
bo->resource is NULL and a bo doesn't have any backing store assigned to
it.
The Intel CI seems to be happy with this change, so I'm pretty sure it
is correct.
It looks like DG2/DG1 (which happen to use TTM here) are no longer
loading the module:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_107680v1/index.html?
According to the logs the firmware is failing to load, so perhaps
related to I915_BO_ALLOC_CPU_CLEAR, since that is one of the rare users.
See below.
Thanks,
Christian.
Am 24.08.22 um 16:23 schrieb Luben Tuikov:
From: Christian König <christian.koenig@xxxxxxx>
Make sure we can at least move and alloc TT objects without backing
store.
Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 6 ++----
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 2 +-
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index bc9c432edffe03..45ce2d1f754cc4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -271,8 +271,6 @@ static struct ttm_tt *i915_ttm_tt_create(struct
ttm_buffer_object *bo,
{
struct drm_i915_private *i915 = container_of(bo->bdev,
typeof(*i915),
bdev);
- struct ttm_resource_manager *man =
- ttm_manager_type(bo->bdev, bo->resource->mem_type);
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
unsigned long ccs_pages = 0;
enum ttm_caching caching;
@@ -286,8 +284,8 @@ static struct ttm_tt *i915_ttm_tt_create(struct
ttm_buffer_object *bo,
if (!i915_tt)
return NULL;
- if (obj->flags & I915_BO_ALLOC_CPU_CLEAR &&
- man->use_tt)
+ if (obj->flags & I915_BO_ALLOC_CPU_CLEAR && bo->resource &&
+ ttm_manager_type(bo->bdev, bo->resource->mem_type)->use_tt)
page_flags |= TTM_TT_FLAG_ZERO_ALLOC;
AFAICT i915 was massively relying on everything starting out in a
"dummy" system memory resource (ttm_tt), where it then later
"transitions" to the real resource. And if we need to clear the memory
we rely on ZERO_ALLOC being set before calling into the i915_ttm_move()
callback (even when allocating local-memory).
For ttm_bo_type_device objects (userspace stuff) it looks like this was
previously handled by ttm_bo_validate() always doing:
ret = ttm_tt_create(bo, true); /* clear = true */
Which we would always hit since the resource was always "compatible" for
the dummy case. But it looks like this is no longer even called, since
we can now call into ttm_move with bo->resource == NULL, which still
calls tt_create eventually, which not always with clear = true.
All other objects are then ttm_bo_type_kernel so we don't care about
clearing, except in the rare case of ALLOC_CPU_CLEAR, which was handled
as per above in i915_ttm_tt_create(). But I think here bo->resource is
NULL at the start when first creating the object, which will skip
setting ZERO_ALLOC, which might explain the CI failure.
The other possible concern (not sure since CI didn't get that far) is
around ttm_bo_pipeline_gutting(), which now leaves bo->resource = NULL.
It looks like i915_ttm_shrink() was relying on that to unpopulate the
ttm_tt. When later calling ttm_bo_validate(), i915_ttm_move() would see
the SWAPPED flag set on the ttm_tt , re-populate it and then potentially
move it back to local-memory.
What are your thoughts here? Also sorry if i915 is making a bit of mess
here.
caching = i915_ttm_select_tt_caching(obj);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index 9a7e50534b84bb..c420d1ab605b6f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -560,7 +560,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo,
bool evict,
bool clear;
int ret;
- if (GEM_WARN_ON(!obj)) {
+ if (GEM_WARN_ON(!obj) || !bo->resource) {
ttm_bo_move_null(bo, dst_mem);
return 0;
}
