> 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. intel_dpt_suspend is "suspending" the whole VM where, not only the dpt objects are mapped into, but also the framebuffer objects. I don't have much knowledge on how the framebuffer objects are managed, but the suspend resume path still look necessary to me, unless the content of these framebuffer objects are also 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 >