Re: [Intel-gfx] [PATCH v2] drm/i915/dpt: Use shmem for dpt objects

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@vger.kernel.org> # v6.0+
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Fei Yang <fei.yang@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
>   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