| Message ID | 20231105172718.18673-1-pazz@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Fix phys_base to be relative not absolute | expand |
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? > 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 >
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@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 > >
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. 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 >>>
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 >>>> >
On Wed, Nov 22, 2023 at 02:26:55PM +0100, Andrzej Hajda wrote: > > > 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: > > > > > > 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 Hey Andrzej, Sorry for the late response. I was OOO :) I tried using the code you mentioned. It translates (in the very specific case of MTL + GOP driver) to phys_base == 0080_0000h. Unfortunately, it results in a corrupted screen -- the framebuffer is filled with zeros. It seems like `i915_vma_pin_ww` already reserves and binds the GEM BO to the correct address space independently of the value of `phys_base`. The only thing `phys_base` affects is the value of `stolen->start` https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/gem/i915_gem_stolen.c#L747 So it seems to me that the maybe `phys_base` is named incorrectly and that it does not reflect the physical address, but the start offset of i915->mm.stolen_region. I'm happy to run more tests / debug further. Do you have more ideas of things to try? Many thanks, Paz
On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya <pazz@chromium.org> wrote: > > 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: > > > > > > 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 Hey Andrzej, On a second thought, what do you think about something like + gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm; + gen8_pte_t pte; + gte += base / I915_GTT_PAGE_SIZE; + pte = ioread64(gte); + pte = pte & I915_GTT_PAGE_MASK; + phys_base = pte - i915->mm.stolen_region->region.start; The only difference is the last line. Based on what I wrote before, I think `phys_base` is named incorrectly and that it does not reflect the physical address, but the start offset of i915->mm.stolen_region. So if we offset the start value of the stolen region, this code looks correct to me (and it also works on my MeteorLake device). What do you think? Many thanks, Paz
On 28.11.2023 04:47, Paz Zcharya wrote: > > On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya <pazz@chromium.org> wrote: >> >> 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: >>>> >>>> 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 > > Hey Andrzej, > > On a second thought, what do you think about something like > > + gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm; > + gen8_pte_t pte; > + gte += base / I915_GTT_PAGE_SIZE; > + pte = ioread64(gte); > + pte = pte & I915_GTT_PAGE_MASK; > + phys_base = pte - i915->mm.stolen_region->region.start; > > The only difference is the last line. Bingo :) It seems to be generic algorithm to get phys_base for all platforms: - on older platforms stolen_region points to system memory which starts at 0, - on DG2 it uses lmem region which starts at 0 as well, - on MTL stolen_region points to stolen-local which starts at 0x800000. So this whole "if (IS_DGFX(i915)) {...} else {...}" could be replaced with sth generic. 1. Find pte. 2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem = i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region 3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start; Regards Andrzej > > Based on what I wrote before, I think `phys_base` is named incorrectly and > that it does not reflect the physical address, but the start offset of > i915->mm.stolen_region. So if we offset the start value of the stolen > region, this code looks correct to me (and it also works on my > MeteorLake device). > > What do you think? > > > Many thanks, > Paz >
On Tue, Nov 28, 2023 at 12:12:08PM +0100, Andrzej Hajda wrote: > On 28.11.2023 04:47, Paz Zcharya wrote: > > > > On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya <pazz@chromium.org> wrote: > > > > Hey Andrzej, > > > > On a second thought, what do you think about something like > > > > + gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm; > > + gen8_pte_t pte; > > + gte += base / I915_GTT_PAGE_SIZE; > > + pte = ioread64(gte); > > + pte = pte & I915_GTT_PAGE_MASK; > > + phys_base = pte - i915->mm.stolen_region->region.start; > > > > The only difference is the last line. > > Bingo :) It seems to be generic algorithm to get phys_base for all > platforms: > - on older platforms stolen_region points to system memory which starts at > 0, > - on DG2 it uses lmem region which starts at 0 as well, > - on MTL stolen_region points to stolen-local which starts at 0x800000. > > So this whole "if (IS_DGFX(i915)) {...} else {...}" could be replaced > with sth generic. > 1. Find pte. > 2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem = > i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region > 3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start; > > Regards > Andrzej > > Good stuff!! I'll work on this revision and resubmit. Thank you so much Andrzej!
On Tue, Nov 28, 2023 at 12:12:08PM +0100, Andrzej Hajda wrote: > On 28.11.2023 04:47, Paz Zcharya wrote: > > > > On Mon, Nov 27, 2023 at 8:20 PM Paz Zcharya <pazz@chromium.org> wrote: > > > > > > On 21.11.2023 13:06, Andrzej Hajda wrote: > > > > > > > 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 > > > > Hey Andrzej, > > > > On a second thought, what do you think about something like > > > > + gen8_pte_t __iomem *gte = to_gt(i915)->ggtt->gsm; > > + gen8_pte_t pte; > > + gte += base / I915_GTT_PAGE_SIZE; > > + pte = ioread64(gte); > > + pte = pte & I915_GTT_PAGE_MASK; > > + phys_base = pte - i915->mm.stolen_region->region.start; > > > > The only difference is the last line. > > Bingo :) It seems to be generic algorithm to get phys_base for all > platforms: > - on older platforms stolen_region points to system memory which starts at > 0, > - on DG2 it uses lmem region which starts at 0 as well, > - on MTL stolen_region points to stolen-local which starts at 0x800000. > > So this whole "if (IS_DGFX(i915)) {...} else {...}" could be replaced > with sth generic. > 1. Find pte. > 2. if(IS_DGFX(i915) && pte & GEN12_GGTT_PTE_LM) mem = > i915->mm.regions[INTEL_REGION_LMEM_0] else mem = i915->mm.stolen_region > 3. phys_base = (pte & I915_GTT_PAGE_MASK) - mem->region.start; > > Regards > Andrzej > > Hey Andrzej, I uploaded https://patchwork.freedesktop.org/series/127130/ based on algorithm. Please take a look and let me know if you'd like me to change anything. Really appreciate all of your help! Best, Paz
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; }
Fix the value of variable `phys_base` to be the relative offset in stolen memory, and not the absolute offset of the GSM. Currently, the value of `phys_base` is set to "Surface Base Address," which in the case of Meter Lake is 0xfc00_0000. 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(-)