[Intel-gfx] [PATCH v3 02/21] drm/i915: introduce intel_memory_region

[Chris Wilson]

Quoting Matthew Auld (2019-10-04 18:04:33)
> +int
> +i915_gem_object_get_pages_buddy(struct drm_i915_gem_object *obj)
> +{
> +       struct intel_memory_region *mem = obj->mm.region;
> +       struct list_head *blocks = &obj->mm.blocks;
> +       unsigned int flags = I915_ALLOC_MIN_PAGE_SIZE;
> +       resource_size_t size = obj->base.size;
> +       resource_size_t prev_end;
> +       struct i915_buddy_block *block;
> +       struct sg_table *st;
> +       struct scatterlist *sg;
> +       unsigned int sg_page_sizes;
> +       unsigned long i;
> +       int ret;
> +
> +       st = kmalloc(sizeof(*st), GFP_KERNEL);
> +       if (!st)
> +               return -ENOMEM;
> +
> +       if (sg_alloc_table(st, size >> ilog2(mem->mm.chunk_size), GFP_KERNEL)) {
> +               kfree(st);
> +               return -ENOMEM;
> +       }
> +
> +       ret = __intel_memory_region_get_pages_buddy(mem, size, flags, blocks);
> +       if (ret)
> +               goto err_free_sg;
> +
> +       GEM_BUG_ON(list_empty(blocks));
> +
> +       sg = st->sgl;
> +       st->nents = 0;
> +       sg_page_sizes = 0;
> +       i = 0;
> +
> +       list_for_each_entry(block, blocks, link) {
> +               u64 block_size, offset;
> +
> +               block_size = i915_buddy_block_size(&mem->mm, block);
> +               offset = i915_buddy_block_offset(block);
> +
> +               GEM_BUG_ON(overflows_type(block_size, sg->length));
> +
> +               if (!i || offset != prev_end ||
> +                   add_overflows_t(typeof(sg->length), sg->length, block_size)) {
> +                       if (i) {

i is only being here to detect the start.
prev_end = (resource_size_t)-1;
Then I don't have to worry about i overflowing, although that seems
harmless, and as ridiculous as that may be.

> +                               sg_page_sizes |= sg->length;
> +                               sg = __sg_next(sg);
> +                       }
> +
> +                       sg_dma_address(sg) = mem->region.start + offset;
> +                       sg_dma_len(sg) = block_size;
> +
> +                       sg->length = block_size;
> +
> +                       st->nents++;
> +               } else {
> +                       sg->length += block_size;
> +                       sg_dma_len(sg) += block_size;
> +               }
> +
> +               prev_end = offset + block_size;
> +               i++;
> +       };
> +
> +       sg_page_sizes |= sg->length;
> +       sg_mark_end(sg);
> +       i915_sg_trim(st);
> +
> +       __i915_gem_object_set_pages(obj, st, sg_page_sizes);
> +
> +       return 0;
> +
> +err_free_sg:
> +       sg_free_table(st);
> +       kfree(st);
> +       return ret;
> +}

> +struct drm_i915_gem_object *
> +i915_gem_object_create_region(struct intel_memory_region *mem,
> +                             resource_size_t size,
> +                             unsigned int flags)
> +{
> +       struct drm_i915_gem_object *obj;
> +
> +       if (!mem)
> +               return ERR_PTR(-ENODEV);
> +
> +       size = round_up(size, mem->min_page_size);
> +
> +       GEM_BUG_ON(!size);
> +       GEM_BUG_ON(!IS_ALIGNED(size, I915_GTT_MIN_ALIGNMENT));
> +

Put the FIXME here from the start

> +       if (size >> PAGE_SHIFT > INT_MAX)
> +               return ERR_PTR(-E2BIG);
> +
> +       if (overflows_type(size, obj->base.size))
> +               return ERR_PTR(-E2BIG);
> +
> +       return mem->ops->create_object(mem, size, flags);
> +}

I'm happy with this. But I would like a discussion on why
resource_size_t was chosen and for that rationale to be captured in the
code comments. At the end of the day, we need to bump obj->base.size to
suit.
-Chris