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

[Andrzej Hajda <andrzej.hajda@intel.com>]

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;