Message-ID: <8dd6f4da-dcc9-4ea3-8395-bf048b0dbc93@intel.com>
Date:   Tue, 21 Nov 2023 13:06:17 +0100
From:   Andrzej Hajda <andrzej.hajda@...el.com>
To:     Paz Zcharya <pazz@...omium.org>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>
Cc:     Subrata Banik <subratabanik@...gle.com>,
        Tvrtko Ursulin <tvrtko.ursulin@...el.com>,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, Sean Paul <seanpaul@...omium.org>,
        matthew.auld@...el.com, Daniel Vetter <daniel@...ll.ch>,
        Marcin Wojtas <mwojtas@...omium.org>,
        Drew Davenport <ddavenport@...omium.org>,
        David Airlie <airlied@...il.com>,
        Nirmoy Das <nirmoy.das@...el.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be
 relative not absolute

On 18.11.2023 00:01, Paz Zcharya wrote:
> On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
>> On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
>>> Fix the value of variable `phys_base` to be the relative offset in
>>> stolen memory, and not the absolute offset of the GSM.
>>
>> to me it looks like the other way around. phys_base is the physical
>> base address for the frame_buffer. Setting it to zero doesn't seem
>> to make that relative. And also doesn't look right.
>>
>>>
>>> Currently, the value of `phys_base` is set to "Surface Base Address,"
>>> which in the case of Meter Lake is 0xfc00_0000.
>>
>> I don't believe this is a fixed value. IIRC this comes from the register
>> set by video bios, where the idea is to reuse the fb that was used so
>> far.
>>
>> With this in mind I don't understand how that could overflow. Maybe
>> the size of the stolen is not right? maybe the size? maybe different
>> memory region?
>>
> 
> Hi Rodrigo, thanks for the great comments.
> 
> Apologies for using a wrong/confusing terminology. I think 'phys_base'
> is supposed to be the offset in the GEM BO, where base (or
> "Surface Base Address") is supposed to be the GTT offset.

Since base is taken from PLANE_SURF register it should be resolvable via 
GGTT to physical address pointing to actual framebuffer.
I couldn't find anything in the specs.
The simplest approach would be then do the same as in case of DGFX:
		gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm;
		gen8_pte_t pte;

		gte += base / I915_GTT_PAGE_SIZE;

		pte = ioread64(gte);
		phys_base = pte & I915_GTT_PAGE_MASK;

Regards
Andrzej


> 
> Other than what I wrote before, I noticed that the function 'i915_vma_pin'
> which calls to 'i915_gem_gtt_reserve' is the one that binds the right
> address space in the GTT for that stolen region.
> 
> I see that in the function 'i915_vma_insert' (full call stack below),
> where if (flags & PIN_OFFSET_FIXED), then when calling 'i915_gem_gtt_reserve'
> we add an offset.
> 
> Specifically in MeteorLake, and specifically when using GOP driver, this
> offset is equal to 0xfc00_0000. But as you mentioned, this is not strict.
> 
> The if statement always renders true because in the function
> 'initial_plane_vma' we always set
> pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
> where pinctl == flags (see file 'intel_plane_initial.c' line 145).
> 
> Call stack:
> drm_mm_reserve_node
> i915_gem_gtt_reserve
> 	i915_vma_insert
> i915_vma_pin_ww
> i915_vma_pin
> initial_plane_vma
> intel_alloc_initial_plane_obj
> intel_find_initial_plane_obj
> 
> Therefore, I believe the variable 'phys_base' in the
> function 'initial_plane_vma,' should be the the offset in the GEM BO
> and not the GTT offset, and because the base is added later on
> in the function 'i915_gem_gtt_reserve', this value should not be
> equal to base and be 0.
> 
> Hope it makes more sense.
> 
>>> This causes the
>>> function `i915_gem_object_create_region_at` to fail in line 128, when
>>> it attempts to verify that the range does not overflow:
>>>
>>> if (range_overflows(offset, size, resource_size(&mem->region)))
>>>        return ERR_PTR(-EINVAL);
>>>
>>> where:
>>>    offset = 0xfc000000
>>>    size = 0x8ca000
>>>    mem->region.end + 1 = 0x4400000
>>>    mem->region.start = 0x800000
>>>    resource_size(&mem->region) = 0x3c00000
>>>
>>> call stack:
>>>    i915_gem_object_create_region_at
>>>    initial_plane_vma
>>>    intel_alloc_initial_plane_obj
>>>    intel_find_initial_plane_obj
>>>    intel_crtc_initial_plane_config
>>>
>>> Looking at the flow coming next, we see that `phys_base` is only used
>>> once, in function `_i915_gem_object_stolen_init`, in the context of
>>> the offset *in* the stolen memory. Combining that with an
>>> examinination of the history of the file seems to indicate the
>>> current value set is invalid.
>>>
>>> call stack (functions using `phys_base`)
>>>    _i915_gem_object_stolen_init
>>>    __i915_gem_object_create_region
>>>    i915_gem_object_create_region_at
>>>    initial_plane_vma
>>>    intel_alloc_initial_plane_obj
>>>    intel_find_initial_plane_obj
>>>    intel_crtc_initial_plane_config
>>>
>>> [drm:_i915_gem_object_stolen_init] creating preallocated stolen
>>> object: stolen_offset=0x0000000000000000, size=0x00000000008ca000
>>>
>>> Signed-off-by: Paz Zcharya <pazz@...omium.org>
>>> ---
>>>
>>>   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..e696cb13756a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>>> @@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
>>>   			"Using phys_base=%pa, based on initial plane programming\n",
>>>   			&phys_base);
>>>   	} else {
>>> -		phys_base = base;
>>> +		phys_base = 0;
>>>   		mem = i915->mm.stolen_region;
>>>   	}
>>>   
>>> -- 
>>> 2.42.0.869.gea05f2083d-goog
>>>

Powered by blists - more mailing lists