Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <f94b02b4-6ac8-4b3e-8aac-4a1a2b27b829@intel.com>
Date:   Tue, 5 Dec 2023 08:42:16 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drm/i915/display: Check GGTT to determine phys_base
Content-Language: en-US
To:     Paz Zcharya <pazz@chromium.org>,
        Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc:     =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>,
        matthew.auld@intel.com, intel-gfx@lists.freedesktop.org,
        Drew Davenport <ddavenport@chromium.org>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Nirmoy Das <nirmoy.das@intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        =?UTF-8?Q?Jouni_H=C3=B6gander?= <jouni.hogander@intel.com>,
        Subrata Banik <subratabanik@google.com>,
        dri-devel@lists.freedesktop.org,
        Manasi Navare <navaremanasi@chromium.org>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Sean Paul <seanpaul@chromium.org>,
        Daniel Vetter <daniel@ffwll.ch>,
        Marcin Wojtas <mwojtas@chromium.org>,
        linux-kernel@vger.kernel.org, David Airlie <airlied@gmail.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
References: <20231130161651.1836047-1-pazz@chromium.org>
From:   Andrzej Hajda <andrzej.hajda@intel.com>
Organization: Intel Technology Poland sp. z o.o. - ul. Slowackiego 173, 80-298
 Gdansk - KRS 101882 - NIP 957-07-52-316
In-Reply-To: <20231130161651.1836047-1-pazz@chromium.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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;