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

[Yang, Fei]

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

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
>