Re: [PATCH v3 03/37] drm/i915/region: support basic eviction

Tang, CQ
2019-08-11T05:59:44Z

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Chris Wilson
> Sent: Saturday, August 10, 2019 3:19 AM
> To: Auld, Matthew <matthew.auld@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v3 03/37] drm/i915/region: support basic
> eviction
>
> Quoting Matthew Auld (2019-08-09 23:26:09)
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 6ff01a404346..8735dea74809
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1105,6 +1105,23 @@ i915_gem_madvise_ioctl(struct drm_device
> *dev, void *data,
> > !i915_gem_object_has_pages(obj))
> > i915_gem_object_truncate(obj);
> >
> > + if (obj->mm.region) {
> > + mutex_lock(&obj->mm.region->obj_lock);
> > +
> > + switch (obj->mm.madv) {
> > + case I915_MADV_WILLNEED:
> > + list_move(&obj->mm.region_link,
> > + &obj->mm.region->objects);
> > + break;
> > + default:
> > + list_move(&obj->mm.region_link,
> > + &obj->mm.region->purgeable);
> > + break;
> > + }
> > +
> > + mutex_unlock(&obj->mm.region->obj_lock);
> > + }
> > +
> > args->retained = obj->mm.madv != __I915_MADV_PURGED;
>
> Little bit of an impedance mismatch, I hope this turns out fine when
> everything is a memory region.
>
> > mutex_unlock(&obj->mm.lock);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_memory_region.c
> > b/drivers/gpu/drm/i915/intel_memory_region.c
> > index ef12e462acb8..3a3caaadea1f 100644
> > --- a/drivers/gpu/drm/i915/intel_memory_region.c
> > +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> > @@ -12,6 +12,51 @@ const u32 intel_region_map[] = {
> > [INTEL_MEMORY_STOLEN] = BIT(INTEL_STOLEN +
> > INTEL_MEMORY_TYPE_SHIFT) | BIT(0), };
> >
> > +static int
> > +intel_memory_region_evict(struct intel_memory_region *mem,
> > + resource_size_t target,
> > + unsigned int flags) {
> > + struct drm_i915_gem_object *obj;
> > + resource_size_t found;
> > + int err;
> > +
> > + err = 0;
> > + found = 0;
> > +
> > + mutex_lock(&mem->obj_lock);
> > + list_for_each_entry(obj, &mem->purgeable, mm.region_link) {
> > + if (!i915_gem_object_has_pages(obj))
> > + continue;
> > +
> > + if (READ_ONCE(obj->pin_global))
> > + continue;
> > +
> > + if (atomic_read(&obj->bind_count))
> > + continue;
> > +
> > + mutex_unlock(&mem->obj_lock);
> > +
> > + __i915_gem_object_put_pages(obj, I915_MM_SHRINKER);
>
> So we don't really care about the object being bound then? As all we care
> about is the page's pin_count.
>
> So instead of obj->pin_global, obj->bind_bound, you just want
>
> if (atomic_read(&obj->pages.pin_count))
> continue;
>
> as the quick check to see if it is worth preceding.
>
> > + mutex_lock_nested(&obj->mm.lock, I915_MM_SHRINKER);
> > + if (!i915_gem_object_has_pages(obj)) {
> > + obj->mm.madv = __I915_MADV_PURGED;
> > + found += obj->base.size;
> > + }
> > + mutex_unlock(&obj->mm.lock);
>
> The locking here accomplishes what? You just want a boolean from
> put_pages().

I have the same question. But looked the i915_gem_shrink() function, it has similar code. Do we prevent any race condition here?
I want to use this function for swapping so hope to understand more.

--CQ

>
> > +
> > + if (found >= target)
> > + return 0;
> > +
> > + mutex_lock(&mem->obj_lock);
> > + }
> > +
> > + err = -ENOSPC;
> > + mutex_unlock(&mem->obj_lock);
> > + return err;
> > +}
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org