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