Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext
On 11/27/20 2:25 PM, Chris Wilson wrote:
Quoting Matthew Auld (2020-11-27 12:06:08)
Same old gem_create but with now with extensions support. This is needed
to support various upcoming usecases. For now we use the extensions
mechanism to support setting an immutable-priority-list of potential
placements, at creation time.
If we wish to set the placements/regions we can simply do:
struct drm_i915_gem_object_param region_param = { … }; /* Unchanged */
struct drm_i915_gem_create_ext_setparam setparam_region = {
.base = { .name = I915_GEM_CREATE_EXT_SETPARAM },
.param = region_param,
}
struct drm_i915_gem_create_ext create_ext = {
.size = 16 * PAGE_SIZE,
.extensions = (uintptr_t)&setparam_region,
};
int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
if (err) ...
If we use the normal gem_create or gem_create_ext without the
extensions/placements then we still get the old behaviour with only
placing the object in system memory.
One important change here is the returned size will now be rounded up to
the correct size, depending on the list of placements, where we might
have minimum page-size restrictions on some platforms when dealing with
device local-memory.
Also, we still keep around the i915_gem_object_setparam ioctl, although
that is now restricted by the placement list(i.e we are not allowed to
add new placements), and longer term that will be going away wrt setting
placements, since it was deemed that the kernel doesn't need to support
a dynamic list of placements, which is now solidified by this uapi
change.
Testcase: igt/gem_create/create-ext-placement-sanity-check
Testcase: igt/gem_create/create-ext-placement-each
Testcase: igt/gem_create/create-ext-placement-all
Signed-off-by: Matthew Auld
Signed-off-by: CQ Tang
Cc: Joonas Lahtinen
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gem/i915_gem_create.c| 398 ++
drivers/gpu/drm/i915/gem/i915_gem_object.c| 2 +
.../gpu/drm/i915/gem/i915_gem_object_types.h | 9 +
drivers/gpu/drm/i915/gem/i915_gem_region.c| 4 +
drivers/gpu/drm/i915/i915_drv.c | 2 +-
drivers/gpu/drm/i915/i915_gem.c | 103 +
drivers/gpu/drm/i915/intel_memory_region.c| 20 +
drivers/gpu/drm/i915/intel_memory_region.h| 4 +
include/uapi/drm/i915_drm.h | 60 +++
10 files changed, 500 insertions(+), 103 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_create.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index ec361d61230b..3955134feca7 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -134,6 +134,7 @@ gem-y += \
gem/i915_gem_clflush.o \
gem/i915_gem_client_blt.o \
gem/i915_gem_context.o \
+ gem/i915_gem_create.o \
gem/i915_gem_dmabuf.o \
gem/i915_gem_domain.o \
gem/i915_gem_execbuffer.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
b/drivers/gpu/drm/i915/gem/i915_gem_create.c
new file mode 100644
index ..6f6dd4f1ce7e
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -0,0 +1,398 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2020 Intel Corporation
+ */
+
+#include "gem/i915_gem_ioctls.h"
+#include "gem/i915_gem_lmem.h"
+#include "gem/i915_gem_object_blt.h"
+#include "gem/i915_gem_region.h"
+
+#include "i915_drv.h"
+#include "i915_user_extensions.h"
+
+static u32 max_page_size(struct intel_memory_region **placements,
+int n_placements)
+{
+ u32 max_page_size = 0;
+ int i;
+
+ for (i = 0; i < n_placements; ++i) {
+ max_page_size = max_t(u32, max_page_size,
+ placements[i]->min_page_size);
+ }
+
+ GEM_BUG_ON(!max_page_size);
+ return max_page_size;
+}
+
+static int
+i915_gem_create(struct drm_file *file,
+ struct intel_memory_region **placements,
+ int n_placements,
+ u64 *size_p,
+ u32 *handle_p)
+{
+ struct drm_i915_gem_object *obj;
+ u32 handle;
+ u64 size;
+ int ret;
+
+ size = round_up(*size_p, max_page_size(placements, n_placements));
+ if (size == 0)
+ return -EINVAL;
+
+ /* For most of the ABI (e.g. mmap) we think in system pages */
+ GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE));
+
+ /* Allocate the new object */
+ obj = i915_gem_object_create_region(placements[0], size, 0);
+ if (IS_ERR(obj))
+ return PTR_ERR(obj);
+
+ if (i915_gem_object_is_lmem(obj)) {
+ struct intel_gt *gt = obj->mm.region->gt;
+ struct intel_context *ce = gt->engine[BCS0]->blitter_context;
+
+ /*
+* XXX: We really want to move this to get_pages(), but we
+* require grabbing the BKL for the blitting operati
Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext
On 01/12/2020 12:55, Chris Wilson wrote:
Quoting Matthew Auld (2020-11-27 12:06:08)
Same old gem_create but with now with extensions support. This is needed
to support various upcoming usecases. For now we use the extensions
mechanism to support setting an immutable-priority-list of potential
placements, at creation time.
If we wish to set the placements/regions we can simply do:
struct drm_i915_gem_object_param region_param = { … }; /* Unchanged */
struct drm_i915_gem_create_ext_setparam setparam_region = {
.base = { .name = I915_GEM_CREATE_EXT_SETPARAM },
.param = region_param,
}
struct drm_i915_gem_create_ext create_ext = {
.size = 16 * PAGE_SIZE,
.extensions = (uintptr_t)&setparam_region,
};
int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
if (err) ...
Looking at the existing gem_create, there is no detection of an
unsupported extension. That is there is no rejection of new userspace
asking for placement on an old kernel. (As erroneous as that would be
for many other reasons.)
Unless I've missed something, we need a new ioctl number for CREATEv2.
+Joonas
Right, and I guess it's not a good idea for userspace to implement
something like has_gem_create_ext()?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext
Quoting Matthew Auld (2020-11-27 12:06:08)
> Same old gem_create but with now with extensions support. This is needed
> to support various upcoming usecases. For now we use the extensions
> mechanism to support setting an immutable-priority-list of potential
> placements, at creation time.
>
> If we wish to set the placements/regions we can simply do:
>
> struct drm_i915_gem_object_param region_param = { … }; /* Unchanged */
> struct drm_i915_gem_create_ext_setparam setparam_region = {
> .base = { .name = I915_GEM_CREATE_EXT_SETPARAM },
> .param = region_param,
> }
>
> struct drm_i915_gem_create_ext create_ext = {
> .size = 16 * PAGE_SIZE,
> .extensions = (uintptr_t)&setparam_region,
> };
> int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
> if (err) ...
Looking at the existing gem_create, there is no detection of an
unsupported extension. That is there is no rejection of new userspace
asking for placement on an old kernel. (As erroneous as that would be
for many other reasons.)
Unless I've missed something, we need a new ioctl number for CREATEv2.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext
Quoting Matthew Auld (2020-11-27 12:06:08)
> +int
> +i915_gem_create_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file)
> +{
> + struct drm_i915_private *i915 = to_i915(dev);
> + struct create_ext ext_data = { .i915 = i915 };
> + struct drm_i915_gem_create_ext *args = data;
> + int ret;
> +
> + i915_gem_flush_free_objects(i915);
> +
> + ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
> + create_extensions,
> + ARRAY_SIZE(create_extensions),
> + &ext_data);
> + if (ret)
> + goto err_free;
> +
> + if (!ext_data.placements) {
> + struct intel_memory_region **placements;
> + enum intel_memory_type mem_type = INTEL_MEMORY_SYSTEM;
> +
> + placements = kmalloc(sizeof(struct intel_memory_region *),
> +GFP_KERNEL);
> + if (!placements)
> + return -ENOMEM;
> +
> + placements[0] = intel_memory_region_by_type(i915, mem_type);
> +
> + ext_data.placements = placements;
> + ext_data.n_placements = 1;
> + }
> +
> + ret = i915_gem_create(file,
> + ext_data.placements,
> + ext_data.n_placements,
> + &args->size, &args->handle);
> + if (!ret)
> + return 0;
Applying the extensions has to happen after creating the vanilla object.
It literally is the equivalent of applying the setparam ioctl to a fresh
object.
Look at the PXP series for how badly wrong this goes if you try it this
way around.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext
Quoting Matthew Auld (2020-11-27 12:06:08)
> Same old gem_create but with now with extensions support. This is needed
> to support various upcoming usecases. For now we use the extensions
> mechanism to support setting an immutable-priority-list of potential
> placements, at creation time.
>
> If we wish to set the placements/regions we can simply do:
>
> struct drm_i915_gem_object_param region_param = { … }; /* Unchanged */
> struct drm_i915_gem_create_ext_setparam setparam_region = {
> .base = { .name = I915_GEM_CREATE_EXT_SETPARAM },
> .param = region_param,
> }
>
> struct drm_i915_gem_create_ext create_ext = {
> .size = 16 * PAGE_SIZE,
> .extensions = (uintptr_t)&setparam_region,
> };
> int err = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE_EXT, &create_ext);
> if (err) ...
>
> If we use the normal gem_create or gem_create_ext without the
> extensions/placements then we still get the old behaviour with only
> placing the object in system memory.
>
> One important change here is the returned size will now be rounded up to
> the correct size, depending on the list of placements, where we might
> have minimum page-size restrictions on some platforms when dealing with
> device local-memory.
>
> Also, we still keep around the i915_gem_object_setparam ioctl, although
> that is now restricted by the placement list(i.e we are not allowed to
> add new placements), and longer term that will be going away wrt setting
> placements, since it was deemed that the kernel doesn't need to support
> a dynamic list of placements, which is now solidified by this uapi
> change.
>
> Testcase: igt/gem_create/create-ext-placement-sanity-check
> Testcase: igt/gem_create/create-ext-placement-each
> Testcase: igt/gem_create/create-ext-placement-all
> Signed-off-by: Matthew Auld
> Signed-off-by: CQ Tang
> Cc: Joonas Lahtinen
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/gem/i915_gem_create.c| 398 ++
> drivers/gpu/drm/i915/gem/i915_gem_object.c| 2 +
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 9 +
> drivers/gpu/drm/i915/gem/i915_gem_region.c| 4 +
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_gem.c | 103 +
> drivers/gpu/drm/i915/intel_memory_region.c| 20 +
> drivers/gpu/drm/i915/intel_memory_region.h| 4 +
> include/uapi/drm/i915_drm.h | 60 +++
> 10 files changed, 500 insertions(+), 103 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_create.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index ec361d61230b..3955134feca7 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -134,6 +134,7 @@ gem-y += \
> gem/i915_gem_clflush.o \
> gem/i915_gem_client_blt.o \
> gem/i915_gem_context.o \
> + gem/i915_gem_create.o \
> gem/i915_gem_dmabuf.o \
> gem/i915_gem_domain.o \
> gem/i915_gem_execbuffer.o \
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> new file mode 100644
> index ..6f6dd4f1ce7e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> @@ -0,0 +1,398 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2020 Intel Corporation
> + */
> +
> +#include "gem/i915_gem_ioctls.h"
> +#include "gem/i915_gem_lmem.h"
> +#include "gem/i915_gem_object_blt.h"
> +#include "gem/i915_gem_region.h"
> +
> +#include "i915_drv.h"
> +#include "i915_user_extensions.h"
> +
> +static u32 max_page_size(struct intel_memory_region **placements,
> +int n_placements)
> +{
> + u32 max_page_size = 0;
> + int i;
> +
> + for (i = 0; i < n_placements; ++i) {
> + max_page_size = max_t(u32, max_page_size,
> + placements[i]->min_page_size);
> + }
> +
> + GEM_BUG_ON(!max_page_size);
> + return max_page_size;
> +}
> +
> +static int
> +i915_gem_create(struct drm_file *file,
> + struct intel_memory_region **placements,
> + int n_placements,
> + u64 *size_p,
> + u32 *handle_p)
> +{
> + struct drm_i915_gem_object *obj;
> + u32 handle;
> + u64 size;
> + int ret;
> +
> + size = round_up(*size_p, max_page_size(placements, n_placements));
> + if (size == 0)
> + return -EINVAL;
> +
> + /* For most of the ABI (e.g. mmap) we think in system pages */
> + GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE));
> +
> + /* Allocate the new object */
> + obj = i915_gem_object_create_region(placements[0], size, 0);
> + if (IS_ERR(obj))
> + return PTR_ERR(obj);
> +
> + if (i915_gem_object_is_lmem(obj)) {
> + struct intel_gt *gt = obj->mm.region->gt;
> + struct intel_context *ce = gt->en
