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

[Chris Wilson]

Quoting Zbigniew Kempczyński (2019-11-22 14:40:09)
> On Fri, Nov 22, 2019 at 09:46:11AM +0000, Chris Wilson wrote:
> > Quoting Zbigniew Kempczyński (2019-11-22 09:33:07)
> > > > > +       /* 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?

Sure, the first patch can go in after we land mmap_offset and do a check
before breaking everything.

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

Just pick a limit that is reasonable for the quick queries, and leave
full queries to spill to heap. If we can't do a quick query, it would be
tempting to raise it as an API issue.

Or if there's no such possibility, ignore me; I'm used to being able to
do most things in 256B.

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

I have no complaints about the function name; it's the uAPI type name
that I think is less than ideal.
-Chris