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

Chris Wilson
2019-08-10T10:37:10Z

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org