[igt-dev] [PATCH i-g-t v5 3/4] lib/i915/intel_memory_region: Add lib to manage memory regions
Chris Wilson
chris at chris-wilson.co.uk
Fri Nov 22 09:11:46 UTC 2019
Quoting Zbigniew Kempczyński (2019-11-22 08:13:56)
> From: Lukasz Kalamarz <lukasz.kalamarz at 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 at intel.com>
> Signed-off-by: Zbigniew Kempczyński <lukasz.kalamarz at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Vanshidhar Konda <vanshidhar.r.konda at 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
More information about the igt-dev
mailing list