[igt-dev] [PATCH i-g-t v5 3/4] lib/i915/intel_memory_region: Add lib to manage memory regions

[Zbigniew Kempczyński]

On Fri, Nov 22, 2019 at 09:46:11AM +0000, Chris Wilson wrote:
> Quoting Zbigniew Kempczyński (2019-11-22 09:33:07)
> > On Fri, Nov 22, 2019 at 09:11:46AM +0000, Chris Wilson wrote:
> > 
> > > > +const struct intel_memory_region intel_memory_regions[] = {
> > > > +       { "SMEM", LOCAL_I915_SYSTEM_MEMORY, 0 },
> > > 
> > > I love how the friendly names are so terse and do not follow the api /s
> > > 
> > > > +       { "LMEM", LOCAL_I915_DEVICE_MEMORY, 0 },
> > > > +       { "STLN", LOCAL_I915_STOLEN_MEMORY, 0 },
> > > > +       { NULL, 0, 0 },
> > > > +};
> > > > +
> > > > +const char *memory_region_name(uint32_t region) {
> > > > +       uint32_t type;
> > > > +
> > > > +       type = MEMORY_TYPE_FROM_REGION(region);
> > > 
> > > We will never end up with garbage here, but we are so polite in our
> > > testing.
> > 
> > If you have better idea I'll fix that.
> 
> All I mean here is that type < ARRAY_SIZE(intel_memory_regions) else
> report "unknown" or something.

You're right. Argument checking should be definitely added here.

> 
> > > > +       return intel_memory_regions[type].region_name;
> > > > +}
> > > > +/**
> > > > + *  gem_get_batch_size:
> > > > + *  @fd: open i915 drm file descriptor
> > > > + *  @region: region in which we want to create a batch
> > > > + *
> > > > + *  FIXME: Currently function assumes we have 64K on DEVICE and 4K
> > > > + *  on SYSTEM memory. To be fixed after memory region page size detection
> > > > + *  patch will be merged.
> > > > + */
> > > > +uint32_t gem_get_batch_size(int fd, uint32_t region)
> > > > +{
> > > > +       return IS_DEVICE_MEMORY_REGION(region) ? SZ_64K : SZ_4K;
> > > > +}
> > > > +
> > > > +static bool __try_gem_create_in_region(int fd, uint32_t region)
> > > > +{
> > > > +       uint32_t size, handle;
> > > > +       int ret;
> > > > +
> > > > +       size = gem_get_batch_size(fd, region);
> > > 
> > > size=1 will always be correct, that has now been enshrined in the ABI.
> > 
> > Should I change it or region "page size" is acceptable? 
> 
> I don't mind; just suggesting it was redundant. So not even worth the
> rant :)
> 
> > > 
> > > > +       handle = gem_create(fd, size);
> > > > +       ret = gem_migrate_to_memory_region(fd, handle, region);
> > > 
> > > Migrate is definitely not supported in the first wave of landings;
> > > memory placement will be a construction time property with possible
> > > later extension to runtime.
> > 
> > If I would have gem_create_ext() I would definitely use it. Unfortunately
> > only mechanism I know at the moment to place BO in memory region is 
> > to migrate it. Is anything I missed? Will we have gem_create_ext() with
> > memory region as an argument?
> 
> I mean that this needs to be raised as an issue in the kernel
> implementation as I have not seen a single suggestion for handling
> gem_create with multiple memory regions since about 5 years ago.
> And yet it is absolutely crucial...
> 
> > > > +       /* Fallback, there's no memregion i915_query supported. Try probe(). */
> > > > +       query_info = __probe_regions(fd);
> > > 
> > > Oh, we will not have memory regions without a query. If anything we will
> > > get memory regions described before you can make a choice.
> > 
> > Unfortunately after your comment on previous series I concluded after first
> > memory region patches landing we would support them without query (current 
> > patches doesn't support them, thus this fallback).
> 
> No, I mean we are landing GEM_MMAP_OFFSET_IOCTL shortly before we have
> memory regions, as that ioctl simply extends existing API and can be
> used currently.
> 
> The query and memory regions are part of the same API bundle that comes
> later

That means that this patch series should be splitted, gem_mman first
(you've already added R-B there). Will you merge this patch (gem_mman)
or should I send it as separate with R-B?


> > > > +       igt_assert(query_info);
> > > > +
> > > > +       return query_info;
> > > > +}
> > > > +
> > > > +/**
> > > > + * gem_get_lmem_region_count:
> > > > + * @fd: open i915 drm file descriptor
> > > > + *
> > > > + * Helper function to check how many lmem regions are available on device.
> > > > + *
> > > > + * Returns: Number of found lmem regions.
> > > > + */
> > > > +uint8_t gem_get_lmem_region_count(int fd)
> > > > +{
> > > > +       struct local_i915_query_memory_region_info *query_info;
> > > > +       uint8_t num_regions;
> > > > +       uint8_t lmem_regions = 0;
> > > > +
> > > > +       query_info = gem_query_memory_regions(fd);
> > > > +       num_regions = query_info->num_regions;
> > > > +
> > > > +       for (int i = 0; i < num_regions; i++) {
> > > > +               if (IS_DEVICE_MEMORY_REGION(query_info->regions[i].id))
> > > > +                       lmem_regions += 1;
> > > > +       }
> > > > +       free(query_info);
> > > 
> > > Meh. Didn't need the heap.
> > 
> > You're suggesting doing allocation in lmem regions until it fails?
> > A lot of ioctl() will be called in this case.
> 
> Nope. Look at the ioctl interface again; for most of the trivial
> alloc+free patterns you can supply a stack buffer and do a single ioctl
> with no allocations.

To be sure I will support allocation on-stack (via alloca for example
or variable length array C99 extention or hardcoded size surely enough)
I should multiply possible number of memory regions (currently 16) by
number or instances (using current split also 16) by sizeof(*..region_info)
+ sizeof(...query_info). From my calculation it gives 16B + 14336B so this
require around 16KB on the stack. Have I missed something?

> 
> > > > +bool gem_query_has_memory_region(struct local_i915_query_memory_region_info *query_info,
> > > 
> > > Remind me to have words with the naming committee.
> > > 
> > > The rest is for an interface that has yet to be seen and discussed, so
> > > shelving for later.
> > > -Chris
> > 
> > To be ownest I don't know what's wrong with this function name. 
> > If you have better candidate you're welcome.
> 
> It's the uAPI type name that has gone a little overboard with its Hungarian
> inheritance. I'm just making a note to complain to its author.
> -Chris

I don't know what Hungarian inheritance means. I know why I likely left
this function - previously interface allows having different memory region 
query views depending on fd_setparam settings. I'll remove it.

Zbigniew