On 31/08/2022 09:16, Christian König wrote:
Hi Matthew,
Am 30.08.22 um 12:45 schrieb Matthew Auld:
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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fintel-gfx-ci.01.org%2Ftree%2Fdrm-tip%2FPatchwork_107680v1%2Findex.html&data=05%7C01%7Cchristian.koenig%40amd.com%7Caa9bdb0e31064859568708da8a74b899%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637974531164663116%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=UW8BEnIFXHawhAfLUcknGmE88g2wwAiTLAQ3Y5v1pFA%3D&reserved=0?
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.
First of all thanks a lot for taking a look. We previously got reports
about strange crashes with this patch, but couldn't really reproduce
them (even not by sending them out again). This explains that a bit.
The simplest solution would be to just change the && into a ||, e.g.
when previously either no resource is allocated or the resource requires
to use a tt we clear it.
That should give you the same behavior as before, but I agree that this
is a bit messy.
Yeah, that should do the trick.
That hopefully just leaves i915_ttm_shrink(), which is swapping out
shmem ttm_tt and is calling ttm_bo_validate() with empty placements to
force the pipeline-gutting path, which importantly unpopulates the
ttm_tt for us (since ttm_tt_unpopulate is not exported it seems). But
AFAICT it looks like that will now also nuke the bo->resource, instead
of just leaving it in system memory. My assumption is that when later
calling ttm_bo_validate(), it will just do the bo_move_null() in
i915_ttm_move(), instead of re-populating the ttm_tt and then
potentially copying it back to local-memory?
I've been considering to replacing the ttm_bo_type with a bunch of
behavior flags for a bo. I'm hoping that this will clean things up a bit.
Regards,
Christian.
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;
}
