Re: [PATCH] drm/i915/dpt: Make DPT object unshrinkable

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 23, 2024 at 02:14:56PM +0100, Tvrtko Ursulin wrote:
> 
> On 23/05/2024 13:24, Ville Syrjälä wrote:
> > 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@linux.intel.com>
> >>>>> Cc: stable@vger.kernel.org
> >>>>> Fixes: 0dc987b699ce ("drm/i915/display: Add smem fallback allocation for dpt")
> >>>>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> >>>>> ---
> >>>>>     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()?
> 
> Briefly, and also looked around how it is used. I don't immediately 
> understand which part concerns you and it is also quite possible I am 
> missing something.

The shrink_pin magic says you can't use this willy nilly.

> 
> But see for example how it is used in intel_context.c+intel_lrc.c to 
> protect the context state object from the shrinker while it is in use by 
> the GPU. It does not appear any black magic is required.
> 
> Question also is does that kind of lifetime aligns with the DPT use case.
> 
> >> 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.
> 
> I also see who added that one! ;)
> 
> > 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.
> 
> Oops - i915_vma_make_(un)shrinkable.

That just calls the obj version of the function.

> 
> Anyway, I think it is worth giving it a try if the DPT lifetimes makes 
> it possible.
> 
> Regards,
> 
> Tvrtko

-- 
Ville Syrjälä
Intel