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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
