On Wed, Jun 03, 2026 at 02:57:48PM +0200, Maarten Lankhorst wrote: > Hello, > > On 5/27/26 16:45, Rodrigo Vivi wrote: > > The PF GGTT allocator was initialised over a relative [0, usable_size) > > range, with ggtt->start added on every address conversion to get the > > actual hardware address. Two consequences of that model were considered > > "horrible hacks": > > > > - ggtt->start (the WOPCM offset) had to be carried around and added > > to every drm_mm result. > This is still the case, only we add a separate variable now that is usually 0. > We always have to ensure that this is correctly accounted > for, otherwise we get subtle bugs, so there's no reason not to use it, > if it's already used everywhere. > > Whether it's 0 or wopcm_start doesn't change the result. > > > - The GUC_GGTT_TOP ceiling silently truncated the GGTT range instead > > of being made explicit, leaving PTEs in [GUC_GGTT_TOP, total_size) > > untouched during the initial clear. > This is a valid concern, but it's very easily be fixed inside the initial > clear > by calling xe_ggtt_clear once more with a comment for !VF case. > > Something like below is enough to fix it. Adding a reserved region at the > end/start > is not the way to do so and just adds more complications. > ---8<--- > When initialising GGTT, we never clear the part above GUC_GGTT_TOP. > Add a member to &xe_ggtt that holds the full GGTT size, and clear it > during the pass at xe_ggtt_initial_clear. > > Reported-by: Ville Syrjälä <[email protected]> > Signed-off-by: Maarten Lankhorst <[email protected]> > --- > diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c > index 8ec23862477fc..497b99a932f0d 100644 > --- a/drivers/gpu/drm/xe/xe_ggtt.c > +++ b/drivers/gpu/drm/xe/xe_ggtt.c > @@ -115,6 +115,8 @@ struct xe_ggtt { > u64 start; > /** @size: Total usable size of this GGTT */ > u64 size; > + /** @entire_size: complete size of the accessible GGTT, reserved > regions inclusive */ > + u64 entire_size; > /** > * @flags: Flags for this GGTT. > * Acceptable flags: > @@ -406,7 +408,8 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt) > return -ENOMEM; > } > ggtt_start = wopcm; > - ggtt_size = (gsm_size / 8) * (u64)XE_PAGE_SIZE - ggtt_start; > + ggtt->entire_size = (gsm_size / 8) * (u64)XE_PAGE_SIZE; > + ggtt_size = ggtt->entire_size - ggtt_start; > } else { > ggtt_start = xe_tile_sriov_vf_ggtt_base(ggtt->tile); > ggtt_size = xe_tile_sriov_vf_ggtt(ggtt->tile); > @@ -417,6 +420,7 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt) > ggtt_start, ggtt_start + ggtt_size - 1); > return -ERANGE; > } > + ggtt->entire_size = ggtt_size; > } > > ggtt->gsm = ggtt->tile->mmio.regs + SZ_8M; > @@ -461,6 +465,10 @@ static void xe_ggtt_initial_clear(struct xe_ggtt *ggtt) > drm_mm_for_each_hole(hole, &ggtt->mm, start, end) > xe_ggtt_clear(ggtt, ggtt->start + start, end - start); > > + end = ggtt->start + ggtt->size; > + if (ggtt->entire_size > end) > + xe_ggtt_clear(ggtt, end, ggtt->entire_size - end);
You'll need the same thing for the 0 to ggtt->start range. > + > xe_ggtt_invalidate(ggtt); > mutex_unlock(&ggtt->lock); > } -- Ville Syrjälä Intel
