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@xxxxxxxxxxxx>
---
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
