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

[Chris Wilson]

Quoting Matthew Auld (2019-10-08 12:57:50)
> 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?

resource_size_t is definitely a better starting point than size_t, and
afaik is the preferred type for this physical usage, even when
dma_addr_t conflicts. But that just feels strange, and I couldn't find
any definitive guide. So writing down why we chose the type will help
the inevitable later discussions.
 
> On that topic should we also stop using "struct resource" here, since 
> that's starting to get confusing, maybe use u64 there also?

Well, I'm still looking at struct resource and thinking/hoping it may
lead to us reusing more shared lib/ routines. So I'm ambivalent here. :)

My core objective here is supporting [in thought experiments at least]
4+GiB local memory on 32b platforms. To pull it off would need a bit
more cleaning up (so that we never expose more than we have to) to stay
within the dma budget -- and that has interesting ramifications onto
iomem (we would need a mappable/non-mappable split again, or maybe the
HW will only allow what can be mapped???)
-Chris