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

Vanshidhar Konda
2019-11-19T18:03:34Z

On Tue, Nov 19, 2019 at 05:02:21PM +0100, Zbigniew Kempczyński wrote:
>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.
>
>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: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>---
> lib/Makefile.sources | 2 +
> lib/i915/intel_memory_region.c | 295 +++++++++++++++++++++++++++++++++
> lib/i915/intel_memory_region.h | 139 ++++++++++++++++
> lib/ioctl_wrappers.h | 1 +
> lib/meson.build | 1 +
> 5 files changed, 438 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 6333923e..de2ad73a 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..3768214f
>--- /dev/null
>+++ b/lib/i915/intel_memory_region.c
>@@ -0,0 +1,295 @@
>+/*
>+ * 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)
>+
>+#define i915_query_items(fd, items, n_items) do { \
>+ igt_assert_eq(__i915_query_items(fd, items, n_items), 0); \
>+ errno = 0; \
>+ } while (0)
>+#define i915_query_items_err(fd, items, n_items, err) do { \
>+ igt_assert_eq(__i915_query_items(fd, items, n_items), -err); \
>+ } while (0)
>+
>+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_items(int fd, struct drm_i915_query_item *items, uint32_t n_items)
>+{
>+ struct drm_i915_query q = {
>+ .num_items = n_items,
>+ .items_ptr = to_user_pointer(items),
>+ };
>+ 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 },
>+ { "LMEM", LOCAL_I915_DEVICE_MEMORY, 0 },
>+ { NULL, 0, 0}
>+};
>+
>+/**
>+ * 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. I know Chris is going to kill me for that
>+ * but I'll fix this when patch with memory region page size detection
>+ * will be merged.
>+ */
>+uint32_t gem_get_batch_size(int fd, uint32_t region)
>+{
>+ return IS_DEVICE_MEMORY_REGION(region) ? SZ_64K : SZ_4K;
>+}
>+
>+/**
>+ * gem_get_query_memory_regions:
>+ * @fd: open i915 drm file descriptor
>+ *
>+ * This function wraps query mechanism for memory regions.
>+ *
>+ * Returns: Filled struct with available memory regions.
>+ */
>+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;
>+
>+ memset(&item, 0, sizeof(item));
>+ item.query_id = LOCAL_I915_QUERY_MEMREGION_INFO;
>+ i915_query_items(fd, &item, 1);
>+
>+ query_info = calloc(1, item.length);
>+
>+ item.data_ptr = to_user_pointer(query_info);
>+ i915_query_items(fd, &item, 1);
>+
>+ 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);
>+
>+ 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;
>+}
>+
>+/**
>+ * 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,
>+ uint32_t region)
>+{
>+ for (int i = 0; i < query_info->num_regions; i++)
>+ if (query_info->regions[i].id == region)
>+ return true;
>+
>+ return false;
>+}
>+
>+/**
>+ * gem_query_require_region:
>+ * @query_info: query result of memory regions
>+ * @region: region to check inside query
>+ *
>+ * Function lead to skipping test if @region doesn't exists in @query_info.
>+ */
>+void gem_query_require_region(struct local_i915_query_memory_region_info *query_info,
>+ uint32_t region)

Fix formatting for second line

>+{
>+ igt_require(gem_query_has_memory_region(query_info, region));
>+}
>+
>+/**
>+ * __gem_migrate_to_memory_regions:
>+ * @fd: open i915 drm file descriptor
>+ * @handle: buffer object handle
>+ * @mem_regions: memory regions id array
>+ * @size: memory regions array size
>+ *
>+ * Wrapper function on IOCTL_I915_GEM_OBJECT_SETPARAM. It sets object to be
>+ * migrated into one of memory region specified in the array. Array contains
>+ * memory regions in requested priority order - if no migration to first
>+ * memory region is possible next one is selected and so on.
>+ *
>+ * Returns: errno
>+ */
>+static int __gem_migrate_to_memory_regions(int fd, int handle,
>+ uint32_t *mem_regions,
>+ uint32_t size)
>+{
>+ struct local_i915_gem_object_param obj;
>+ int err = 0;
>+
>+ memset(&obj, 0, sizeof(obj));
>+ obj.handle = handle;
>+ obj.size = size;
>+ obj.param = I915_PARAM_MEMORY_REGION | I915_OBJECT_PARAM;
>+ obj.data = to_user_pointer(mem_regions);
>+
>+ if (igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_OBJECT_SETPARAM, &obj)) {
>+ err = -errno;
>+ errno = 0;
>+ }
>+
>+ return err;
>+}
>+
>+/**
>+ * gem_migrate_to_memory_region:
>+ * @fd: open i915 drm file descriptor
>+ * @handle: handle to GEM bo
>+ * @type: memory region type
>+ * @instance: memory region instance
>+ *
>+ * This function wraps GEM_OBJECT_SETPARAM into user friendly version. Object
>+ * which user pass @handle will be migrated to memory region, specified
>+ * by @type and @instance.
>+ *
>+ * Returns: errno if error occurred.
>+ */
>+int gem_migrate_to_memory_region(int fd, int handle, uint32_t region)
>+{
>+ return __gem_migrate_to_memory_regions(fd, handle, &region, 1);
>+}
>+
>+/**
>+ * gem_migrate_to_lmem:
>+ * @fd: open i915 drm file descriptor
>+ * @handle: handle to object that user want to migrate to LMEM
>+ *
>+ * This function wraps GEM_OBJECT_SETPARAM into user friendly version. Object
>+ * which user pass @handle will be migrated to LMEM.
>+ *
>+ * Returns: errno if error occurred.
>+ */
>+int gem_migrate_to_lmem(int fd, int handle)
>+{
>+ return gem_migrate_to_memory_region(fd, handle, REGION_DEVICE_MEMORY(0));
>+}
>+
>+/**
>+ * gem_migrate_to_smem:
>+ * @fd: open i915 drm file descriptor
>+ * @hadle: handle to object that user want to migrate to SMEM
>+ *
>+ * This function wraps GEM_OBJECT_SETPARAM into user friendly version. Object
>+ * onto which user pass @handle will be migrated to SMEM.
>+ *
>+ * Returns: errno if error occurred.
>+ */
>+int gem_migrate_to_smem(int fd, int handle)
>+{
>+ return gem_migrate_to_memory_region(fd, handle, REGION_SYSTEM_MEMORY(0));
>+}
>+
>+/* gem_create_in_memory_region_list:
>+ * @fd: opened i915 drm file descriptor
>+ * @size: requested size of the buffer
>+ * @mem_regions: memory regions array (priority list)
>+ * @num_regions: @mem_regions length
>+ */
>+uint32_t gem_create_in_memory_region_list(int fd, uint64_t size,
>+ uint32_t *mem_regions,
>+ int num_regions)
>+{
>+ uint32_t handle = gem_create(fd, size);
>+
>+ if (gem_has_lmem(fd)) {

Why have a check for LMEM only? Is it not possible to have multiple
SMEM regions? Or that the object is always created in SMEM?

Other than these two comments, looks good to me.

Acked-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com>

>+ int ret = __gem_migrate_to_memory_regions(fd, handle,
>+ mem_regions,
>+ num_regions);
>+ igt_assert_eq(ret, 0);
>+ }
>+
>+ return handle;
>+}
>diff --git a/lib/i915/intel_memory_region.h b/lib/i915/intel_memory_region.h
>new file mode 100644
>index 00000000..68fd13a9
>--- /dev/null
>+++ b/lib/i915/intel_memory_region.h
>@@ -0,0 +1,139 @@
>+/*
>+ * 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 <stdint.h>
>+#include "benchmarks/ilog2.h"
>+#include "i915_drm.h"
>+
>+#ifndef INTEL_MEMORY_REGION_H
>+#define INTEL_MEMORY_REGION_H
>+
>+#define INTEL_MEMORY_TYPE_SHIFT 16
>+
>+#define INTEL_MEMORY_REGION_ID(type, instance) \
>+ ((BIT((type) + INTEL_MEMORY_TYPE_SHIFT)) | BIT(instance))
>+#define MEMORY_TYPE_FROM_REGION(r) (ilog2(r >> INTEL_MEMORY_TYPE_SHIFT))
>+#define MEMORY_INSTANCE_FROM_REGION(r) (ilog2(r & 0xffff))
>+
>+#define IS_DEVICE_MEMORY_REGION(region) \
>+ (MEMORY_TYPE_FROM_REGION(region) == LOCAL_I915_DEVICE_MEMORY)
>+#define IS_SYSTEM_MEMORY_REGION(region) \
>+ (MEMORY_TYPE_FROM_REGION(region) == LOCAL_I915_SYSTEM_MEMORY)
>+
>+/* Region macros for migration */
>+#define REGION_SYSTEM_MEMORY(n) INTEL_MEMORY_REGION_ID(LOCAL_I915_SYSTEM_MEMORY, n)
>+#define REGION_DEVICE_MEMORY(n) INTEL_MEMORY_REGION_ID(LOCAL_I915_DEVICE_MEMORY, n)
>+
>+#define LOCAL_I915_QUERY_MEMREGION_INFO 4
>+struct local_i915_memory_region_info {
>+
>+ /** Base type of a region */
>+#define LOCAL_I915_SYSTEM_MEMORY 0
>+#define LOCAL_I915_DEVICE_MEMORY 1
>+
>+ /** The region id is encoded in a layout which makes it possible to
>+ * retrieve the following information:
>+ *
>+ * Base type: log2(ID >> 16)
>+ */
>+ __u32 id;
>+
>+ /** Reserved field. MBZ */
>+ __u32 rsvd0;
>+
>+ /** Unused for now. MBZ */
>+ __u64 flags;
>+
>+ __u64 size;
>+
>+ /** Reserved fields must be cleared to zero. */
>+ __u64 rsvd1[4];
>+};
>+
>+struct local_i915_query_memory_region_info {
>+
>+ /** Number of struct drm_i915_memory_region_info structs */
>+ __u32 num_regions;
>+
>+ /** MBZ */
>+ __u32 rsvd[3];
>+
>+ struct local_i915_memory_region_info regions[];
>+};
>+
>+struct local_i915_gem_object_param {
>+ /** Handle for the object */
>+ __u32 handle;
>+
>+ __u32 size;
>+
>+ /** Set the memory region for the object listed in preference order
>+ * as an array of region ids within data. To force an object
>+ * to a particular memory region, set the region as the sole entry.
>+ *
>+ * Valid region ids are derived from the id field of
>+ * struct drm_i915_memory_region_info.
>+ * See struct drm_i915_query_memory_region_info.
>+ */
>+#define I915_OBJECT_PARAM (1ull<<32)
>+#define I915_PARAM_MEMORY_REGION 0x1
>+ __u64 param;
>+
>+ __u64 data;
>+};
>+
>+#define LOCAL_I915_GEM_OBJECT_SETPARAM DRM_I915_GEM_CONTEXT_SETPARAM
>+#define LOCAL_IOCTL_I915_GEM_OBJECT_SETPARAM DRM_IOWR(DRM_COMMAND_BASE \
>+ + LOCAL_I915_GEM_OBJECT_SETPARAM, struct local_i915_gem_object_param)
>+
>+extern const struct intel_memory_region {
>+ const char *region_name;
>+ uint8_t memory_type;
>+ uint8_t memory_instance;
>+} intel_memory_regions[];
>+
>+bool gem_has_query_support(int fd);
>+
>+uint32_t gem_get_batch_size(int fd, uint32_t region);
>+
>+struct local_i915_query_memory_region_info *gem_query_memory_regions(int fd);
>+
>+uint8_t gem_get_lmem_region_count(int fd);
>+
>+bool gem_has_lmem(int fd);
>+bool gem_query_has_memory_region(struct local_i915_query_memory_region_info *query_info,
>+ uint32_t region);
>+void gem_query_require_region(struct local_i915_query_memory_region_info *query_info,
>+ uint32_t region);
>+int gem_migrate_to_memory_region(int fd, int handle, uint32_t region);
>+int gem_migrate_to_lmem(int fd, int handle);
>+int gem_migrate_to_smem(int fd, int handle);
>+
>+uint32_t gem_create_in_memory_region_list(int fd, uint64_t size,
>+ uint32_t *mem_regions,
>+ int num_regions);
>+#define gem_create_in_memory_regions(fd, size, regions...) ({ \
>+ unsigned int arr__[] = { regions }; \
>+ gem_create_in_memory_region_list(fd, size, arr__, ARRAY_SIZE(arr__)); \
>+})
>+
>+#endif /* INTEL_MEMORY_REGION_H */
>diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
>index f2412d78..885cbb06 100644
>--- a/lib/ioctl_wrappers.h
>+++ b/lib/ioctl_wrappers.h
>@@ -38,6 +38,7 @@
>
> #include "i915/gem_context.h"
> #include "i915/gem_scheduler.h"
>+#include "i915/intel_memory_region.h"
>
> /**
> * igt_ioctl:
>diff --git a/lib/meson.build b/lib/meson.build
>index 73c07b0f..f25bd3e2 100644
>--- a/lib/meson.build
>+++ b/lib/meson.build
>@@ -7,6 +7,7 @@ lib_sources = [
> 'i915/gem_ring.c',
> 'i915/gem_mman.c',
> 'i915/gem_vm.c',
>+ 'i915/intel_memory_region.c',
> 'igt_color_encoding.c',
> 'igt_debugfs.c',
> 'igt_device.c',
>--
>2.23.0
>
>_______________________________________________
>igt-dev mailing list
>igt-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/igt-dev