[Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute

[Andrzej Hajda]

On 21.11.2023 13:06, Andrzej Hajda wrote:
> 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.

It was quite cryptic. I meant I have not found anything about assumption 
from commit history that for iGPU there should be 1:1 mapping, this is 
why there was an assignment "phys_base = base". Possibly the assumption 
is not valid anymore for MTL(?).
Without the assumption we need to check GGTT to determine phys address.

> 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 at chromium.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
>>>>
>