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

[Matthew Auld]

On 08/10/2019 09:59, Chris Wilson wrote:
> 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.

No good reason, I guess it just stems from using "struct resource 
region". Maybe for the object_create interface we should just use u64?

On that topic should we also stop using "struct resource" here, since 
that's starting to get confusing, maybe use u64 there also?

> -Chris
>