linux-kernel - Re: [Intel-gfx] [PATCH] [v2] drm/i915/display: Check GGTT to determine phys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1d6b9ffa093404631ca8218f3b5b72057a7da6a6.camel@intel.com>
Date:   Fri, 8 Dec 2023 01:00:04 +0000
From:   "Almahallawy, Khaled" <khaled.almahallawy@...el.com>
To:     "pazz@...omium.org" <pazz@...omium.org>,
        "Ursulin, Tvrtko" <tvrtko.ursulin@...el.com>,
        "Hajda, Andrzej" <andrzej.hajda@...el.com>
CC:     "Das, Nirmoy" <nirmoy.das@...el.com>,
        "ddavenport@...omium.org" <ddavenport@...omium.org>,
        "seanpaul@...omium.org" <seanpaul@...omium.org>,
        "Vivi, Rodrigo" <rodrigo.vivi@...el.com>,
        "Auld, Matthew" <matthew.auld@...el.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "airlied@...il.com" <airlied@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Banik, Subrata" <subratabanik@...gle.com>,
        "intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "mwojtas@...omium.org" <mwojtas@...omium.org>
Subject: Re: [Intel-gfx] [PATCH] [v2] drm/i915/display: Check GGTT to
 determine phys_base

Thank You for the patch. We noticed a break in the customer board with
the latest GOP + this patch.


Thank You
Khaled	

On Wed, 2023-12-06 at 18:46 +0000, Paz Zcharya wrote:
> There was an assumption that for iGPU there should be a 1:1 mapping
> of GGTT to physical address pointing to the framebuffer.
> This assumption is not strictly true effective generation 8 or newer.
> Fix that by checking GGTT to determine the phys address on gen8+.
> 
> 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
> 
> Changes from v1:
>   - Add an if statement for gen7-, where there is a 1:1 mapping
> 
> Signed-off-by: Paz Zcharya <pazz@...omium.org>
> ---
> 
>  .../drm/i915/display/intel_plane_initial.c    | 64 +++++++++++----
> ----
>  1 file changed, 39 insertions(+), 25 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..7d9bb631b93b 100644
> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> @@ -59,44 +59,58 @@ 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 (GRAPHICS_VER(i915) < 8) {
> +		/*
> +		 * In gen7-, there is a 1:1 mapping
> +		 * between GSM and physical address.
> +		 */
> +		phys_base = base;
> +		mem = i915->mm.stolen_region;
> +	} else {
> +		/*
> +		 * In gen8+, there is no 1:1 mapping between
> +		 * GSM and physical address, so we need to
> +		 * check GGTT to determine the physical address.
> +		 */
>  		gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
>  		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;
> -		}
> -
> -		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;
> +		if (IS_DGFX(i915)) {
> +			if (!(pte & GEN12_GGTT_PTE_LM)) {
> +				drm_err(&i915->drm,
> +					"Initial plane programming
> missing PTE_LM bit\n");
> +				return NULL;
> +			}
> +			mem = i915->mm.regions[INTEL_REGION_LMEM_0];
> +		} else {
> +			mem = i915->mm.stolen_region;
>  		}
>  
> -		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;