[Intel-gfx] [PATCH v3 07/37] drm/i915: support creating LMEM objects

[Chris Wilson]

Quoting Matthew Auld (2019-08-09 23:26:13)
> @@ -1369,6 +1371,8 @@ struct drm_i915_private {
>          */
>         resource_size_t stolen_usable_size;     /* Total size minus reserved ranges */
>  
> +       struct intel_memory_region *regions[INTEL_MEMORY_UKNOWN];

If there was ever an object to put inside i915_mm, this is sorely the
one!

>         struct intel_uncore uncore;
>  
>         struct i915_virtual_gpu vgpu;
> @@ -2213,6 +2217,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)
>  
>  #define HAS_GT_UC(dev_priv)    (INTEL_INFO(dev_priv)->has_gt_uc)
>  
> 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..ca906d1ff631
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_region_lmem.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_memory_region.h"
> +#include "gem/i915_gem_lmem.h"
> +#include "gem/i915_gem_region.h"
> +#include "intel_region_lmem.h"
> +
> +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);

? I would say that is not your decision to make. The flag says that the
allocation should one contiguous block.

> +
> +       if (size > BIT(mem->mm.max_order) * mem->mm.chunk_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, &i915_gem_lmem_obj_ops);
> +
> +       obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;

Is it really? Device local memory starts off inside the CPU cache?

> +       cache_level = HAS_LLC(i915) ? I915_CACHE_LLC : I915_CACHE_NONE;

This seems to be a false generalisation. You should be asking the
memory region?

Besides, the current thinking is not to mark any object as cacheable
until the user commands it.
-Chris