Re: [PATCH v2 07/37] drm/i915: support creating LMEM objects

Chris Wilson
2019-06-27T23:17:02Z

Quoting Matthew Auld (2019-06-27 21:56:03)
> We currently define LMEM, or local memory, as just another memory
> region, like system memory or stolen, which we can expose to userspace
> and can be mapped to the CPU via some BAR.
>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/i915_drv.h | 5 ++
> drivers/gpu/drm/i915/intel_region_lmem.c | 66 +++++++++++++++++++
> drivers/gpu/drm/i915/intel_region_lmem.h | 16 +++++
> .../drm/i915/selftests/i915_live_selftests.h | 1 +
> .../drm/i915/selftests/intel_memory_region.c | 43 ++++++++++++
> 6 files changed, 132 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/intel_region_lmem.c
> create mode 100644 drivers/gpu/drm/i915/intel_region_lmem.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 28fac19f7b04..e782f7d10524 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -132,6 +132,7 @@ i915-y += \
> i915_scheduler.o \
> i915_trace_points.o \
> i915_vma.o \
> + intel_region_lmem.o \
> intel_wopcm.o
>
> # general-purpose microcontroller (GuC) support
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 838a796d9c55..7cbdffe3f129 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -93,6 +93,8 @@
> #include "gt/intel_timeline.h"
> #include "i915_vma.h"
>
> +#include "intel_region_lmem.h"
> +
> #include "intel_gvt.h"
>
> /* General customization:
> @@ -1341,6 +1343,8 @@ struct drm_i915_private {
> */
> resource_size_t stolen_usable_size; /* Total size minus reserved ranges */
>
> + struct intel_memory_region *regions[ARRAY_SIZE(intel_region_map)];
> +
> struct intel_uncore uncore;
>
> struct i915_virtual_gpu vgpu;
> @@ -2289,6 +2293,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> #define HAS_IPC(dev_priv) (INTEL_INFO(dev_priv)->display.has_ipc)
>
> #define HAS_REGION(i915, i) (INTEL_INFO(i915)->memory_regions & (i))
> +#define HAS_LMEM(i915) HAS_REGION(i915, REGION_LMEM)
>
> /*
> * For now, anything with a GuC requires uCode loading, and then supports
> diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c b/drivers/gpu/drm/i915/intel_region_lmem.c
> new file mode 100644
> index 000000000000..c4b5a88627a3
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_memory_region.h"
> +#include "intel_region_lmem.h"
> +
> +static const struct drm_i915_gem_object_ops region_lmem_obj_ops = {
> + .get_pages = i915_memory_region_get_pages_buddy,
> + .put_pages = i915_memory_region_put_pages_buddy,
> + .release = i915_gem_object_release_memory_region,
> +};
> +
> +static struct drm_i915_gem_object *
> +lmem_create_object(struct intel_memory_region *mem,
> + resource_size_t size,
> + unsigned int flags)
> +{
> + struct drm_i915_private *i915 = mem->i915;
> + struct drm_i915_gem_object *obj;
> + unsigned int cache_level;
> +
> + if (flags & I915_BO_ALLOC_CONTIGUOUS)
> + size = roundup_pow_of_two(size);

That should not be required. Seems like a missed opportunity to
pass the flag down to the allocator.

> + if (size > BIT(mem->mm.max_order) * mem->mm.min_size)
> + return ERR_PTR(-E2BIG);
> +
> + obj = i915_gem_object_alloc();
> + if (!obj)
> + return ERR_PTR(-ENOMEM);
> +
> + drm_gem_private_object_init(&i915->drm, &obj->base, size);
> + i915_gem_object_init(obj, &region_lmem_obj_ops);

> + obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
> + cache_level = HAS_LLC(i915) ? I915_CACHE_LLC : I915_CACHE_NONE;
> + i915_gem_object_set_cache_coherency(obj, cache_level);

That seems a little optimistic. I would strongly suggest pulling that
information from the intel_memory_region.
-Chris