On Thu, May 23, 2024 at 01:07:24PM +0100, Tvrtko Ursulin wrote:
>
> On 23/05/2024 12:19, Ville Syrjälä wrote:
> > On Thu, May 23, 2024 at 09:25:45AM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 22/05/2024 16:29, Vidya Srinivas wrote:
> >>> In some scenarios, the DPT object gets shrunk but
> >>> the actual framebuffer did not and thus its still
> >>> there on the DPT's vm->bound_list. Then it tries to
> >>> rewrite the PTEs via a stale CPU mapping. This causes panic.
> >>>
> >>> Suggested-by: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> >>> Cc: stable@xxxxxxxxxxxxxxx
> >>> Fixes: 0dc987b699ce ("drm/i915/display: Add smem fallback allocation for dpt")
> >>> Signed-off-by: Vidya Srinivas <vidya.srinivas@xxxxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> >>> index 3560a062d287..e6b485fc54d4 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> >>> @@ -284,7 +284,8 @@ bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj);
> >>> static inline bool
> >>> i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj)
> >>> {
> >>> - return i915_gem_object_type_has(obj, I915_GEM_OBJECT_IS_SHRINKABLE);
> >>> + return i915_gem_object_type_has(obj, I915_GEM_OBJECT_IS_SHRINKABLE) &&
> >>> + !obj->is_dpt;
> >>
> >> Is there a reason i915_gem_object_make_unshrinkable() cannot be used to
> >> mark the object at a suitable place?
> >
> > Do you have a suitable place in mind?
> > i915_gem_object_make_unshrinkable() contains some magic
> > ingredients so doesn't look like it can be called willy
> > nilly.
>
> After it is created in intel_dpt_create?
>
> I don't see that helper couldn't be called. It is called from madvise
> and tiling for instance without any apparent special considerations.
Did you actually read through i915_gem_object_make_unshrinkable()?
>
> Also, there is no mention of this angle in the commit message so I
> assumed it wasn't considered. If it was, then it should have been
> mentioned why hacky solution was chosen instead...
I suppose.
>
> > Anyways, looks like I forgot to reply that I already pushed this
> > with this extra comment added:
> > /* TODO: make DPT shrinkable when it has no bound vmas */
>
> ... becuase IMO the special case is quite ugly and out of place. :(
Yeah, not the nicest. But there's already a is_dpt check in the
i915_gem_object_is_framebuffer() right next door, so it's not
*that* out of place.
Another option maybe could be to manually clear
I915_GEM_OBJECT_IS_SHRINKABLE but I don't think that is
supposed to be mutable, so might also have other issues.
So a more proper solution with that approach would perhaps
need some kind of gem_create_shmem_unshrinkable() function.
>
> I don't remember from the top of my head how DPT magic works but if
> shrinker protection needs to be tied with VMAs there is also
> i915_make_make(un)shrinkable to try.
I presume you mistyped something there.
--
Ville Syrjälä
Intel
