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