Messages in this thread | | | Date | Tue, 28 Nov 2023 12:12:08 +0100 | Subject | Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be relative not absolute | From | Andrzej Hajda <> |
| |
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 >
| |