[PATCH 00/12] drm/i915: (stolen) memory region related fixes

[PATCH 00/12] drm/i915: (stolen) memory region related fixes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Attempt to fix the mess around stolen memory, especially on MTL
with it's special (and apparenly broken) not-actually-lmem stolen.

The series is made up of roughtly three parts:
1. General refactoring/debug improvement for mem regions
2. Deal with the broken BAR stuff on MTL
3. Fix initial display plane readout for MTL

Ville Syrjälä (12):
  drm/i915: Use struct resource for memory region IO as well
  drm/i915: Print memory region info during probe
  drm/i915: Remove ad-hoc lmem/stolen debugs
  drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access
  drm/i915: Disable the "binder"
  drm/i915: Rename the DSM/GSM registers
  drm/i915: Fix PTE decode during initial plane readout
  drm/i915: Fix region start during initial plane readout
  drm/i915: Fix MTL initial plane readout
  drm/i915: s/phys_base/dma_addr/
  drm/i915: Split the smem and lmem plane readout apart
  drm/i915: Simplify intel_initial_plane_config() calling convention

 .../drm/i915/display/intel_display_driver.c   |   7 +-
 .../drm/i915/display/intel_display_types.h    |   2 +
 drivers/gpu/drm/i915/display/intel_fbdev_fb.c |   2 +-
 .../drm/i915/display/intel_plane_initial.c    | 169 ++++++++++++------
 .../drm/i915/display/intel_plane_initial.h    |   4 +-
 drivers/gpu/drm/i915/gem/i915_gem_region.c    |   2 +-
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c    |  30 ++--
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c       |   8 +-
 .../drm/i915/gem/selftests/i915_gem_mman.c    |  18 +-
 drivers/gpu/drm/i915/gt/intel_ggtt.c          |  13 +-
 drivers/gpu/drm/i915/gt/intel_gtt.c           |   2 +-
 drivers/gpu/drm/i915/gt/intel_region_lmem.c   |  14 +-
 drivers/gpu/drm/i915/gt/selftest_tlb.c        |   4 +-
 drivers/gpu/drm/i915/i915_gpu_error.c         |   2 +-
 drivers/gpu/drm/i915/i915_query.c             |   2 +-
 drivers/gpu/drm/i915/i915_reg.h               |   7 +-
 drivers/gpu/drm/i915/intel_memory_region.c    |  33 +++-
 drivers/gpu/drm/i915/intel_memory_region.h    |   3 +-
 drivers/gpu/drm/i915/intel_region_ttm.c       |   8 +-
 .../drm/i915/selftests/intel_memory_region.c  |   4 +-
 20 files changed, 209 insertions(+), 125 deletions(-)

-- 
2.41.0

[PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

mem->region is a struct resource, but mem->io_start and
mem->io_size are not for whatever reason. Let's unify this
and convert the io stuff into a struct resource as well.
Should make life a little less annoying when you don't have
juggle between two different approaches all the time.

Mostly done using cocci (with manual tweaks at all the
places where we mutate io_size by hand):
@@
struct intel_memory_region *M;
expression START, SIZE;
@@
- M->io_start = START;
- M->io_size = SIZE;
+ M->io = DEFINE_RES_MEM(START, SIZE);

@@
struct intel_memory_region *M;
@@
- M->io_start
+ M->io.start

@@
struct intel_memory_region M;
@@
- M.io_start
+ M.io.start

@@
expression M;
@@
- M->io_size
+ resource_size(&M->io)

@@
expression M;
@@
- M.io_size
+ resource_size(&M.io)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbdev_fb.c  |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_region.c     |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c     | 17 +++++++++--------
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c        |  8 ++++----
 .../gpu/drm/i915/gem/selftests/i915_gem_mman.c | 18 +++++++++---------
 drivers/gpu/drm/i915/gt/intel_region_lmem.c    | 11 +++--------
 drivers/gpu/drm/i915/gt/selftest_tlb.c         |  4 ++--
 drivers/gpu/drm/i915/i915_gpu_error.c          |  2 +-
 drivers/gpu/drm/i915/i915_query.c              |  2 +-
 drivers/gpu/drm/i915/intel_memory_region.c     | 15 +++++++--------
 drivers/gpu/drm/i915/intel_memory_region.h     |  3 +--
 drivers/gpu/drm/i915/intel_region_ttm.c        |  8 ++++----
 .../drm/i915/selftests/intel_memory_region.c   |  4 ++--
 13 files changed, 45 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
index 717c3a3237c4..1ac05d90b2e8 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
@@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, struct fb_info *info
 
 		/* Use fbdev's framebuffer from lmem for discrete */
 		info->fix.smem_start =
-			(unsigned long)(mem->io_start +
+			(unsigned long)(mem->io.start +
 					i915_gem_object_get_dma_address(obj, 0));
 		info->fix.smem_len = obj->base.size;
 	} else {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c
index a4fb577eceb4..b09b74a2448b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
@@ -129,7 +129,7 @@ i915_gem_object_create_region_at(struct intel_memory_region *mem,
 		return ERR_PTR(-EINVAL);
 
 	if (!(flags & I915_BO_ALLOC_GPU_ONLY) &&
-	    offset + size > mem->io_size &&
+	    offset + size > resource_size(&mem->io) &&
 	    !i915_ggtt_has_aperture(to_gt(mem->i915)->ggtt))
 		return ERR_PTR(-ENOSPC);
 
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index 8c88075eeab2..d2440c793f84 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -541,7 +541,9 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
 
 	/* Exclude the reserved region from driver use */
 	mem->region.end = i915->dsm.reserved.start - 1;
-	mem->io_size = min(mem->io_size, resource_size(&mem->region));
+	mem->io = DEFINE_RES_MEM(mem->io.start,
+				 min(resource_size(&mem->io),
+				     resource_size(&mem->region)));
 
 	i915->dsm.usable_size = resource_size(&mem->region);
 
@@ -752,7 +754,7 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem,
 	 * With discrete devices, where we lack a mappable aperture there is no
 	 * possible way to ever access this memory on the CPU side.
 	 */
-	if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !mem->io_size &&
+	if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !resource_size(&mem->io) &&
 	    !(flags & I915_BO_ALLOC_GPU_ONLY))
 		return -ENOSPC;
 
@@ -838,13 +840,12 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
 		return 0;
 	}
 
-	if (mem->io_size &&
-	    !io_mapping_init_wc(&mem->iomap, mem->io_start, mem->io_size))
+	if (resource_size(&mem->io) &&
+	    !io_mapping_init_wc(&mem->iomap, mem->io.start, resource_size(&mem->io)))
 		goto err_cleanup;
 
-	drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n",
-		&mem->io_start);
-	drm_dbg(&i915->drm, "Stolen Local DSM base: %pa\n", &mem->region.start);
+	drm_dbg(&i915->drm, "Stolen Local DSM: %pR\n", &mem->region);
+	drm_dbg(&i915->drm, "Stolen Local memory IO: %pR\n", &mem->io);
 
 	return 0;
 
@@ -855,7 +856,7 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
 
 static int release_stolen_lmem(struct intel_memory_region *mem)
 {
-	if (mem->io_size)
+	if (resource_size(&mem->io))
 		io_mapping_fini(&mem->iomap);
 	i915_gem_cleanup_stolen(mem->i915);
 	return 0;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 9227f8146a58..42cc69a0a5fe 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -144,13 +144,13 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr,
 		place->fpfn = offset >> PAGE_SHIFT;
 		WARN_ON(overflows_type(place->fpfn + (size >> PAGE_SHIFT), place->lpfn));
 		place->lpfn = place->fpfn + (size >> PAGE_SHIFT);
-	} else if (mr->io_size && mr->io_size < mr->total) {
+	} else if (resource_size(&mr->io) && resource_size(&mr->io) < mr->total) {
 		if (flags & I915_BO_ALLOC_GPU_ONLY) {
 			place->flags |= TTM_PL_FLAG_TOPDOWN;
 		} else {
 			place->fpfn = 0;
-			WARN_ON(overflows_type(mr->io_size >> PAGE_SHIFT, place->lpfn));
-			place->lpfn = mr->io_size >> PAGE_SHIFT;
+			WARN_ON(overflows_type(resource_size(&mr->io) >> PAGE_SHIFT, place->lpfn));
+			place->lpfn = resource_size(&mr->io) >> PAGE_SHIFT;
 		}
 	}
 }
@@ -1090,7 +1090,7 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
 			struct intel_memory_region *mr = obj->mm.placements[i];
 			unsigned int flags;
 
-			if (!mr->io_size && mr->type != INTEL_MEMORY_SYSTEM)
+			if (!resource_size(&mr->io) && mr->type != INTEL_MEMORY_SYSTEM)
 				continue;
 
 			flags = obj->flags;
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 2c51a2c452fc..99a9ade73956 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -1054,7 +1054,7 @@ static int igt_fill_mappable(struct intel_memory_region *mr,
 	int err;
 
 	total = 0;
-	size = mr->io_size;
+	size = resource_size(&mr->io);
 	do {
 		struct drm_i915_gem_object *obj;
 
@@ -1315,28 +1315,28 @@ static int igt_mmap_migrate(void *arg)
 		struct intel_memory_region *mixed[] = { mr, system };
 		struct intel_memory_region *single[] = { mr };
 		struct ttm_resource_manager *man = mr->region_private;
-		resource_size_t saved_io_size;
+		struct resource saved_io;
 		int err;
 
 		if (mr->private)
 			continue;
 
-		if (!mr->io_size)
+		if (!resource_size(&mr->io))
 			continue;
 
 		/*
 		 * For testing purposes let's force small BAR, if not already
 		 * present.
 		 */
-		saved_io_size = mr->io_size;
-		if (mr->io_size == mr->total) {
-			resource_size_t io_size = mr->io_size;
+		saved_io = mr->io;
+		if (resource_size(&mr->io) == mr->total) {
+			resource_size_t io_size = resource_size(&mr->io);
 
 			io_size = rounddown_pow_of_two(io_size >> 1);
 			if (io_size < PAGE_SIZE)
 				continue;
 
-			mr->io_size = io_size;
+			mr->io = DEFINE_RES_MEM(mr->io.start, io_size);
 			i915_ttm_buddy_man_force_visible_size(man,
 							      io_size >> PAGE_SHIFT);
 		}
@@ -1396,9 +1396,9 @@ static int igt_mmap_migrate(void *arg)
 					 IGT_MMAP_MIGRATE_FAIL_GPU |
 					 IGT_MMAP_MIGRATE_UNFAULTABLE);
 out_io_size:
-		mr->io_size = saved_io_size;
+		mr->io = saved_io;
 		i915_ttm_buddy_man_force_visible_size(man,
-						      mr->io_size >> PAGE_SHIFT);
+						      resource_size(&mr->io) >> PAGE_SHIFT);
 		if (err)
 			return err;
 	}
diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
index f8512aee58a8..6f96a6b70601 100644
--- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
+++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
@@ -144,8 +144,8 @@ region_lmem_init(struct intel_memory_region *mem)
 	int ret;
 
 	if (!io_mapping_init_wc(&mem->iomap,
-				mem->io_start,
-				mem->io_size))
+				mem->io.start,
+				resource_size(&mem->io)))
 		return -EIO;
 
 	ret = intel_region_ttm_init(mem);
@@ -274,12 +274,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)
 		goto err_region_put;
 
 	drm_dbg(&i915->drm, "Local memory: %pR\n", &mem->region);
-	drm_dbg(&i915->drm, "Local memory IO start: %pa\n",
-		&mem->io_start);
-	drm_info(&i915->drm, "Local memory IO size: %pa\n",
-		 &mem->io_size);
-	drm_info(&i915->drm, "Local memory available: %pa\n",
-		 &lmem_size);
+	drm_dbg(&i915->drm, "Local memory IO: %pR\n", &mem->io);
 
 	if (io_size < lmem_size)
 		drm_info(&i915->drm, "Using a reduced BAR size of %lluMiB. Consider enabling 'Resizable BAR' or similar, if available in the BIOS.\n",
diff --git a/drivers/gpu/drm/i915/gt/selftest_tlb.c b/drivers/gpu/drm/i915/gt/selftest_tlb.c
index 00b872b6380b..3941f2d6fa47 100644
--- a/drivers/gpu/drm/i915/gt/selftest_tlb.c
+++ b/drivers/gpu/drm/i915/gt/selftest_tlb.c
@@ -206,8 +206,8 @@ static struct drm_i915_gem_object *create_lmem(struct intel_gt *gt)
 	 * of pages. To succeed with both allocations, especially in case of Small
 	 * BAR, try to allocate no more than quarter of mappable memory.
 	 */
-	if (mr && size > mr->io_size / 4)
-		size = mr->io_size / 4;
+	if (mr && size > resource_size(&mr->io) / 4)
+		size = resource_size(&mr->io) / 4;
 
 	return i915_gem_object_create_lmem(gt->i915, size, I915_BO_ALLOC_CONTIGUOUS);
 }
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index d04660b60046..a0b784ebaddd 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1157,7 +1157,7 @@ i915_vma_coredump_create(const struct intel_gt *gt,
 			dma_addr_t offset = dma - mem->region.start;
 			void __iomem *s;
 
-			if (offset + PAGE_SIZE > mem->io_size) {
+			if (offset + PAGE_SIZE > resource_size(&mem->io)) {
 				ret = -EINVAL;
 				break;
 			}
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 00871ef99792..fa3e937ed3f5 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -502,7 +502,7 @@ static int query_memregion_info(struct drm_i915_private *i915,
 		info.probed_size = mr->total;
 
 		if (mr->type == INTEL_MEMORY_LOCAL)
-			info.probed_cpu_visible_size = mr->io_size;
+			info.probed_cpu_visible_size = resource_size(&mr->io);
 		else
 			info.probed_cpu_visible_size = mr->total;
 
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index 60a03340bbd4..b2708f8cac2a 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -50,7 +50,7 @@ static int __iopagetest(struct intel_memory_region *mem,
 	if (memchr_inv(result, value, sizeof(result))) {
 		dev_err(mem->i915->drm.dev,
 			"Failed to read back from memory region:%pR at [%pa + %pa] for %ps; wrote %x, read (%x, %x, %x)\n",
-			&mem->region, &mem->io_start, &offset, caller,
+			&mem->region, &mem->io.start, &offset, caller,
 			value, result[0], result[1], result[2]);
 		return -EINVAL;
 	}
@@ -67,11 +67,11 @@ static int iopagetest(struct intel_memory_region *mem,
 	int err;
 	int i;
 
-	va = ioremap_wc(mem->io_start + offset, PAGE_SIZE);
+	va = ioremap_wc(mem->io.start + offset, PAGE_SIZE);
 	if (!va) {
 		dev_err(mem->i915->drm.dev,
 			"Failed to ioremap memory region [%pa + %pa] for %ps\n",
-			&mem->io_start, &offset, caller);
+			&mem->io.start, &offset, caller);
 		return -EFAULT;
 	}
 
@@ -102,10 +102,10 @@ static int iomemtest(struct intel_memory_region *mem,
 	resource_size_t last, page;
 	int err;
 
-	if (mem->io_size < PAGE_SIZE)
+	if (resource_size(&mem->io) < PAGE_SIZE)
 		return 0;
 
-	last = mem->io_size - PAGE_SIZE;
+	last = resource_size(&mem->io) - PAGE_SIZE;
 
 	/*
 	 * Quick test to check read/write access to the iomap (backing store).
@@ -207,7 +207,7 @@ static int intel_memory_region_memtest(struct intel_memory_region *mem,
 	struct drm_i915_private *i915 = mem->i915;
 	int err = 0;
 
-	if (!mem->io_start)
+	if (!mem->io.start)
 		return 0;
 
 	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) || i915->params.memtest)
@@ -252,8 +252,7 @@ intel_memory_region_create(struct drm_i915_private *i915,
 
 	mem->i915 = i915;
 	mem->region = DEFINE_RES_MEM(start, size);
-	mem->io_start = io_start;
-	mem->io_size = io_size;
+	mem->io = DEFINE_RES_MEM(io_start, io_size);
 	mem->min_page_size = min_page_size;
 	mem->ops = ops;
 	mem->total = size;
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
index 9ba36454e51b..40810cfb3fd9 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.h
+++ b/drivers/gpu/drm/i915/intel_memory_region.h
@@ -71,8 +71,7 @@ struct intel_memory_region {
 	struct io_mapping iomap;
 	struct resource region;
 
-	resource_size_t io_start;
-	resource_size_t io_size;
+	struct resource io;
 	resource_size_t min_page_size;
 	resource_size_t total;
 
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
index bf6097e7433d..04525d92bec5 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.c
+++ b/drivers/gpu/drm/i915/intel_region_ttm.c
@@ -87,7 +87,7 @@ int intel_region_ttm_init(struct intel_memory_region *mem)
 
 	ret = i915_ttm_buddy_man_init(bdev, mem_type, false,
 				      resource_size(&mem->region),
-				      mem->io_size,
+				      resource_size(&mem->io),
 				      mem->min_page_size, PAGE_SIZE);
 	if (ret)
 		return ret;
@@ -219,16 +219,16 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
 			goto out;
 		}
 		place.lpfn = place.fpfn + (size >> PAGE_SHIFT);
-	} else if (mem->io_size && mem->io_size < mem->total) {
+	} else if (resource_size(&mem->io) && resource_size(&mem->io) < mem->total) {
 		if (flags & I915_BO_ALLOC_GPU_ONLY) {
 			place.flags |= TTM_PL_FLAG_TOPDOWN;
 		} else {
 			place.fpfn = 0;
-			if (WARN_ON(overflows_type(mem->io_size >> PAGE_SHIFT, place.lpfn))) {
+			if (WARN_ON(overflows_type(resource_size(&mem->io) >> PAGE_SHIFT, place.lpfn))) {
 				ret = -E2BIG;
 				goto out;
 			}
-			place.lpfn = mem->io_size >> PAGE_SHIFT;
+			place.lpfn = resource_size(&mem->io) >> PAGE_SHIFT;
 		}
 	}
 
diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
index d985d9bae2e8..ae6070b5bf07 100644
--- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
@@ -544,8 +544,8 @@ static u64 igt_object_mappable_total(struct drm_i915_gem_object *obj)
 		u64 start = drm_buddy_block_offset(block);
 		u64 end = start + drm_buddy_block_size(mm, block);
 
-		if (start < mr->io_size)
-			total += min_t(u64, end, mr->io_size) - start;
+		if (start < resource_size(&mr->io))
+			total += min_t(u64, end, resource_size(&mr->io)) - start;
 	}
 
 	return total;
-- 
2.41.0

Re: [PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

[Ville Syrjälä]

On Wed, Dec 13, 2023 at 02:42:26AM +0200, Ville Syrjala wrote:
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> index 717c3a3237c4..1ac05d90b2e8 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> @@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, struct fb_info *info
>  
>  		/* Use fbdev's framebuffer from lmem for discrete */
>  		info->fix.smem_start =
> -			(unsigned long)(mem->io_start +
> +			(unsigned long)(mem->io.start +
>  					i915_gem_object_get_dma_address(obj, 0));

Hmm. That looks wrong for MTL actually since dma address is relative
to the start of LMEMBAR but stolen io.start will be LMEMBAR+8MiB (or
just DSMBASE which points to the same place, with the w/a in place).
So we need to subtract mem->region.start from this to get the correct
value.

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

[Ville Syrjälä]

On Wed, Dec 13, 2023 at 02:59:07AM +0200, Ville Syrjälä wrote:
> On Wed, Dec 13, 2023 at 02:42:26AM +0200, Ville Syrjala wrote:
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > index 717c3a3237c4..1ac05d90b2e8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > @@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, struct fb_info *info
> >  
> >  		/* Use fbdev's framebuffer from lmem for discrete */
> >  		info->fix.smem_start =
> > -			(unsigned long)(mem->io_start +
> > +			(unsigned long)(mem->io.start +
> >  					i915_gem_object_get_dma_address(obj, 0));
> 
> Hmm. That looks wrong for MTL actually since dma address is relative
> to the start of LMEMBAR but stolen io.start will be LMEMBAR+8MiB (or
> just DSMBASE which points to the same place, with the w/a in place).
> So we need to subtract mem->region.start from this to get the correct
> value.

I suppose this doesn't matter anymore as we have our own .fb_mmap()
these days. So presumably we could just always leave this set to zero.

-- 
Ville Syrjälä
Intel

Re: [PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

[Andrzej Hajda]

On 13.12.2023 01:42, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> mem->region is a struct resource, but mem->io_start and
> mem->io_size are not for whatever reason. Let's unify this
> and convert the io stuff into a struct resource as well.
> Should make life a little less annoying when you don't have
> juggle between two different approaches all the time.
> 
> Mostly done using cocci (with manual tweaks at all the
> places where we mutate io_size by hand):
> @@
> struct intel_memory_region *M;
> expression START, SIZE;
> @@
> - M->io_start = START;
> - M->io_size = SIZE;
> + M->io = DEFINE_RES_MEM(START, SIZE);
> 
> @@
> struct intel_memory_region *M;
> @@
> - M->io_start
> + M->io.start
> 
> @@
> struct intel_memory_region M;
> @@
> - M.io_start
> + M.io.start
> 
> @@
> expression M;
> @@
> - M->io_size
> + resource_size(&M->io)
> 
> @@
> expression M;
> @@
> - M.io_size
> + resource_size(&M.io)
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>


You can try to replace local vars and/or arguments as well:
In i915_gem_stolen_lmem_setup:
	resource_size_t io_start, io_size;

(intel_memory|mock)_region_create(struct drm_i915_private *i915,
			   resource_size_t start,
			   resource_size_t size,
			   resource_size_t min_page_size,
			   resource_size_t io_start,
			   resource_size_t io_size,
			   u16 type,
			   u16 instance,
			   const struct intel_memory_region_ops *ops);

With or without this change:
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej


> ---
>   drivers/gpu/drm/i915/display/intel_fbdev_fb.c  |  2 +-
>   drivers/gpu/drm/i915/gem/i915_gem_region.c     |  2 +-
>   drivers/gpu/drm/i915/gem/i915_gem_stolen.c     | 17 +++++++++--------
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c        |  8 ++++----
>   .../gpu/drm/i915/gem/selftests/i915_gem_mman.c | 18 +++++++++---------
>   drivers/gpu/drm/i915/gt/intel_region_lmem.c    | 11 +++--------
>   drivers/gpu/drm/i915/gt/selftest_tlb.c         |  4 ++--
>   drivers/gpu/drm/i915/i915_gpu_error.c          |  2 +-
>   drivers/gpu/drm/i915/i915_query.c              |  2 +-
>   drivers/gpu/drm/i915/intel_memory_region.c     | 15 +++++++--------
>   drivers/gpu/drm/i915/intel_memory_region.h     |  3 +--
>   drivers/gpu/drm/i915/intel_region_ttm.c        |  8 ++++----
>   .../drm/i915/selftests/intel_memory_region.c   |  4 ++--
>   13 files changed, 45 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> index 717c3a3237c4..1ac05d90b2e8 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> @@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, struct fb_info *info
>   
>   		/* Use fbdev's framebuffer from lmem for discrete */
>   		info->fix.smem_start =
> -			(unsigned long)(mem->io_start +
> +			(unsigned long)(mem->io.start +
>   					i915_gem_object_get_dma_address(obj, 0));
>   		info->fix.smem_len = obj->base.size;
>   	} else {
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> index a4fb577eceb4..b09b74a2448b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> @@ -129,7 +129,7 @@ i915_gem_object_create_region_at(struct intel_memory_region *mem,
>   		return ERR_PTR(-EINVAL);
>   
>   	if (!(flags & I915_BO_ALLOC_GPU_ONLY) &&
> -	    offset + size > mem->io_size &&
> +	    offset + size > resource_size(&mem->io) &&
>   	    !i915_ggtt_has_aperture(to_gt(mem->i915)->ggtt))
>   		return ERR_PTR(-ENOSPC);
>   
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index 8c88075eeab2..d2440c793f84 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -541,7 +541,9 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
>   
>   	/* Exclude the reserved region from driver use */
>   	mem->region.end = i915->dsm.reserved.start - 1;
> -	mem->io_size = min(mem->io_size, resource_size(&mem->region));
> +	mem->io = DEFINE_RES_MEM(mem->io.start,
> +				 min(resource_size(&mem->io),
> +				     resource_size(&mem->region)));
>   
>   	i915->dsm.usable_size = resource_size(&mem->region);
>   
> @@ -752,7 +754,7 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem,
>   	 * With discrete devices, where we lack a mappable aperture there is no
>   	 * possible way to ever access this memory on the CPU side.
>   	 */
> -	if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !mem->io_size &&
> +	if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !resource_size(&mem->io) &&
>   	    !(flags & I915_BO_ALLOC_GPU_ONLY))
>   		return -ENOSPC;
>   
> @@ -838,13 +840,12 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
>   		return 0;
>   	}
>   
> -	if (mem->io_size &&
> -	    !io_mapping_init_wc(&mem->iomap, mem->io_start, mem->io_size))
> +	if (resource_size(&mem->io) &&
> +	    !io_mapping_init_wc(&mem->iomap, mem->io.start, resource_size(&mem->io)))
>   		goto err_cleanup;
>   
> -	drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n",
> -		&mem->io_start);
> -	drm_dbg(&i915->drm, "Stolen Local DSM base: %pa\n", &mem->region.start);
> +	drm_dbg(&i915->drm, "Stolen Local DSM: %pR\n", &mem->region);
> +	drm_dbg(&i915->drm, "Stolen Local memory IO: %pR\n", &mem->io);
>   
>   	return 0;
>   
> @@ -855,7 +856,7 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
>   
>   static int release_stolen_lmem(struct intel_memory_region *mem)
>   {
> -	if (mem->io_size)
> +	if (resource_size(&mem->io))
>   		io_mapping_fini(&mem->iomap);
>   	i915_gem_cleanup_stolen(mem->i915);
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 9227f8146a58..42cc69a0a5fe 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -144,13 +144,13 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr,
>   		place->fpfn = offset >> PAGE_SHIFT;
>   		WARN_ON(overflows_type(place->fpfn + (size >> PAGE_SHIFT), place->lpfn));
>   		place->lpfn = place->fpfn + (size >> PAGE_SHIFT);
> -	} else if (mr->io_size && mr->io_size < mr->total) {
> +	} else if (resource_size(&mr->io) && resource_size(&mr->io) < mr->total) {
>   		if (flags & I915_BO_ALLOC_GPU_ONLY) {
>   			place->flags |= TTM_PL_FLAG_TOPDOWN;
>   		} else {
>   			place->fpfn = 0;
> -			WARN_ON(overflows_type(mr->io_size >> PAGE_SHIFT, place->lpfn));
> -			place->lpfn = mr->io_size >> PAGE_SHIFT;
> +			WARN_ON(overflows_type(resource_size(&mr->io) >> PAGE_SHIFT, place->lpfn));
> +			place->lpfn = resource_size(&mr->io) >> PAGE_SHIFT;
>   		}
>   	}
>   }
> @@ -1090,7 +1090,7 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
>   			struct intel_memory_region *mr = obj->mm.placements[i];
>   			unsigned int flags;
>   
> -			if (!mr->io_size && mr->type != INTEL_MEMORY_SYSTEM)
> +			if (!resource_size(&mr->io) && mr->type != INTEL_MEMORY_SYSTEM)
>   				continue;
>   
>   			flags = obj->flags;
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> index 2c51a2c452fc..99a9ade73956 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> @@ -1054,7 +1054,7 @@ static int igt_fill_mappable(struct intel_memory_region *mr,
>   	int err;
>   
>   	total = 0;
> -	size = mr->io_size;
> +	size = resource_size(&mr->io);
>   	do {
>   		struct drm_i915_gem_object *obj;
>   
> @@ -1315,28 +1315,28 @@ static int igt_mmap_migrate(void *arg)
>   		struct intel_memory_region *mixed[] = { mr, system };
>   		struct intel_memory_region *single[] = { mr };
>   		struct ttm_resource_manager *man = mr->region_private;
> -		resource_size_t saved_io_size;
> +		struct resource saved_io;
>   		int err;
>   
>   		if (mr->private)
>   			continue;
>   
> -		if (!mr->io_size)
> +		if (!resource_size(&mr->io))
>   			continue;
>   
>   		/*
>   		 * For testing purposes let's force small BAR, if not already
>   		 * present.
>   		 */
> -		saved_io_size = mr->io_size;
> -		if (mr->io_size == mr->total) {
> -			resource_size_t io_size = mr->io_size;
> +		saved_io = mr->io;
> +		if (resource_size(&mr->io) == mr->total) {
> +			resource_size_t io_size = resource_size(&mr->io);
>   
>   			io_size = rounddown_pow_of_two(io_size >> 1);
>   			if (io_size < PAGE_SIZE)
>   				continue;
>   
> -			mr->io_size = io_size;
> +			mr->io = DEFINE_RES_MEM(mr->io.start, io_size);
>   			i915_ttm_buddy_man_force_visible_size(man,
>   							      io_size >> PAGE_SHIFT);
>   		}
> @@ -1396,9 +1396,9 @@ static int igt_mmap_migrate(void *arg)
>   					 IGT_MMAP_MIGRATE_FAIL_GPU |
>   					 IGT_MMAP_MIGRATE_UNFAULTABLE);
>   out_io_size:
> -		mr->io_size = saved_io_size;
> +		mr->io = saved_io;
>   		i915_ttm_buddy_man_force_visible_size(man,
> -						      mr->io_size >> PAGE_SHIFT);
> +						      resource_size(&mr->io) >> PAGE_SHIFT);
>   		if (err)
>   			return err;
>   	}
> diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> index f8512aee58a8..6f96a6b70601 100644
> --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> @@ -144,8 +144,8 @@ region_lmem_init(struct intel_memory_region *mem)
>   	int ret;
>   
>   	if (!io_mapping_init_wc(&mem->iomap,
> -				mem->io_start,
> -				mem->io_size))
> +				mem->io.start,
> +				resource_size(&mem->io)))
>   		return -EIO;
>   
>   	ret = intel_region_ttm_init(mem);
> @@ -274,12 +274,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)
>   		goto err_region_put;
>   
>   	drm_dbg(&i915->drm, "Local memory: %pR\n", &mem->region);
> -	drm_dbg(&i915->drm, "Local memory IO start: %pa\n",
> -		&mem->io_start);
> -	drm_info(&i915->drm, "Local memory IO size: %pa\n",
> -		 &mem->io_size);
> -	drm_info(&i915->drm, "Local memory available: %pa\n",
> -		 &lmem_size);
> +	drm_dbg(&i915->drm, "Local memory IO: %pR\n", &mem->io);
>   
>   	if (io_size < lmem_size)
>   		drm_info(&i915->drm, "Using a reduced BAR size of %lluMiB. Consider enabling 'Resizable BAR' or similar, if available in the BIOS.\n",
> diff --git a/drivers/gpu/drm/i915/gt/selftest_tlb.c b/drivers/gpu/drm/i915/gt/selftest_tlb.c
> index 00b872b6380b..3941f2d6fa47 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_tlb.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_tlb.c
> @@ -206,8 +206,8 @@ static struct drm_i915_gem_object *create_lmem(struct intel_gt *gt)
>   	 * of pages. To succeed with both allocations, especially in case of Small
>   	 * BAR, try to allocate no more than quarter of mappable memory.
>   	 */
> -	if (mr && size > mr->io_size / 4)
> -		size = mr->io_size / 4;
> +	if (mr && size > resource_size(&mr->io) / 4)
> +		size = resource_size(&mr->io) / 4;
>   
>   	return i915_gem_object_create_lmem(gt->i915, size, I915_BO_ALLOC_CONTIGUOUS);
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index d04660b60046..a0b784ebaddd 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1157,7 +1157,7 @@ i915_vma_coredump_create(const struct intel_gt *gt,
>   			dma_addr_t offset = dma - mem->region.start;
>   			void __iomem *s;
>   
> -			if (offset + PAGE_SIZE > mem->io_size) {
> +			if (offset + PAGE_SIZE > resource_size(&mem->io)) {
>   				ret = -EINVAL;
>   				break;
>   			}
> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> index 00871ef99792..fa3e937ed3f5 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -502,7 +502,7 @@ static int query_memregion_info(struct drm_i915_private *i915,
>   		info.probed_size = mr->total;
>   
>   		if (mr->type == INTEL_MEMORY_LOCAL)
> -			info.probed_cpu_visible_size = mr->io_size;
> +			info.probed_cpu_visible_size = resource_size(&mr->io);
>   		else
>   			info.probed_cpu_visible_size = mr->total;
>   
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> index 60a03340bbd4..b2708f8cac2a 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -50,7 +50,7 @@ static int __iopagetest(struct intel_memory_region *mem,
>   	if (memchr_inv(result, value, sizeof(result))) {
>   		dev_err(mem->i915->drm.dev,
>   			"Failed to read back from memory region:%pR at [%pa + %pa] for %ps; wrote %x, read (%x, %x, %x)\n",
> -			&mem->region, &mem->io_start, &offset, caller,
> +			&mem->region, &mem->io.start, &offset, caller,
>   			value, result[0], result[1], result[2]);
>   		return -EINVAL;
>   	}
> @@ -67,11 +67,11 @@ static int iopagetest(struct intel_memory_region *mem,
>   	int err;
>   	int i;
>   
> -	va = ioremap_wc(mem->io_start + offset, PAGE_SIZE);
> +	va = ioremap_wc(mem->io.start + offset, PAGE_SIZE);
>   	if (!va) {
>   		dev_err(mem->i915->drm.dev,
>   			"Failed to ioremap memory region [%pa + %pa] for %ps\n",
> -			&mem->io_start, &offset, caller);
> +			&mem->io.start, &offset, caller);
>   		return -EFAULT;
>   	}
>   
> @@ -102,10 +102,10 @@ static int iomemtest(struct intel_memory_region *mem,
>   	resource_size_t last, page;
>   	int err;
>   
> -	if (mem->io_size < PAGE_SIZE)
> +	if (resource_size(&mem->io) < PAGE_SIZE)
>   		return 0;
>   
> -	last = mem->io_size - PAGE_SIZE;
> +	last = resource_size(&mem->io) - PAGE_SIZE;
>   
>   	/*
>   	 * Quick test to check read/write access to the iomap (backing store).
> @@ -207,7 +207,7 @@ static int intel_memory_region_memtest(struct intel_memory_region *mem,
>   	struct drm_i915_private *i915 = mem->i915;
>   	int err = 0;
>   
> -	if (!mem->io_start)
> +	if (!mem->io.start)
>   		return 0;
>   
>   	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) || i915->params.memtest)
> @@ -252,8 +252,7 @@ intel_memory_region_create(struct drm_i915_private *i915,
>   
>   	mem->i915 = i915;
>   	mem->region = DEFINE_RES_MEM(start, size);
> -	mem->io_start = io_start;
> -	mem->io_size = io_size;
> +	mem->io = DEFINE_RES_MEM(io_start, io_size);
>   	mem->min_page_size = min_page_size;
>   	mem->ops = ops;
>   	mem->total = size;
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
> index 9ba36454e51b..40810cfb3fd9 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.h
> +++ b/drivers/gpu/drm/i915/intel_memory_region.h
> @@ -71,8 +71,7 @@ struct intel_memory_region {
>   	struct io_mapping iomap;
>   	struct resource region;
>   
> -	resource_size_t io_start;
> -	resource_size_t io_size;
> +	struct resource io;
>   	resource_size_t min_page_size;
>   	resource_size_t total;
>   
> diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
> index bf6097e7433d..04525d92bec5 100644
> --- a/drivers/gpu/drm/i915/intel_region_ttm.c
> +++ b/drivers/gpu/drm/i915/intel_region_ttm.c
> @@ -87,7 +87,7 @@ int intel_region_ttm_init(struct intel_memory_region *mem)
>   
>   	ret = i915_ttm_buddy_man_init(bdev, mem_type, false,
>   				      resource_size(&mem->region),
> -				      mem->io_size,
> +				      resource_size(&mem->io),
>   				      mem->min_page_size, PAGE_SIZE);
>   	if (ret)
>   		return ret;
> @@ -219,16 +219,16 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
>   			goto out;
>   		}
>   		place.lpfn = place.fpfn + (size >> PAGE_SHIFT);
> -	} else if (mem->io_size && mem->io_size < mem->total) {
> +	} else if (resource_size(&mem->io) && resource_size(&mem->io) < mem->total) {
>   		if (flags & I915_BO_ALLOC_GPU_ONLY) {
>   			place.flags |= TTM_PL_FLAG_TOPDOWN;
>   		} else {
>   			place.fpfn = 0;
> -			if (WARN_ON(overflows_type(mem->io_size >> PAGE_SHIFT, place.lpfn))) {
> +			if (WARN_ON(overflows_type(resource_size(&mem->io) >> PAGE_SHIFT, place.lpfn))) {
>   				ret = -E2BIG;
>   				goto out;
>   			}
> -			place.lpfn = mem->io_size >> PAGE_SHIFT;
> +			place.lpfn = resource_size(&mem->io) >> PAGE_SHIFT;
>   		}
>   	}
>   
> diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> index d985d9bae2e8..ae6070b5bf07 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> @@ -544,8 +544,8 @@ static u64 igt_object_mappable_total(struct drm_i915_gem_object *obj)
>   		u64 start = drm_buddy_block_offset(block);
>   		u64 end = start + drm_buddy_block_size(mm, block);
>   
> -		if (start < mr->io_size)
> -			total += min_t(u64, end, mr->io_size) - start;
> +		if (start < resource_size(&mr->io))
> +			total += min_t(u64, end, resource_size(&mr->io)) - start;
>   	}
>   
>   	return total;

Re: [PATCH 01/12] drm/i915: Use struct resource for memory region IO as well

[Ville Syrjälä]

On Wed, Dec 13, 2023 at 04:52:54PM +0100, Andrzej Hajda wrote:
> On 13.12.2023 01:42, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > mem->region is a struct resource, but mem->io_start and
> > mem->io_size are not for whatever reason. Let's unify this
> > and convert the io stuff into a struct resource as well.
> > Should make life a little less annoying when you don't have
> > juggle between two different approaches all the time.
> > 
> > Mostly done using cocci (with manual tweaks at all the
> > places where we mutate io_size by hand):
> > @@
> > struct intel_memory_region *M;
> > expression START, SIZE;
> > @@
> > - M->io_start = START;
> > - M->io_size = SIZE;
> > + M->io = DEFINE_RES_MEM(START, SIZE);
> > 
> > @@
> > struct intel_memory_region *M;
> > @@
> > - M->io_start
> > + M->io.start
> > 
> > @@
> > struct intel_memory_region M;
> > @@
> > - M.io_start
> > + M.io.start
> > 
> > @@
> > expression M;
> > @@
> > - M->io_size
> > + resource_size(&M->io)
> > 
> > @@
> > expression M;
> > @@
> > - M.io_size
> > + resource_size(&M.io)
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 
> You can try to replace local vars and/or arguments as well:
> In i915_gem_stolen_lmem_setup:
> 	resource_size_t io_start, io_size;
> 
> (intel_memory|mock)_region_create(struct drm_i915_private *i915,
> 			   resource_size_t start,
> 			   resource_size_t size,
> 			   resource_size_t min_page_size,
> 			   resource_size_t io_start,
> 			   resource_size_t io_size,
> 			   u16 type,
> 			   u16 instance,
> 			   const struct intel_memory_region_ops *ops);

I think it's easier to let that guy deal with the
size->end conversion. The callers are generally
more interested in the size itself.

> 
> With or without this change:
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Thanks

> 
> Regards
> Andrzej
> 
> 
> > ---
> >   drivers/gpu/drm/i915/display/intel_fbdev_fb.c  |  2 +-
> >   drivers/gpu/drm/i915/gem/i915_gem_region.c     |  2 +-
> >   drivers/gpu/drm/i915/gem/i915_gem_stolen.c     | 17 +++++++++--------
> >   drivers/gpu/drm/i915/gem/i915_gem_ttm.c        |  8 ++++----
> >   .../gpu/drm/i915/gem/selftests/i915_gem_mman.c | 18 +++++++++---------
> >   drivers/gpu/drm/i915/gt/intel_region_lmem.c    | 11 +++--------
> >   drivers/gpu/drm/i915/gt/selftest_tlb.c         |  4 ++--
> >   drivers/gpu/drm/i915/i915_gpu_error.c          |  2 +-
> >   drivers/gpu/drm/i915/i915_query.c              |  2 +-
> >   drivers/gpu/drm/i915/intel_memory_region.c     | 15 +++++++--------
> >   drivers/gpu/drm/i915/intel_memory_region.h     |  3 +--
> >   drivers/gpu/drm/i915/intel_region_ttm.c        |  8 ++++----
> >   .../drm/i915/selftests/intel_memory_region.c   |  4 ++--
> >   13 files changed, 45 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > index 717c3a3237c4..1ac05d90b2e8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
> > @@ -78,7 +78,7 @@ int intel_fbdev_fb_fill_info(struct drm_i915_private *i915, struct fb_info *info
> >   
> >   		/* Use fbdev's framebuffer from lmem for discrete */
> >   		info->fix.smem_start =
> > -			(unsigned long)(mem->io_start +
> > +			(unsigned long)(mem->io.start +
> >   					i915_gem_object_get_dma_address(obj, 0));
> >   		info->fix.smem_len = obj->base.size;
> >   	} else {
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> > index a4fb577eceb4..b09b74a2448b 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> > @@ -129,7 +129,7 @@ i915_gem_object_create_region_at(struct intel_memory_region *mem,
> >   		return ERR_PTR(-EINVAL);
> >   
> >   	if (!(flags & I915_BO_ALLOC_GPU_ONLY) &&
> > -	    offset + size > mem->io_size &&
> > +	    offset + size > resource_size(&mem->io) &&
> >   	    !i915_ggtt_has_aperture(to_gt(mem->i915)->ggtt))
> >   		return ERR_PTR(-ENOSPC);
> >   
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > index 8c88075eeab2..d2440c793f84 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > @@ -541,7 +541,9 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
> >   
> >   	/* Exclude the reserved region from driver use */
> >   	mem->region.end = i915->dsm.reserved.start - 1;
> > -	mem->io_size = min(mem->io_size, resource_size(&mem->region));
> > +	mem->io = DEFINE_RES_MEM(mem->io.start,
> > +				 min(resource_size(&mem->io),
> > +				     resource_size(&mem->region)));
> >   
> >   	i915->dsm.usable_size = resource_size(&mem->region);
> >   
> > @@ -752,7 +754,7 @@ static int _i915_gem_object_stolen_init(struct intel_memory_region *mem,
> >   	 * With discrete devices, where we lack a mappable aperture there is no
> >   	 * possible way to ever access this memory on the CPU side.
> >   	 */
> > -	if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !mem->io_size &&
> > +	if (mem->type == INTEL_MEMORY_STOLEN_LOCAL && !resource_size(&mem->io) &&
> >   	    !(flags & I915_BO_ALLOC_GPU_ONLY))
> >   		return -ENOSPC;
> >   
> > @@ -838,13 +840,12 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
> >   		return 0;
> >   	}
> >   
> > -	if (mem->io_size &&
> > -	    !io_mapping_init_wc(&mem->iomap, mem->io_start, mem->io_size))
> > +	if (resource_size(&mem->io) &&
> > +	    !io_mapping_init_wc(&mem->iomap, mem->io.start, resource_size(&mem->io)))
> >   		goto err_cleanup;
> >   
> > -	drm_dbg(&i915->drm, "Stolen Local memory IO start: %pa\n",
> > -		&mem->io_start);
> > -	drm_dbg(&i915->drm, "Stolen Local DSM base: %pa\n", &mem->region.start);
> > +	drm_dbg(&i915->drm, "Stolen Local DSM: %pR\n", &mem->region);
> > +	drm_dbg(&i915->drm, "Stolen Local memory IO: %pR\n", &mem->io);
> >   
> >   	return 0;
> >   
> > @@ -855,7 +856,7 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
> >   
> >   static int release_stolen_lmem(struct intel_memory_region *mem)
> >   {
> > -	if (mem->io_size)
> > +	if (resource_size(&mem->io))
> >   		io_mapping_fini(&mem->iomap);
> >   	i915_gem_cleanup_stolen(mem->i915);
> >   	return 0;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > index 9227f8146a58..42cc69a0a5fe 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > @@ -144,13 +144,13 @@ i915_ttm_place_from_region(const struct intel_memory_region *mr,
> >   		place->fpfn = offset >> PAGE_SHIFT;
> >   		WARN_ON(overflows_type(place->fpfn + (size >> PAGE_SHIFT), place->lpfn));
> >   		place->lpfn = place->fpfn + (size >> PAGE_SHIFT);
> > -	} else if (mr->io_size && mr->io_size < mr->total) {
> > +	} else if (resource_size(&mr->io) && resource_size(&mr->io) < mr->total) {
> >   		if (flags & I915_BO_ALLOC_GPU_ONLY) {
> >   			place->flags |= TTM_PL_FLAG_TOPDOWN;
> >   		} else {
> >   			place->fpfn = 0;
> > -			WARN_ON(overflows_type(mr->io_size >> PAGE_SHIFT, place->lpfn));
> > -			place->lpfn = mr->io_size >> PAGE_SHIFT;
> > +			WARN_ON(overflows_type(resource_size(&mr->io) >> PAGE_SHIFT, place->lpfn));
> > +			place->lpfn = resource_size(&mr->io) >> PAGE_SHIFT;
> >   		}
> >   	}
> >   }
> > @@ -1090,7 +1090,7 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
> >   			struct intel_memory_region *mr = obj->mm.placements[i];
> >   			unsigned int flags;
> >   
> > -			if (!mr->io_size && mr->type != INTEL_MEMORY_SYSTEM)
> > +			if (!resource_size(&mr->io) && mr->type != INTEL_MEMORY_SYSTEM)
> >   				continue;
> >   
> >   			flags = obj->flags;
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > index 2c51a2c452fc..99a9ade73956 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> > @@ -1054,7 +1054,7 @@ static int igt_fill_mappable(struct intel_memory_region *mr,
> >   	int err;
> >   
> >   	total = 0;
> > -	size = mr->io_size;
> > +	size = resource_size(&mr->io);
> >   	do {
> >   		struct drm_i915_gem_object *obj;
> >   
> > @@ -1315,28 +1315,28 @@ static int igt_mmap_migrate(void *arg)
> >   		struct intel_memory_region *mixed[] = { mr, system };
> >   		struct intel_memory_region *single[] = { mr };
> >   		struct ttm_resource_manager *man = mr->region_private;
> > -		resource_size_t saved_io_size;
> > +		struct resource saved_io;
> >   		int err;
> >   
> >   		if (mr->private)
> >   			continue;
> >   
> > -		if (!mr->io_size)
> > +		if (!resource_size(&mr->io))
> >   			continue;
> >   
> >   		/*
> >   		 * For testing purposes let's force small BAR, if not already
> >   		 * present.
> >   		 */
> > -		saved_io_size = mr->io_size;
> > -		if (mr->io_size == mr->total) {
> > -			resource_size_t io_size = mr->io_size;
> > +		saved_io = mr->io;
> > +		if (resource_size(&mr->io) == mr->total) {
> > +			resource_size_t io_size = resource_size(&mr->io);
> >   
> >   			io_size = rounddown_pow_of_two(io_size >> 1);
> >   			if (io_size < PAGE_SIZE)
> >   				continue;
> >   
> > -			mr->io_size = io_size;
> > +			mr->io = DEFINE_RES_MEM(mr->io.start, io_size);
> >   			i915_ttm_buddy_man_force_visible_size(man,
> >   							      io_size >> PAGE_SHIFT);
> >   		}
> > @@ -1396,9 +1396,9 @@ static int igt_mmap_migrate(void *arg)
> >   					 IGT_MMAP_MIGRATE_FAIL_GPU |
> >   					 IGT_MMAP_MIGRATE_UNFAULTABLE);
> >   out_io_size:
> > -		mr->io_size = saved_io_size;
> > +		mr->io = saved_io;
> >   		i915_ttm_buddy_man_force_visible_size(man,
> > -						      mr->io_size >> PAGE_SHIFT);
> > +						      resource_size(&mr->io) >> PAGE_SHIFT);
> >   		if (err)
> >   			return err;
> >   	}
> > diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> > index f8512aee58a8..6f96a6b70601 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> > @@ -144,8 +144,8 @@ region_lmem_init(struct intel_memory_region *mem)
> >   	int ret;
> >   
> >   	if (!io_mapping_init_wc(&mem->iomap,
> > -				mem->io_start,
> > -				mem->io_size))
> > +				mem->io.start,
> > +				resource_size(&mem->io)))
> >   		return -EIO;
> >   
> >   	ret = intel_region_ttm_init(mem);
> > @@ -274,12 +274,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)
> >   		goto err_region_put;
> >   
> >   	drm_dbg(&i915->drm, "Local memory: %pR\n", &mem->region);
> > -	drm_dbg(&i915->drm, "Local memory IO start: %pa\n",
> > -		&mem->io_start);
> > -	drm_info(&i915->drm, "Local memory IO size: %pa\n",
> > -		 &mem->io_size);
> > -	drm_info(&i915->drm, "Local memory available: %pa\n",
> > -		 &lmem_size);
> > +	drm_dbg(&i915->drm, "Local memory IO: %pR\n", &mem->io);
> >   
> >   	if (io_size < lmem_size)
> >   		drm_info(&i915->drm, "Using a reduced BAR size of %lluMiB. Consider enabling 'Resizable BAR' or similar, if available in the BIOS.\n",
> > diff --git a/drivers/gpu/drm/i915/gt/selftest_tlb.c b/drivers/gpu/drm/i915/gt/selftest_tlb.c
> > index 00b872b6380b..3941f2d6fa47 100644
> > --- a/drivers/gpu/drm/i915/gt/selftest_tlb.c
> > +++ b/drivers/gpu/drm/i915/gt/selftest_tlb.c
> > @@ -206,8 +206,8 @@ static struct drm_i915_gem_object *create_lmem(struct intel_gt *gt)
> >   	 * of pages. To succeed with both allocations, especially in case of Small
> >   	 * BAR, try to allocate no more than quarter of mappable memory.
> >   	 */
> > -	if (mr && size > mr->io_size / 4)
> > -		size = mr->io_size / 4;
> > +	if (mr && size > resource_size(&mr->io) / 4)
> > +		size = resource_size(&mr->io) / 4;
> >   
> >   	return i915_gem_object_create_lmem(gt->i915, size, I915_BO_ALLOC_CONTIGUOUS);
> >   }
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index d04660b60046..a0b784ebaddd 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1157,7 +1157,7 @@ i915_vma_coredump_create(const struct intel_gt *gt,
> >   			dma_addr_t offset = dma - mem->region.start;
> >   			void __iomem *s;
> >   
> > -			if (offset + PAGE_SIZE > mem->io_size) {
> > +			if (offset + PAGE_SIZE > resource_size(&mem->io)) {
> >   				ret = -EINVAL;
> >   				break;
> >   			}
> > diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
> > index 00871ef99792..fa3e937ed3f5 100644
> > --- a/drivers/gpu/drm/i915/i915_query.c
> > +++ b/drivers/gpu/drm/i915/i915_query.c
> > @@ -502,7 +502,7 @@ static int query_memregion_info(struct drm_i915_private *i915,
> >   		info.probed_size = mr->total;
> >   
> >   		if (mr->type == INTEL_MEMORY_LOCAL)
> > -			info.probed_cpu_visible_size = mr->io_size;
> > +			info.probed_cpu_visible_size = resource_size(&mr->io);
> >   		else
> >   			info.probed_cpu_visible_size = mr->total;
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> > index 60a03340bbd4..b2708f8cac2a 100644
> > --- a/drivers/gpu/drm/i915/intel_memory_region.c
> > +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> > @@ -50,7 +50,7 @@ static int __iopagetest(struct intel_memory_region *mem,
> >   	if (memchr_inv(result, value, sizeof(result))) {
> >   		dev_err(mem->i915->drm.dev,
> >   			"Failed to read back from memory region:%pR at [%pa + %pa] for %ps; wrote %x, read (%x, %x, %x)\n",
> > -			&mem->region, &mem->io_start, &offset, caller,
> > +			&mem->region, &mem->io.start, &offset, caller,
> >   			value, result[0], result[1], result[2]);
> >   		return -EINVAL;
> >   	}
> > @@ -67,11 +67,11 @@ static int iopagetest(struct intel_memory_region *mem,
> >   	int err;
> >   	int i;
> >   
> > -	va = ioremap_wc(mem->io_start + offset, PAGE_SIZE);
> > +	va = ioremap_wc(mem->io.start + offset, PAGE_SIZE);
> >   	if (!va) {
> >   		dev_err(mem->i915->drm.dev,
> >   			"Failed to ioremap memory region [%pa + %pa] for %ps\n",
> > -			&mem->io_start, &offset, caller);
> > +			&mem->io.start, &offset, caller);
> >   		return -EFAULT;
> >   	}
> >   
> > @@ -102,10 +102,10 @@ static int iomemtest(struct intel_memory_region *mem,
> >   	resource_size_t last, page;
> >   	int err;
> >   
> > -	if (mem->io_size < PAGE_SIZE)
> > +	if (resource_size(&mem->io) < PAGE_SIZE)
> >   		return 0;
> >   
> > -	last = mem->io_size - PAGE_SIZE;
> > +	last = resource_size(&mem->io) - PAGE_SIZE;
> >   
> >   	/*
> >   	 * Quick test to check read/write access to the iomap (backing store).
> > @@ -207,7 +207,7 @@ static int intel_memory_region_memtest(struct intel_memory_region *mem,
> >   	struct drm_i915_private *i915 = mem->i915;
> >   	int err = 0;
> >   
> > -	if (!mem->io_start)
> > +	if (!mem->io.start)
> >   		return 0;
> >   
> >   	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) || i915->params.memtest)
> > @@ -252,8 +252,7 @@ intel_memory_region_create(struct drm_i915_private *i915,
> >   
> >   	mem->i915 = i915;
> >   	mem->region = DEFINE_RES_MEM(start, size);
> > -	mem->io_start = io_start;
> > -	mem->io_size = io_size;
> > +	mem->io = DEFINE_RES_MEM(io_start, io_size);
> >   	mem->min_page_size = min_page_size;
> >   	mem->ops = ops;
> >   	mem->total = size;
> > diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
> > index 9ba36454e51b..40810cfb3fd9 100644
> > --- a/drivers/gpu/drm/i915/intel_memory_region.h
> > +++ b/drivers/gpu/drm/i915/intel_memory_region.h
> > @@ -71,8 +71,7 @@ struct intel_memory_region {
> >   	struct io_mapping iomap;
> >   	struct resource region;
> >   
> > -	resource_size_t io_start;
> > -	resource_size_t io_size;
> > +	struct resource io;
> >   	resource_size_t min_page_size;
> >   	resource_size_t total;
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
> > index bf6097e7433d..04525d92bec5 100644
> > --- a/drivers/gpu/drm/i915/intel_region_ttm.c
> > +++ b/drivers/gpu/drm/i915/intel_region_ttm.c
> > @@ -87,7 +87,7 @@ int intel_region_ttm_init(struct intel_memory_region *mem)
> >   
> >   	ret = i915_ttm_buddy_man_init(bdev, mem_type, false,
> >   				      resource_size(&mem->region),
> > -				      mem->io_size,
> > +				      resource_size(&mem->io),
> >   				      mem->min_page_size, PAGE_SIZE);
> >   	if (ret)
> >   		return ret;
> > @@ -219,16 +219,16 @@ intel_region_ttm_resource_alloc(struct intel_memory_region *mem,
> >   			goto out;
> >   		}
> >   		place.lpfn = place.fpfn + (size >> PAGE_SHIFT);
> > -	} else if (mem->io_size && mem->io_size < mem->total) {
> > +	} else if (resource_size(&mem->io) && resource_size(&mem->io) < mem->total) {
> >   		if (flags & I915_BO_ALLOC_GPU_ONLY) {
> >   			place.flags |= TTM_PL_FLAG_TOPDOWN;
> >   		} else {
> >   			place.fpfn = 0;
> > -			if (WARN_ON(overflows_type(mem->io_size >> PAGE_SHIFT, place.lpfn))) {
> > +			if (WARN_ON(overflows_type(resource_size(&mem->io) >> PAGE_SHIFT, place.lpfn))) {
> >   				ret = -E2BIG;
> >   				goto out;
> >   			}
> > -			place.lpfn = mem->io_size >> PAGE_SHIFT;
> > +			place.lpfn = resource_size(&mem->io) >> PAGE_SHIFT;
> >   		}
> >   	}
> >   
> > diff --git a/drivers/gpu/drm/i915/selftests/intel_memory_region.c b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> > index d985d9bae2e8..ae6070b5bf07 100644
> > --- a/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> > +++ b/drivers/gpu/drm/i915/selftests/intel_memory_region.c
> > @@ -544,8 +544,8 @@ static u64 igt_object_mappable_total(struct drm_i915_gem_object *obj)
> >   		u64 start = drm_buddy_block_offset(block);
> >   		u64 end = start + drm_buddy_block_size(mm, block);
> >   
> > -		if (start < mr->io_size)
> > -			total += min_t(u64, end, mr->io_size) - start;
> > +		if (start < resource_size(&mr->io))
> > +			total += min_t(u64, end, resource_size(&mr->io)) - start;
> >   	}
> >   
> >   	return total;

-- 
Ville Syrjälä
Intel

[PATCH 02/12] drm/i915: Print memory region info during probe

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Dump the details about every memory region into dmesg at probe time.
Avoids having to dig those out from random places when debugging stuff.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_memory_region.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index b2708f8cac2a..52d998e5c21a 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -372,6 +372,24 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
 		i915->mm.regions[i] = mem;
 	}
 
+	for (i = 0; i < ARRAY_SIZE(i915->mm.regions); i++) {
+		struct intel_memory_region *mem = i915->mm.regions[i];
+		u64 region_size, io_size;
+
+		if (!mem)
+			continue;
+
+		region_size = resource_size(&mem->region) >> 20;
+		io_size = resource_size(&mem->io) >> 20;
+
+		if (resource_size(&mem->io))
+			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: %llu MiB %pR\n",
+				mem->id, mem->name, region_size, &mem->region, io_size, &mem->io);
+		else
+			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: n/a\n",
+				mem->id, mem->name, region_size, &mem->region);
+	}
+
 	return 0;
 
 out_cleanup:
-- 
2.41.0

Re: [PATCH 02/12] drm/i915: Print memory region info during probe

[Andrzej Hajda]

On 13.12.2023 01:42, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Dump the details about every memory region into dmesg at probe time.
> Avoids having to dig those out from random places when debugging stuff.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_memory_region.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> index b2708f8cac2a..52d998e5c21a 100644
> --- a/drivers/gpu/drm/i915/intel_memory_region.c
> +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> @@ -372,6 +372,24 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
>   		i915->mm.regions[i] = mem;
>   	}
>   
> +	for (i = 0; i < ARRAY_SIZE(i915->mm.regions); i++) {
> +		struct intel_memory_region *mem = i915->mm.regions[i];
> +		u64 region_size, io_size;
> +
> +		if (!mem)
> +			continue;
> +
> +		region_size = resource_size(&mem->region) >> 20;
> +		io_size = resource_size(&mem->io) >> 20;
> +
> +		if (resource_size(&mem->io))
> +			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: %llu MiB %pR\n",
> +				mem->id, mem->name, region_size, &mem->region, io_size, &mem->io);
> +		else
> +			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: n/a\n",
> +				mem->id, mem->name, region_size, &mem->region);

Doesn't printk handle properly 0-length resources?

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej


> +	}
> +
>   	return 0;
>   
>   out_cleanup:

Re: [PATCH 02/12] drm/i915: Print memory region info during probe

[Ville Syrjälä]

On Wed, Dec 13, 2023 at 05:05:21PM +0100, Andrzej Hajda wrote:
> On 13.12.2023 01:42, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Dump the details about every memory region into dmesg at probe time.
> > Avoids having to dig those out from random places when debugging stuff.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_memory_region.c | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> > index b2708f8cac2a..52d998e5c21a 100644
> > --- a/drivers/gpu/drm/i915/intel_memory_region.c
> > +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> > @@ -372,6 +372,24 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
> >   		i915->mm.regions[i] = mem;
> >   	}
> >   
> > +	for (i = 0; i < ARRAY_SIZE(i915->mm.regions); i++) {
> > +		struct intel_memory_region *mem = i915->mm.regions[i];
> > +		u64 region_size, io_size;
> > +
> > +		if (!mem)
> > +			continue;
> > +
> > +		region_size = resource_size(&mem->region) >> 20;
> > +		io_size = resource_size(&mem->io) >> 20;
> > +
> > +		if (resource_size(&mem->io))
> > +			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: %llu MiB %pR\n",
> > +				mem->id, mem->name, region_size, &mem->region, io_size, &mem->io);
> > +		else
> > +			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: n/a\n",
> > +				mem->id, mem->name, region_size, &mem->region);
> 
> Doesn't printk handle properly 0-length resources?

Not without extra help. Apparently there is IORESOURCE_UNSET
that would just print the size, but you have to explicitly
set that yourself, which I suppose we could do.

> 
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> 
> Regards
> Andrzej
> 
> 
> > +	}
> > +
> >   	return 0;
> >   
> >   out_cleanup:

-- 
Ville Syrjälä
Intel

Re: [PATCH 02/12] drm/i915: Print memory region info during probe

[Ville Syrjälä]

On Wed, Dec 13, 2023 at 06:26:13PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 13, 2023 at 05:05:21PM +0100, Andrzej Hajda wrote:
> > On 13.12.2023 01:42, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Dump the details about every memory region into dmesg at probe time.
> > > Avoids having to dig those out from random places when debugging stuff.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_memory_region.c | 18 ++++++++++++++++++
> > >   1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
> > > index b2708f8cac2a..52d998e5c21a 100644
> > > --- a/drivers/gpu/drm/i915/intel_memory_region.c
> > > +++ b/drivers/gpu/drm/i915/intel_memory_region.c
> > > @@ -372,6 +372,24 @@ int intel_memory_regions_hw_probe(struct drm_i915_private *i915)
> > >   		i915->mm.regions[i] = mem;
> > >   	}
> > >   
> > > +	for (i = 0; i < ARRAY_SIZE(i915->mm.regions); i++) {
> > > +		struct intel_memory_region *mem = i915->mm.regions[i];
> > > +		u64 region_size, io_size;
> > > +
> > > +		if (!mem)
> > > +			continue;
> > > +
> > > +		region_size = resource_size(&mem->region) >> 20;
> > > +		io_size = resource_size(&mem->io) >> 20;
> > > +
> > > +		if (resource_size(&mem->io))
> > > +			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: %llu MiB %pR\n",
> > > +				mem->id, mem->name, region_size, &mem->region, io_size, &mem->io);
> > > +		else
> > > +			drm_dbg(&i915->drm, "Memory region(%d): %s: %llu MiB %pR, io: n/a\n",
> > > +				mem->id, mem->name, region_size, &mem->region);
> > 
> > Doesn't printk handle properly 0-length resources?
> 
> Not without extra help. Apparently there is IORESOURCE_UNSET
> that would just print the size, but you have to explicitly
> set that yourself, which I suppose we could do.

This is what we get with nothing:
Memory region(6): stolen-local: 20 MiB [mem 0x3fe800000-0x3ffbfffff], io: 0 MiB [mem 0x00000000-0xffffffffffffffff]

This is with IORESOURCE_UNSET:
Memory region(6): stolen-local: 20 MiB [mem 0x3fe800000-0x3ffbfffff], io: 0 MiB [mem size 0x00000000]

And this is the original:
Memory region(6): stolen-local: 20 MiB [mem 0x3fe800000-0x3ffbfffff], io: n/a

I kinda like the explicit 'n/a' to make it obvious there is noting
there.

Also using IORESOURCE_UNSET would be a bit more annoying as we'd have
to explicitly preserve it in places where we resize the resource
(stolen reseved handling and selftests). But I suppose it would be
just a matter of using the full DEFINE_RES_NAMED() instead of
DEFINE_RES_MEM(). Hmm, maybe I should do that anyway just to be
safe in case someone starts to use some extra flags on these...

> 
> > 
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > 
> > Regards
> > Andrzej
> > 
> > 
> > > +	}
> > > +
> > >   	return 0;
> > >   
> > >   out_cleanup:
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel

[PATCH 03/12] drm/i915: Remove ad-hoc lmem/stolen debugs

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Now that intel_memory_regions_hw_probe() prints out each and every
memory region there's no reason to have ad-hoc debugs to do similar
things elsewhere.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c  | 4 ----
 drivers/gpu/drm/i915/gt/intel_region_lmem.c | 3 ---
 2 files changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index d2440c793f84..ee237043c302 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -828,7 +828,6 @@ static const struct intel_memory_region_ops i915_region_stolen_smem_ops = {
 
 static int init_stolen_lmem(struct intel_memory_region *mem)
 {
-	struct drm_i915_private *i915 = mem->i915;
 	int err;
 
 	if (GEM_WARN_ON(resource_size(&mem->region) == 0))
@@ -844,9 +843,6 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
 	    !io_mapping_init_wc(&mem->iomap, mem->io.start, resource_size(&mem->io)))
 		goto err_cleanup;
 
-	drm_dbg(&i915->drm, "Stolen Local DSM: %pR\n", &mem->region);
-	drm_dbg(&i915->drm, "Stolen Local memory IO: %pR\n", &mem->io);
-
 	return 0;
 
 err_cleanup:
diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
index 6f96a6b70601..af357089da6e 100644
--- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
+++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
@@ -273,9 +273,6 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)
 	if (err)
 		goto err_region_put;
 
-	drm_dbg(&i915->drm, "Local memory: %pR\n", &mem->region);
-	drm_dbg(&i915->drm, "Local memory IO: %pR\n", &mem->io);
-
 	if (io_size < lmem_size)
 		drm_info(&i915->drm, "Using a reduced BAR size of %lluMiB. Consider enabling 'Resizable BAR' or similar, if available in the BIOS.\n",
 			 (u64)io_size >> 20);
-- 
2.41.0

Re: [PATCH 03/12] drm/i915: Remove ad-hoc lmem/stolen debugs

[Andrzej Hajda]

On 13.12.2023 01:42, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Now that intel_memory_regions_hw_probe() prints out each and every
> memory region there's no reason to have ad-hoc debugs to do similar
> things elsewhere.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

> ---
>   drivers/gpu/drm/i915/gem/i915_gem_stolen.c  | 4 ----
>   drivers/gpu/drm/i915/gt/intel_region_lmem.c | 3 ---
>   2 files changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index d2440c793f84..ee237043c302 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -828,7 +828,6 @@ static const struct intel_memory_region_ops i915_region_stolen_smem_ops = {
>   
>   static int init_stolen_lmem(struct intel_memory_region *mem)
>   {
> -	struct drm_i915_private *i915 = mem->i915;
>   	int err;
>   
>   	if (GEM_WARN_ON(resource_size(&mem->region) == 0))
> @@ -844,9 +843,6 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
>   	    !io_mapping_init_wc(&mem->iomap, mem->io.start, resource_size(&mem->io)))
>   		goto err_cleanup;
>   
> -	drm_dbg(&i915->drm, "Stolen Local DSM: %pR\n", &mem->region);
> -	drm_dbg(&i915->drm, "Stolen Local memory IO: %pR\n", &mem->io);
> -
>   	return 0;
>   
>   err_cleanup:
> diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> index 6f96a6b70601..af357089da6e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
> @@ -273,9 +273,6 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)
>   	if (err)
>   		goto err_region_put;
>   
> -	drm_dbg(&i915->drm, "Local memory: %pR\n", &mem->region);
> -	drm_dbg(&i915->drm, "Local memory IO: %pR\n", &mem->io);
> -
>   	if (io_size < lmem_size)
>   		drm_info(&i915->drm, "Using a reduced BAR size of %lluMiB. Consider enabling 'Resizable BAR' or similar, if available in the BIOS.\n",
>   			 (u64)io_size >> 20);

[PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On MTL accessing stolen memory via the BARs is somehow borked,
and it can hang the machine. As a workaround let's bypass the
BARs and just go straight to DSMBASE/GSMBASE instead.

Note that on every other platform this itself would hang the
machine, but on MTL the system firmware is expected to relax
the access permission guarding stolen memory to enable this
workaround, and thus direct CPU accesses should be fine.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 11 ++++++++++-
 drivers/gpu/drm/i915/gt/intel_ggtt.c       | 13 ++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index ee237043c302..252fe5cd6ede 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -941,7 +941,16 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915, u16 type,
 		dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
 	}
 
-	if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
+	if (IS_METEORLAKE(i915)) {
+		/*
+		 * Workaround: access via BAR can hang MTL, go directly to DSM.
+		 *
+		 * Normally this would not work but on MTL the system firmware
+		 * should have relaxed the access permissions sufficiently.
+		 */
+		io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) & GEN12_BDSM_MASK;
+		io_size = dsm_size;
+	} else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
 		io_start = 0;
 		io_size = 0;
 	} else {
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 21a7e3191c18..ab71d74ec426 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -24,6 +24,7 @@
 #include "intel_ring.h"
 #include "i915_drv.h"
 #include "i915_pci.h"
+#include "i915_reg.h"
 #include "i915_request.h"
 #include "i915_scatterlist.h"
 #include "i915_utils.h"
@@ -1152,13 +1153,23 @@ static unsigned int gen6_gttadr_offset(struct drm_i915_private *i915)
 static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
 {
 	struct drm_i915_private *i915 = ggtt->vm.i915;
+	struct intel_uncore *uncore = ggtt->vm.gt->uncore;
 	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
 	phys_addr_t phys_addr;
 	u32 pte_flags;
 	int ret;
 
 	GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) != gen6_gttmmadr_size(i915));
-	phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) + gen6_gttadr_offset(i915);
+	/*
+	 * Workaround: access via BAR can hang MTL, go directly to GSM.
+	 *
+	 * Normally this would not work but on MTL the system firmware
+	 * should have relaxed the access permissions sufficiently.
+	 */
+	if (IS_METEORLAKE(i915))
+		phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) & GEN12_BDSM_MASK;
+	else
+		phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) + gen6_gttadr_offset(i915);
 
 	if (needs_wc_ggtt_mapping(i915))
 		ggtt->gsm = ioremap_wc(phys_addr, size);
-- 
2.41.0

Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access

[Joonas Lahtinen]

Quoting Ville Syrjala (2023-12-13 02:42:29)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On MTL accessing stolen memory via the BARs is somehow borked,
> and it can hang the machine. As a workaround let's bypass the
> BARs and just go straight to DSMBASE/GSMBASE instead.
> 
> Note that on every other platform this itself would hang the
> machine, but on MTL the system firmware is expected to relax
> the access permission guarding stolen memory to enable this
> workaround, and thus direct CPU accesses should be fine.

Shouldn't this have a proper workaround number assigned?

Regards, Joonas

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 11 ++++++++++-
>  drivers/gpu/drm/i915/gt/intel_ggtt.c       | 13 ++++++++++++-
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index ee237043c302..252fe5cd6ede 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -941,7 +941,16 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915, u16 type,
>                 dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
>         }
>  
> -       if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> +       if (IS_METEORLAKE(i915)) {
> +               /*
> +                * Workaround: access via BAR can hang MTL, go directly to DSM.
> +                *
> +                * Normally this would not work but on MTL the system firmware
> +                * should have relaxed the access permissions sufficiently.
> +                */
> +               io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) & GEN12_BDSM_MASK;
> +               io_size = dsm_size;
> +       } else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
>                 io_start = 0;
>                 io_size = 0;
>         } else {
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 21a7e3191c18..ab71d74ec426 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -24,6 +24,7 @@
>  #include "intel_ring.h"
>  #include "i915_drv.h"
>  #include "i915_pci.h"
> +#include "i915_reg.h"
>  #include "i915_request.h"
>  #include "i915_scatterlist.h"
>  #include "i915_utils.h"
> @@ -1152,13 +1153,23 @@ static unsigned int gen6_gttadr_offset(struct drm_i915_private *i915)
>  static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
>  {
>         struct drm_i915_private *i915 = ggtt->vm.i915;
> +       struct intel_uncore *uncore = ggtt->vm.gt->uncore;
>         struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>         phys_addr_t phys_addr;
>         u32 pte_flags;
>         int ret;
>  
>         GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) != gen6_gttmmadr_size(i915));
> -       phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) + gen6_gttadr_offset(i915);
> +       /*
> +        * Workaround: access via BAR can hang MTL, go directly to GSM.
> +        *
> +        * Normally this would not work but on MTL the system firmware
> +        * should have relaxed the access permissions sufficiently.
> +        */
> +       if (IS_METEORLAKE(i915))
> +               phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) & GEN12_BDSM_MASK;
> +       else
> +               phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) + gen6_gttadr_offset(i915);
>  
>         if (needs_wc_ggtt_mapping(i915))
>                 ggtt->gsm = ioremap_wc(phys_addr, size);
> -- 
> 2.41.0
> 

Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access

[Ville Syrjälä]

On Wed, Dec 13, 2023 at 11:09:38AM +0200, Joonas Lahtinen wrote:
> Quoting Ville Syrjala (2023-12-13 02:42:29)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On MTL accessing stolen memory via the BARs is somehow borked,
> > and it can hang the machine. As a workaround let's bypass the
> > BARs and just go straight to DSMBASE/GSMBASE instead.
> > 
> > Note that on every other platform this itself would hang the
> > machine, but on MTL the system firmware is expected to relax
> > the access permission guarding stolen memory to enable this
> > workaround, and thus direct CPU accesses should be fine.
> 
> Shouldn't this have a proper workaround number assigned?

I think there are various numbers, half of which I couldn't even
find in any database, and the other half I couldn't access for
whatever reason. So dunno what situation really is apart from
the firmware clearly implemening its part (since I can poke 
DSM/GSM directly without killing the machine).

RK do you know what we should call this?

> 
> Regards, Joonas
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 11 ++++++++++-
> >  drivers/gpu/drm/i915/gt/intel_ggtt.c       | 13 ++++++++++++-
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > index ee237043c302..252fe5cd6ede 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > @@ -941,7 +941,16 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915, u16 type,
> >                 dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
> >         }
> >  
> > -       if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > +       if (IS_METEORLAKE(i915)) {
> > +               /*
> > +                * Workaround: access via BAR can hang MTL, go directly to DSM.
> > +                *
> > +                * Normally this would not work but on MTL the system firmware
> > +                * should have relaxed the access permissions sufficiently.
> > +                */
> > +               io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) & GEN12_BDSM_MASK;
> > +               io_size = dsm_size;
> > +       } else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> >                 io_start = 0;
> >                 io_size = 0;
> >         } else {
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > index 21a7e3191c18..ab71d74ec426 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > @@ -24,6 +24,7 @@
> >  #include "intel_ring.h"
> >  #include "i915_drv.h"
> >  #include "i915_pci.h"
> > +#include "i915_reg.h"
> >  #include "i915_request.h"
> >  #include "i915_scatterlist.h"
> >  #include "i915_utils.h"
> > @@ -1152,13 +1153,23 @@ static unsigned int gen6_gttadr_offset(struct drm_i915_private *i915)
> >  static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
> >  {
> >         struct drm_i915_private *i915 = ggtt->vm.i915;
> > +       struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> >         struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> >         phys_addr_t phys_addr;
> >         u32 pte_flags;
> >         int ret;
> >  
> >         GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) != gen6_gttmmadr_size(i915));
> > -       phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) + gen6_gttadr_offset(i915);
> > +       /*
> > +        * Workaround: access via BAR can hang MTL, go directly to GSM.
> > +        *
> > +        * Normally this would not work but on MTL the system firmware
> > +        * should have relaxed the access permissions sufficiently.
> > +        */
> > +       if (IS_METEORLAKE(i915))
> > +               phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) & GEN12_BDSM_MASK;
> > +       else
> > +               phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) + gen6_gttadr_offset(i915);
> >  
> >         if (needs_wc_ggtt_mapping(i915))
> >                 ggtt->gsm = ioremap_wc(phys_addr, size);
> > -- 
> > 2.41.0
> > 

-- 
Ville Syrjälä
Intel

RE: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access

[Sripada, Radhakrishna]

Hi Ville,

+Nirmoy

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, December 13, 2023 1:30 AM
> To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Sripada, Radhakrishna
> <radhakrishna.sripada@intel.com>
> Subject: Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL
> stolen memory access
> 
> On Wed, Dec 13, 2023 at 11:09:38AM +0200, Joonas Lahtinen wrote:
> > Quoting Ville Syrjala (2023-12-13 02:42:29)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > On MTL accessing stolen memory via the BARs is somehow borked,
> > > and it can hang the machine. As a workaround let's bypass the
> > > BARs and just go straight to DSMBASE/GSMBASE instead.
> > >
> > > Note that on every other platform this itself would hang the
> > > machine, but on MTL the system firmware is expected to relax
> > > the access permission guarding stolen memory to enable this
> > > workaround, and thus direct CPU accesses should be fine.
> >
> > Shouldn't this have a proper workaround number assigned?
> 
> I think there are various numbers, half of which I couldn't even
> find in any database, and the other half I couldn't access for
> whatever reason. So dunno what situation really is apart from
> the firmware clearly implemening its part (since I can poke
> DSM/GSM directly without killing the machine).
> 
> RK do you know what we should call this?
Nirmoy previously used Wa_22018444074 in [1].

There were some associated issues Wa_13010847436 and Wa_14019519902
which Nirmoy quoted in [2].

Wa_22018529850 is derived from Wa_22018444074, is targeting the latest Gop
driver change which is installed in bat-mtlp-8 hopefully it should help debug the issue further.


Regarding the patch itself,
Do we need to carve out the range from e820 the area around DSM if we can directly access the range from CPU
without the bar?


Thanks,
Radhakrishna(RK) Sripada
1. https://patchwork.freedesktop.org/series/120683/
2. https://patchwork.freedesktop.org/series/123329/

> 
> >
> > Regards, Joonas
> >
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 11 ++++++++++-
> > >  drivers/gpu/drm/i915/gt/intel_ggtt.c       | 13 ++++++++++++-
> > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > index ee237043c302..252fe5cd6ede 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > @@ -941,7 +941,16 @@ i915_gem_stolen_lmem_setup(struct
> drm_i915_private *i915, u16 type,
> > >                 dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
> > >         }
> > >
> > > -       if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > > +       if (IS_METEORLAKE(i915)) {
> > > +               /*
> > > +                * Workaround: access via BAR can hang MTL, go directly to DSM.
> > > +                *
> > > +                * Normally this would not work but on MTL the system firmware
> > > +                * should have relaxed the access permissions sufficiently.
> > > +                */
> > > +               io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) &
> GEN12_BDSM_MASK;
> > > +               io_size = dsm_size;
> > > +       } else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > >                 io_start = 0;
> > >                 io_size = 0;
> > >         } else {
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > index 21a7e3191c18..ab71d74ec426 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > @@ -24,6 +24,7 @@
> > >  #include "intel_ring.h"
> > >  #include "i915_drv.h"
> > >  #include "i915_pci.h"
> > > +#include "i915_reg.h"
> > >  #include "i915_request.h"
> > >  #include "i915_scatterlist.h"
> > >  #include "i915_utils.h"
> > > @@ -1152,13 +1153,23 @@ static unsigned int gen6_gttadr_offset(struct
> drm_i915_private *i915)
> > >  static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
> > >  {
> > >         struct drm_i915_private *i915 = ggtt->vm.i915;
> > > +       struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> > >         struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > >         phys_addr_t phys_addr;
> > >         u32 pte_flags;
> > >         int ret;
> > >
> > >         GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) !=
> gen6_gttmmadr_size(i915));
> > > -       phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
> gen6_gttadr_offset(i915);
> > > +       /*
> > > +        * Workaround: access via BAR can hang MTL, go directly to GSM.
> > > +        *
> > > +        * Normally this would not work but on MTL the system firmware
> > > +        * should have relaxed the access permissions sufficiently.
> > > +        */
> > > +       if (IS_METEORLAKE(i915))
> > > +               phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) &
> GEN12_BDSM_MASK;
> > > +       else
> > > +               phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
> gen6_gttadr_offset(i915);
> > >
> > >         if (needs_wc_ggtt_mapping(i915))
> > >                 ggtt->gsm = ioremap_wc(phys_addr, size);
> > > --
> > > 2.41.0
> > >
> 
> --
> Ville Syrjälä
> Intel

Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access

[Ville Syrjälä]

On Wed, Dec 13, 2023 at 08:18:15PM +0000, Sripada, Radhakrishna wrote:
> Hi Ville,
> 
> +Nirmoy
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Wednesday, December 13, 2023 1:30 AM
> > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Sripada, Radhakrishna
> > <radhakrishna.sripada@intel.com>
> > Subject: Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL
> > stolen memory access
> > 
> > On Wed, Dec 13, 2023 at 11:09:38AM +0200, Joonas Lahtinen wrote:
> > > Quoting Ville Syrjala (2023-12-13 02:42:29)
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > On MTL accessing stolen memory via the BARs is somehow borked,
> > > > and it can hang the machine. As a workaround let's bypass the
> > > > BARs and just go straight to DSMBASE/GSMBASE instead.
> > > >
> > > > Note that on every other platform this itself would hang the
> > > > machine, but on MTL the system firmware is expected to relax
> > > > the access permission guarding stolen memory to enable this
> > > > workaround, and thus direct CPU accesses should be fine.
> > >
> > > Shouldn't this have a proper workaround number assigned?
> > 
> > I think there are various numbers, half of which I couldn't even
> > find in any database, and the other half I couldn't access for
> > whatever reason. So dunno what situation really is apart from
> > the firmware clearly implemening its part (since I can poke
> > DSM/GSM directly without killing the machine).
> > 
> > RK do you know what we should call this?
> Nirmoy previously used Wa_22018444074 in [1].
> 
> There were some associated issues Wa_13010847436 and Wa_14019519902
> which Nirmoy quoted in [2].
> 
> Wa_22018529850 is derived from Wa_22018444074, is targeting the latest Gop
> driver change which is installed in bat-mtlp-8 hopefully it should help debug the issue further.
> 
> 
> Regarding the patch itself,
> Do we need to carve out the range from e820 the area around DSM if we can directly access the range from CPU
> without the bar?

IIRC we dropped the early quirks on new platforms under the
assumption that the BIOS no longer forgets to mark the DSM
as not-RAM/whatever. I don't think anything should change
there even when we are allowed direct CPU access.

But I don't recall if I double checked the e820 listing on
the MTL I was using. I was able to direct access to both DSM
and GSM for sure, and the address the GOP handed over to efifb
also pointed directly to DSM.

> 
> 
> Thanks,
> Radhakrishna(RK) Sripada
> 1. https://patchwork.freedesktop.org/series/120683/
> 2. https://patchwork.freedesktop.org/series/123329/
> 
> > 
> > >
> > > Regards, Joonas
> > >
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 11 ++++++++++-
> > > >  drivers/gpu/drm/i915/gt/intel_ggtt.c       | 13 ++++++++++++-
> > > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > index ee237043c302..252fe5cd6ede 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > @@ -941,7 +941,16 @@ i915_gem_stolen_lmem_setup(struct
> > drm_i915_private *i915, u16 type,
> > > >                 dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
> > > >         }
> > > >
> > > > -       if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > > > +       if (IS_METEORLAKE(i915)) {
> > > > +               /*
> > > > +                * Workaround: access via BAR can hang MTL, go directly to DSM.
> > > > +                *
> > > > +                * Normally this would not work but on MTL the system firmware
> > > > +                * should have relaxed the access permissions sufficiently.
> > > > +                */
> > > > +               io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) &
> > GEN12_BDSM_MASK;
> > > > +               io_size = dsm_size;
> > > > +       } else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > > >                 io_start = 0;
> > > >                 io_size = 0;
> > > >         } else {
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > index 21a7e3191c18..ab71d74ec426 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > @@ -24,6 +24,7 @@
> > > >  #include "intel_ring.h"
> > > >  #include "i915_drv.h"
> > > >  #include "i915_pci.h"
> > > > +#include "i915_reg.h"
> > > >  #include "i915_request.h"
> > > >  #include "i915_scatterlist.h"
> > > >  #include "i915_utils.h"
> > > > @@ -1152,13 +1153,23 @@ static unsigned int gen6_gttadr_offset(struct
> > drm_i915_private *i915)
> > > >  static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
> > > >  {
> > > >         struct drm_i915_private *i915 = ggtt->vm.i915;
> > > > +       struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> > > >         struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > > >         phys_addr_t phys_addr;
> > > >         u32 pte_flags;
> > > >         int ret;
> > > >
> > > >         GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) !=
> > gen6_gttmmadr_size(i915));
> > > > -       phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
> > gen6_gttadr_offset(i915);
> > > > +       /*
> > > > +        * Workaround: access via BAR can hang MTL, go directly to GSM.
> > > > +        *
> > > > +        * Normally this would not work but on MTL the system firmware
> > > > +        * should have relaxed the access permissions sufficiently.
> > > > +        */
> > > > +       if (IS_METEORLAKE(i915))
> > > > +               phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) &
> > GEN12_BDSM_MASK;
> > > > +       else
> > > > +               phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
> > gen6_gttadr_offset(i915);
> > > >
> > > >         if (needs_wc_ggtt_mapping(i915))
> > > >                 ggtt->gsm = ioremap_wc(phys_addr, size);
> > > > --
> > > > 2.41.0
> > > >
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

RE: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access

[Sripada, Radhakrishna]

HI Ville,

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, December 13, 2023 6:03 PM
> To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Das, Nirmoy
> <nirmoy.das@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL
> stolen memory access
> 
> On Wed, Dec 13, 2023 at 08:18:15PM +0000, Sripada, Radhakrishna wrote:
> > Hi Ville,
> >
> > +Nirmoy
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Sent: Wednesday, December 13, 2023 1:30 AM
> > > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Sripada, Radhakrishna
> > > <radhakrishna.sripada@intel.com>
> > > Subject: Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL
> > > stolen memory access
> > >
> > > On Wed, Dec 13, 2023 at 11:09:38AM +0200, Joonas Lahtinen wrote:
> > > > Quoting Ville Syrjala (2023-12-13 02:42:29)
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >
> > > > > On MTL accessing stolen memory via the BARs is somehow borked,
> > > > > and it can hang the machine. As a workaround let's bypass the
> > > > > BARs and just go straight to DSMBASE/GSMBASE instead.
> > > > >
> > > > > Note that on every other platform this itself would hang the
> > > > > machine, but on MTL the system firmware is expected to relax
> > > > > the access permission guarding stolen memory to enable this
> > > > > workaround, and thus direct CPU accesses should be fine.
> > > >
> > > > Shouldn't this have a proper workaround number assigned?
> > >
> > > I think there are various numbers, half of which I couldn't even
> > > find in any database, and the other half I couldn't access for
> > > whatever reason. So dunno what situation really is apart from
> > > the firmware clearly implemening its part (since I can poke
> > > DSM/GSM directly without killing the machine).
> > >
> > > RK do you know what we should call this?
> > Nirmoy previously used Wa_22018444074 in [1].
> >
> > There were some associated issues Wa_13010847436 and Wa_14019519902
> > which Nirmoy quoted in [2].
> >
> > Wa_22018529850 is derived from Wa_22018444074, is targeting the latest Gop
> > driver change which is installed in bat-mtlp-8 hopefully it should help debug the
> issue further.
> >
> >
> > Regarding the patch itself,
> > Do we need to carve out the range from e820 the area around DSM if we can
> directly access the range from CPU
> > without the bar?
> 
> IIRC we dropped the early quirks on new platforms under the
> assumption that the BIOS no longer forgets to mark the DSM
> as not-RAM/whatever. I don't think anything should change
> there even when we are allowed direct CPU access.
> 
> But I don't recall if I double checked the e820 listing on
> the MTL I was using. I was able to direct access to both DSM
> and GSM for sure, and the address the GOP handed over to efifb
> also pointed directly to DSM.

Up until adl-p/rpl, the PCI config space had the mirror registers for the stolen memory
base and size, since the stolen meory is carved out of the available physical ram. Starting MTL
this was removed from pci config space due to the introduction of stolen lmem which should not
be cpu addressable aperture.

With the new gop driver allocating the FB memory in dram, should the e820 mark the FB area
as reserved for the system use? Do we still preserve the efifb after doing a memtest before loading the driver?

Thanks,
Radhakrishna(RK) Sripada 
> 
> >
> >
> > Thanks,
> > Radhakrishna(RK) Sripada
> > 1. https://patchwork.freedesktop.org/series/120683/
> > 2. https://patchwork.freedesktop.org/series/123329/
> >
> > >
> > > >
> > > > Regards, Joonas
> > > >
> > > > >
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 11 ++++++++++-
> > > > >  drivers/gpu/drm/i915/gt/intel_ggtt.c       | 13 ++++++++++++-
> > > > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > > index ee237043c302..252fe5cd6ede 100644
> > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > > @@ -941,7 +941,16 @@ i915_gem_stolen_lmem_setup(struct
> > > drm_i915_private *i915, u16 type,
> > > > >                 dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
> > > > >         }
> > > > >
> > > > > -       if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > > > > +       if (IS_METEORLAKE(i915)) {
> > > > > +               /*
> > > > > +                * Workaround: access via BAR can hang MTL, go directly to
> DSM.
> > > > > +                *
> > > > > +                * Normally this would not work but on MTL the system
> firmware
> > > > > +                * should have relaxed the access permissions sufficiently.
> > > > > +                */
> > > > > +               io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) &
> > > GEN12_BDSM_MASK;
> > > > > +               io_size = dsm_size;
> > > > > +       } else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > > > >                 io_start = 0;
> > > > >                 io_size = 0;
> > > > >         } else {
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > > index 21a7e3191c18..ab71d74ec426 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > > @@ -24,6 +24,7 @@
> > > > >  #include "intel_ring.h"
> > > > >  #include "i915_drv.h"
> > > > >  #include "i915_pci.h"
> > > > > +#include "i915_reg.h"
> > > > >  #include "i915_request.h"
> > > > >  #include "i915_scatterlist.h"
> > > > >  #include "i915_utils.h"
> > > > > @@ -1152,13 +1153,23 @@ static unsigned int gen6_gttadr_offset(struct
> > > drm_i915_private *i915)
> > > > >  static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
> > > > >  {
> > > > >         struct drm_i915_private *i915 = ggtt->vm.i915;
> > > > > +       struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> > > > >         struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > > > >         phys_addr_t phys_addr;
> > > > >         u32 pte_flags;
> > > > >         int ret;
> > > > >
> > > > >         GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) !=
> > > gen6_gttmmadr_size(i915));
> > > > > -       phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
> > > gen6_gttadr_offset(i915);
> > > > > +       /*
> > > > > +        * Workaround: access via BAR can hang MTL, go directly to GSM.
> > > > > +        *
> > > > > +        * Normally this would not work but on MTL the system firmware
> > > > > +        * should have relaxed the access permissions sufficiently.
> > > > > +        */
> > > > > +       if (IS_METEORLAKE(i915))
> > > > > +               phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) &
> > > GEN12_BDSM_MASK;
> > > > > +       else
> > > > > +               phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR)
> +
> > > gen6_gttadr_offset(i915);
> > > > >
> > > > >         if (needs_wc_ggtt_mapping(i915))
> > > > >                 ggtt->gsm = ioremap_wc(phys_addr, size);
> > > > > --
> > > > > 2.41.0
> > > > >
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> 
> --
> Ville Syrjälä
> Intel

Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access

[Ville Syrjälä]

On Thu, Dec 14, 2023 at 09:06:03PM +0000, Sripada, Radhakrishna wrote:
> HI Ville,
> 
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Wednesday, December 13, 2023 6:03 PM
> > To: Sripada, Radhakrishna <radhakrishna.sripada@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Das, Nirmoy
> > <nirmoy.das@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL
> > stolen memory access
> > 
> > On Wed, Dec 13, 2023 at 08:18:15PM +0000, Sripada, Radhakrishna wrote:
> > > Hi Ville,
> > >
> > > +Nirmoy
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Sent: Wednesday, December 13, 2023 1:30 AM
> > > > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org; Sripada, Radhakrishna
> > > > <radhakrishna.sripada@intel.com>
> > > > Subject: Re: [PATCH 04/12] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL
> > > > stolen memory access
> > > >
> > > > On Wed, Dec 13, 2023 at 11:09:38AM +0200, Joonas Lahtinen wrote:
> > > > > Quoting Ville Syrjala (2023-12-13 02:42:29)
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > >
> > > > > > On MTL accessing stolen memory via the BARs is somehow borked,
> > > > > > and it can hang the machine. As a workaround let's bypass the
> > > > > > BARs and just go straight to DSMBASE/GSMBASE instead.
> > > > > >
> > > > > > Note that on every other platform this itself would hang the
> > > > > > machine, but on MTL the system firmware is expected to relax
> > > > > > the access permission guarding stolen memory to enable this
> > > > > > workaround, and thus direct CPU accesses should be fine.
> > > > >
> > > > > Shouldn't this have a proper workaround number assigned?
> > > >
> > > > I think there are various numbers, half of which I couldn't even
> > > > find in any database, and the other half I couldn't access for
> > > > whatever reason. So dunno what situation really is apart from
> > > > the firmware clearly implemening its part (since I can poke
> > > > DSM/GSM directly without killing the machine).
> > > >
> > > > RK do you know what we should call this?
> > > Nirmoy previously used Wa_22018444074 in [1].
> > >
> > > There were some associated issues Wa_13010847436 and Wa_14019519902
> > > which Nirmoy quoted in [2].
> > >
> > > Wa_22018529850 is derived from Wa_22018444074, is targeting the latest Gop
> > > driver change which is installed in bat-mtlp-8 hopefully it should help debug the
> > issue further.
> > >
> > >
> > > Regarding the patch itself,
> > > Do we need to carve out the range from e820 the area around DSM if we can
> > directly access the range from CPU
> > > without the bar?
> > 
> > IIRC we dropped the early quirks on new platforms under the
> > assumption that the BIOS no longer forgets to mark the DSM
> > as not-RAM/whatever. I don't think anything should change
> > there even when we are allowed direct CPU access.
> > 
> > But I don't recall if I double checked the e820 listing on
> > the MTL I was using. I was able to direct access to both DSM
> > and GSM for sure, and the address the GOP handed over to efifb
> > also pointed directly to DSM.
> 
> Up until adl-p/rpl, the PCI config space had the mirror registers for the stolen memory
> base and size, since the stolen meory is carved out of the available physical ram. Starting MTL
> this was removed from pci config space due to the introduction of stolen lmem which should not
> be cpu addressable aperture.
> 
> With the new gop driver allocating the FB memory in dram, should the e820 mark the FB area
> as reserved for the system use? Do we still preserve the efifb after doing a memtest before loading the driver?

While the LMEMBAR is a new thing, nothing else should really 
have changed in the way the BIOS carves out the DSM/GSM
from physical RAM.

On one MTL I see:
[    0.000896] e820: update [mem 0x7a000000-0xffffffff] usable ==> reserved
[    0.036542] [mem 0x7e800000-0xfed1ffff] available for PCI devices
[i915]] Memory region(6): stolen-local: 60 MiB [mem 0x00800000-0x043fffff], io: 60 MiB [mem 0x7a800000-0x7e3fffff]

So we have
 0x7a000000-0x7a800000 GSM
 0x7a800000-0x7e400000 DSM
both are marked as reserved and neither overlaps with that
PCI window. So looks all right to me.

We should probably also dump the GSM range to dmesg just to
have all the information available in one place...

> 
> Thanks,
> Radhakrishna(RK) Sripada 
> > 
> > >
> > >
> > > Thanks,
> > > Radhakrishna(RK) Sripada
> > > 1. https://patchwork.freedesktop.org/series/120683/
> > > 2. https://patchwork.freedesktop.org/series/123329/
> > >
> > > >
> > > > >
> > > > > Regards, Joonas
> > > > >
> > > > > >
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 11 ++++++++++-
> > > > > >  drivers/gpu/drm/i915/gt/intel_ggtt.c       | 13 ++++++++++++-
> > > > > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > > > index ee237043c302..252fe5cd6ede 100644
> > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > > > > > @@ -941,7 +941,16 @@ i915_gem_stolen_lmem_setup(struct
> > > > drm_i915_private *i915, u16 type,
> > > > > >                 dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
> > > > > >         }
> > > > > >
> > > > > > -       if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > > > > > +       if (IS_METEORLAKE(i915)) {
> > > > > > +               /*
> > > > > > +                * Workaround: access via BAR can hang MTL, go directly to
> > DSM.
> > > > > > +                *
> > > > > > +                * Normally this would not work but on MTL the system
> > firmware
> > > > > > +                * should have relaxed the access permissions sufficiently.
> > > > > > +                */
> > > > > > +               io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) &
> > > > GEN12_BDSM_MASK;
> > > > > > +               io_size = dsm_size;
> > > > > > +       } else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > > > > >                 io_start = 0;
> > > > > >                 io_size = 0;
> > > > > >         } else {
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > > > index 21a7e3191c18..ab71d74ec426 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > > > > > @@ -24,6 +24,7 @@
> > > > > >  #include "intel_ring.h"
> > > > > >  #include "i915_drv.h"
> > > > > >  #include "i915_pci.h"
> > > > > > +#include "i915_reg.h"
> > > > > >  #include "i915_request.h"
> > > > > >  #include "i915_scatterlist.h"
> > > > > >  #include "i915_utils.h"
> > > > > > @@ -1152,13 +1153,23 @@ static unsigned int gen6_gttadr_offset(struct
> > > > drm_i915_private *i915)
> > > > > >  static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
> > > > > >  {
> > > > > >         struct drm_i915_private *i915 = ggtt->vm.i915;
> > > > > > +       struct intel_uncore *uncore = ggtt->vm.gt->uncore;
> > > > > >         struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > > > > >         phys_addr_t phys_addr;
> > > > > >         u32 pte_flags;
> > > > > >         int ret;
> > > > > >
> > > > > >         GEM_WARN_ON(pci_resource_len(pdev, GEN4_GTTMMADR_BAR) !=
> > > > gen6_gttmmadr_size(i915));
> > > > > > -       phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) +
> > > > gen6_gttadr_offset(i915);
> > > > > > +       /*
> > > > > > +        * Workaround: access via BAR can hang MTL, go directly to GSM.
> > > > > > +        *
> > > > > > +        * Normally this would not work but on MTL the system firmware
> > > > > > +        * should have relaxed the access permissions sufficiently.
> > > > > > +        */
> > > > > > +       if (IS_METEORLAKE(i915))
> > > > > > +               phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) &
> > > > GEN12_BDSM_MASK;
> > > > > > +       else
> > > > > > +               phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR)
> > +
> > > > gen6_gttadr_offset(i915);
> > > > > >
> > > > > >         if (needs_wc_ggtt_mapping(i915))
> > > > > >                 ggtt->gsm = ioremap_wc(phys_addr, size);
> > > > > > --
> > > > > > 2.41.0
> > > > > >
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

[PATCH 05/12] drm/i915: Disable the "binder"

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Now that the GGTT PTE updates go straight to GSMBASE (bypassing
GTTMMADR) there should be no more risk of system hangs? So the
"binder" (ie. update the PTEs via MI_UPDATE_GTT) is no longer
necessary, disable it.

TODO: MI_UPDATE_GTT might be interesting as an optimization
though, so perhaps someone should look into always using it
(assuming the GPU is alive and well)?

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 86f73fe558ca..5bc7a4fb7485 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -24,7 +24,7 @@
 bool i915_ggtt_require_binder(struct drm_i915_private *i915)
 {
 	/* Wa_13010847436 & Wa_14019519902 */
-	return MEDIA_VER_FULL(i915) == IP_VER(13, 0);
+	return false && MEDIA_VER_FULL(i915) == IP_VER(13, 0);
 }
 
 static bool intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
-- 
2.41.0

[PATCH 06/12] drm/i915: Rename the DSM/GSM registers

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

0x108100 and 0x1080c0 have been around since snb. Rename the
defines appropriately.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c  | 4 ++--
 drivers/gpu/drm/i915/gt/intel_ggtt.c        | 2 +-
 drivers/gpu/drm/i915/gt/intel_region_lmem.c | 2 +-
 drivers/gpu/drm/i915/i915_reg.h             | 7 ++++---
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index 252fe5cd6ede..6185a5f73a48 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -935,7 +935,7 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915, u16 type,
 		GEM_BUG_ON((dsm_base + dsm_size) > lmem_size);
 	} else {
 		/* Use DSM base address instead for stolen memory */
-		dsm_base = intel_uncore_read64(uncore, GEN12_DSMBASE) & GEN12_BDSM_MASK;
+		dsm_base = intel_uncore_read64(uncore, GEN6_DSMBASE) & GEN11_BDSM_MASK;
 		if (WARN_ON(lmem_size < dsm_base))
 			return ERR_PTR(-ENODEV);
 		dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
@@ -948,7 +948,7 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915, u16 type,
 		 * Normally this would not work but on MTL the system firmware
 		 * should have relaxed the access permissions sufficiently.
 		 */
-		io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) & GEN12_BDSM_MASK;
+		io_start = intel_uncore_read64(uncore, GEN6_DSMBASE) & GEN11_BDSM_MASK;
 		io_size = dsm_size;
 	} else if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
 		io_start = 0;
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index ab71d74ec426..05c5525e7e2d 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -1167,7 +1167,7 @@ static int ggtt_probe_common(struct i915_ggtt *ggtt, u64 size)
 	 * should have relaxed the access permissions sufficiently.
 	 */
 	if (IS_METEORLAKE(i915))
-		phys_addr = intel_uncore_read64(uncore, GEN12_GSMBASE) & GEN12_BDSM_MASK;
+		phys_addr = intel_uncore_read64(uncore, GEN6_GSMBASE) & GEN11_BDSM_MASK;
 	else
 		phys_addr = pci_resource_start(pdev, GEN4_GTTMMADR_BAR) + gen6_gttadr_offset(i915);
 
diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
index af357089da6e..51bb27e10a4f 100644
--- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
+++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
@@ -240,7 +240,7 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)
 		lmem_size -= tile_stolen;
 	} else {
 		/* Stolen starts from GSMBASE without CCS */
-		lmem_size = intel_uncore_read64(&i915->uncore, GEN12_GSMBASE);
+		lmem_size = intel_uncore_read64(&i915->uncore, GEN6_GSMBASE);
 	}
 
 	i915_resize_lmem_bar(i915, lmem_size);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 27dc903f0553..b54d62952a53 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6314,9 +6314,10 @@ enum skl_power_gate {
 #define   GMS_MASK			REG_GENMASK(15, 8)
 #define   GGMS_MASK			REG_GENMASK(7, 6)
 
-#define GEN12_GSMBASE			_MMIO(0x108100)
-#define GEN12_DSMBASE			_MMIO(0x1080C0)
-#define   GEN12_BDSM_MASK		REG_GENMASK64(63, 20)
+#define GEN6_GSMBASE			_MMIO(0x108100)
+#define GEN6_DSMBASE			_MMIO(0x1080C0)
+#define   GEN6_BDSM_MASK		REG_GENMASK64(31, 20)
+#define   GEN11_BDSM_MASK		REG_GENMASK64(63, 20)
 
 #define XEHP_CLOCK_GATE_DIS		_MMIO(0x101014)
 #define   SGSI_SIDECLK_DIS		REG_BIT(17)
-- 
2.41.0

[PATCH 07/12] drm/i915: Fix PTE decode during initial plane readout

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When multiple pipes are enabled by the BIOS we try to read out each
in turn. But we do the readout for the second only after the inherited
vma for the first has been rebound into its original place (and thus
the PTEs have been rewritten). Unlike the BIOS we set some high caching
bits in the PTE on MTL which confuses the readout for the second plane.
Filter out the non-address bits from the PTE value appropriately to
fix this.

I suppose it might also be possible that the BIOS would already set
some caching bits as well, in which case we'd run into this same
issue already for the first plane.

TODO:
- should abstract the PTE decoding to avoid details leaking all over
- should probably do the readout for all the planes before
  we touch anything (including the PTEs) so that we truly read
  out the BIOS state

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_plane_initial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index a55c09cbd0e4..ffc92b18fcf5 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -72,7 +72,7 @@ initial_plane_vma(struct drm_i915_private *i915,
 			return NULL;
 		}
 
-		phys_base = pte & I915_GTT_PAGE_MASK;
+		phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK;
 		mem = i915->mm.regions[INTEL_REGION_LMEM_0];
 
 		/*
-- 
2.41.0

[PATCH 08/12] drm/i915: Fix region start during initial plane readout

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On MTL the stolen region starts at offset 8MiB from the start of
LMEMBAR. The dma addresses are thus also offset by 8MiB. However the
mm_node/etc. is zero based, and i915_pages_create_for_stolen() will
add the appropriate region.start into the sg dma address. So when
we do the readout we need to convert the dma address read from
the PTE to be zero based as well.

Note that currently we don't take this path on MTL, but we should
and thus this needs to be fixed. For lmem this works correctly
already as the lmem region.start==0.

While at it let's also make sure the address points to somewhere within
the memory region. We don't need to check the size as
i915_gem_object_create_region_at() should later fail if the object size
exceeds the region size.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_plane_initial.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index ffc92b18fcf5..db594ccf0323 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -79,16 +79,18 @@ initial_plane_vma(struct drm_i915_private *i915,
 		 * We don't currently expect this to ever be placed in the
 		 * stolen portion.
 		 */
-		if (phys_base >= resource_size(&mem->region)) {
+		if (phys_base < mem->region.start || phys_base > mem->region.end) {
 			drm_err(&i915->drm,
-				"Initial plane programming using invalid range, phys_base=%pa\n",
-				&phys_base);
+				"Initial plane programming using invalid range, phys_base=%pa (%s [%pa-%pa])\n",
+				&phys_base, mem->region.name, &mem->region.start, &mem->region.end);
 			return NULL;
 		}
 
 		drm_dbg(&i915->drm,
 			"Using phys_base=%pa, based on initial plane programming\n",
 			&phys_base);
+
+		phys_base -= mem->region.start;
 	} else {
 		phys_base = base;
 		mem = i915->mm.stolen_region;
-- 
2.41.0

[PATCH 09/12] drm/i915: Fix MTL initial plane readout

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

MTL stolen memory looks more like local memory, so use the
(now fixed) lmem path when doing the initial plane readout.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../drm/i915/display/intel_plane_initial.c    | 25 +++++++++++++------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index db594ccf0323..c72d4cacf631 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -59,7 +59,7 @@ initial_plane_vma(struct drm_i915_private *i915,
 		return NULL;
 
 	base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
-	if (IS_DGFX(i915)) {
+	if (IS_DGFX(i915) || HAS_LMEMBAR_SMEM_STOLEN(i915)) {
 		gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
 		gen8_pte_t pte;
 
@@ -73,11 +73,20 @@ initial_plane_vma(struct drm_i915_private *i915,
 		}
 
 		phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK;
-		mem = i915->mm.regions[INTEL_REGION_LMEM_0];
+
+		if (IS_DGFX(i915))
+			mem = i915->mm.regions[INTEL_REGION_LMEM_0];
+		else
+			mem = i915->mm.stolen_region;
+		if (!mem) {
+			drm_dbg_kms(&i915->drm,
+				    "Initial plane memory region not initialized\n");
+			return NULL;
+		}
 
 		/*
-		 * We don't currently expect this to ever be placed in the
-		 * stolen portion.
+		 * On lmem we don't currently expect this to
+		 * ever be placed in the stolen portion.
 		 */
 		if (phys_base < mem->region.start || phys_base > mem->region.end) {
 			drm_err(&i915->drm,
@@ -94,11 +103,13 @@ initial_plane_vma(struct drm_i915_private *i915,
 	} else {
 		phys_base = base;
 		mem = i915->mm.stolen_region;
+		if (!mem) {
+			drm_dbg_kms(&i915->drm,
+				    "Initial plane memory region not initialized\n");
+			return NULL;
+		}
 	}
 
-	if (!mem)
-		return NULL;
-
 	size = round_up(plane_config->base + plane_config->size,
 			mem->min_page_size);
 	size -= base;
-- 
2.41.0

[PATCH 10/12] drm/i915: s/phys_base/dma_addr/

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The address we read from the PTE is a dma address, not a physical
address. Rename the variable to say so.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../gpu/drm/i915/display/intel_plane_initial.c    | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index c72d4cacf631..48b74319f45c 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -61,6 +61,7 @@ initial_plane_vma(struct drm_i915_private *i915,
 	base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
 	if (IS_DGFX(i915) || HAS_LMEMBAR_SMEM_STOLEN(i915)) {
 		gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
+		dma_addr_t dma_addr;
 		gen8_pte_t pte;
 
 		gte += base / I915_GTT_PAGE_SIZE;
@@ -72,7 +73,7 @@ initial_plane_vma(struct drm_i915_private *i915,
 			return NULL;
 		}
 
-		phys_base = pte & GEN12_GGTT_PTE_ADDR_MASK;
+		dma_addr = pte & GEN12_GGTT_PTE_ADDR_MASK;
 
 		if (IS_DGFX(i915))
 			mem = i915->mm.regions[INTEL_REGION_LMEM_0];
@@ -88,18 +89,18 @@ initial_plane_vma(struct drm_i915_private *i915,
 		 * On lmem we don't currently expect this to
 		 * ever be placed in the stolen portion.
 		 */
-		if (phys_base < mem->region.start || phys_base > mem->region.end) {
+		if (dma_addr < mem->region.start || dma_addr > mem->region.end) {
 			drm_err(&i915->drm,
-				"Initial plane programming using invalid range, phys_base=%pa (%s [%pa-%pa])\n",
-				&phys_base, mem->region.name, &mem->region.start, &mem->region.end);
+				"Initial plane programming using invalid range, dma_addr=%pa (%s [%pa-%pa])\n",
+				&dma_addr, mem->region.name, &mem->region.start, &mem->region.end);
 			return NULL;
 		}
 
 		drm_dbg(&i915->drm,
-			"Using phys_base=%pa, based on initial plane programming\n",
-			&phys_base);
+			"Using dma_addr=%pa, based on initial plane programming\n",
+			&dma_addr);
 
-		phys_base -= mem->region.start;
+		phys_base = dma_addr - mem->region.start;
 	} else {
 		phys_base = base;
 		mem = i915->mm.stolen_region;
-- 
2.41.0

[PATCH 11/12] drm/i915: Split the smem and lmem plane readout apart

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Declutter initial_plane_vma() a bit by pulling the lmem and smem
readout paths into their own functions.

TODO: the smem path should still be fixed to get and validate
      the dma address from the pte as well

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../drm/i915/display/intel_display_types.h    |   2 +
 .../drm/i915/display/intel_plane_initial.c    | 145 +++++++++++-------
 2 files changed, 95 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 341d80c2b9de..d2b0cc754667 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -782,6 +782,8 @@ struct intel_plane_state {
 
 struct intel_initial_plane_config {
 	struct intel_framebuffer *fb;
+	struct intel_memory_region *mem;
+	resource_size_t phys_base;
 	struct i915_vma *vma;
 	unsigned int tiling;
 	int size;
diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index 48b74319f45c..78bff6181ceb 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -44,6 +44,93 @@ intel_reuse_initial_plane_obj(struct drm_i915_private *i915,
 	return false;
 }
 
+static bool
+initial_plane_phys_lmem(struct drm_i915_private *i915,
+			struct intel_initial_plane_config *plane_config)
+{
+	gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
+	struct intel_memory_region *mem;
+	dma_addr_t dma_addr;
+	gen8_pte_t pte;
+	u32 base;
+
+	base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
+
+	gte += base / I915_GTT_PAGE_SIZE;
+
+	pte = ioread64(gte);
+	if (!(pte & GEN12_GGTT_PTE_LM)) {
+		drm_err(&i915->drm,
+			"Initial plane programming missing PTE_LM bit\n");
+		return false;
+	}
+
+	dma_addr = pte & GEN12_GGTT_PTE_ADDR_MASK;
+
+	if (IS_DGFX(i915))
+		mem = i915->mm.regions[INTEL_REGION_LMEM_0];
+	else
+		mem = i915->mm.stolen_region;
+	if (!mem) {
+		drm_dbg_kms(&i915->drm,
+			    "Initial plane memory region not initialized\n");
+		return false;
+	}
+
+	/*
+	 * On lmem we don't currently expect this to
+	 * ever be placed in the stolen portion.
+	 */
+	if (dma_addr < mem->region.start || dma_addr > mem->region.end) {
+		drm_err(&i915->drm,
+			"Initial plane programming using invalid range, dma_addr=%pa (%s [%pa-%pa])\n",
+			&dma_addr, mem->region.name, &mem->region.start, &mem->region.end);
+		return false;
+	}
+
+	drm_dbg(&i915->drm,
+		"Using dma_addr=%pa, based on initial plane programming\n",
+		&dma_addr);
+
+	plane_config->phys_base = dma_addr - mem->region.start;
+	plane_config->mem = mem;
+
+	return true;
+}
+
+static bool
+initial_plane_phys_smem(struct drm_i915_private *i915,
+			struct intel_initial_plane_config *plane_config)
+{
+	struct intel_memory_region *mem;
+	u32 base;
+
+	base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
+
+	mem = i915->mm.stolen_region;
+	if (!mem) {
+		drm_dbg_kms(&i915->drm,
+			    "Initial plane memory region not initialized\n");
+		return false;
+	}
+
+	/* FIXME get and validate the dma_addr from the PTE */
+	plane_config->phys_base = base;
+	plane_config->mem = mem;
+
+	return true;
+}
+
+static bool
+initial_plane_phys(struct drm_i915_private *i915,
+		   struct intel_initial_plane_config *plane_config)
+{
+	if (IS_DGFX(i915) || HAS_LMEMBAR_SMEM_STOLEN(i915))
+		return initial_plane_phys_lmem(i915, plane_config);
+	else
+		return initial_plane_phys_smem(i915, plane_config);
+}
+
 static struct i915_vma *
 initial_plane_vma(struct drm_i915_private *i915,
 		  struct intel_initial_plane_config *plane_config)
@@ -58,59 +145,13 @@ initial_plane_vma(struct drm_i915_private *i915,
 	if (plane_config->size == 0)
 		return NULL;
 
+	if (!initial_plane_phys(i915, plane_config))
+		return NULL;
+
+	phys_base = plane_config->phys_base;
+	mem = plane_config->mem;
+
 	base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
-	if (IS_DGFX(i915) || HAS_LMEMBAR_SMEM_STOLEN(i915)) {
-		gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
-		dma_addr_t dma_addr;
-		gen8_pte_t pte;
-
-		gte += base / I915_GTT_PAGE_SIZE;
-
-		pte = ioread64(gte);
-		if (!(pte & GEN12_GGTT_PTE_LM)) {
-			drm_err(&i915->drm,
-				"Initial plane programming missing PTE_LM bit\n");
-			return NULL;
-		}
-
-		dma_addr = pte & GEN12_GGTT_PTE_ADDR_MASK;
-
-		if (IS_DGFX(i915))
-			mem = i915->mm.regions[INTEL_REGION_LMEM_0];
-		else
-			mem = i915->mm.stolen_region;
-		if (!mem) {
-			drm_dbg_kms(&i915->drm,
-				    "Initial plane memory region not initialized\n");
-			return NULL;
-		}
-
-		/*
-		 * On lmem we don't currently expect this to
-		 * ever be placed in the stolen portion.
-		 */
-		if (dma_addr < mem->region.start || dma_addr > mem->region.end) {
-			drm_err(&i915->drm,
-				"Initial plane programming using invalid range, dma_addr=%pa (%s [%pa-%pa])\n",
-				&dma_addr, mem->region.name, &mem->region.start, &mem->region.end);
-			return NULL;
-		}
-
-		drm_dbg(&i915->drm,
-			"Using dma_addr=%pa, based on initial plane programming\n",
-			&dma_addr);
-
-		phys_base = dma_addr - mem->region.start;
-	} else {
-		phys_base = base;
-		mem = i915->mm.stolen_region;
-		if (!mem) {
-			drm_dbg_kms(&i915->drm,
-				    "Initial plane memory region not initialized\n");
-			return NULL;
-		}
-	}
-
 	size = round_up(plane_config->base + plane_config->size,
 			mem->min_page_size);
 	size -= base;
-- 
2.41.0

[PATCH 12/12] drm/i915: Simplify intel_initial_plane_config() calling convention

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There's no reason the caller of intel_initial_plane_config() should
have to loop over the CRTCs. Pull the loop into the function to
make life simpler for the caller.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../drm/i915/display/intel_display_driver.c   |  7 +---
 .../drm/i915/display/intel_plane_initial.c    | 40 +++++++++++--------
 .../drm/i915/display/intel_plane_initial.h    |  4 +-
 3 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
index 62f7b10484be..2fe0f4ad359c 100644
--- a/drivers/gpu/drm/i915/display/intel_display_driver.c
+++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
@@ -285,7 +285,6 @@ int intel_display_driver_probe_nogem(struct drm_i915_private *i915)
 {
 	struct drm_device *dev = &i915->drm;
 	enum pipe pipe;
-	struct intel_crtc *crtc;
 	int ret;
 
 	if (!HAS_DISPLAY(i915))
@@ -335,11 +334,7 @@ int intel_display_driver_probe_nogem(struct drm_i915_private *i915)
 	intel_acpi_assign_connector_fwnodes(i915);
 	drm_modeset_unlock_all(dev);
 
-	for_each_intel_crtc(dev, crtc) {
-		if (!to_intel_crtc_state(crtc->base.state)->uapi.active)
-			continue;
-		intel_crtc_initial_plane_config(crtc);
-	}
+	intel_initial_plane_config(i915);
 
 	/*
 	 * Make sure hardware watermarks really match the state we read out.
diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index 78bff6181ceb..b7e12b60d68b 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -357,25 +357,31 @@ static void plane_config_fini(struct intel_initial_plane_config *plane_config)
 		i915_vma_put(plane_config->vma);
 }
 
-void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
+void intel_initial_plane_config(struct drm_i915_private *i915)
 {
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	struct intel_initial_plane_config plane_config = {};
+	struct intel_crtc *crtc;
 
-	/*
-	 * Note that reserving the BIOS fb up front prevents us
-	 * from stuffing other stolen allocations like the ring
-	 * on top.  This prevents some ugliness at boot time, and
-	 * can even allow for smooth boot transitions if the BIOS
-	 * fb is large enough for the active pipe configuration.
-	 */
-	dev_priv->display.funcs.display->get_initial_plane_config(crtc, &plane_config);
+	for_each_intel_crtc(&i915->drm, crtc) {
+		struct intel_initial_plane_config plane_config = {};
 
-	/*
-	 * If the fb is shared between multiple heads, we'll
-	 * just get the first one.
-	 */
-	intel_find_initial_plane_obj(crtc, &plane_config);
+		if (!to_intel_crtc_state(crtc->base.state)->uapi.active)
+			continue;
 
-	plane_config_fini(&plane_config);
+		/*
+		 * Note that reserving the BIOS fb up front prevents us
+		 * from stuffing other stolen allocations like the ring
+		 * on top.  This prevents some ugliness at boot time, and
+		 * can even allow for smooth boot transitions if the BIOS
+		 * fb is large enough for the active pipe configuration.
+		 */
+		i915->display.funcs.display->get_initial_plane_config(crtc, &plane_config);
+
+		/*
+		 * If the fb is shared between multiple heads, we'll
+		 * just get the first one.
+		 */
+		intel_find_initial_plane_obj(crtc, &plane_config);
+
+		plane_config_fini(&plane_config);
+	}
 }
diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.h b/drivers/gpu/drm/i915/display/intel_plane_initial.h
index c7e35ab3182b..64ab95239cd4 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.h
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.h
@@ -6,8 +6,8 @@
 #ifndef __INTEL_PLANE_INITIAL_H__
 #define __INTEL_PLANE_INITIAL_H__
 
-struct intel_crtc;
+struct drm_i915_private;
 
-void intel_crtc_initial_plane_config(struct intel_crtc *crtc);
+void intel_initial_plane_config(struct drm_i915_private *i915);
 
 #endif
-- 
2.41.0

✗ Fi.CI.CHECKPATCH: warning for drm/i915: (stolen) memory region related fixes

[Patchwork]

== Series Details ==

Series: drm/i915: (stolen) memory region related fixes
URL   : https://patchwork.freedesktop.org/series/127721/
State : warning

== Summary ==

Error: dim checkpatch failed
57aa292bab73 drm/i915: Use struct resource for memory region IO as well
-:385: WARNING:LONG_LINE: line length of 105 exceeds 100 columns
#385: FILE: drivers/gpu/drm/i915/intel_region_ttm.c:227:
+			if (WARN_ON(overflows_type(resource_size(&mem->io) >> PAGE_SHIFT, place.lpfn))) {

total: 0 errors, 1 warnings, 0 checks, 281 lines checked
6f1daec92bb9 drm/i915: Print memory region info during probe
5bbcb4180a39 drm/i915: Remove ad-hoc lmem/stolen debugs
3cac577dc645 drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access
ddb1a05b1d8d drm/i915: Disable the "binder"
4ee33d48194e drm/i915: Rename the DSM/GSM registers
b34d6c789845 drm/i915: Fix PTE decode during initial plane readout
225e329229bd drm/i915: Fix region start during initial plane readout
24bf9674cb6f drm/i915: Fix MTL initial plane readout
30b9f4a02a3b drm/i915: s/phys_base/dma_addr/
8e6abaa24f65 drm/i915: Split the smem and lmem plane readout apart
5da25c7a5304 drm/i915: Simplify intel_initial_plane_config() calling convention

✗ Fi.CI.SPARSE: warning for drm/i915: (stolen) memory region related fixes

[Patchwork]

== Series Details ==

Series: drm/i915: (stolen) memory region related fixes
URL   : https://patchwork.freedesktop.org/series/127721/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 12846 bytes --]

== Series Details ==

Series: drm/i915: (stolen) memory region related fixes
URL   : https://patchwork.freedesktop.org/series/127721/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_127721v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_127721v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html

Participating hosts (31 -> 33)
------------------------------

  Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510 
  Missing    (2): bat-adlp-11 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_127721v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - bat-mtlp-8:         NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@load.html

  * igt@i915_selftest@live@gem_contexts:
    - bat-atsm-1:         [PASS][2] -> [INCOMPLETE][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14010/bat-atsm-1/igt@i915_selftest@live@gem_contexts.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-atsm-1/igt@i915_selftest@live@gem_contexts.html

  
Known issues
------------

  Here are the changes found in Patchwork_127721v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s0@smem:
    - bat-dg2-9:          NOTRUN -> [INCOMPLETE][4] ([i915#9275])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@gem_lmem_swapping@basic:
    - fi-pnv-d510:        NOTRUN -> [SKIP][5] ([fdo#109271]) +28 other tests skip
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-pnv-d510/igt@gem_lmem_swapping@basic.html

  * igt@gem_mmap@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][6] ([i915#4083])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_mmap@basic.html
    - bat-dg2-8:          NOTRUN -> [SKIP][7] ([i915#4083])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][8] ([i915#4077]) +2 other tests skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_mmap_gtt@basic.html
    - bat-dg2-8:          NOTRUN -> [SKIP][9] ([i915#4077]) +2 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][10] ([i915#4079]) +1 other test skip
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-dg2-8:          NOTRUN -> [SKIP][11] ([i915#4079]) +1 other test skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-9:          NOTRUN -> [SKIP][12] ([i915#6621])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@i915_pm_rps@basic-api.html
    - bat-dg2-8:          NOTRUN -> [SKIP][13] ([i915#6621])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@i915_pm_rps@basic-api.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-dg2-8:          NOTRUN -> [SKIP][14] ([i915#6645])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][15] ([i915#5190])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html
    - bat-dg2-8:          NOTRUN -> [SKIP][16] ([i915#5190])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][17] ([i915#4215] / [i915#5190])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@basic-y-tiled-legacy.html
    - bat-dg2-8:          NOTRUN -> [SKIP][18] ([i915#4215] / [i915#5190])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@framebuffer-vs-set-tiling:
    - bat-dg2-9:          NOTRUN -> [SKIP][19] ([i915#4212]) +6 other tests skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html
    - bat-dg2-8:          NOTRUN -> [SKIP][20] ([i915#4212]) +6 other tests skip
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html

  * igt@kms_addfb_basic@tile-pitch-mismatch:
    - bat-dg2-9:          NOTRUN -> [SKIP][21] ([i915#4212] / [i915#5608])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@tile-pitch-mismatch.html
    - bat-dg2-8:          NOTRUN -> [SKIP][22] ([i915#4212] / [i915#5608])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@tile-pitch-mismatch.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][23] ([i915#4103] / [i915#4213] / [i915#5608]) +1 other test skip
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - bat-dg2-8:          NOTRUN -> [SKIP][24] ([i915#4103] / [i915#4213] / [i915#5608]) +1 other test skip
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg2-9:          NOTRUN -> [SKIP][25] ([fdo#109285])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_force_connector_basic@force-load-detect.html
    - bat-dg2-8:          NOTRUN -> [SKIP][26] ([fdo#109285])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-dg2-9:          NOTRUN -> [SKIP][27] ([i915#5274])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_force_connector_basic@prune-stale-modes.html
    - bat-dg2-8:          NOTRUN -> [SKIP][28] ([i915#5274])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - fi-ivb-3770:        NOTRUN -> [SKIP][29] ([fdo#109271])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-ivb-3770/igt@kms_pipe_crc_basic@suspend-read-crc.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-dg2-8:          NOTRUN -> [SKIP][30] ([i915#5354])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_pm_backlight@basic-brightness.html
    - bat-dg2-9:          NOTRUN -> [SKIP][31] ([i915#5354])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg2-9:          NOTRUN -> [SKIP][32] ([i915#3555])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-dg2-8:          NOTRUN -> [SKIP][33] ([i915#3555])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-dg2-9:          NOTRUN -> [SKIP][34] ([i915#3708])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-fence-flip.html
    - bat-dg2-8:          NOTRUN -> [SKIP][35] ([i915#3708])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-dg2-8:          NOTRUN -> [SKIP][36] ([i915#3708] / [i915#4077]) +1 other test skip
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-fence-mmap.html
    - bat-dg2-9:          NOTRUN -> [SKIP][37] ([i915#3708] / [i915#4077]) +1 other test skip
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-write:
    - bat-dg2-9:          NOTRUN -> [SKIP][38] ([i915#3291] / [i915#3708]) +2 other tests skip
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-write.html
    - bat-dg2-8:          NOTRUN -> [SKIP][39] ([i915#3291] / [i915#3708]) +2 other tests skip
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-glk-j4005:       [INCOMPLETE][40] -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14010/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5608]: https://gitlab.freedesktop.org/drm/intel/issues/5608
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#9275]: https://gitlab.freedesktop.org/drm/intel/issues/9275
  [i915#9673]: https://gitlab.freedesktop.org/drm/intel/issues/9673


Build changes
-------------

  * Linux: CI_DRM_14010 -> Patchwork_127721v1

  CI-20190529: 20190529
  CI_DRM_14010: b4182ec1538e8cebf630083ec4296bed0061d594 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7635: 0b796be8ce05cb2070ce5136d248f438c962d11e @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_127721v1: b4182ec1538e8cebf630083ec4296bed0061d594 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

499717049180 drm/i915: Simplify intel_initial_plane_config() calling convention
3f12c223d707 drm/i915: Split the smem and lmem plane readout apart
2d21369e26a3 drm/i915: s/phys_base/dma_addr/
be622e53615d drm/i915: Fix MTL initial plane readout
8ad9926fb883 drm/i915: Fix region start during initial plane readout
55b4a59fce27 drm/i915: Fix PTE decode during initial plane readout
db38b8a8227c drm/i915: Rename the DSM/GSM registers
f7b2179e3b69 drm/i915: Disable the "binder"
77e7c3085969 drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access
ef59e22db9f6 drm/i915: Remove ad-hoc lmem/stolen debugs
b001bcb6a48f drm/i915: Print memory region info during probe
5b54e278890d drm/i915: Use struct resource for memory region IO as well

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html

[-- Attachment #2: Type: text/html, Size: 16331 bytes --]

Re: â Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes

[Andrzej Hajda]

On 13.12.2023 02:37, Patchwork wrote:
> *Patch Details*
> *Series:*	drm/i915: (stolen) memory region related fixes
> *URL:*	https://patchwork.freedesktop.org/series/127721/ 
> <https://patchwork.freedesktop.org/series/127721/>
> *State:*	failure
> *Details:* 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html 
> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html>
> 
> 
>   CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1
> 
> 
>     Summary
> 
> *FAILURE*
> 
> Serious unknown changes coming with Patchwork_127721v1 absolutely need to be
> verified manually.
> 
> If you think the reported changes have nothing to do with the changes
> introduced in Patchwork_127721v1, please notify your bug team 
> (I915-ci-infra@lists.freedesktop.org) to allow them
> to document this new failure mode, which will reduce false positives in CI.
> 
> External URL: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html
> 
> 
>     Participating hosts (31 -> 33)
> 
> Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510
> Missing (2): bat-adlp-11 fi-snb-2520m
> 
> 
>     Possible new issues
> 
> Here are the unknown changes that may have been introduced in 
> Patchwork_127721v1:
> 
> 
>       IGT changes
> 
> 
>         Possible regressions
> 
>   *
> 
>     igt@i915_module_load@load:
> 
>       o bat-mtlp-8: NOTRUN -> INCOMPLETE
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@load.html>


This is due to overlapping initial fb's vma with GuC reserved area on 
MTL, see ggtt_reserve_guc_top.

My dirty quick_fix proposed:
@@ -143,6 +143,9 @@ initial_plane_vma(struct drm_i915_private *i915,
         if (IS_ERR(vma))
                 goto err_obj;

+       if (base + size > GUC_GGTT_TOP)
+               base = min(base, GUC_GGTT_TOP) - size;
+
         pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
         if (HAS_GMCH(i915))
                 pinctl |= PIN_MAPPABLE;


Regards
Andrzej


>   *
> 
>     igt@i915_selftest@live@gem_contexts:
> 
>       o bat-atsm-1: PASS
>         <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14010/bat-atsm-1/igt@i915_selftest@live@gem_contexts.html> -> INCOMPLETE <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-atsm-1/igt@i915_selftest@live@gem_contexts.html>
> 
> 
>     Known issues
> 
> Here are the changes found in Patchwork_127721v1 that come from known 
> issues:
> 
> 
>       IGT changes
> 
> 
>         Issues hit
> 
>   *
> 
>     igt@gem_exec_suspend@basic-s0@smem:
> 
>       o bat-dg2-9: NOTRUN -> INCOMPLETE
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_exec_suspend@basic-s0@smem.html> (i915#9275 <https://gitlab.freedesktop.org/drm/intel/issues/9275>)
>   *
> 
>     igt@gem_lmem_swapping@basic:
> 
>       o fi-pnv-d510: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-pnv-d510/igt@gem_lmem_swapping@basic.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +28 other tests skip
>   *
> 
>     igt@gem_mmap@basic:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_mmap@basic.html> (i915#4083 <https://gitlab.freedesktop.org/drm/intel/issues/4083>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_mmap@basic.html> (i915#4083 <https://gitlab.freedesktop.org/drm/intel/issues/4083>)
> 
>   *
> 
>     igt@gem_mmap_gtt@basic:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_mmap_gtt@basic.html> (i915#4077 <https://gitlab.freedesktop.org/drm/intel/issues/4077>) +2 other tests skip
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_mmap_gtt@basic.html> (i915#4077 <https://gitlab.freedesktop.org/drm/intel/issues/4077>) +2 other tests skip
> 
>   *
> 
>     igt@gem_render_tiled_blits@basic:
> 
>       o bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_render_tiled_blits@basic.html> (i915#4079 <https://gitlab.freedesktop.org/drm/intel/issues/4079>) +1 other test skip
>   *
> 
>     igt@gem_tiled_pread_basic:
> 
>       o bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_tiled_pread_basic.html> (i915#4079 <https://gitlab.freedesktop.org/drm/intel/issues/4079>) +1 other test skip
>   *
> 
>     igt@i915_pm_rps@basic-api:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@i915_pm_rps@basic-api.html> (i915#6621 <https://gitlab.freedesktop.org/drm/intel/issues/6621>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@i915_pm_rps@basic-api.html> (i915#6621 <https://gitlab.freedesktop.org/drm/intel/issues/6621>)
> 
>   *
> 
>     igt@i915_suspend@basic-s3-without-i915:
> 
>       o bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@i915_suspend@basic-s3-without-i915.html> (i915#6645 <https://gitlab.freedesktop.org/drm/intel/issues/6645>)
>   *
> 
>     igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html> (i915#5190 <https://gitlab.freedesktop.org/drm/intel/issues/5190>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html> (i915#5190 <https://gitlab.freedesktop.org/drm/intel/issues/5190>)
> 
>   *
> 
>     igt@kms_addfb_basic@basic-y-tiled-legacy:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@basic-y-tiled-legacy.html> (i915#4215 <https://gitlab.freedesktop.org/drm/intel/issues/4215> / i915#5190 <https://gitlab.freedesktop.org/drm/intel/issues/5190>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html> (i915#4215 <https://gitlab.freedesktop.org/drm/intel/issues/4215> / i915#5190 <https://gitlab.freedesktop.org/drm/intel/issues/5190>)
> 
>   *
> 
>     igt@kms_addfb_basic@framebuffer-vs-set-tiling:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html> (i915#4212 <https://gitlab.freedesktop.org/drm/intel/issues/4212>) +6 other tests skip
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html> (i915#4212 <https://gitlab.freedesktop.org/drm/intel/issues/4212>) +6 other tests skip
> 
>   *
> 
>     igt@kms_addfb_basic@tile-pitch-mismatch:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@tile-pitch-mismatch.html> (i915#4212 <https://gitlab.freedesktop.org/drm/intel/issues/4212> / i915#5608 <https://gitlab.freedesktop.org/drm/intel/issues/5608>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@tile-pitch-mismatch.html> (i915#4212 <https://gitlab.freedesktop.org/drm/intel/issues/4212> / i915#5608 <https://gitlab.freedesktop.org/drm/intel/issues/5608>)
> 
>   *
> 
>     igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html> (i915#4103 <https://gitlab.freedesktop.org/drm/intel/issues/4103> / i915#4213 <https://gitlab.freedesktop.org/drm/intel/issues/4213> / i915#5608 <https://gitlab.freedesktop.org/drm/intel/issues/5608>) +1 other test skip
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html> (i915#4103 <https://gitlab.freedesktop.org/drm/intel/issues/4103> / i915#4213 <https://gitlab.freedesktop.org/drm/intel/issues/4213> / i915#5608 <https://gitlab.freedesktop.org/drm/intel/issues/5608>) +1 other test skip
> 
>   *
> 
>     igt@kms_force_connector_basic@force-load-detect:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_force_connector_basic@force-load-detect.html> (fdo#109285 <https://bugs.freedesktop.org/show_bug.cgi?id=109285>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_force_connector_basic@force-load-detect.html> (fdo#109285 <https://bugs.freedesktop.org/show_bug.cgi?id=109285>)
> 
>   *
> 
>     igt@kms_force_connector_basic@prune-stale-modes:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_force_connector_basic@prune-stale-modes.html> (i915#5274 <https://gitlab.freedesktop.org/drm/intel/issues/5274>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_force_connector_basic@prune-stale-modes.html> (i915#5274 <https://gitlab.freedesktop.org/drm/intel/issues/5274>)
> 
>   *
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc:
> 
>       o fi-ivb-3770: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-ivb-3770/igt@kms_pipe_crc_basic@suspend-read-crc.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271>)
>   *
> 
>     igt@kms_pm_backlight@basic-brightness:
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_pm_backlight@basic-brightness.html> (i915#5354 <https://gitlab.freedesktop.org/drm/intel/issues/5354>)
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_pm_backlight@basic-brightness.html> (i915#5354 <https://gitlab.freedesktop.org/drm/intel/issues/5354>)
> 
>   *
> 
>     igt@kms_setmode@basic-clone-single-crtc:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_setmode@basic-clone-single-crtc.html> (i915#3555 <https://gitlab.freedesktop.org/drm/intel/issues/3555>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_setmode@basic-clone-single-crtc.html> (i915#3555 <https://gitlab.freedesktop.org/drm/intel/issues/3555>)
> 
>   *
> 
>     igt@prime_vgem@basic-fence-flip:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-fence-flip.html> (i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708>)
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-fence-flip.html> (i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708>)
> 
>   *
> 
>     igt@prime_vgem@basic-fence-mmap:
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-fence-mmap.html> (i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708> / i915#4077 <https://gitlab.freedesktop.org/drm/intel/issues/4077>) +1 other test skip
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-fence-mmap.html> (i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708> / i915#4077 <https://gitlab.freedesktop.org/drm/intel/issues/4077>) +1 other test skip
> 
>   *
> 
>     igt@prime_vgem@basic-write:
> 
>       o
> 
>         bat-dg2-9: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-write.html> (i915#3291 <https://gitlab.freedesktop.org/drm/intel/issues/3291> / i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708>) +2 other tests skip
> 
>       o
> 
>         bat-dg2-8: NOTRUN -> SKIP
>         <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-write.html> (i915#3291 <https://gitlab.freedesktop.org/drm/intel/issues/3291> / i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708>) +2 other tests skip
> 
> 
>         Possible fixes
> 
>   * igt@i915_selftest@live@gt_heartbeat:
>       o fi-glk-j4005: INCOMPLETE
>         <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14010/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html> -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html>
> 
> {name}: This element is suppressed. This means it is ignored when computing
> the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
> 
>     Build changes
> 
>   * Linux: CI_DRM_14010 -> Patchwork_127721v1
> 
> CI-20190529: 20190529
> CI_DRM_14010: b4182ec1538e8cebf630083ec4296bed0061d594 @ 
> git://anongit.freedesktop.org/gfx-ci/linux
> IGT_7635: 0b796be8ce05cb2070ce5136d248f438c962d11e @ 
> https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
> Patchwork_127721v1: b4182ec1538e8cebf630083ec4296bed0061d594 @ 
> git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
>       Linux commits
> 
> 499717049180 drm/i915: Simplify intel_initial_plane_config() calling 
> convention
> 3f12c223d707 drm/i915: Split the smem and lmem plane readout apart
> 2d21369e26a3 drm/i915: s/phys_base/dma_addr/
> be622e53615d drm/i915: Fix MTL initial plane readout
> 8ad9926fb883 drm/i915: Fix region start during initial plane readout
> 55b4a59fce27 drm/i915: Fix PTE decode during initial plane readout
> db38b8a8227c drm/i915: Rename the DSM/GSM registers
> f7b2179e3b69 drm/i915: Disable the "binder"
> 77e7c3085969 drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access
> ef59e22db9f6 drm/i915: Remove ad-hoc lmem/stolen debugs
> b001bcb6a48f drm/i915: Print memory region info during probe
> 5b54e278890d drm/i915: Use struct resource for memory region IO as well
> 

Re: â Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes

[Tvrtko Ursulin]

Hi Andrzej,

On 13/12/2023 16:13, Andrzej Hajda wrote:
> On 13.12.2023 02:37, Patchwork wrote:
>> *Patch Details*
>> *Series:*    drm/i915: (stolen) memory region related fixes
>> *URL:*    https://patchwork.freedesktop.org/series/127721/ 
>> <https://patchwork.freedesktop.org/series/127721/>
>> *State:*    failure
>> *Details:* 
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html 
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html>
>>
>>
>>   CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1
>>
>>
>>     Summary
>>
>> *FAILURE*
>>
>> Serious unknown changes coming with Patchwork_127721v1 absolutely need 
>> to be
>> verified manually.
>>
>> If you think the reported changes have nothing to do with the changes
>> introduced in Patchwork_127721v1, please notify your bug team 
>> (I915-ci-infra@lists.freedesktop.org) to allow them
>> to document this new failure mode, which will reduce false positives 
>> in CI.
>>
>> External URL: 
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html
>>
>>
>>     Participating hosts (31 -> 33)
>>
>> Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510
>> Missing (2): bat-adlp-11 fi-snb-2520m
>>
>>
>>     Possible new issues
>>
>> Here are the unknown changes that may have been introduced in 
>> Patchwork_127721v1:
>>
>>
>>       IGT changes
>>
>>
>>         Possible regressions
>>
>>   *
>>
>>     igt@i915_module_load@load:
>>
>>       o bat-mtlp-8: NOTRUN -> INCOMPLETE
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@load.html>
> 
> 
> This is due to overlapping initial fb's vma with GuC reserved area on 
> MTL, see ggtt_reserve_guc_top.
> 
> My dirty quick_fix proposed:
> @@ -143,6 +143,9 @@ initial_plane_vma(struct drm_i915_private *i915,
>          if (IS_ERR(vma))
>                  goto err_obj;
> 
> +       if (base + size > GUC_GGTT_TOP)
> +               base = min(base, GUC_GGTT_TOP) - size;
> +

I saw you posting this before but forgot to comment. I couldn't quite 
figure out what the logic is supposed to do and how it wouldn't fail to 
inherit any firmware setup splash screen?

Other than making the firmware aware and not use that range, I was 
thinking what else we could do in i915? Copy the fb to a non-conflicting 
location and re-program the display engine? If that is doable without 
visible glitching and can be fast enough not to slow the boot a lot.

Unless I am missing something in your proposal?

Regards,

Tvrtko

>          pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
>          if (HAS_GMCH(i915))
>                  pinctl |= PIN_MAPPABLE;
> 
> 
> Regards
> Andrzej
> 
> 
>>   *
>>
>>     igt@i915_selftest@live@gem_contexts:
>>
>>       o bat-atsm-1: PASS
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14010/bat-atsm-1/igt@i915_selftest@live@gem_contexts.html> -> INCOMPLETE <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-atsm-1/igt@i915_selftest@live@gem_contexts.html>
>>
>>
>>     Known issues
>>
>> Here are the changes found in Patchwork_127721v1 that come from known 
>> issues:
>>
>>
>>       IGT changes
>>
>>
>>         Issues hit
>>
>>   *
>>
>>     igt@gem_exec_suspend@basic-s0@smem:
>>
>>       o bat-dg2-9: NOTRUN -> INCOMPLETE
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_exec_suspend@basic-s0@smem.html> (i915#9275 <https://gitlab.freedesktop.org/drm/intel/issues/9275>)
>>   *
>>
>>     igt@gem_lmem_swapping@basic:
>>
>>       o fi-pnv-d510: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-pnv-d510/igt@gem_lmem_swapping@basic.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271>) +28 other tests skip
>>   *
>>
>>     igt@gem_mmap@basic:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_mmap@basic.html> (i915#4083 <https://gitlab.freedesktop.org/drm/intel/issues/4083>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_mmap@basic.html> (i915#4083 <https://gitlab.freedesktop.org/drm/intel/issues/4083>)
>>
>>   *
>>
>>     igt@gem_mmap_gtt@basic:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_mmap_gtt@basic.html> (i915#4077 <https://gitlab.freedesktop.org/drm/intel/issues/4077>) +2 other tests skip
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_mmap_gtt@basic.html> (i915#4077 <https://gitlab.freedesktop.org/drm/intel/issues/4077>) +2 other tests skip
>>
>>   *
>>
>>     igt@gem_render_tiled_blits@basic:
>>
>>       o bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@gem_render_tiled_blits@basic.html> (i915#4079 <https://gitlab.freedesktop.org/drm/intel/issues/4079>) +1 other test skip
>>   *
>>
>>     igt@gem_tiled_pread_basic:
>>
>>       o bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@gem_tiled_pread_basic.html> (i915#4079 <https://gitlab.freedesktop.org/drm/intel/issues/4079>) +1 other test skip
>>   *
>>
>>     igt@i915_pm_rps@basic-api:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@i915_pm_rps@basic-api.html> (i915#6621 <https://gitlab.freedesktop.org/drm/intel/issues/6621>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@i915_pm_rps@basic-api.html> (i915#6621 <https://gitlab.freedesktop.org/drm/intel/issues/6621>)
>>
>>   *
>>
>>     igt@i915_suspend@basic-s3-without-i915:
>>
>>       o bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@i915_suspend@basic-s3-without-i915.html> (i915#6645 <https://gitlab.freedesktop.org/drm/intel/issues/6645>)
>>   *
>>
>>     igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html> (i915#5190 <https://gitlab.freedesktop.org/drm/intel/issues/5190>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html> (i915#5190 <https://gitlab.freedesktop.org/drm/intel/issues/5190>)
>>
>>   *
>>
>>     igt@kms_addfb_basic@basic-y-tiled-legacy:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@basic-y-tiled-legacy.html> (i915#4215 <https://gitlab.freedesktop.org/drm/intel/issues/4215> / i915#5190 <https://gitlab.freedesktop.org/drm/intel/issues/5190>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html> (i915#4215 <https://gitlab.freedesktop.org/drm/intel/issues/4215> / i915#5190 <https://gitlab.freedesktop.org/drm/intel/issues/5190>)
>>
>>   *
>>
>>     igt@kms_addfb_basic@framebuffer-vs-set-tiling:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html> (i915#4212 <https://gitlab.freedesktop.org/drm/intel/issues/4212>) +6 other tests skip
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html> (i915#4212 <https://gitlab.freedesktop.org/drm/intel/issues/4212>) +6 other tests skip
>>
>>   *
>>
>>     igt@kms_addfb_basic@tile-pitch-mismatch:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_addfb_basic@tile-pitch-mismatch.html> (i915#4212 <https://gitlab.freedesktop.org/drm/intel/issues/4212> / i915#5608 <https://gitlab.freedesktop.org/drm/intel/issues/5608>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_addfb_basic@tile-pitch-mismatch.html> (i915#4212 <https://gitlab.freedesktop.org/drm/intel/issues/4212> / i915#5608 <https://gitlab.freedesktop.org/drm/intel/issues/5608>)
>>
>>   *
>>
>>     igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html> (i915#4103 <https://gitlab.freedesktop.org/drm/intel/issues/4103> / i915#4213 <https://gitlab.freedesktop.org/drm/intel/issues/4213> / i915#5608 <https://gitlab.freedesktop.org/drm/intel/issues/5608>) +1 other test skip
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html> (i915#4103 <https://gitlab.freedesktop.org/drm/intel/issues/4103> / i915#4213 <https://gitlab.freedesktop.org/drm/intel/issues/4213> / i915#5608 <https://gitlab.freedesktop.org/drm/intel/issues/5608>) +1 other test skip
>>
>>   *
>>
>>     igt@kms_force_connector_basic@force-load-detect:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_force_connector_basic@force-load-detect.html> (fdo#109285 <https://bugs.freedesktop.org/show_bug.cgi?id=109285>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_force_connector_basic@force-load-detect.html> (fdo#109285 <https://bugs.freedesktop.org/show_bug.cgi?id=109285>)
>>
>>   *
>>
>>     igt@kms_force_connector_basic@prune-stale-modes:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_force_connector_basic@prune-stale-modes.html> (i915#5274 <https://gitlab.freedesktop.org/drm/intel/issues/5274>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_force_connector_basic@prune-stale-modes.html> (i915#5274 <https://gitlab.freedesktop.org/drm/intel/issues/5274>)
>>
>>   *
>>
>>     igt@kms_pipe_crc_basic@suspend-read-crc:
>>
>>       o fi-ivb-3770: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-ivb-3770/igt@kms_pipe_crc_basic@suspend-read-crc.html> (fdo#109271 <https://bugs.freedesktop.org/show_bug.cgi?id=109271>)
>>   *
>>
>>     igt@kms_pm_backlight@basic-brightness:
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_pm_backlight@basic-brightness.html> (i915#5354 <https://gitlab.freedesktop.org/drm/intel/issues/5354>)
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_pm_backlight@basic-brightness.html> (i915#5354 <https://gitlab.freedesktop.org/drm/intel/issues/5354>)
>>
>>   *
>>
>>     igt@kms_setmode@basic-clone-single-crtc:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@kms_setmode@basic-clone-single-crtc.html> (i915#3555 <https://gitlab.freedesktop.org/drm/intel/issues/3555>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@kms_setmode@basic-clone-single-crtc.html> (i915#3555 <https://gitlab.freedesktop.org/drm/intel/issues/3555>)
>>
>>   *
>>
>>     igt@prime_vgem@basic-fence-flip:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-fence-flip.html> (i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708>)
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-fence-flip.html> (i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708>)
>>
>>   *
>>
>>     igt@prime_vgem@basic-fence-mmap:
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-fence-mmap.html> (i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708> / i915#4077 <https://gitlab.freedesktop.org/drm/intel/issues/4077>) +1 other test skip
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-fence-mmap.html> (i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708> / i915#4077 <https://gitlab.freedesktop.org/drm/intel/issues/4077>) +1 other test skip
>>
>>   *
>>
>>     igt@prime_vgem@basic-write:
>>
>>       o
>>
>>         bat-dg2-9: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-9/igt@prime_vgem@basic-write.html> (i915#3291 <https://gitlab.freedesktop.org/drm/intel/issues/3291> / i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708>) +2 other tests skip
>>
>>       o
>>
>>         bat-dg2-8: NOTRUN -> SKIP
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-dg2-8/igt@prime_vgem@basic-write.html> (i915#3291 <https://gitlab.freedesktop.org/drm/intel/issues/3291> / i915#3708 <https://gitlab.freedesktop.org/drm/intel/issues/3708>) +2 other tests skip
>>
>>
>>         Possible fixes
>>
>>   * igt@i915_selftest@live@gt_heartbeat:
>>       o fi-glk-j4005: INCOMPLETE
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14010/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html> -> PASS <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html>
>>
>> {name}: This element is suppressed. This means it is ignored when 
>> computing
>> the status of the difference (SUCCESS, WARNING, or FAILURE).
>>
>>
>>     Build changes
>>
>>   * Linux: CI_DRM_14010 -> Patchwork_127721v1
>>
>> CI-20190529: 20190529
>> CI_DRM_14010: b4182ec1538e8cebf630083ec4296bed0061d594 @ 
>> git://anongit.freedesktop.org/gfx-ci/linux
>> IGT_7635: 0b796be8ce05cb2070ce5136d248f438c962d11e @ 
>> https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
>> Patchwork_127721v1: b4182ec1538e8cebf630083ec4296bed0061d594 @ 
>> git://anongit.freedesktop.org/gfx-ci/linux
>>
>>
>>       Linux commits
>>
>> 499717049180 drm/i915: Simplify intel_initial_plane_config() calling 
>> convention
>> 3f12c223d707 drm/i915: Split the smem and lmem plane readout apart
>> 2d21369e26a3 drm/i915: s/phys_base/dma_addr/
>> be622e53615d drm/i915: Fix MTL initial plane readout
>> 8ad9926fb883 drm/i915: Fix region start during initial plane readout
>> 55b4a59fce27 drm/i915: Fix PTE decode during initial plane readout
>> db38b8a8227c drm/i915: Rename the DSM/GSM registers
>> f7b2179e3b69 drm/i915: Disable the "binder"
>> 77e7c3085969 drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory 
>> access
>> ef59e22db9f6 drm/i915: Remove ad-hoc lmem/stolen debugs
>> b001bcb6a48f drm/i915: Print memory region info during probe
>> 5b54e278890d drm/i915: Use struct resource for memory region IO as well
>>
> 

Re: â Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes

[Tvrtko Ursulin]

On 13/12/2023 17:24, Tvrtko Ursulin wrote:
> 
> Hi Andrzej,
> 
> On 13/12/2023 16:13, Andrzej Hajda wrote:
>> On 13.12.2023 02:37, Patchwork wrote:
>>> *Patch Details*
>>> *Series:*    drm/i915: (stolen) memory region related fixes
>>> *URL:*    https://patchwork.freedesktop.org/series/127721/ 
>>> <https://patchwork.freedesktop.org/series/127721/>
>>> *State:*    failure
>>> *Details:* 
>>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html>
>>>
>>>
>>>   CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1
>>>
>>>
>>>     Summary
>>>
>>> *FAILURE*
>>>
>>> Serious unknown changes coming with Patchwork_127721v1 absolutely 
>>> need to be
>>> verified manually.
>>>
>>> If you think the reported changes have nothing to do with the changes
>>> introduced in Patchwork_127721v1, please notify your bug team 
>>> (I915-ci-infra@lists.freedesktop.org) to allow them
>>> to document this new failure mode, which will reduce false positives 
>>> in CI.
>>>
>>> External URL: 
>>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html
>>>
>>>
>>>     Participating hosts (31 -> 33)
>>>
>>> Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510
>>> Missing (2): bat-adlp-11 fi-snb-2520m
>>>
>>>
>>>     Possible new issues
>>>
>>> Here are the unknown changes that may have been introduced in 
>>> Patchwork_127721v1:
>>>
>>>
>>>       IGT changes
>>>
>>>
>>>         Possible regressions
>>>
>>>   *
>>>
>>>     igt@i915_module_load@load:
>>>
>>>       o bat-mtlp-8: NOTRUN -> INCOMPLETE
>>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@load.html>
>>
>>
>> This is due to overlapping initial fb's vma with GuC reserved area on 
>> MTL, see ggtt_reserve_guc_top.
>>
>> My dirty quick_fix proposed:
>> @@ -143,6 +143,9 @@ initial_plane_vma(struct drm_i915_private *i915,
>>          if (IS_ERR(vma))
>>                  goto err_obj;
>>
>> +       if (base + size > GUC_GGTT_TOP)
>> +               base = min(base, GUC_GGTT_TOP) - size;
>> +
> 
> I saw you posting this before but forgot to comment. I couldn't quite 
> figure out what the logic is supposed to do and how it wouldn't fail to 
> inherit any firmware setup splash screen?
> 
> Other than making the firmware aware and not use that range, I was 
> thinking what else we could do in i915? Copy the fb to a non-conflicting 
> location and re-program the display engine? If that is doable without 
> visible glitching and can be fast enough not to slow the boot a lot.
> 
> Unless I am missing something in your proposal?

Aha I missed the fact we write out new PTEs!

Display code will update without glitching to the new plane address?

Regards,

Tvrtko

Re: â Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes

[Andrzej Hajda]

On 13.12.2023 17:13, Andrzej Hajda wrote:
> On 13.12.2023 02:37, Patchwork wrote:
>> *Patch Details*
>> *Series:*    drm/i915: (stolen) memory region related fixes
>> *URL:*    https://patchwork.freedesktop.org/series/127721/ 
>> <https://patchwork.freedesktop.org/series/127721/>
>> *State:*    failure
>> *Details:* 
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html 
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html>
>>
>>
>>   CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1
>>
>>
>>     Summary
>>
>> *FAILURE*
>>
>> Serious unknown changes coming with Patchwork_127721v1 absolutely need 
>> to be
>> verified manually.
>>
>> If you think the reported changes have nothing to do with the changes
>> introduced in Patchwork_127721v1, please notify your bug team 
>> (I915-ci-infra@lists.freedesktop.org) to allow them
>> to document this new failure mode, which will reduce false positives 
>> in CI.
>>
>> External URL: 
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html
>>
>>
>>     Participating hosts (31 -> 33)
>>
>> Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510
>> Missing (2): bat-adlp-11 fi-snb-2520m
>>
>>
>>     Possible new issues
>>
>> Here are the unknown changes that may have been introduced in 
>> Patchwork_127721v1:
>>
>>
>>       IGT changes
>>
>>
>>         Possible regressions
>>
>>   *
>>
>>     igt@i915_module_load@load:
>>
>>       o bat-mtlp-8: NOTRUN -> INCOMPLETE
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@load.html>
> 
> 
> This is due to overlapping initial fb's vma with GuC reserved area on 
> MTL, see ggtt_reserve_guc_top.
> 
> My dirty quick_fix proposed:
> @@ -143,6 +143,9 @@ initial_plane_vma(struct drm_i915_private *i915,
>          if (IS_ERR(vma))
>                  goto err_obj;
> 
> +       if (base + size > GUC_GGTT_TOP)
> +               base = min(base, GUC_GGTT_TOP) - size;
> +
>          pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
>          if (HAS_GMCH(i915))
>                  pinctl |= PIN_MAPPABLE;


Copy/Paste Ville response for this proposition from another thread:

On 13.12.2023 17:03, Ville Syrjälä wrote:
 >
 > This is not a solution. The correct fix is either:
 > 1. fix the guc code to not insist on a fixed chunk of the ggtt
 >    and instead allocate it normally like a good boy. It could
 >    still try to allocate as high as possible to ideally
 >    land in the GUC_GGTT_TOP range

This would be the best solution from initial plane PoV for sure, I am 
not sure about GuC PoV.

 >
 > 2. relocate the display vma to a different part of the ggtt

I think this point is what I have proposed.

 >
 >
 > 1. should be far simpler by the looks of it, as 2. would involve:
 > a) pinning the same object into two places in the ggtt simultanously
 >    I don't think we have support for that except for special ggtt views,
 >    and xe doesn't have even that (should we need this trick there as 
well)

AFAIU the fb is not yet pinned in terms of kernel structures, so it 
doesn't seems to be an issue.
Of course there is problem with PLANE_SURF still pointing to old VA, it 
should be replaced with new VA before ggtt will be populated with new stuff.

 >
 > b) flip the plane(s) over to the relocated vma
 > c) wait for vblank(s)
 > d) discard the original vma
 > e) all of that would need to happen pretty early so we may not have
 >    a lot of the required normal machinery fully up and running yet

Async update to PLANE_SURF shouldn't be enough?

Regards
Andrzej

Re: â Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes

[Ville Syrjälä]

On Thu, Dec 14, 2023 at 12:06:58PM +0100, Andrzej Hajda wrote:
> 
> 
> On 13.12.2023 17:13, Andrzej Hajda wrote:
> > On 13.12.2023 02:37, Patchwork wrote:
> >> *Patch Details*
> >> *Series:*    drm/i915: (stolen) memory region related fixes
> >> *URL:*    https://patchwork.freedesktop.org/series/127721/ 
> >> <https://patchwork.freedesktop.org/series/127721/>
> >> *State:*    failure
> >> *Details:* 
> >> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html 
> >> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html>
> >>
> >>
> >>   CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1
> >>
> >>
> >>     Summary
> >>
> >> *FAILURE*
> >>
> >> Serious unknown changes coming with Patchwork_127721v1 absolutely need 
> >> to be
> >> verified manually.
> >>
> >> If you think the reported changes have nothing to do with the changes
> >> introduced in Patchwork_127721v1, please notify your bug team 
> >> (I915-ci-infra@lists.freedesktop.org) to allow them
> >> to document this new failure mode, which will reduce false positives 
> >> in CI.
> >>
> >> External URL: 
> >> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html
> >>
> >>
> >>     Participating hosts (31 -> 33)
> >>
> >> Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510
> >> Missing (2): bat-adlp-11 fi-snb-2520m
> >>
> >>
> >>     Possible new issues
> >>
> >> Here are the unknown changes that may have been introduced in 
> >> Patchwork_127721v1:
> >>
> >>
> >>       IGT changes
> >>
> >>
> >>         Possible regressions
> >>
> >>   *
> >>
> >>     igt@i915_module_load@load:
> >>
> >>       o bat-mtlp-8: NOTRUN -> INCOMPLETE
> >>         
> >> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@load.html>
> > 
> > 
> > This is due to overlapping initial fb's vma with GuC reserved area on 
> > MTL, see ggtt_reserve_guc_top.
> > 
> > My dirty quick_fix proposed:
> > @@ -143,6 +143,9 @@ initial_plane_vma(struct drm_i915_private *i915,
> >          if (IS_ERR(vma))
> >                  goto err_obj;
> > 
> > +       if (base + size > GUC_GGTT_TOP)
> > +               base = min(base, GUC_GGTT_TOP) - size;
> > +
> >          pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
> >          if (HAS_GMCH(i915))
> >                  pinctl |= PIN_MAPPABLE;
> 
> 
> Copy/Paste Ville response for this proposition from another thread:
> 
> On 13.12.2023 17:03, Ville Syrjälä wrote:
>  >
>  > This is not a solution. The correct fix is either:
>  > 1. fix the guc code to not insist on a fixed chunk of the ggtt
>  >    and instead allocate it normally like a good boy. It could
>  >    still try to allocate as high as possible to ideally
>  >    land in the GUC_GGTT_TOP range
> 
> This would be the best solution from initial plane PoV for sure, I am 
> not sure about GuC PoV.

I was going to submit a patch to simply change this to insert(PIN_HIGH)
as that is all that is needed from the GuC FW POV. And whether the
GOP framebuffer or the GuC FW lives there doesn't matter one bit,
both fit the bill of not needing to be accessed by the GuC.

But I suspect this function might be serving a double duty, despite
not saying so anywhere. Basically I think it's perhaps used as
a quick and dirty way to restrict everything below GUC_GGTT_TOP,
presumably because we don't otherwise restrict allocations
below that even if the GuC needs to access them.

So the insert(PIN_HIGH) approach could result in some troubles
iff the GOP framebuffer is preventing the uc_fw to be placed at
GUC_GGTT_TOP and we later free the GOP framebuffer (happens if
we find it unsuitable due to one reason or another). This opens
up a hole above GUC_GGTT_TOP again and something else could
get bound there.

Is my assumption about ggtt_reserve_guc_top() correct? And
if so do we know what are all the things the GuC needs to access
so that we could properly restrict those below GUC_GGTT_TOP
and get rid of this weird way of achieving that same result?

> 
>  >
>  > 2. relocate the display vma to a different part of the ggtt
> 
> I think this point is what I have proposed.
> 
>  >
>  >
>  > 1. should be far simpler by the looks of it, as 2. would involve:
>  > a) pinning the same object into two places in the ggtt simultanously
>  >    I don't think we have support for that except for special ggtt views,
>  >    and xe doesn't have even that (should we need this trick there as 
> well)
> 
> AFAIU the fb is not yet pinned in terms of kernel structures, so it 
> doesn't seems to be an issue.

Hmm. Yeah, I suppose we don't really care where in the ggtt the thing
is bound as we rewrite the PTEs anyway when we bind the vma. So
whether we bind it into its current place or not doesn't really
matter. Well, assuming we move the thing to a non-overlapping section
of the ggtt. Any overlap with the current positon would clobber the
currently used PTEs and cause display corruption.

If we did try this we'd also have to think where to move it. Due
the requirement of no overlaps I suppose offset 0 might be the most
obvious choice. That would also avoid permanently fragmentating
the ggtt.

Though I suppose one has to wonder why the GOP doesn't just use
offset 0 anyway. Is there perhaps some other weird requirement
that needs the bottom end of ggtt to be available for something
else?

> Of course there is problem with PLANE_SURF still pointing to old VA, it 
> should be replaced with new VA before ggtt will be populated with new stuff.
> 
>  >
>  > b) flip the plane(s) over to the relocated vma
>  > c) wait for vblank(s)
>  > d) discard the original vma
>  > e) all of that would need to happen pretty early so we may not have
>  >    a lot of the required normal machinery fully up and running yet
> 
> Async update to PLANE_SURF shouldn't be enough?

Async flips:
- Can't be assume to be universally available
- Don't really help anyway as they don't happen instaneously.
  Ie. you still need to wait for them to have happened
  before clobbering old the PTEs.

-- 
Ville Syrjälä
Intel

Re: â Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes

[Ville Syrjälä]

On Fri, Dec 15, 2023 at 01:01:39AM +0200, Ville Syrjälä wrote:
> On Thu, Dec 14, 2023 at 12:06:58PM +0100, Andrzej Hajda wrote:
> > 
> > 
> > On 13.12.2023 17:13, Andrzej Hajda wrote:
> > > On 13.12.2023 02:37, Patchwork wrote:
> > >> *Patch Details*
> > >> *Series:*    drm/i915: (stolen) memory region related fixes
> > >> *URL:*    https://patchwork.freedesktop.org/series/127721/ 
> > >> <https://patchwork.freedesktop.org/series/127721/>
> > >> *State:*    failure
> > >> *Details:* 
> > >> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html 
> > >> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html>
> > >>
> > >>
> > >>   CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1
> > >>
> > >>
> > >>     Summary
> > >>
> > >> *FAILURE*
> > >>
> > >> Serious unknown changes coming with Patchwork_127721v1 absolutely need 
> > >> to be
> > >> verified manually.
> > >>
> > >> If you think the reported changes have nothing to do with the changes
> > >> introduced in Patchwork_127721v1, please notify your bug team 
> > >> (I915-ci-infra@lists.freedesktop.org) to allow them
> > >> to document this new failure mode, which will reduce false positives 
> > >> in CI.
> > >>
> > >> External URL: 
> > >> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html
> > >>
> > >>
> > >>     Participating hosts (31 -> 33)
> > >>
> > >> Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510
> > >> Missing (2): bat-adlp-11 fi-snb-2520m
> > >>
> > >>
> > >>     Possible new issues
> > >>
> > >> Here are the unknown changes that may have been introduced in 
> > >> Patchwork_127721v1:
> > >>
> > >>
> > >>       IGT changes
> > >>
> > >>
> > >>         Possible regressions
> > >>
> > >>   *
> > >>
> > >>     igt@i915_module_load@load:
> > >>
> > >>       o bat-mtlp-8: NOTRUN -> INCOMPLETE
> > >>         
> > >> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@load.html>
> > > 
> > > 
> > > This is due to overlapping initial fb's vma with GuC reserved area on 
> > > MTL, see ggtt_reserve_guc_top.
> > > 
> > > My dirty quick_fix proposed:
> > > @@ -143,6 +143,9 @@ initial_plane_vma(struct drm_i915_private *i915,
> > >          if (IS_ERR(vma))
> > >                  goto err_obj;
> > > 
> > > +       if (base + size > GUC_GGTT_TOP)
> > > +               base = min(base, GUC_GGTT_TOP) - size;
> > > +
> > >          pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
> > >          if (HAS_GMCH(i915))
> > >                  pinctl |= PIN_MAPPABLE;
> > 
> > 
> > Copy/Paste Ville response for this proposition from another thread:
> > 
> > On 13.12.2023 17:03, Ville Syrjälä wrote:
> >  >
> >  > This is not a solution. The correct fix is either:
> >  > 1. fix the guc code to not insist on a fixed chunk of the ggtt
> >  >    and instead allocate it normally like a good boy. It could
> >  >    still try to allocate as high as possible to ideally
> >  >    land in the GUC_GGTT_TOP range
> > 
> > This would be the best solution from initial plane PoV for sure, I am 
> > not sure about GuC PoV.
> 
> I was going to submit a patch to simply change this to insert(PIN_HIGH)
> as that is all that is needed from the GuC FW POV. And whether the
> GOP framebuffer or the GuC FW lives there doesn't matter one bit,
> both fit the bill of not needing to be accessed by the GuC.
> 
> But I suspect this function might be serving a double duty, despite
> not saying so anywhere. Basically I think it's perhaps used as
> a quick and dirty way to restrict everything below GUC_GGTT_TOP,
> presumably because we don't otherwise restrict allocations
> below that even if the GuC needs to access them.
> 
> So the insert(PIN_HIGH) approach could result in some troubles
> iff the GOP framebuffer is preventing the uc_fw to be placed at
> GUC_GGTT_TOP and we later free the GOP framebuffer (happens if
> we find it unsuitable due to one reason or another). This opens
> up a hole above GUC_GGTT_TOP again and something else could
> get bound there.
> 
> Is my assumption about ggtt_reserve_guc_top() correct? And
> if so do we know what are all the things the GuC needs to access
> so that we could properly restrict those below GUC_GGTT_TOP
> and get rid of this weird way of achieving that same result?
> 
> > 
> >  >
> >  > 2. relocate the display vma to a different part of the ggtt
> > 
> > I think this point is what I have proposed.
> > 
> >  >
> >  >
> >  > 1. should be far simpler by the looks of it, as 2. would involve:
> >  > a) pinning the same object into two places in the ggtt simultanously
> >  >    I don't think we have support for that except for special ggtt views,
> >  >    and xe doesn't have even that (should we need this trick there as 
> > well)
> > 
> > AFAIU the fb is not yet pinned in terms of kernel structures, so it 
> > doesn't seems to be an issue.
> 
> Hmm. Yeah, I suppose we don't really care where in the ggtt the thing
> is bound as we rewrite the PTEs anyway when we bind the vma. So
> whether we bind it into its current place or not doesn't really
> matter. Well, assuming we move the thing to a non-overlapping section
> of the ggtt. Any overlap with the current positon would clobber the
> currently used PTEs and cause display corruption.
> 
> If we did try this we'd also have to think where to move it. Due
> the requirement of no overlaps I suppose offset 0 might be the most
> obvious choice. That would also avoid permanently fragmentating
> the ggtt.

OK, so I've gone and implemnted this, and what I did was to just
pin it low. That part is sort of OK, but then I noticed that
something had already allocated two pages at offset 0. And to my
surprise that was some hdcp stuff. So I then changed the hdcp code
to pin high instead (since there's no point in stuffing what is a
normal shmem object into the precious mappable portion of ggtt).
But then I realized that would now interfere with the guc thing
again. I then proceeded to move the guc thing into the pre-display
ggtt_init_hw() call, but then that's going to interfere with my
BIOS framebuffer takeover again since I temporarily reserve the
original range of the framebuffer so that we don't accidentally
bind the the new range over the original range.

So now I need to either get rid of that guc thing for real,
or I need to keep ggtt_reserve_guc_top() where is is right
now and figure out how to defer that hdcp stuff to a much
later point in the driver init.

Additonally ggtt->error_capture would prefer to get pinned to
offet 0 on platforms with mappable. I don't think that's going
to happen because the BIOS framebuffer is likely there. I guess
one option would be to attempt to move the BIOS fb to the end
of mappable. But currenlty PIN_MAPPABLE implies DRM_MM_INSERT_LOW
so I'd need to redesign the pin flags. So what likely happens
now is the BIOS of gets offset 0, and error_capture gets pinned
right after it. But that's also quite terrible in cases where
we later get rid of the BIOS fb (likely to happen when booting
with multiple different sized displays), as then we've managed
to permanently fragment the mappable region into two chunks.
So I think the easiest solution for that is to move error_capture
to the end of mappable, assuming we failed to bind it to offset 0.

Who knew changing one lightbulb could be this complicated...

> 
> Though I suppose one has to wonder why the GOP doesn't just use
> offset 0 anyway. Is there perhaps some other weird requirement
> that needs the bottom end of ggtt to be available for something
> else?
> 
> > Of course there is problem with PLANE_SURF still pointing to old VA, it 
> > should be replaced with new VA before ggtt will be populated with new stuff.
> > 
> >  >
> >  > b) flip the plane(s) over to the relocated vma
> >  > c) wait for vblank(s)
> >  > d) discard the original vma
> >  > e) all of that would need to happen pretty early so we may not have
> >  >    a lot of the required normal machinery fully up and running yet
> > 
> > Async update to PLANE_SURF shouldn't be enough?
> 
> Async flips:
> - Can't be assume to be universally available
> - Don't really help anyway as they don't happen instaneously.
>   Ie. you still need to wait for them to have happened
>   before clobbering old the PTEs.
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel

✗ Fi.CI.CHECKPATCH: warning for drm/i915: (stolen) memory region related fixes (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: (stolen) memory region related fixes (rev2)
URL   : https://patchwork.freedesktop.org/series/127721/
State : warning

== Summary ==

Error: dim checkpatch failed
43431d68ff99 drm/i915: Use struct resource for memory region IO as well
-:385: WARNING:LONG_LINE: line length of 105 exceeds 100 columns
#385: FILE: drivers/gpu/drm/i915/intel_region_ttm.c:227:
+			if (WARN_ON(overflows_type(resource_size(&mem->io) >> PAGE_SHIFT, place.lpfn))) {

total: 0 errors, 1 warnings, 0 checks, 281 lines checked
4740f0098482 drm/i915: Print memory region info during probe
f5fd743f6ee4 drm/i915: Remove ad-hoc lmem/stolen debugs
b9de2397f4ee drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access
2f5eb71f0cc5 drm/i915: Disable the "binder"
ad5d174a7ad5 drm/i915: Rename the DSM/GSM registers
8943d3a47536 drm/i915: Fix PTE decode during initial plane readout
c0c84c45e7e3 drm/i915: Fix region start during initial plane readout
0e72f7c4959c drm/i915: Fix MTL initial plane readout
73e35d41526a drm/i915: s/phys_base/dma_addr/
0f775764deae drm/i915: Split the smem and lmem plane readout apart
a3e805b3a298 drm/i915: Simplify intel_initial_plane_config() calling convention

✗ Fi.CI.SPARSE: warning for drm/i915: (stolen) memory region related fixes (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: (stolen) memory region related fixes (rev2)
URL   : https://patchwork.freedesktop.org/series/127721/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 9760 bytes --]

== Series Details ==

Series: drm/i915: (stolen) memory region related fixes (rev2)
URL   : https://patchwork.freedesktop.org/series/127721/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_127721v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_127721v2, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/index.html

Participating hosts (31 -> 33)
------------------------------

  Additional (4): bat-kbl-2 bat-dg2-9 bat-mtlp-8 fi-pnv-d510 
  Missing    (2): bat-adlp-11 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_127721v2:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - bat-mtlp-8:         NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-mtlp-8/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in Patchwork_127721v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@fbdev@info:
    - bat-kbl-2:          NOTRUN -> [SKIP][2] ([fdo#109271] / [i915#1849])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-kbl-2/igt@fbdev@info.html

  * igt@gem_exec_suspend@basic-s0@smem:
    - bat-dg2-9:          NOTRUN -> [INCOMPLETE][3] ([i915#9275])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@gem_lmem_swapping@basic:
    - fi-pnv-d510:        NOTRUN -> [SKIP][4] ([fdo#109271]) +28 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/fi-pnv-d510/igt@gem_lmem_swapping@basic.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-kbl-2:          NOTRUN -> [SKIP][5] ([fdo#109271]) +36 other tests skip
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-kbl-2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_mmap@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][6] ([i915#4083])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][7] ([i915#4077]) +2 other tests skip
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][8] ([i915#4079]) +1 other test skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@gem_render_tiled_blits@basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-9:          NOTRUN -> [SKIP][9] ([i915#6621])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@gt_lrc:
    - bat-dg1-7:          [PASS][10] -> [ABORT][11] ([i915#9413])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14010/bat-dg1-7/igt@i915_selftest@live@gt_lrc.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg1-7/igt@i915_selftest@live@gt_lrc.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][12] ([i915#5190])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][13] ([i915#4215] / [i915#5190])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@framebuffer-vs-set-tiling:
    - bat-dg2-9:          NOTRUN -> [SKIP][14] ([i915#4212]) +6 other tests skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html

  * igt@kms_addfb_basic@tile-pitch-mismatch:
    - bat-dg2-9:          NOTRUN -> [SKIP][15] ([i915#4212] / [i915#5608])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_addfb_basic@tile-pitch-mismatch.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][16] ([i915#4103] / [i915#4213] / [i915#5608]) +1 other test skip
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg2-9:          NOTRUN -> [SKIP][17] ([fdo#109285])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-dg2-9:          NOTRUN -> [SKIP][18] ([i915#5274])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-dg2-9:          NOTRUN -> [SKIP][19] ([i915#5354])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg2-9:          NOTRUN -> [SKIP][20] ([i915#3555])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-dg2-9:          NOTRUN -> [SKIP][21] ([i915#3708])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-dg2-9:          NOTRUN -> [SKIP][22] ([i915#3708] / [i915#4077]) +1 other test skip
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-write:
    - bat-dg2-9:          NOTRUN -> [SKIP][23] ([i915#3291] / [i915#3708]) +2 other tests skip
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/bat-dg2-9/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-glk-j4005:       [INCOMPLETE][24] -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14010/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5608]: https://gitlab.freedesktop.org/drm/intel/issues/5608
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#9275]: https://gitlab.freedesktop.org/drm/intel/issues/9275
  [i915#9413]: https://gitlab.freedesktop.org/drm/intel/issues/9413
  [i915#9673]: https://gitlab.freedesktop.org/drm/intel/issues/9673


Build changes
-------------

  * Linux: CI_DRM_14010 -> Patchwork_127721v2

  CI-20190529: 20190529
  CI_DRM_14010: b4182ec1538e8cebf630083ec4296bed0061d594 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7635: 0b796be8ce05cb2070ce5136d248f438c962d11e @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_127721v2: b4182ec1538e8cebf630083ec4296bed0061d594 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

d51030edf2d5 drm/i915: Simplify intel_initial_plane_config() calling convention
0e0358dd411f drm/i915: Split the smem and lmem plane readout apart
18049ebf5f0e drm/i915: s/phys_base/dma_addr/
c6e777ff430b drm/i915: Fix MTL initial plane readout
c490108fccb8 drm/i915: Fix region start during initial plane readout
acad26da3bd4 drm/i915: Fix PTE decode during initial plane readout
6e003e279e18 drm/i915: Rename the DSM/GSM registers
315634ce3a33 drm/i915: Disable the "binder"
869c9b9bae2b drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access
f636cf0c2b7f drm/i915: Remove ad-hoc lmem/stolen debugs
2647844fee66 drm/i915: Print memory region info during probe
38b95d8b37f8 drm/i915: Use struct resource for memory region IO as well

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v2/index.html

[-- Attachment #2: Type: text/html, Size: 11494 bytes --]