[Intel-gfx] [RFC PATCH 092/162] drm/i915/uapi: introduce drm_i915_gem_create_ext

[Chris Wilson]

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 <matthew.auld at intel.com>
> Signed-off-by: CQ Tang <cq.tang at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
>  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 000000000000..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 operation which is
> +                * annoying. In the pipeline is support for async get_pages()
> +                * which should fit nicely for this. Also note that the actual
> +                * clear should be done async(we currently do an object_wait
> +                * which is pure garbage), we just need to take care if
> +                * userspace opts of implicit sync for the execbuf, to avoid any
> +                * potential info leak.
> +                */

Not just XXX, but the design should be completed first.
-Chris