Messages in this thread | | | Date | Wed, 22 Nov 2023 14:26:55 +0100 | Subject | Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute | From | 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@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 >>>> >
| |