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 08:13:56)
> From: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> 
> LMEM series introduced concept of memory_regions. This patch implement
> helper functions that allow user to manage them in more convenient way.
> 
> v2: when driver doesn't support i915_query supports memory region
>     switch to fallback and probe system and device memory regions.
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> Signed-off-by: Zbigniew Kempczyński <lukasz.kalamarz@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Vanshidhar Konda <vanshidhar.r.konda@intel.com>
> ---
>  lib/Makefile.sources           |   2 +
>  lib/i915/intel_memory_region.c | 369 +++++++++++++++++++++++++++++++++
>  lib/i915/intel_memory_region.h |  88 ++++++++
>  lib/ioctl_wrappers.h           |   1 +
>  lib/meson.build                |   1 +
>  5 files changed, 461 insertions(+)
>  create mode 100644 lib/i915/intel_memory_region.c
>  create mode 100644 lib/i915/intel_memory_region.h
> 
> diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> index 9d1a4e06..87b9f146 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -17,6 +17,8 @@ lib_source_list =             \
>         i915/gem_mman.h \
>         i915/gem_vm.c   \
>         i915/gem_vm.h   \
> +       i915/intel_memory_region.c      \
> +       i915/intel_memory_region.h      \
>         i915_3d.h               \
>         i915_reg.h              \
>         i915_pciids.h           \
> diff --git a/lib/i915/intel_memory_region.c b/lib/i915/intel_memory_region.c
> new file mode 100644
> index 00000000..56f6c527
> --- /dev/null
> +++ b/lib/i915/intel_memory_region.c
> @@ -0,0 +1,369 @@
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include <signal.h>
> +#include <sys/ioctl.h>
> +#include <sys/time.h>
> +#include <stdarg.h>
> +#include <alloca.h>
> +
> +#include "intel_reg.h"
> +#include "drmtest.h"
> +#include "ioctl_wrappers.h"
> +#include "igt_dummyload.h"
> +#include "igt_gt.h"
> +#include "intel_chipset.h"
> +
> +#include "i915/intel_memory_region.h"
> +
> +#define SZ_4K (4096)
> +#define SZ_64K (65536)
> +
> +static int __i915_query(int fd, struct drm_i915_query *q)
> +{
> +       if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
> +               return -errno;
> +
> +       return 0;
> +}
> +
> +static int __i915_query_item(int fd, struct drm_i915_query_item *item)
> +{
> +       struct drm_i915_query q = {
> +               .num_items = 1,
> +               .items_ptr = to_user_pointer(item),
> +       };
> +
> +       return __i915_query(fd, &q);
> +}
> +
> +bool gem_has_query_support(int fd)
> +{
> +       struct drm_i915_query query = {};
> +
> +       return __i915_query(fd, &query) == 0;
> +}
> +
> +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.

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

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

> +
> +       gem_close(fd, handle);
> +
> +       return !ret;
> +}
> +
> +static struct local_i915_query_memory_region_info *__probe_regions(int fd)
> +{
> +       struct local_i915_query_memory_region_info *query_info;
> +       uint32_t length = sizeof(*query_info);
> +       uint32_t *regions = NULL;
> +       int max_regions = INTEL_MEMORY_TYPE_SHIFT;
> +       int num_regions = 0;
> +
> +       for (int i = 0; intel_memory_regions[i].region_name; i++) {
> +               uint8_t type;
> +
> +               /* We're not interested of other memory region types */
> +               type = intel_memory_regions[i].memory_type;
> +               if (type != LOCAL_I915_SYSTEM_MEMORY &&
> +                   type != LOCAL_I915_DEVICE_MEMORY)
> +                       continue;
> +
> +               for (int instance = 0; instance <= max_regions; instance++) {   
> +                       uint32_t region;
> +                       int exists;
> +
> +                       region = INTEL_MEMORY_REGION_ID(type, instance);
> +                       igt_debug("Probing memory region: %08x\n", region);
> +                       exists = __try_gem_create_in_region(fd, region);
> +                       if (!exists)
> +                               break;
> +
> +                       igt_debug("Region %08x exists\n", region);
> +                       regions = realloc(regions, (num_regions + 1) *
> +                                         sizeof(uint32_t));
> +                       igt_assert(regions);
> +                       regions[num_regions++] = region;
> +               }
> +       }
> +
> +       if (!num_regions)
> +               return NULL;
> +
> +       length += sizeof(struct local_i915_memory_region_info) * num_regions;
> +       query_info = calloc(1, length);
> +       igt_assert(query_info);
> +
> +       /* Prepare query_info structure with regions id filled */
> +       for (int i = 0; i < num_regions; i++) {
> +               query_info->regions[i].id = regions[i];
> +               query_info->regions[i].size = 0;
> +       }
> +       free(regions);
> +       query_info->num_regions = num_regions;
> +
> +       return query_info;
> +}
> +
> +/**
> + * gem_query_memory_regions:
> + * @fd: open i915 drm file descriptor
> + *
> + * This function wraps query mechanism for memory regions. If there's
> + * no i915_query probe memory regions and return same structure as from query.
> + *
> + * Returns: Filled struct with available memory regions. Allocates structure
> + * which must by freed by caller.
> + */
> +struct local_i915_query_memory_region_info *gem_query_memory_regions(int fd)
> +{
> +       struct drm_i915_query_item item;
> +       struct local_i915_query_memory_region_info *query_info;
> +       int ret;
> +
> +       memset(&item, 0, sizeof(item));
> +       item.query_id = LOCAL_I915_QUERY_MEMREGION_INFO;
> +
> +       ret = __i915_query_item(fd, &item);
> +       if (!ret) {
> +               query_info = calloc(1, item.length);
> +               igt_assert(query_info);
> +
> +               item.data_ptr = to_user_pointer(query_info);
> +               __i915_query_item(fd, &item);
> +
> +               return query_info;
> +       }
> +
> +       /* 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.

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

> +       return lmem_regions;
> +}
> +
> +/**
> + * gem_has_lmem:
> + * @fd: open i915 drm file descriptor
> + *
> + * Helper function to check if lmem is available on device.
> + *
> + * Returns: True if at least one lmem region was found.
> + */
> +bool gem_has_lmem(int fd)
> +{
> +       return gem_get_lmem_region_count(fd) > 0;

That seems even more of a waste of heap, as above.

> +}
> +
> +/**
> + * gem_query_has_memory_region:
> + * @query_info: query result of memory regions
> + * @region: region existance to check inside @query_info regions
> + *
> + * This function check existence of region in @query_info
> + *
> + * Returns: true if memory region was found. Otherwise false.
> + */
> +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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev