On Thu, 14 Feb 2019 at 15:16, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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?
Also I just noticed that get_pages_buddy() returns -ENOSPC if we can't
satisfy the request, for consistency that should probably be -ENOMEM?
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
