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

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

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()?

> 
> 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