[Intel-gfx] [RFC PATCH 04/42] drm/i915: introduce intel_memory_region

[Chris Wilson]

Quoting Matthew Auld (2019-02-26 14:03:10)
> On Thu, 14 Feb 2019 at 15:16, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >
> > Quoting Matthew Auld (2019-02-14 14:57:02)
> > > +int
> > > +i915_memory_region_get_pages_buddy(struct drm_i915_gem_object *obj)
> > > +{
> > > +       struct intel_memory_region *mem = obj->memory_region;
> > > +       resource_size_t size = obj->base.size;
> > > +       struct sg_table *st;
> > > +       struct scatterlist *sg;
> > > +       unsigned int sg_page_sizes;
> > > +       unsigned long n_pages;
> > > +
> > > +       GEM_BUG_ON(!IS_ALIGNED(size, mem->mm.min_size));
> > > +       GEM_BUG_ON(!list_empty(&obj->blocks));
> > > +
> > > +       st = kmalloc(sizeof(*st), GFP_KERNEL);
> > > +       if (!st)
> > > +               return -ENOMEM;
> > > +
> > > +       n_pages = div64_u64(size, mem->mm.min_size);
> >
> > min_size is a power of two.
> >
> > n_pages = size >> ilog2(min_size);
> >
> > would suffice. Keeping min_order would come in handy later.
> >
> > > +
> > > +       if (sg_alloc_table(st, n_pages, GFP_KERNEL)) {
> > > +               kfree(st);
> > > +               return -ENOMEM;
> > > +       }
> > > +
> > > +       sg = st->sgl;
> > > +       st->nents = 0;
> > > +       sg_page_sizes = 0;
> > > +
> > > +       mutex_lock(&mem->mm_lock);
> > > +
> > > +       do {
> > > +               struct i915_gem_buddy_block *block;
> > > +               unsigned int order;
> > > +               u64 block_size;
> > > +               u64 offset;
> > > +
> > > +               order = fls(n_pages) - 1;
> > > +               GEM_BUG_ON(order > mem->mm.max_order);
> > > +
> > > +               do {
> > > +                       block = i915_gem_buddy_alloc(&mem->mm, order);
> > > +                       if (!IS_ERR(block))
> > > +                               break;
> > > +
> > > +                       /* XXX: some kind of eviction pass, local to the device */
> > > +                       if (!order--)
> > > +                               goto err_free_blocks;
> > > +               } while (1);
> > > +
> > > +               n_pages -= 1 << order;
> >
> > BIT(order) so you don't have sign extension fun. Need to fix
> > i915_gem_internal.c!
> >
> > > +struct intel_memory_region_ops {
> > > +       unsigned int flags;
> > > +
> > > +       int (*init)(struct intel_memory_region *);
> > > +       void (*release)(struct intel_memory_region *);
> > > +
> > > +       struct drm_i915_gem_object *
> > > +       (*object_create)(struct intel_memory_region *,
> > > +                        resource_size_t,
> > > +                        unsigned int);
> >
> > create_object()
> >
> > ops is acting as a factory here; and we are not operating on the object
> > itself.
> >
> > > +static int igt_mock_fill(void *arg)
> > > +{
> > > +       struct intel_memory_region *mem = arg;
> > > +       resource_size_t total = resource_size(&mem->region);
> > > +       resource_size_t page_size;
> > > +       resource_size_t rem;
> > > +       unsigned long max_pages;
> > > +       unsigned long page_num;
> > > +       LIST_HEAD(objects);
> > > +       int err = 0;
> > > +
> > > +       page_size = mem->mm.min_size;
> > > +       max_pages = total / page_size;
> >
> > Hmm, 32b? Can resource_size_t be 64b on a 32b system?
> >
> > It must be to accommodate PAE.
> >
> > > +static struct drm_i915_gem_object *
> > > +mock_object_create(struct intel_memory_region *mem,
> > > +                  resource_size_t size,
> > > +                  unsigned int flags)
> > > +{
> > > +       struct drm_i915_private *i915 = mem->i915;
> > > +       struct drm_i915_gem_object *obj;
> > > +
> > > +       if (size > BIT(mem->mm.max_order) * mem->mm.min_size)
> >
> > A mix of 64b and 32b types.
> >
> > if (size >> mem->mm.max_order + mem->mm.min_order)
> >
> > > +               return ERR_PTR(-E2BIG);
> >
> > GEM_BUG_ON(overflows_type(size, obj->base.size);
> >
> > > +       obj = i915_gem_object_alloc(i915);
> > > +       if (!obj)
> > > +               return ERR_PTR(-ENOMEM);
> > > +
> > > +       drm_gem_private_object_init(&i915->drm, &obj->base, size);
> > > +       i915_gem_object_init(obj, &mock_region_obj_ops);
> > > +
> > > +       obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
> > > +       obj->cache_level = HAS_LLC(i915) ? I915_CACHE_LLC : I915_CACHE_NONE;
> >
> > i915_gem_object_set_cache_coherency() ?
> 
> We do that as part of object_create_region(), but maybe that's not so hot?

I think tidier done together; and maybe you'll feel inspired to clean up
it a bit more as well.

> Also I just noticed that get_pages_buddy() returns -ENOSPC if we can't
> satisfy the request, for consistency that should probably be -ENOMEM?

Yes. ENOSPC returned to userspace currently means "batch could not fit
into the GTT after all constraints applied", and since this errno can
propagate to execbuf, we should strive for uniqueness. In this case not
being able to find memory on the device... ENOMEM. Although -ENOXMEM
would be better to indicate we ran out of memory on the device. But that
doesn't exist, maybe -ENOBUFS?

If we restrict ourselves to errno-base.h (and I think we should try to);
ENOMEM or ENXIO.
-Chris