[Intel-gfx] [PATCH] drm/i915: Hide unshrinkable context objects from the shrinker

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-19 16:38:28)
> 
> On 19/07/2019 14:04, Chris Wilson wrote:
> > +void i915_gem_object_make_unshrinkable(struct drm_i915_gem_object *obj)
> > +{
> > +     if (!list_empty(&obj->mm.link)) {
> 
> Which lock protects this list head?

Hmm, I was thinking this would be nicely ordered by the caller. But no,
not strongly protected against the shrinker...

> > +             struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > +             unsigned long flags;
> > +
> > +             spin_lock_irqsave(&i915->mm.obj_lock, flags);
> > +             GEM_BUG_ON(list_empty(&obj->mm.link));

..and so this should be a regular if() not BUG_ON.

> > +             list_del_init(&obj->mm.link);
> > +             i915->mm.shrink_count--;
> > +             i915->mm.shrink_memory -= obj->base.size;
> > +
> > +             spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
> > +     }
> > +}
> > +
> > +void i915_gem_object_make_shrinkable(struct drm_i915_gem_object *obj)
> > +{
> > +     GEM_BUG_ON(!i915_gem_object_has_pages(obj));
> > +     GEM_BUG_ON(!list_empty(&obj->mm.link));
> > +
> > +     if (i915_gem_object_is_shrinkable(obj)) {
> > +             struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > +             unsigned long flags;
> > +
> > +             spin_lock_irqsave(&i915->mm.obj_lock, flags);
> > +             GEM_BUG_ON(!kref_read(&obj->base.refcount));
> > +
> > +             list_add_tail(&obj->mm.link, &i915->mm.shrink_list);
> > +             i915->mm.shrink_count++;
> > +             i915->mm.shrink_memory += obj->base.size;
> > +
> > +             spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
> > +     }
> > +}
> > +
> > +void i915_gem_object_make_purgeable(struct drm_i915_gem_object *obj)
> > +{
> > +     GEM_BUG_ON(!i915_gem_object_has_pages(obj));
> > +     GEM_BUG_ON(!list_empty(&obj->mm.link));
> > +
> > +     if (i915_gem_object_is_shrinkable(obj)) {
> > +             struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > +             unsigned long flags;
> > +
> > +             spin_lock_irqsave(&i915->mm.obj_lock, flags);
> > +             GEM_BUG_ON(!kref_read(&obj->base.refcount));
> > +
> > +             list_add_tail(&obj->mm.link, &i915->mm.purge_list);
> > +             i915->mm.shrink_count++;
> > +             i915->mm.shrink_memory += obj->base.size;
> > +
> > +             spin_unlock_irqrestore(&i915->mm.obj_lock, flags);
> > +     }
> > +}
> 
> Common helper for the above to passing in the correct list from each?

It's also worth making that has_pages into a has_pinned_pages.

> > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> > index 9e4f51ce52ff..9830edda1ade 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> > @@ -118,7 +118,7 @@ static int __context_pin_state(struct i915_vma *vma)
> >        * And mark it as a globally pinned object to let the shrinker know
> >        * it cannot reclaim the object until we release it.
> >        */
> > -     vma->obj->pin_global++;
> > +     i915_vma_make_unshrinkable(vma);
> >       vma->obj->mm.dirty = true;
> >   
> >       return 0;
> > @@ -126,8 +126,8 @@ static int __context_pin_state(struct i915_vma *vma)
> >   
> >   static void __context_unpin_state(struct i915_vma *vma)
> >   {
> > -     vma->obj->pin_global--;
> >       __i915_vma_unpin(vma);
> > +     i915_vma_make_shrinkable(vma);
> >   }
> >   
> >   static void __intel_context_retire(struct i915_active *active)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index f7e69db4019d..5b16b233a059 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -231,6 +231,8 @@ int intel_gt_init_scratch(struct intel_gt *gt, unsigned int size)
> >       if (ret)
> >               goto err_unref;
> >   
> > +     i915_gem_object_make_unshrinkable(obj);
> > +
> >       gt->scratch = vma;
> >       return 0;
> >   
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> > index 38ec11ae6ed7..d8efb88f33f3 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> > @@ -1238,7 +1238,7 @@ int intel_ring_pin(struct intel_ring *ring)
> >               goto err_ring;
> >       }
> >   
> > -     vma->obj->pin_global++;
> > +     i915_vma_make_unshrinkable(vma);
> >   
> >       GEM_BUG_ON(ring->vaddr);
> >       ring->vaddr = addr;
> > @@ -1267,6 +1267,8 @@ void intel_ring_reset(struct intel_ring *ring, u32 tail)
> >   
> >   void intel_ring_unpin(struct intel_ring *ring)
> >   {
> > +     struct i915_vma *vma = ring->vma;
> > +
> >       if (!atomic_dec_and_test(&ring->pin_count))
> >               return;
> >   
> > @@ -1275,18 +1277,17 @@ void intel_ring_unpin(struct intel_ring *ring)
> >       /* Discard any unused bytes beyond that submitted to hw. */
> >       intel_ring_reset(ring, ring->tail);
> >   
> > -     GEM_BUG_ON(!ring->vma);
> > -     i915_vma_unset_ggtt_write(ring->vma);
> > -     if (i915_vma_is_map_and_fenceable(ring->vma))
> > -             i915_vma_unpin_iomap(ring->vma);
> > +     i915_vma_unset_ggtt_write(vma);
> > +     if (i915_vma_is_map_and_fenceable(vma))
> > +             i915_vma_unpin_iomap(vma);
> >       else
> > -             i915_gem_object_unpin_map(ring->vma->obj);
> > +             i915_gem_object_unpin_map(vma->obj);
> >   
> >       GEM_BUG_ON(!ring->vaddr);
> >       ring->vaddr = NULL;
> >   
> > -     ring->vma->obj->pin_global--;
> > -     i915_vma_unpin(ring->vma);
> > +     i915_vma_unpin(vma);
> > +     i915_vma_make_purgeable(vma);
> 
> Why is ring purgeable and scratch or context state shrinkable?

Because the ring contains nothing, but context state must be preserved.
I was thinking the explicit selection would be clearer.

scratch will be thrown away at the end of the driver. I don't see an
instance where we make scratch shrinkable (except for a brief period it
is an internal object but is pinned).

> > +void i915_vma_make_unshrinkable(struct i915_vma *vma)
> > +{
> > +     i915_gem_object_make_unshrinkable(vma->obj);
> > +}
> > +
> > +void i915_vma_make_shrinkable(struct i915_vma *vma)
> > +{
> > +     i915_gem_object_make_shrinkable(vma->obj);
> > +}
> > +
> > +void i915_vma_make_purgeable(struct i915_vma *vma)
> > +{
> > +     i915_gem_object_make_purgeable(vma->obj);
> > +}
> 
> Would i915_vma_make_*object*_... be a better name? I am thinking the 
> concept does not apply to vma's.

I'm planning ahead for a common backing store shared between objects and
vma, where vma doesn't operate on the object per-se, and we have a
first-class reference counted vma.
-Chris