[PATCH] drm/i915/display: Check GGTT to determine phys_base

[PATCH] drm/i915/display: Check GGTT to determine phys_base

[Paz Zcharya]

There was an assumption that for iGPU there should be a 1:1 mapping
of GGTT to physical address pointing to actual framebuffer.
This assumption is not valid anymore for MTL.
Fix that by checking GGTT to determine the phys address.

The following algorithm for phys_base should be valid for all platforms:
1. Find pte
2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem =
i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;

- On older platforms, stolen_region points to system memory, starting at 0
- on DG2, it uses lmem region which starts at 0 as well
- on MTL, stolen_region points to stolen-local which starts at 0x800000

Signed-off-by: Paz Zcharya <pazz@chromium.org>
---

 .../drm/i915/display/intel_plane_initial.c    | 45 +++++++++----------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
index a55c09cbd0e4..c4bf02ea966c 100644
--- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
+++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
@@ -51,6 +51,8 @@ initial_plane_vma(struct drm_i915_private *i915,
 	struct intel_memory_region *mem;
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
+	gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
+	gen8_pte_t pte;
 	resource_size_t phys_base;
 	u32 base, size;
 	u64 pinctl;
@@ -59,44 +61,41 @@ initial_plane_vma(struct drm_i915_private *i915,
 		return NULL;
 
 	base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
-	if (IS_DGFX(i915)) {
-		gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
-		gen8_pte_t pte;
+	gte += base / I915_GTT_PAGE_SIZE;
 
-		gte += base / I915_GTT_PAGE_SIZE;
+	pte = ioread64(gte);
 
-		pte = ioread64(gte);
+	if (IS_DGFX(i915)) {
 		if (!(pte & GEN12_GGTT_PTE_LM)) {
 			drm_err(&i915->drm,
 				"Initial plane programming missing PTE_LM bit\n");
 			return NULL;
 		}
-
-		phys_base = pte & I915_GTT_PAGE_MASK;
 		mem = i915->mm.regions[INTEL_REGION_LMEM_0];
-
-		/*
-		 * We don't currently expect this to ever be placed in the
-		 * stolen portion.
-		 */
-		if (phys_base >= resource_size(&mem->region)) {
-			drm_err(&i915->drm,
-				"Initial plane programming using invalid range, phys_base=%pa\n",
-				&phys_base);
-			return NULL;
-		}
-
-		drm_dbg(&i915->drm,
-			"Using phys_base=%pa, based on initial plane programming\n",
-			&phys_base);
 	} else {
-		phys_base = base;
 		mem = i915->mm.stolen_region;
 	}
 
+	phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
+
 	if (!mem)
 		return NULL;
 
+	/*
+	 * We don't currently expect this to ever be placed in the
+	 * stolen portion.
+	 */
+	if (phys_base >= resource_size(&mem->region)) {
+		drm_err(&i915->drm,
+			"Initial plane programming using invalid range, phys_base=%pa\n",
+			&phys_base);
+		return NULL;
+	}
+
+	drm_dbg(&i915->drm,
+		"Using phys_base=%pa, based on initial plane programming\n",
+		&phys_base);
+
 	size = round_up(plane_config->base + plane_config->size,
 			mem->min_page_size);
 	size -= base;
-- 
2.43.0.rc1.413.gea7ed67945-goog

Re: [PATCH] drm/i915/display: Check GGTT to determine phys_base

[Andrzej Hajda]

On 30.11.2023 17:16, Paz Zcharya wrote:
> There was an assumption that for iGPU there should be a 1:1 mapping
> of GGTT to physical address pointing to actual framebuffer.
> This assumption is not valid anymore for MTL.
> Fix that by checking GGTT to determine the phys address.
>
> The following algorithm for phys_base should be valid for all platforms:
> 1. Find pte
> 2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem =
> i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region
> 3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
>
> - On older platforms, stolen_region points to system memory, starting at 0
> - on DG2, it uses lmem region which starts at 0 as well
> - on MTL, stolen_region points to stolen-local which starts at 0x800000
>
> Signed-off-by: Paz Zcharya <pazz@chromium.org>

I was waiting for CI with my comments, but without success.
For some reason it didn't run, that's pity next time you can trigger it 
manually via patchwork.

> ---
>
>   .../drm/i915/display/intel_plane_initial.c    | 45 +++++++++----------
>   1 file changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> index a55c09cbd0e4..c4bf02ea966c 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -51,6 +51,8 @@ initial_plane_vma(struct drm_i915_private *i915,
>   	struct intel_memory_region *mem;
>   	struct drm_i915_gem_object *obj;
>   	struct i915_vma *vma;
> +	gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;

I have realized this will work only for gen8+. Older gens uses 32bit 
gen6_pte_t.
So another 'if' is necessary. Then in case of older platforms IMO one 
can use
     pte = base;
with comment that it is 1:1 mapping.

Regards
Andrzej

> +	gen8_pte_t pte;
>   	resource_size_t phys_base;
>   	u32 base, size;
>   	u64 pinctl;
> @@ -59,44 +61,41 @@ initial_plane_vma(struct drm_i915_private *i915,
>   		return NULL;
>   
>   	base = round_down(plane_config->base, I915_GTT_MIN_ALIGNMENT);
> -	if (IS_DGFX(i915)) {
> -		gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
> -		gen8_pte_t pte;
> +	gte += base / I915_GTT_PAGE_SIZE;
>   
> -		gte += base / I915_GTT_PAGE_SIZE;
> +	pte = ioread64(gte);
>   
> -		pte = ioread64(gte);
> +	if (IS_DGFX(i915)) {
>   		if (!(pte & GEN12_GGTT_PTE_LM)) {
>   			drm_err(&i915->drm,
>   				"Initial plane programming missing PTE_LM bit\n");
>   			return NULL;
>   		}
> -
> -		phys_base = pte & I915_GTT_PAGE_MASK;
>   		mem = i915->mm.regions[INTEL_REGION_LMEM_0];
> -
> -		/*
> -		 * We don't currently expect this to ever be placed in the
> -		 * stolen portion.
> -		 */
> -		if (phys_base >= resource_size(&mem->region)) {
> -			drm_err(&i915->drm,
> -				"Initial plane programming using invalid range, phys_base=%pa\n",
> -				&phys_base);
> -			return NULL;
> -		}
> -
> -		drm_dbg(&i915->drm,
> -			"Using phys_base=%pa, based on initial plane programming\n",
> -			&phys_base);
>   	} else {
> -		phys_base = base;
>   		mem = i915->mm.stolen_region;
>   	}
>   
> +	phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start;
> +
>   	if (!mem)
>   		return NULL;
>   
> +	/*
> +	 * We don't currently expect this to ever be placed in the
> +	 * stolen portion.
> +	 */
> +	if (phys_base >= resource_size(&mem->region)) {
> +		drm_err(&i915->drm,
> +			"Initial plane programming using invalid range, phys_base=%pa\n",
> +			&phys_base);
> +		return NULL;
> +	}
> +
> +	drm_dbg(&i915->drm,
> +		"Using phys_base=%pa, based on initial plane programming\n",
> +		&phys_base);
> +
>   	size = round_up(plane_config->base + plane_config->size,
>   			mem->min_page_size);
>   	size -= base;