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

[Chris Wilson <chris@chris-wilson.co.uk>]

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.

> > > +       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.

> > > +       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.

> > > +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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev