[PATCH 1/3] drm/i915: Create dumb buffer from LMEM

[PATCH 1/3] drm/i915: Create dumb buffer from LMEM

[Ramalingam C]

When LMEM is supported, dumb buffer preferred to be created from LMEM.

This is developed on top of v3 LMEM series
https://patchwork.freedesktop.org/series/56683/.

v2:
  Parameters are reshuffled. [Chris]

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
cc: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e458507b1558..6810a549ee98 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -44,6 +44,7 @@
 #include "gem/i915_gem_clflush.h"
 #include "gem/i915_gem_context.h"
 #include "gem/i915_gem_ioctls.h"
+#include "gem/i915_gem_lmem.h"
 #include "gem/i915_gem_pm.h"
 #include "gt/intel_engine_user.h"
 #include "gt/intel_gt.h"
@@ -160,6 +161,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
 static int
 i915_gem_create(struct drm_file *file,
 		struct drm_i915_private *dev_priv,
+		enum intel_region_id mem_region,
 		u64 *size_p,
 		u32 *handle_p)
 {
@@ -173,7 +175,12 @@ i915_gem_create(struct drm_file *file,
 		return -EINVAL;
 
 	/* Allocate the new object */
-	obj = i915_gem_object_create_shmem(dev_priv, size);
+	if (mem_region == INTEL_MEMORY_LMEM)
+		obj = i915_gem_object_create_lmem(dev_priv, size, 0);
+	else if (mem_region == INTEL_MEMORY_STOLEN)
+		obj = i915_gem_object_create_stolen(dev_priv, size);
+	else
+		obj = i915_gem_object_create_shmem(dev_priv, size);
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
@@ -193,6 +200,7 @@ i915_gem_dumb_create(struct drm_file *file,
 		     struct drm_device *dev,
 		     struct drm_mode_create_dumb *args)
 {
+	enum intel_region_id mem_region = INTEL_MEMORY_UKNOWN;
 	int cpp = DIV_ROUND_UP(args->bpp, 8);
 	u32 format;
 
@@ -219,7 +227,11 @@ i915_gem_dumb_create(struct drm_file *file,
 		args->pitch = ALIGN(args->pitch, 4096);
 
 	args->size = args->pitch * args->height;
-	return i915_gem_create(file, to_i915(dev),
+
+	if (HAS_LMEM(to_i915(dev)))
+		mem_region = INTEL_MEMORY_LMEM;
+
+	return i915_gem_create(file, to_i915(dev), mem_region,
 			       &args->size, &args->handle);
 }
 
@@ -238,7 +250,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
 
 	i915_gem_flush_free_objects(dev_priv);
 
-	return i915_gem_create(file, dev_priv,
+	return i915_gem_create(file, dev_priv, INTEL_MEMORY_UKNOWN,
 			       &args->size, &args->handle);
 }
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Allowed memory region for GEM obj

[Ramalingam C]

Each GEM object is initialized with allowed memory regions for
it's migration across memory region.

In future patch we are restricting the memory regions or few objects.

This is developed on top of v3 LMEM series
https://patchwork.freedesktop.org/series/56683/

CC: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c           | 2 +-
 drivers/gpu/drm/i915/gem/i915_gem_internal.c         | 2 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.c           | 8 ++++++++
 drivers/gpu/drm/i915/gem/i915_gem_object_types.h     | 3 +++
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c            | 2 +-
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c           | 2 +-
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c          | 2 +-
 drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c | 2 +-
 drivers/gpu/drm/i915/gem/selftests/huge_pages.c      | 6 +++---
 drivers/gpu/drm/i915/gvt/dmabuf.c                    | 2 +-
 drivers/gpu/drm/i915/intel_region_lmem.c             | 2 +-
 drivers/gpu/drm/i915/selftests/i915_gem_gtt.c        | 2 +-
 drivers/gpu/drm/i915/selftests/mock_region.c         | 2 +-
 13 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index 96ce95c8ac5a..c87c6e445541 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -287,7 +287,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 	}
 
 	drm_gem_private_object_init(dev, &obj->base, dma_buf->size);
-	i915_gem_object_init(obj, &i915_gem_object_dmabuf_ops);
+	i915_gem_object_init(obj, to_i915(dev), &i915_gem_object_dmabuf_ops);
 	obj->base.import_attach = attach;
 	obj->base.resv = dma_buf->resv;
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
index c2e237702e8c..8f94704eb6cb 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
@@ -180,7 +180,7 @@ i915_gem_object_create_internal(struct drm_i915_private *i915,
 		return ERR_PTR(-ENOMEM);
 
 	drm_gem_private_object_init(&i915->drm, &obj->base, size);
-	i915_gem_object_init(obj, &i915_gem_object_internal_ops);
+	i915_gem_object_init(obj, i915, &i915_gem_object_internal_ops);
 
 	/*
 	 * Mark the object as volatile, such that the pages are marked as
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 0f33da5e541d..e6f8426dedff 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -49,6 +49,7 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj)
 }
 
 void i915_gem_object_init(struct drm_i915_gem_object *obj,
+			  struct drm_i915_private *dev_priv,
 			  const struct drm_i915_gem_object_ops *ops)
 {
 	mutex_init(&obj->mm.lock);
@@ -70,6 +71,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
 	obj->mm.madv = I915_MADV_WILLNEED;
 	INIT_RADIX_TREE(&obj->mm.get_page.radix, GFP_KERNEL | __GFP_NOWARN);
 	mutex_init(&obj->mm.get_page.lock);
+
+	obj->memory_regions = INTEL_INFO(i915)->memory_regions;
 }
 
 /**
@@ -534,6 +537,11 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
 		u32 region = uregions_copy[i];
 		enum intel_region_id id = __region_id(region);
 
+		if (!(obj->memory_region & region)) {
+			ret = -EINVAL;
+			continue;
+		}
+
 		if (id == INTEL_MEMORY_UKNOWN) {
 			ret = -EINVAL;
 			goto err;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 7b93450a860b..af5505e0bd0a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -286,6 +286,9 @@ struct drm_i915_gem_object {
 
 	/** for phys allocated objects */
 	struct drm_dma_handle *phys_handle;
+
+	/* Allowed memory regions for this obj to reside in. */
+	u32 memory_regions;
 };
 
 static inline struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index ac7a552349b4..296c784a39ca 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -492,7 +492,7 @@ create_shmem(struct intel_memory_region *mem,
 	mapping_set_gfp_mask(mapping, mask);
 	GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));
 
-	i915_gem_object_init(obj, &i915_gem_shmem_ops);
+	i915_gem_object_init(obj, i915, &i915_gem_shmem_ops);
 
 	obj->write_domain = I915_GEM_DOMAIN_CPU;
 	obj->read_domains = I915_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index 762659011de7..6dbf84ebf904 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -564,7 +564,7 @@ __i915_gem_object_create_stolen(struct drm_i915_private *dev_priv,
 		return NULL;
 
 	drm_gem_private_object_init(&dev_priv->drm, &obj->base, stolen->size);
-	i915_gem_object_init(obj, &i915_gem_object_stolen_ops);
+	i915_gem_object_init(obj, dev_priv, &i915_gem_object_stolen_ops);
 
 	obj->stolen = stolen;
 	obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 11b231c187c5..0af0c81f8724 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -792,7 +792,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 		return -ENOMEM;
 
 	drm_gem_private_object_init(dev, &obj->base, args->user_size);
-	i915_gem_object_init(obj, &i915_gem_userptr_ops);
+	i915_gem_object_init(obj, dev_priv, &i915_gem_userptr_ops);
 	obj->read_domains = I915_GEM_DOMAIN_CPU;
 	obj->write_domain = I915_GEM_DOMAIN_CPU;
 	i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
index 686e0e909280..6a12bbd8e338 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
@@ -113,7 +113,7 @@ huge_gem_object(struct drm_i915_private *i915,
 		return ERR_PTR(-ENOMEM);
 
 	drm_gem_private_object_init(&i915->drm, &obj->base, dma_size);
-	i915_gem_object_init(obj, &huge_ops);
+	i915_gem_object_init(obj, i915, &huge_ops);
 
 	obj->read_domains = I915_GEM_DOMAIN_CPU;
 	obj->write_domain = I915_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
index 0282f15d6937..1bdfbbda5547 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
@@ -166,7 +166,7 @@ huge_pages_object(struct drm_i915_private *i915,
 		return ERR_PTR(-ENOMEM);
 
 	drm_gem_private_object_init(&i915->drm, &obj->base, size);
-	i915_gem_object_init(obj, &huge_page_ops);
+	i915_gem_object_init(obj, i915, &huge_page_ops);
 
 	obj->flags = I915_BO_ALLOC_VOLATILE;
 
@@ -314,9 +314,9 @@ fake_huge_pages_object(struct drm_i915_private *i915, u64 size, bool single)
 	drm_gem_private_object_init(&i915->drm, &obj->base, size);
 
 	if (single)
-		i915_gem_object_init(obj, &fake_ops_single);
+		i915_gem_object_init(obj, i915, &fake_ops_single);
 	else
-		i915_gem_object_init(obj, &fake_ops);
+		i915_gem_object_init(obj, i915, &fake_ops);
 
 	obj->flags = I915_BO_ALLOC_VOLATILE;
 
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c
index 13044c027f27..92f074943b97 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -161,7 +161,7 @@ static struct drm_i915_gem_object *vgpu_create_gem(struct drm_device *dev,
 
 	drm_gem_private_object_init(dev, &obj->base,
 		roundup(info->size, PAGE_SIZE));
-	i915_gem_object_init(obj, &intel_vgpu_gem_ops);
+	i915_gem_object_init(obj, dev_priv, &intel_vgpu_gem_ops);
 
 	obj->read_domains = I915_GEM_DOMAIN_GTT;
 	obj->write_domain = 0;
diff --git a/drivers/gpu/drm/i915/intel_region_lmem.c b/drivers/gpu/drm/i915/intel_region_lmem.c
index b8f671634919..3f86b6034a1b 100644
--- a/drivers/gpu/drm/i915/intel_region_lmem.c
+++ b/drivers/gpu/drm/i915/intel_region_lmem.c
@@ -29,7 +29,7 @@ lmem_create_object(struct intel_memory_region *mem,
 		return ERR_PTR(-ENOMEM);
 
 	drm_gem_private_object_init(&i915->drm, &obj->base, size);
-	i915_gem_object_init(obj, &i915_gem_lmem_obj_ops);
+	i915_gem_object_init(obj, i915, &i915_gem_lmem_obj_ops);
 
 	obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
index 1f4880307198..0d8766ec974f 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
@@ -123,7 +123,7 @@ fake_dma_object(struct drm_i915_private *i915, u64 size)
 		goto err;
 
 	drm_gem_private_object_init(&i915->drm, &obj->base, size);
-	i915_gem_object_init(obj, &fake_ops);
+	i915_gem_object_init(obj, i915, &fake_ops);
 
 	obj->flags = I915_BO_ALLOC_VOLATILE;
 
diff --git a/drivers/gpu/drm/i915/selftests/mock_region.c b/drivers/gpu/drm/i915/selftests/mock_region.c
index d73f37712c44..dd33f83ef79d 100644
--- a/drivers/gpu/drm/i915/selftests/mock_region.c
+++ b/drivers/gpu/drm/i915/selftests/mock_region.c
@@ -34,7 +34,7 @@ mock_object_create(struct intel_memory_region *mem,
 		return ERR_PTR(-ENOMEM);
 
 	drm_gem_private_object_init(&i915->drm, &obj->base, size);
-	i915_gem_object_init(obj, &mock_region_obj_ops);
+	i915_gem_object_init(obj, i915, &mock_region_obj_ops);
 
 	obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Allowed memory region for GEM obj

[Chris Wilson]

Quoting Ramalingam C (2019-09-26 06:21:34)
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 0f33da5e541d..e6f8426dedff 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -49,6 +49,7 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj)
>  }
>  
>  void i915_gem_object_init(struct drm_i915_gem_object *obj,
> +                         struct drm_i915_private *dev_priv,

to_i915(obj) is valid at this point.

Please never add dev_priv.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/3] drm/i915: Allowed memory region for GEM obj

[Ramalingam C]

Each GEM object is initialized with allowed memory regions for
it's migration across memory region.

In future patch we are restricting the memory regions or few objects.

This is developed on top of v3 LMEM series
https://patchwork.freedesktop.org/series/56683/

v2:
  dev_priv is retrieved from obj [Chris]

CC: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c       | 9 +++++++++
 drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 0f33da5e541d..165ae03c6774 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -51,6 +51,8 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj)
 void i915_gem_object_init(struct drm_i915_gem_object *obj,
 			  const struct drm_i915_gem_object_ops *ops)
 {
+	struct drm_i915_file_private *dev_priv = to_i915(obj->base.drm);
+
 	mutex_init(&obj->mm.lock);
 
 	spin_lock_init(&obj->vma.lock);
@@ -70,6 +72,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
 	obj->mm.madv = I915_MADV_WILLNEED;
 	INIT_RADIX_TREE(&obj->mm.get_page.radix, GFP_KERNEL | __GFP_NOWARN);
 	mutex_init(&obj->mm.get_page.lock);
+
+	obj->memory_regions = INTEL_INFO(dev_priv)->memory_regions;
 }
 
 /**
@@ -534,6 +538,11 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
 		u32 region = uregions_copy[i];
 		enum intel_region_id id = __region_id(region);
 
+		if (!(obj->memory_regions & region)) {
+			ret = -EINVAL;
+			continue;
+		}
+
 		if (id == INTEL_MEMORY_UKNOWN) {
 			ret = -EINVAL;
 			goto err;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 7b93450a860b..af5505e0bd0a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -286,6 +286,9 @@ struct drm_i915_gem_object {
 
 	/** for phys allocated objects */
 	struct drm_dma_handle *phys_handle;
+
+	/* Allowed memory regions for this obj to reside in. */
+	u32 memory_regions;
 };
 
 static inline struct drm_i915_gem_object *
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Ramalingam C]

If Local memory is supported by hardware, we want framebuffer backing
gem objects out of local memory.

If local memory is supported and gem object if not from local memory we
migrate the obj into local memory. And once framebuffer is created we
block the migration of the associated object out of local memory.

This is developed on top of v3 LMEM series
https://patchwork.freedesktop.org/series/56683/

cc: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++
 drivers/gpu/drm/i915/gem/i915_gem_object.c   | 58 ++++++++++++--------
 drivers/gpu/drm/i915/gem/i915_gem_object.h   |  3 +
 3 files changed, 62 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 1cc74844d3ea..d1921a317066 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -56,6 +56,8 @@
 #include "display/intel_tv.h"
 #include "display/intel_vdsc.h"
 
+#include "gem/i915_gem_object.h"
+
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include "intel_acpi.h"
@@ -15496,6 +15498,10 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 {
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_private *dev_priv = to_i915(fb->dev);
+
+	/* removing the FB memory region restriction on obj, if any */
+	intel_fb->front_buffer->obj = INTEL_INFO(dev_priv)->memory_regions;
 
 	drm_framebuffer_cleanup(fb);
 	intel_frontbuffer_put(intel_fb->frontbuffer);
@@ -15543,11 +15549,26 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
 	struct drm_framebuffer *fb = &intel_fb->base;
+	u32 *region_map;
 	u32 max_stride;
 	unsigned int tiling, stride;
 	int ret = -EINVAL;
 	int i;
 
+	/* GEM Obj for frame buffer is expected to be in LMEM. */
+	if (HAS_LMEM(dev_priv))
+		if (obj->mm.region->type != INTEL_LMEM) {
+			region_map = &intel_region_map[INTEL_MEMORY_LMEM];
+			ret = i915_gem_object_mem_region_migrate(dev_priv, obj,
+								 region_map,
+								 1);
+			if (ret) {
+				DRM_ERROR("FB migration to LMEM Failed(%d)\n",
+					  ret);
+				return ret;
+			}
+		}
+
 	intel_fb->frontbuffer = intel_frontbuffer_get(obj);
 	if (!intel_fb->frontbuffer)
 		return -ENOMEM;
@@ -15666,6 +15687,10 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		goto err;
 	}
 
+	/* Blocking the migration of gem obj out of LMEM */
+	if (HAS_LMEM(dev_priv))
+		obj->memory_regions = REGION_LMEM;
+
 	return 0;
 
 err:
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index e6f8426dedff..65b47054130b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -501,30 +501,11 @@ __region_id(u32 region)
 	return INTEL_MEMORY_UKNOWN;
 }
 
-static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
-					 struct drm_i915_gem_object_param *args,
-					 struct drm_file *file,
-					 struct drm_i915_gem_object *obj)
+int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
+				       struct drm_i915_gem_object *obj,
+				       u32 *uregions, u32 region_count)
 {
 	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
-	u32 __user *uregions = u64_to_user_ptr(args->data);
-	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
-	int i, ret;
-
-	if (args->size > INTEL_MEMORY_UKNOWN)
-		return -EINVAL;
-
-	memset(uregions_copy, 0, sizeof(uregions_copy));
-	for (i = 0; i < args->size; i++) {
-		u32 region;
-
-		ret = get_user(region, uregions);
-		if (ret)
-			return ret;
-
-		uregions_copy[i] = region;
-		++uregions;
-	}
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	ret = i915_gem_object_prepare_move(obj);
@@ -533,8 +514,8 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
 	        goto err;
 	}
 
-	for (i = 0; i < args->size; i++) {
-		u32 region = uregions_copy[i];
+	for (i = 0; i < region_count; i++) {
+		u32 region = uregions[i];
 		enum intel_region_id id = __region_id(region);
 
 		if (!(obj->memory_region & region)) {
@@ -576,6 +557,35 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
+static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
+					 struct drm_i915_gem_object_param *args,
+					 struct drm_file *file,
+					 struct drm_i915_gem_object *obj)
+{
+	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
+	u32 __user *uregions = u64_to_user_ptr(args->data);
+	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
+	int i, ret;
+
+	if (args->size > INTEL_MEMORY_UKNOWN)
+		return -EINVAL;
+
+	memset(uregions_copy, 0, sizeof(uregions_copy));
+	for (i = 0; i < args->size; i++) {
+		u32 region;
+
+		ret = get_user(region, uregions);
+		if (ret)
+			return ret;
+
+		uregions_copy[i] = region;
+		++uregions;
+	}
+
+	return i915_gem_object_mem_region_migrate(dev_priv, obj, uregions_copy,
+						  args->size);
+}
+
 int i915_gem_object_setparam_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file)
 {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index a7c073aeb777..d09a9c741b41 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -47,6 +47,9 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
 			    enum intel_region_id id);
 
 void i915_gem_flush_free_objects(struct drm_i915_private *i915);
+int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
+				       struct drm_i915_gem_object *obj,
+				       u32 *uregions, u32 region_count);
 
 void __i915_gem_object_reset_page_iter(struct drm_i915_gem_object *obj);
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Tvrtko Ursulin]

On 26/09/2019 06:21, Ramalingam C wrote:
> If Local memory is supported by hardware, we want framebuffer backing
> gem objects out of local memory.
> 
> If local memory is supported and gem object if not from local memory we
> migrate the obj into local memory. And once framebuffer is created we
> block the migration of the associated object out of local memory.
> 
> This is developed on top of v3 LMEM series
> https://patchwork.freedesktop.org/series/56683/
> 
> cc: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++
>   drivers/gpu/drm/i915/gem/i915_gem_object.c   | 58 ++++++++++++--------
>   drivers/gpu/drm/i915/gem/i915_gem_object.h   |  3 +
>   3 files changed, 62 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 1cc74844d3ea..d1921a317066 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -56,6 +56,8 @@
>   #include "display/intel_tv.h"
>   #include "display/intel_vdsc.h"
>   
> +#include "gem/i915_gem_object.h"
> +
>   #include "i915_drv.h"
>   #include "i915_trace.h"
>   #include "intel_acpi.h"
> @@ -15496,6 +15498,10 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>   static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>   {
>   	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_private *dev_priv = to_i915(fb->dev);
> +
> +	/* removing the FB memory region restriction on obj, if any */
> +	intel_fb->front_buffer->obj = INTEL_INFO(dev_priv)->memory_regions;

Is this right, assigning bitmask to something called obj?

>   
>   	drm_framebuffer_cleanup(fb);
>   	intel_frontbuffer_put(intel_fb->frontbuffer);
> @@ -15543,11 +15549,26 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>   {
>   	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>   	struct drm_framebuffer *fb = &intel_fb->base;
> +	u32 *region_map;
>   	u32 max_stride;
>   	unsigned int tiling, stride;
>   	int ret = -EINVAL;
>   	int i;
>   
> +	/* GEM Obj for frame buffer is expected to be in LMEM. */
> +	if (HAS_LMEM(dev_priv))
> +		if (obj->mm.region->type != INTEL_LMEM) {
> +			region_map = &intel_region_map[INTEL_MEMORY_LMEM];
> +			ret = i915_gem_object_mem_region_migrate(dev_priv, obj,
> +								 region_map,
> +								 1);
> +			if (ret) {
> +				DRM_ERROR("FB migration to LMEM Failed(%d)\n",
> +					  ret);

Probably should be just debug level since it is imaginably user 
triggerable and not really an error for the kernel as such.

> +				return ret;
> +			}
> +		}
> +
>   	intel_fb->frontbuffer = intel_frontbuffer_get(obj);
>   	if (!intel_fb->frontbuffer)
>   		return -ENOMEM;
> @@ -15666,6 +15687,10 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>   		goto err;
>   	}
>   
> +	/* Blocking the migration of gem obj out of LMEM */
> +	if (HAS_LMEM(dev_priv))
> +		obj->memory_regions = REGION_LMEM;
> +
>   	return 0;
>   
>   err:
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index e6f8426dedff..65b47054130b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -501,30 +501,11 @@ __region_id(u32 region)
>   	return INTEL_MEMORY_UKNOWN;
>   }
>   
> -static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
> -					 struct drm_i915_gem_object_param *args,
> -					 struct drm_file *file,
> -					 struct drm_i915_gem_object *obj)
> +int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
> +				       struct drm_i915_gem_object *obj,
> +				       u32 *uregions, u32 region_count)
>   {
>   	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
> -	u32 __user *uregions = u64_to_user_ptr(args->data);
> -	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
> -	int i, ret;
> -
> -	if (args->size > INTEL_MEMORY_UKNOWN)
> -		return -EINVAL;
> -
> -	memset(uregions_copy, 0, sizeof(uregions_copy));
> -	for (i = 0; i < args->size; i++) {
> -		u32 region;
> -
> -		ret = get_user(region, uregions);
> -		if (ret)
> -			return ret;
> -
> -		uregions_copy[i] = region;
> -		++uregions;
> -	}
>   
>   	mutex_lock(&dev_priv->drm.struct_mutex);
>   	ret = i915_gem_object_prepare_move(obj);
> @@ -533,8 +514,8 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
>   	        goto err;
>   	}
>   
> -	for (i = 0; i < args->size; i++) {
> -		u32 region = uregions_copy[i];
> +	for (i = 0; i < region_count; i++) {
> +		u32 region = uregions[i];
>   		enum intel_region_id id = __region_id(region);
>   
>   		if (!(obj->memory_region & region)) {
> @@ -576,6 +557,35 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
>   	return ret;
>   }
>   
> +static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
> +					 struct drm_i915_gem_object_param *args,
> +					 struct drm_file *file,
> +					 struct drm_i915_gem_object *obj)
> +{
> +	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
> +	u32 __user *uregions = u64_to_user_ptr(args->data);
> +	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
> +	int i, ret;
> +
> +	if (args->size > INTEL_MEMORY_UKNOWN)
> +		return -EINVAL;
> +
> +	memset(uregions_copy, 0, sizeof(uregions_copy));
> +	for (i = 0; i < args->size; i++) {
> +		u32 region;
> +
> +		ret = get_user(region, uregions);
> +		if (ret)
> +			return ret;
> +
> +		uregions_copy[i] = region;
> +		++uregions;
> +	}

I do understand this is old code (not yours) but I fail to penetrate why 
is this a loop and not copy_from_user?

> +
> +	return i915_gem_object_mem_region_migrate(dev_priv, obj, uregions_copy,
> +						  args->size);
> +}
> +
>   int i915_gem_object_setparam_ioctl(struct drm_device *dev, void *data,
>   				   struct drm_file *file)
>   {
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index a7c073aeb777..d09a9c741b41 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -47,6 +47,9 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
>   			    enum intel_region_id id);
>   
>   void i915_gem_flush_free_objects(struct drm_i915_private *i915);
> +int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
> +				       struct drm_i915_gem_object *obj,
> +				       u32 *uregions, u32 region_count);
>   
>   void __i915_gem_object_reset_page_iter(struct drm_i915_gem_object *obj);
>   
> 

General approach looks feasible to me, but I'll leave guys directly 
involved with local memory to call it.

Another my comment/question would be - do we have a hook when objects 
are released/disassociated from the framebuffer and should we restore 
the allowed memory region mask to default at that point?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-26 09:53:03)
> 
> On 26/09/2019 06:21, Ramalingam C wrote:
> > If Local memory is supported by hardware, we want framebuffer backing
> > gem objects out of local memory.
> > 
> > If local memory is supported and gem object if not from local memory we
> > migrate the obj into local memory. And once framebuffer is created we
> > block the migration of the associated object out of local memory.
> > 
> > This is developed on top of v3 LMEM series
> > https://patchwork.freedesktop.org/series/56683/
> > 
> > cc: Matthew Auld <matthew.auld@intel.com>
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++
> >   drivers/gpu/drm/i915/gem/i915_gem_object.c   | 58 ++++++++++++--------
> >   drivers/gpu/drm/i915/gem/i915_gem_object.h   |  3 +
> >   3 files changed, 62 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 1cc74844d3ea..d1921a317066 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -56,6 +56,8 @@
> >   #include "display/intel_tv.h"
> >   #include "display/intel_vdsc.h"
> >   
> > +#include "gem/i915_gem_object.h"
> > +
> >   #include "i915_drv.h"
> >   #include "i915_trace.h"
> >   #include "intel_acpi.h"
> > @@ -15496,6 +15498,10 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
> >   static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >   {
> >       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > +     struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > +
> > +     /* removing the FB memory region restriction on obj, if any */
> > +     intel_fb->front_buffer->obj = INTEL_INFO(dev_priv)->memory_regions;
> 
> Is this right, assigning bitmask to something called obj?
> 
> >   
> >       drm_framebuffer_cleanup(fb);
> >       intel_frontbuffer_put(intel_fb->frontbuffer);
> > @@ -15543,11 +15549,26 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >   {
> >       struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> >       struct drm_framebuffer *fb = &intel_fb->base;
> > +     u32 *region_map;
> >       u32 max_stride;
> >       unsigned int tiling, stride;
> >       int ret = -EINVAL;
> >       int i;
> >   
> > +     /* GEM Obj for frame buffer is expected to be in LMEM. */
> > +     if (HAS_LMEM(dev_priv))
> > +             if (obj->mm.region->type != INTEL_LMEM) {
> > +                     region_map = &intel_region_map[INTEL_MEMORY_LMEM];
> > +                     ret = i915_gem_object_mem_region_migrate(dev_priv, obj,
> > +                                                              region_map,
> > +                                                              1);
> > +                     if (ret) {
> > +                             DRM_ERROR("FB migration to LMEM Failed(%d)\n",
> > +                                       ret);
> 
> Probably should be just debug level since it is imaginably user 
> triggerable and not really an error for the kernel as such.

This would be a part of pin_to_display? That's where we do the other
conversions required for scanout objects.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Ramalingam C]

On 2019-09-26 at 10:13:28 +0100, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-09-26 09:53:03)
> > 
> > On 26/09/2019 06:21, Ramalingam C wrote:
> > > If Local memory is supported by hardware, we want framebuffer backing
> > > gem objects out of local memory.
> > > 
> > > If local memory is supported and gem object if not from local memory we
> > > migrate the obj into local memory. And once framebuffer is created we
> > > block the migration of the associated object out of local memory.
> > > 
> > > This is developed on top of v3 LMEM series
> > > https://patchwork.freedesktop.org/series/56683/
> > > 
> > > cc: Matthew Auld <matthew.auld@intel.com>
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++
> > >   drivers/gpu/drm/i915/gem/i915_gem_object.c   | 58 ++++++++++++--------
> > >   drivers/gpu/drm/i915/gem/i915_gem_object.h   |  3 +
> > >   3 files changed, 62 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 1cc74844d3ea..d1921a317066 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -56,6 +56,8 @@
> > >   #include "display/intel_tv.h"
> > >   #include "display/intel_vdsc.h"
> > >   
> > > +#include "gem/i915_gem_object.h"
> > > +
> > >   #include "i915_drv.h"
> > >   #include "i915_trace.h"
> > >   #include "intel_acpi.h"
> > > @@ -15496,6 +15498,10 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
> > >   static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> > >   {
> > >       struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > > +     struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > > +
> > > +     /* removing the FB memory region restriction on obj, if any */
> > > +     intel_fb->front_buffer->obj = INTEL_INFO(dev_priv)->memory_regions;
> > 
> > Is this right, assigning bitmask to something called obj?
> > 
> > >   
> > >       drm_framebuffer_cleanup(fb);
> > >       intel_frontbuffer_put(intel_fb->frontbuffer);
> > > @@ -15543,11 +15549,26 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> > >   {
> > >       struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> > >       struct drm_framebuffer *fb = &intel_fb->base;
> > > +     u32 *region_map;
> > >       u32 max_stride;
> > >       unsigned int tiling, stride;
> > >       int ret = -EINVAL;
> > >       int i;
> > >   
> > > +     /* GEM Obj for frame buffer is expected to be in LMEM. */
> > > +     if (HAS_LMEM(dev_priv))
> > > +             if (obj->mm.region->type != INTEL_LMEM) {
> > > +                     region_map = &intel_region_map[INTEL_MEMORY_LMEM];
> > > +                     ret = i915_gem_object_mem_region_migrate(dev_priv, obj,
> > > +                                                              region_map,
> > > +                                                              1);
> > > +                     if (ret) {
> > > +                             DRM_ERROR("FB migration to LMEM Failed(%d)\n",
> > > +                                       ret);
> > 
> > Probably should be just debug level since it is imaginably user 
> > triggerable and not really an error for the kernel as such.
> 
> This would be a part of pin_to_display? That's where we do the other
> conversions required for scanout objects.

I will relook through it chris. But I am just wondering when the FB is
created/destroyed itself could we fix/release the memory region required?

-Ram.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Chris Wilson]

Quoting Ramalingam C (2019-09-27 10:12:57)
> On 2019-09-26 at 10:13:28 +0100, Chris Wilson wrote:
> > This would be a part of pin_to_display? That's where we do the other
> > conversions required for scanout objects.
> 
> I will relook through it chris. But I am just wondering when the FB is
> created/destroyed itself could we fix/release the memory region required?

That would be atypical; we generally only restrict the user options while
it is attached to HW, and err on the side of laziness.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Ramalingam C]

On 2019-09-27 at 10:26:17 +0100, Chris Wilson wrote:
> Quoting Ramalingam C (2019-09-27 10:12:57)
> > On 2019-09-26 at 10:13:28 +0100, Chris Wilson wrote:
> > > This would be a part of pin_to_display? That's where we do the other
> > > conversions required for scanout objects.
> > 
> > I will relook through it chris. But I am just wondering when the FB is
> > created/destroyed itself could we fix/release the memory region required?
> 
> That would be atypical; we generally only restrict the user options while
> it is attached to HW, and err on the side of laziness.
I will explore the pin_to_diplay path. Thanks Chris!

-Ram
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Ramalingam C]

On 2019-09-26 at 09:53:03 +0100, Tvrtko Ursulin wrote:
> 
> On 26/09/2019 06:21, Ramalingam C wrote:
> > If Local memory is supported by hardware, we want framebuffer backing
> > gem objects out of local memory.
> > 
> > If local memory is supported and gem object if not from local memory we
> > migrate the obj into local memory. And once framebuffer is created we
> > block the migration of the associated object out of local memory.
> > 
> > This is developed on top of v3 LMEM series
> > https://patchwork.freedesktop.org/series/56683/
> > 
> > cc: Matthew Auld <matthew.auld@intel.com>
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++
> >   drivers/gpu/drm/i915/gem/i915_gem_object.c   | 58 ++++++++++++--------
> >   drivers/gpu/drm/i915/gem/i915_gem_object.h   |  3 +
> >   3 files changed, 62 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 1cc74844d3ea..d1921a317066 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -56,6 +56,8 @@
> >   #include "display/intel_tv.h"
> >   #include "display/intel_vdsc.h"
> > +#include "gem/i915_gem_object.h"
> > +
> >   #include "i915_drv.h"
> >   #include "i915_trace.h"
> >   #include "intel_acpi.h"
> > @@ -15496,6 +15498,10 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
> >   static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >   {
> >   	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > +	struct drm_i915_private *dev_priv = to_i915(fb->dev);
> > +
> > +	/* removing the FB memory region restriction on obj, if any */
> > +	intel_fb->front_buffer->obj = INTEL_INFO(dev_priv)->memory_regions;
> 
> Is this right, assigning bitmask to something called obj?
Oops. wanted to assign into the obj->memory_regions. Thanks for
catching it.

> 
> >   	drm_framebuffer_cleanup(fb);
> >   	intel_frontbuffer_put(intel_fb->frontbuffer);
> > @@ -15543,11 +15549,26 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >   {
> >   	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> >   	struct drm_framebuffer *fb = &intel_fb->base;
> > +	u32 *region_map;
> >   	u32 max_stride;
> >   	unsigned int tiling, stride;
> >   	int ret = -EINVAL;
> >   	int i;
> > +	/* GEM Obj for frame buffer is expected to be in LMEM. */
> > +	if (HAS_LMEM(dev_priv))
> > +		if (obj->mm.region->type != INTEL_LMEM) {
> > +			region_map = &intel_region_map[INTEL_MEMORY_LMEM];
> > +			ret = i915_gem_object_mem_region_migrate(dev_priv, obj,
> > +								 region_map,
> > +								 1);
> > +			if (ret) {
> > +				DRM_ERROR("FB migration to LMEM Failed(%d)\n",
> > +					  ret);
> 
> Probably should be just debug level since it is imaginably user triggerable
> and not really an error for the kernel as such.

Sure.
> 
> > +				return ret;
> > +			}
> > +		}
> > +
> >   	intel_fb->frontbuffer = intel_frontbuffer_get(obj);
> >   	if (!intel_fb->frontbuffer)
> >   		return -ENOMEM;
> > @@ -15666,6 +15687,10 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
> >   		goto err;
> >   	}
> > +	/* Blocking the migration of gem obj out of LMEM */
> > +	if (HAS_LMEM(dev_priv))
> > +		obj->memory_regions = REGION_LMEM;
> > +
> >   	return 0;
> >   err:
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > index e6f8426dedff..65b47054130b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -501,30 +501,11 @@ __region_id(u32 region)
> >   	return INTEL_MEMORY_UKNOWN;
> >   }
> > -static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
> > -					 struct drm_i915_gem_object_param *args,
> > -					 struct drm_file *file,
> > -					 struct drm_i915_gem_object *obj)
> > +int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
> > +				       struct drm_i915_gem_object *obj,
> > +				       u32 *uregions, u32 region_count)
> >   {
> >   	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
> > -	u32 __user *uregions = u64_to_user_ptr(args->data);
> > -	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
> > -	int i, ret;
> > -
> > -	if (args->size > INTEL_MEMORY_UKNOWN)
> > -		return -EINVAL;
> > -
> > -	memset(uregions_copy, 0, sizeof(uregions_copy));
> > -	for (i = 0; i < args->size; i++) {
> > -		u32 region;
> > -
> > -		ret = get_user(region, uregions);
> > -		if (ret)
> > -			return ret;
> > -
> > -		uregions_copy[i] = region;
> > -		++uregions;
> > -	}
> >   	mutex_lock(&dev_priv->drm.struct_mutex);
> >   	ret = i915_gem_object_prepare_move(obj);
> > @@ -533,8 +514,8 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
> >   	        goto err;
> >   	}
> > -	for (i = 0; i < args->size; i++) {
> > -		u32 region = uregions_copy[i];
> > +	for (i = 0; i < region_count; i++) {
> > +		u32 region = uregions[i];
> >   		enum intel_region_id id = __region_id(region);
> >   		if (!(obj->memory_region & region)) {
> > @@ -576,6 +557,35 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
> >   	return ret;
> >   }
> > +static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
> > +					 struct drm_i915_gem_object_param *args,
> > +					 struct drm_file *file,
> > +					 struct drm_i915_gem_object *obj)
> > +{
> > +	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
> > +	u32 __user *uregions = u64_to_user_ptr(args->data);
> > +	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
> > +	int i, ret;
> > +
> > +	if (args->size > INTEL_MEMORY_UKNOWN)
> > +		return -EINVAL;
> > +
> > +	memset(uregions_copy, 0, sizeof(uregions_copy));
> > +	for (i = 0; i < args->size; i++) {
> > +		u32 region;
> > +
> > +		ret = get_user(region, uregions);
> > +		if (ret)
> > +			return ret;
> > +
> > +		uregions_copy[i] = region;
> > +		++uregions;
> > +	}
> 
> I do understand this is old code (not yours) but I fail to penetrate why is
> this a loop and not copy_from_user?
What i understood is, list of possible regions_maps are passed here. and
they are reading it one after another.
> 
> > +
> > +	return i915_gem_object_mem_region_migrate(dev_priv, obj, uregions_copy,
> > +						  args->size);
> > +}
> > +
> >   int i915_gem_object_setparam_ioctl(struct drm_device *dev, void *data,
> >   				   struct drm_file *file)
> >   {
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > index a7c073aeb777..d09a9c741b41 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > @@ -47,6 +47,9 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
> >   			    enum intel_region_id id);
> >   void i915_gem_flush_free_objects(struct drm_i915_private *i915);
> > +int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
> > +				       struct drm_i915_gem_object *obj,
> > +				       u32 *uregions, u32 region_count);
> >   void __i915_gem_object_reset_page_iter(struct drm_i915_gem_object *obj);
> > 
> 
> General approach looks feasible to me, but I'll leave guys directly involved
> with local memory to call it.
> 
> Another my comment/question would be - do we have a hook when objects are
> released/disassociated from the framebuffer and should we restore the
> allowed memory region mask to default at that point?
at present I am doing this at intel_user_framebuffer_destroy. Not sure
if we have any better place for this.

-Ram
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Tvrtko Ursulin]

On 26/09/2019 10:14, Ramalingam C wrote:
> On 2019-09-26 at 09:53:03 +0100, Tvrtko Ursulin wrote:
>>
>> On 26/09/2019 06:21, Ramalingam C wrote:
>>> If Local memory is supported by hardware, we want framebuffer backing
>>> gem objects out of local memory.
>>>
>>> If local memory is supported and gem object if not from local memory we
>>> migrate the obj into local memory. And once framebuffer is created we
>>> block the migration of the associated object out of local memory.
>>>
>>> This is developed on top of v3 LMEM series
>>> https://patchwork.freedesktop.org/series/56683/
>>>
>>> cc: Matthew Auld <matthew.auld@intel.com>
>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++
>>>    drivers/gpu/drm/i915/gem/i915_gem_object.c   | 58 ++++++++++++--------
>>>    drivers/gpu/drm/i915/gem/i915_gem_object.h   |  3 +
>>>    3 files changed, 62 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 1cc74844d3ea..d1921a317066 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -56,6 +56,8 @@
>>>    #include "display/intel_tv.h"
>>>    #include "display/intel_vdsc.h"
>>> +#include "gem/i915_gem_object.h"
>>> +
>>>    #include "i915_drv.h"
>>>    #include "i915_trace.h"
>>>    #include "intel_acpi.h"
>>> @@ -15496,6 +15498,10 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
>>>    static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>>>    {
>>>    	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
>>> +	struct drm_i915_private *dev_priv = to_i915(fb->dev);
>>> +
>>> +	/* removing the FB memory region restriction on obj, if any */
>>> +	intel_fb->front_buffer->obj = INTEL_INFO(dev_priv)->memory_regions;
>>
>> Is this right, assigning bitmask to something called obj?
> Oops. wanted to assign into the obj->memory_regions. Thanks for
> catching it.
> 
>>
>>>    	drm_framebuffer_cleanup(fb);
>>>    	intel_frontbuffer_put(intel_fb->frontbuffer);
>>> @@ -15543,11 +15549,26 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>>>    {
>>>    	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>>>    	struct drm_framebuffer *fb = &intel_fb->base;
>>> +	u32 *region_map;
>>>    	u32 max_stride;
>>>    	unsigned int tiling, stride;
>>>    	int ret = -EINVAL;
>>>    	int i;
>>> +	/* GEM Obj for frame buffer is expected to be in LMEM. */
>>> +	if (HAS_LMEM(dev_priv))
>>> +		if (obj->mm.region->type != INTEL_LMEM) {
>>> +			region_map = &intel_region_map[INTEL_MEMORY_LMEM];
>>> +			ret = i915_gem_object_mem_region_migrate(dev_priv, obj,
>>> +								 region_map,
>>> +								 1);
>>> +			if (ret) {
>>> +				DRM_ERROR("FB migration to LMEM Failed(%d)\n",
>>> +					  ret);
>>
>> Probably should be just debug level since it is imaginably user triggerable
>> and not really an error for the kernel as such.
> 
> Sure.
>>
>>> +				return ret;
>>> +			}
>>> +		}
>>> +
>>>    	intel_fb->frontbuffer = intel_frontbuffer_get(obj);
>>>    	if (!intel_fb->frontbuffer)
>>>    		return -ENOMEM;
>>> @@ -15666,6 +15687,10 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>>>    		goto err;
>>>    	}
>>> +	/* Blocking the migration of gem obj out of LMEM */
>>> +	if (HAS_LMEM(dev_priv))
>>> +		obj->memory_regions = REGION_LMEM;
>>> +
>>>    	return 0;
>>>    err:
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>> index e6f8426dedff..65b47054130b 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>>> @@ -501,30 +501,11 @@ __region_id(u32 region)
>>>    	return INTEL_MEMORY_UKNOWN;
>>>    }
>>> -static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
>>> -					 struct drm_i915_gem_object_param *args,
>>> -					 struct drm_file *file,
>>> -					 struct drm_i915_gem_object *obj)
>>> +int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
>>> +				       struct drm_i915_gem_object *obj,
>>> +				       u32 *uregions, u32 region_count)
>>>    {
>>>    	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
>>> -	u32 __user *uregions = u64_to_user_ptr(args->data);
>>> -	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
>>> -	int i, ret;
>>> -
>>> -	if (args->size > INTEL_MEMORY_UKNOWN)
>>> -		return -EINVAL;
>>> -
>>> -	memset(uregions_copy, 0, sizeof(uregions_copy));
>>> -	for (i = 0; i < args->size; i++) {
>>> -		u32 region;
>>> -
>>> -		ret = get_user(region, uregions);
>>> -		if (ret)
>>> -			return ret;
>>> -
>>> -		uregions_copy[i] = region;
>>> -		++uregions;
>>> -	}
>>>    	mutex_lock(&dev_priv->drm.struct_mutex);
>>>    	ret = i915_gem_object_prepare_move(obj);
>>> @@ -533,8 +514,8 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
>>>    	        goto err;
>>>    	}
>>> -	for (i = 0; i < args->size; i++) {
>>> -		u32 region = uregions_copy[i];
>>> +	for (i = 0; i < region_count; i++) {
>>> +		u32 region = uregions[i];
>>>    		enum intel_region_id id = __region_id(region);
>>>    		if (!(obj->memory_region & region)) {
>>> @@ -576,6 +557,35 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
>>>    	return ret;
>>>    }
>>> +static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
>>> +					 struct drm_i915_gem_object_param *args,
>>> +					 struct drm_file *file,
>>> +					 struct drm_i915_gem_object *obj)
>>> +{
>>> +	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
>>> +	u32 __user *uregions = u64_to_user_ptr(args->data);
>>> +	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
>>> +	int i, ret;
>>> +
>>> +	if (args->size > INTEL_MEMORY_UKNOWN)
>>> +		return -EINVAL;
>>> +
>>> +	memset(uregions_copy, 0, sizeof(uregions_copy));
>>> +	for (i = 0; i < args->size; i++) {
>>> +		u32 region;
>>> +
>>> +		ret = get_user(region, uregions);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		uregions_copy[i] = region;
>>> +		++uregions;
>>> +	}
>>
>> I do understand this is old code (not yours) but I fail to penetrate why is
>> this a loop and not copy_from_user?
> What i understood is, list of possible regions_maps are passed here. and
> they are reading it one after another.

Yes, and making a kernel copy. AFAICS i == uregions at the end of the 
loop and args->size worth of u32 has been copied. Equivalent to 
copy_from_user(uregions_copy, uregions, args->size * sizeof(u32)). Maybe 
I am blind yet again..

>>
>>> +
>>> +	return i915_gem_object_mem_region_migrate(dev_priv, obj, uregions_copy,
>>> +						  args->size);
>>> +}
>>> +
>>>    int i915_gem_object_setparam_ioctl(struct drm_device *dev, void *data,
>>>    				   struct drm_file *file)
>>>    {
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> index a7c073aeb777..d09a9c741b41 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
>>> @@ -47,6 +47,9 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
>>>    			    enum intel_region_id id);
>>>    void i915_gem_flush_free_objects(struct drm_i915_private *i915);
>>> +int i915_gem_object_mem_region_migrate(struct drm_i915_private *dev_priv,
>>> +				       struct drm_i915_gem_object *obj,
>>> +				       u32 *uregions, u32 region_count);
>>>    void __i915_gem_object_reset_page_iter(struct drm_i915_gem_object *obj);
>>>
>>
>> General approach looks feasible to me, but I'll leave guys directly involved
>> with local memory to call it.
>>
>> Another my comment/question would be - do we have a hook when objects are
>> released/disassociated from the framebuffer and should we restore the
>> allowed memory region mask to default at that point?
> at present I am doing this at intel_user_framebuffer_destroy. Not sure
> if we have any better place for this.

Oh it's at the top of the patch. My bad for forgetting about it. The 
region copying loop caused some head scratching and I forgot. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/3] drm/i915: FB backing gem obj should reside in LMEM

[Ramalingam C]

If Local memory is supported by hardware, we want framebuffer backing
gem objects out of local memory.

If local memory is supported and gem object if not from local memory we
migrate the obj into local memory. And once framebuffer is created we
block the migration of the associated object out of local memory.

This is developed on top of v3 LMEM series
https://patchwork.freedesktop.org/series/56683/

v2:
  memory regions are correctly assigned to obj->memory_regions [tvrtko]
  migration failure is reported as debug log [Tvrtko]

cc: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 25 +++++++++
 drivers/gpu/drm/i915/gem/i915_gem_object.c   | 58 ++++++++++++--------
 drivers/gpu/drm/i915/gem/i915_gem_object.h   |  2 +
 3 files changed, 61 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 1cc74844d3ea..175c01e24c78 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -56,6 +56,8 @@
 #include "display/intel_tv.h"
 #include "display/intel_vdsc.h"
 
+#include "gem/i915_gem_object.h"
+
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include "intel_acpi.h"
@@ -15496,6 +15498,11 @@ static void intel_setup_outputs(struct drm_i915_private *dev_priv)
 static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 {
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_private *dev_priv = to_i915(fb->dev);
+
+	/* removing the FB memory region restriction on obj, if any */
+	intel_fb->front_buffer->obj->memory_regions =
+				INTEL_INFO(dev_priv)->memory_regions;
 
 	drm_framebuffer_cleanup(fb);
 	intel_frontbuffer_put(intel_fb->frontbuffer);
@@ -15543,11 +15550,25 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 {
 	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
 	struct drm_framebuffer *fb = &intel_fb->base;
+	u32 *region_map;
 	u32 max_stride;
 	unsigned int tiling, stride;
 	int ret = -EINVAL;
 	int i;
 
+	/* GEM Obj for frame buffer is expected to be in LMEM. */
+	if (HAS_LMEM(dev_priv))
+		if (obj->mm.region->type != INTEL_LMEM) {
+			region_map = &intel_region_map[INTEL_MEMORY_LMEM];
+			ret = i915_gem_object_mem_region_migrate(obj,
+								 region_map, 1);
+			if (ret) {
+				DRM_DEBUG_KMS("Migration to LMEM Failed(%d)\n",
+					      ret);
+				return ret;
+			}
+		}
+
 	intel_fb->frontbuffer = intel_frontbuffer_get(obj);
 	if (!intel_fb->frontbuffer)
 		return -ENOMEM;
@@ -15666,6 +15687,10 @@ static int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
 		goto err;
 	}
 
+	/* Blocking the migration of gem obj out of LMEM */
+	if (HAS_LMEM(dev_priv))
+		obj->memory_regions = REGION_LMEM;
+
 	return 0;
 
 err:
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 165ae03c6774..b2e692cb18f1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -502,30 +502,11 @@ __region_id(u32 region)
 	return INTEL_MEMORY_UKNOWN;
 }
 
-static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
-					 struct drm_i915_gem_object_param *args,
-					 struct drm_file *file,
-					 struct drm_i915_gem_object *obj)
+int i915_gem_object_mem_region_migrate(struct drm_i915_gem_object *obj,
+				       u32 *uregions, u32 region_count)
 {
+	struct drm_i915_file_private *dev_priv = to_i915(obj->base.drm);
 	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
-	u32 __user *uregions = u64_to_user_ptr(args->data);
-	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
-	int i, ret;
-
-	if (args->size > INTEL_MEMORY_UKNOWN)
-		return -EINVAL;
-
-	memset(uregions_copy, 0, sizeof(uregions_copy));
-	for (i = 0; i < args->size; i++) {
-		u32 region;
-
-		ret = get_user(region, uregions);
-		if (ret)
-			return ret;
-
-		uregions_copy[i] = region;
-		++uregions;
-	}
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	ret = i915_gem_object_prepare_move(obj);
@@ -534,8 +515,8 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
 	        goto err;
 	}
 
-	for (i = 0; i < args->size; i++) {
-		u32 region = uregions_copy[i];
+	for (i = 0; i < region_count; i++) {
+		u32 region = uregions[i];
 		enum intel_region_id id = __region_id(region);
 
 		if (!(obj->memory_regions & region)) {
@@ -577,6 +558,35 @@ static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
+static int i915_gem_object_region_select(struct drm_i915_private *dev_priv,
+					 struct drm_i915_gem_object_param *args,
+					 struct drm_file *file,
+					 struct drm_i915_gem_object *obj)
+{
+	struct intel_context *ce = dev_priv->engine[BCS0]->kernel_context;
+	u32 __user *uregions = u64_to_user_ptr(args->data);
+	u32 uregions_copy[INTEL_MEMORY_UKNOWN];
+	int i, ret;
+
+	if (args->size > INTEL_MEMORY_UKNOWN)
+		return -EINVAL;
+
+	memset(uregions_copy, 0, sizeof(uregions_copy));
+	for (i = 0; i < args->size; i++) {
+		u32 region;
+
+		ret = get_user(region, uregions);
+		if (ret)
+			return ret;
+
+		uregions_copy[i] = region;
+		++uregions;
+	}
+
+	return i915_gem_object_mem_region_migrate(obj, uregions_copy,
+						  args->size);
+}
+
 int i915_gem_object_setparam_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file)
 {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index a7c073aeb777..996646001d80 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -47,6 +47,8 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
 			    enum intel_region_id id);
 
 void i915_gem_flush_free_objects(struct drm_i915_private *i915);
+int i915_gem_object_mem_region_migrate(struct drm_i915_gem_object *obj,
+				       u32 *uregions, u32 region_count);
 
 void __i915_gem_object_reset_page_iter(struct drm_i915_gem_object *obj);
 
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Create dumb buffer from LMEM

[Tvrtko Ursulin]

On 26/09/2019 06:21, Ramalingam C wrote:
> When LMEM is supported, dumb buffer preferred to be created from LMEM.
> 
> This is developed on top of v3 LMEM series
> https://patchwork.freedesktop.org/series/56683/.
> 
> v2:
>    Parameters are reshuffled. [Chris]
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> cc: Matthew Auld <matthew.auld@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e458507b1558..6810a549ee98 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -44,6 +44,7 @@
>   #include "gem/i915_gem_clflush.h"
>   #include "gem/i915_gem_context.h"
>   #include "gem/i915_gem_ioctls.h"
> +#include "gem/i915_gem_lmem.h"
>   #include "gem/i915_gem_pm.h"
>   #include "gt/intel_engine_user.h"
>   #include "gt/intel_gt.h"
> @@ -160,6 +161,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
>   static int
>   i915_gem_create(struct drm_file *file,
>   		struct drm_i915_private *dev_priv,
> +		enum intel_region_id mem_region,
>   		u64 *size_p,
>   		u32 *handle_p)
>   {
> @@ -173,7 +175,12 @@ i915_gem_create(struct drm_file *file,
>   		return -EINVAL;
>   
>   	/* Allocate the new object */
> -	obj = i915_gem_object_create_shmem(dev_priv, size);
> +	if (mem_region == INTEL_MEMORY_LMEM)
> +		obj = i915_gem_object_create_lmem(dev_priv, size, 0);
> +	else if (mem_region == INTEL_MEMORY_STOLEN)
> +		obj = i915_gem_object_create_stolen(dev_priv, size);
> +	else
> +		obj = i915_gem_object_create_shmem(dev_priv, size);
>   	if (IS_ERR(obj))
>   		return PTR_ERR(obj);
>   
> @@ -193,6 +200,7 @@ i915_gem_dumb_create(struct drm_file *file,
>   		     struct drm_device *dev,
>   		     struct drm_mode_create_dumb *args)
>   {
> +	enum intel_region_id mem_region = INTEL_MEMORY_UKNOWN;
>   	int cpp = DIV_ROUND_UP(args->bpp, 8);
>   	u32 format;
>   
> @@ -219,7 +227,11 @@ i915_gem_dumb_create(struct drm_file *file,
>   		args->pitch = ALIGN(args->pitch, 4096);
>   
>   	args->size = args->pitch * args->height;
> -	return i915_gem_create(file, to_i915(dev),
> +
> +	if (HAS_LMEM(to_i915(dev)))
> +		mem_region = INTEL_MEMORY_LMEM;
> +
> +	return i915_gem_create(file, to_i915(dev), mem_region,
>   			       &args->size, &args->handle);
>   }
>   
> @@ -238,7 +250,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
>   
>   	i915_gem_flush_free_objects(dev_priv);
>   
> -	return i915_gem_create(file, dev_priv,
> +	return i915_gem_create(file, dev_priv, INTEL_MEMORY_UKNOWN,
>   			       &args->size, &args->handle);

We don't have shmem memory region? Or default? Or is unknown supposed to 
mean default for a given platform?

Regards,

Tvrtko

>   }
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Create dumb buffer from LMEM

[Ramalingam C]

On 2019-09-26 at 09:55:16 +0100, Tvrtko Ursulin wrote:
> 
> On 26/09/2019 06:21, Ramalingam C wrote:
> > When LMEM is supported, dumb buffer preferred to be created from LMEM.
> > 
> > This is developed on top of v3 LMEM series
> > https://patchwork.freedesktop.org/series/56683/.
> > 
> > v2:
> >    Parameters are reshuffled. [Chris]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > cc: Matthew Auld <matthew.auld@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c | 18 +++++++++++++++---
> >   1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index e458507b1558..6810a549ee98 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -44,6 +44,7 @@
> >   #include "gem/i915_gem_clflush.h"
> >   #include "gem/i915_gem_context.h"
> >   #include "gem/i915_gem_ioctls.h"
> > +#include "gem/i915_gem_lmem.h"
> >   #include "gem/i915_gem_pm.h"
> >   #include "gt/intel_engine_user.h"
> >   #include "gt/intel_gt.h"
> > @@ -160,6 +161,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
> >   static int
> >   i915_gem_create(struct drm_file *file,
> >   		struct drm_i915_private *dev_priv,
> > +		enum intel_region_id mem_region,
> >   		u64 *size_p,
> >   		u32 *handle_p)
> >   {
> > @@ -173,7 +175,12 @@ i915_gem_create(struct drm_file *file,
> >   		return -EINVAL;
> >   	/* Allocate the new object */
> > -	obj = i915_gem_object_create_shmem(dev_priv, size);
> > +	if (mem_region == INTEL_MEMORY_LMEM)
> > +		obj = i915_gem_object_create_lmem(dev_priv, size, 0);
> > +	else if (mem_region == INTEL_MEMORY_STOLEN)
> > +		obj = i915_gem_object_create_stolen(dev_priv, size);
> > +	else
> > +		obj = i915_gem_object_create_shmem(dev_priv, size);
> >   	if (IS_ERR(obj))
> >   		return PTR_ERR(obj);
> > @@ -193,6 +200,7 @@ i915_gem_dumb_create(struct drm_file *file,
> >   		     struct drm_device *dev,
> >   		     struct drm_mode_create_dumb *args)
> >   {
> > +	enum intel_region_id mem_region = INTEL_MEMORY_UKNOWN;
> >   	int cpp = DIV_ROUND_UP(args->bpp, 8);
> >   	u32 format;
> > @@ -219,7 +227,11 @@ i915_gem_dumb_create(struct drm_file *file,
> >   		args->pitch = ALIGN(args->pitch, 4096);
> >   	args->size = args->pitch * args->height;
> > -	return i915_gem_create(file, to_i915(dev),
> > +
> > +	if (HAS_LMEM(to_i915(dev)))
> > +		mem_region = INTEL_MEMORY_LMEM;
> > +
> > +	return i915_gem_create(file, to_i915(dev), mem_region,
> >   			       &args->size, &args->handle);
> >   }
> > @@ -238,7 +250,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
> >   	i915_gem_flush_free_objects(dev_priv);
> > -	return i915_gem_create(file, dev_priv,
> > +	return i915_gem_create(file, dev_priv, INTEL_MEMORY_UKNOWN,
> >   			       &args->size, &args->handle);
> 
> We don't have shmem memory region? Or default? Or is unknown supposed to
> mean default for a given platform?
I take this as default. Not sure if we need to pass anything (default
region) specifically.

-Ram

> 
> Regards,
> 
> Tvrtko
> 
> >   }
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BUILD: failure for series starting with [1/3] drm/i915: Create dumb buffer from LMEM (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Create dumb buffer from LMEM (rev3)
URL   : https://patchwork.freedesktop.org/series/67259/
State : failure

== Summary ==

Applying: drm/i915: Create dumb buffer from LMEM
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/i915_gem.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915: Create dumb buffer from LMEM
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx