Hi Tvrtko,
> -----Original Message-----
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Sent: Tuesday, July 18, 2023 3:08 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/i915/dpt: Use shmem for dpt objects
>
>
> On 18/07/2023 06:33, 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.
> >
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Fixes: 0dc987b699ce ("drm/i915/display: Add smem fallback allocation for dpt")
> Cc: <stable@xxxxxxxxxxxxxxx> # v6.0+
>
> Not sure which platforms it actually applies so just mentioning to pick the right
> one.
Sure will add the above ones.
>
> > Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Fei Yang <fei.yang@xxxxxxxxx>
> > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_dpt.c | 5 ++++-
> > 1 file changed, 4 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..a57d18550a46 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpt.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpt.c
> > @@ -166,6 +166,9 @@ struct i915_vma *intel_dpt_pin(struct
> i915_address_space *vm)
> > i915_vma_get(vma);
> > }
> >
> > + if (i915_gem_object_is_shmem(dpt->obj))
> > + dpt->obj->cache_dirty = true;
>
> GPU writes to this object behind the covers or what is supposed to be the
> purpose of this?
This is to make sure that pages when writing/obtained back from swap do not
loose its contents. dpt->obj->mm.dirty = true is also achieving the same result.
Without either of the above changes, we get the following corrupt pte's.
Speaking with Chris it was suggested to unconditionally mark dpt->ob->mm.dirty = true
so that contents of the pages do not get lost during swap out/swap in.
<snip>
[ 6991.405300] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0xf2000000, Pte[0]: 0x200000f2000001
[ 6991.405803] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x0, Pte[8696]: 0x20000000000001
[ 6991.406527] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x1000, Pte[8697]: 0x20000000001001
[ 6991.406816] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x2000, Pte[8698]: 0x20000000002001
[ 6991.407071] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x3000, Pte[8699]: 0x20000000003001
[ 6991.407301] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x4000, Pte[8700]: 0x20000000004001
[ 6991.407518] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x5000, Pte[8701]: 0x20000000005001
[ 6991.407727] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x6000, Pte[8702]: 0x20000000006001
[ 6991.407939] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x7000, Pte[8703]: 0x20000000007001
[ 6991.408329] i915 0000:00:02.0: [drm:intel_power_well_enable [i915]] enabling DC_off
[ 6991.409254] i915 0000:00:02.0: [drm:gen9_set_dc_state.part.0 [i915]] Setting DC state from 02 to 00
[ 6991.441831] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0xf6000000, Pte[0]: 0x200000f6000001
[ 6991.442316] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0x0, Pte[8696]: 0x20000000000001
[ 6991.442561] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0x1000, Pte[8697]: 0x20000000001001
[ 6991.442780] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0x2000, Pte[8698]: 0x20000000002001
[ 6991.443043] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0x3000, Pte[8699]: 0x20000000003001
[ 6991.443271] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0x4000, Pte[8700]: 0x20000000004001
[ 6991.443480] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0x5000, Pte[8701]: 0x20000000005001
[ 6991.443693] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0x6000, Pte[8702]: 0x20000000006001
[ 6991.443906] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff888145828800] Addr: 0x7000, Pte[8703]: 0x20000000007001
[ 6991.478042] i915 0000:00:02.0: [drm:intel_power_well_disable [i915]] disabling DC_off
[ 6991.478545] i915 0000:00:02.0: [drm:skl_enable_dc6 [i915]] Enabling DC6
[ 6991.478747] i915 0000:00:02.0: [drm:gen9_set_dc_state.part.0 [i915]] Setting DC state from 00 to 02
// Shrinker debug prints Begin
[ 6991.479711] obj ffff88816cf3bd80 pin_count 2
[ 6991.479718] Pinned vaddr: 1929000
[ 6991.479721] obj ffff88816cf3bd80 pin_count 1
[ 6991.479728] obj ffff88816cf3cf00 pin_count 2
[ 6991.479730] Unpinned vaddr: 2000
[ 6991.479733] obj ffff88816cf3cf00 pin_count 1
[ 6991.479845] obj ffff8881458f2340 pin_count 2
[ 6991.479849] Pinned vaddr: 1940000
[ 6991.479852] obj ffff8881458f2340 pin_count 1
[ 6991.479862] obj ffff88816cf3b4c0 pin_count 2
[ 6991.479866] Unpinned vaddr: fffefdeb0000
[ 6991.479869] Pinned vaddr: 0
[ 6991.479872] obj ffff88816cf3b4c0 pin_count 0
[ 6991.480047] obj ffff88816cf3c640 pin_count 2
[ 6991.480052] Unpinned vaddr: fffefbed0000
[ 6991.480056] Unpinned vaddr: 0
[ 6991.480059] obj ffff88816cf3c640 pin_count 0
//Shrinker debug prints End
[ 6991.490179] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0xf2000000, Pte[0]: 0x200000f2000001
[ 6991.490641] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x0, Pte[8696]: 0x405420003
[ 6991.490907] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x1000, Pte[8697]: 0x18
[ 6991.491247] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x2000, Pte[8698]: 0x100661286
[ 6991.491557] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x3000, Pte[8699]: 0x5a5a5a5a5a5a5a5a
[ 6991.491829] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x4000, Pte[8700]: 0x5a5a5a5a5a5a5a5a
[ 6991.492120] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x5000, Pte[8701]: 0x5a5a5a5a5a5a5a5a
[ 6991.492396] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x6000, Pte[8702]: 0x5a5a5a5a5a5a5a5a
[ 6991.492641] i915 0000:00:02.0: [drm:intel_plane_pin_fb [i915]] DPT OBJ[ffff88814582b800] Addr: 0x7000, Pte[8703]: 0x5a5a5a5a5a5a5a5a
</snip>
>
> > +
> > atomic_dec(&i915->gpu_error.pending_fb_pin);
> > intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> >
> > @@ -261,7 +264,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);
>
> Yeah it "says on the tin" with internal objects that content is not preserved
> across re-pinning.
>
> As a disclaimer, I am not familiar with the lifetime of the DPT objects and their
> content. But the statement that page table entries are corrupt sounds worrying,
> like there is more to it than the backing store type.
We pin the frame buffer object just before programming the DE the ggtt offset part
of updating the plane(hw dma streamer) and unpin it after scanout by the hardware is
complete.
>
> Also, there is "*make_unshrinkable*" pair of functions which could be used. But
> again that would be about object content, not corrupt PTEs.. Unless with
> corrupt what is meant is stale? Maybe put a poison into dpt_clear_range to see
> if unpinned range is what the display engine will hit?
Make unshrinkable actually does not help as the fb object is getting evicted when it is not
pinned. After the shrinker comes into play pinning the framebuffer objects is causing wrong
page table entries. Making object unshrinkable from when it is created to when it is destroyed
might be the way but can cause any user to choke the memory by doing so.
Thanks,
Radhakrishna(RK) Sripada
>
> Regards,
>
> Tvrtko
>
> > }
> > if (IS_ERR(dpt_obj))
> > return ERR_CAST(dpt_obj);
