Re: [RFC PATCH 04/42] drm/i915: introduce intel_memory_region

Chris Wilson
2019-02-14T15:16:51Z

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() ?
-Chris