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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
