On 18/07/2023 23:51, Radhakrishna Sripada wrote:
Dpt objects that are created from internal get evicted when there is
memory pressure and do not get restored when pinned during scanout. The
pinned page table entries look corrupted and programming the display
engine with the incorrect pte's result in DE throwing pipe faults.
Create DPT objects from shmem and mark the object as dirty when pinning so
that the object is restored when shrinker evicts an unpinned buffer object.
v2: Unconditionally mark the dpt objects dirty during pinning(Chris).
Fixes: 0dc987b699ce ("drm/i915/display: Add smem fallback allocation for dpt")
Cc: <stable@xxxxxxxxxxxxxxx> # v6.0+
Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
Suggested-by: Chris Wilson <chris.p.wilson@xxxxxxxxx>
Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
---
drivers/gpu/drm/i915/display/intel_dpt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c b/drivers/gpu/drm/i915/display/intel_dpt.c
index 7c5fddb203ba..fbfd8f959f17 100644
--- a/drivers/gpu/drm/i915/display/intel_dpt.c
+++ b/drivers/gpu/drm/i915/display/intel_dpt.c
@@ -166,6 +166,8 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
i915_vma_get(vma);
}
+ dpt->obj->mm.dirty = true;
+
atomic_dec(&i915->gpu_error.pending_fb_pin);
intel_runtime_pm_put(&i915->runtime_pm, wakeref);
@@ -261,7 +263,7 @@ intel_dpt_create(struct intel_framebuffer *fb)
dpt_obj = i915_gem_object_create_stolen(i915, size);
if (IS_ERR(dpt_obj) && !HAS_LMEM(i915)) {
drm_dbg_kms(&i915->drm, "Allocating dpt from smem\n");
- dpt_obj = i915_gem_object_create_internal(i915, size);
+ dpt_obj = i915_gem_object_create_shmem(i915, size);
}
if (IS_ERR(dpt_obj))
return ERR_CAST(dpt_obj);
Okay I think I get it after some more looking at the DPT code paths.
Problem seems pretty clear - page tables are stored in dpt_obj and so
are lost when backing store is discarded.
Changing to shmem object indeed looks the easiest option.
Some related thoughts:
1)
I wonder if intel_dpt_suspend/resume remain needed after this patch.
Could you investigate please? On a glance their job was to restore the
PTEs which would be lost from internal objects backing storage. With
shmem objects that content should be preserved.
2)
I wonder if i915_vma_flush_writes should be used (as a companion of
i915_vma_pin_iomap) from DPT dpt_bind_vma, dpt_insert_entries, etc. But
then I am also not sure if it does the right thing for the
i915_gem_object_pin_map path of i915_vma_pin_iomap. Perhaps it should
call __i915_gem_object_flush_map itself for that mapping flavour and not
do the ggtt flushing in that case.
In summary I think the fix is safe and correct but at least point 1) I
think needs looking into. It can be a follow up work too.
Regards,
Tvrtko
