On Thu, Dec 14, 2023 at 12:06:58PM +0100, Andrzej Hajda wrote: > > > On 13.12.2023 17:13, Andrzej Hajda wrote: > > On 13.12.2023 02:37, Patchwork wrote: > >> *Patch Details* > >> *Series:* drm/i915: (stolen) memory region related fixes > >> *URL:* https://patchwork.freedesktop.org/series/127721/ > >> <https://patchwork.freedesktop.org/series/127721/> > >> *State:* failure > >> *Details:* > >> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html > >> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html> > >> > >> > >> CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1 > >> > >> > >> Summary > >> > >> *FAILURE* > >> > >> Serious unknown changes coming with Patchwork_127721v1 absolutely need > >> to be > >> verified manually. > >> > >> If you think the reported changes have nothing to do with the changes > >> introduced in Patchwork_127721v1, please notify your bug team > >> (I915-ci-infra@xxxxxxxxxxxxxxxxxxxxx) to allow them > >> to document this new failure mode, which will reduce false positives > >> in CI. > >> > >> External URL: > >> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html > >> > >> > >> Participating hosts (31 -> 33) > >> > >> Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510 > >> Missing (2): bat-adlp-11 fi-snb-2520m > >> > >> > >> Possible new issues > >> > >> Here are the unknown changes that may have been introduced in > >> Patchwork_127721v1: > >> > >> > >> IGT changes > >> > >> > >> Possible regressions > >> > >> * > >> > >> igt@i915_module_load@load: > >> > >> o bat-mtlp-8: NOTRUN -> INCOMPLETE > >> > >> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@xxxxxxxxx> > > > > > > This is due to overlapping initial fb's vma with GuC reserved area on > > MTL, see ggtt_reserve_guc_top. > > > > My dirty quick_fix proposed: > > @@ -143,6 +143,9 @@ initial_plane_vma(struct drm_i915_private *i915, > > if (IS_ERR(vma)) > > goto err_obj; > > > > + if (base + size > GUC_GGTT_TOP) > > + base = min(base, GUC_GGTT_TOP) - size; > > + > > pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base; > > if (HAS_GMCH(i915)) > > pinctl |= PIN_MAPPABLE; > > > Copy/Paste Ville response for this proposition from another thread: > > On 13.12.2023 17:03, Ville Syrjälä wrote: > > > > This is not a solution. The correct fix is either: > > 1. fix the guc code to not insist on a fixed chunk of the ggtt > > and instead allocate it normally like a good boy. It could > > still try to allocate as high as possible to ideally > > land in the GUC_GGTT_TOP range > > This would be the best solution from initial plane PoV for sure, I am > not sure about GuC PoV. I was going to submit a patch to simply change this to insert(PIN_HIGH) as that is all that is needed from the GuC FW POV. And whether the GOP framebuffer or the GuC FW lives there doesn't matter one bit, both fit the bill of not needing to be accessed by the GuC. But I suspect this function might be serving a double duty, despite not saying so anywhere. Basically I think it's perhaps used as a quick and dirty way to restrict everything below GUC_GGTT_TOP, presumably because we don't otherwise restrict allocations below that even if the GuC needs to access them. So the insert(PIN_HIGH) approach could result in some troubles iff the GOP framebuffer is preventing the uc_fw to be placed at GUC_GGTT_TOP and we later free the GOP framebuffer (happens if we find it unsuitable due to one reason or another). This opens up a hole above GUC_GGTT_TOP again and something else could get bound there. Is my assumption about ggtt_reserve_guc_top() correct? And if so do we know what are all the things the GuC needs to access so that we could properly restrict those below GUC_GGTT_TOP and get rid of this weird way of achieving that same result? > > > > > 2. relocate the display vma to a different part of the ggtt > > I think this point is what I have proposed. > > > > > > > 1. should be far simpler by the looks of it, as 2. would involve: > > a) pinning the same object into two places in the ggtt simultanously > > I don't think we have support for that except for special ggtt views, > > and xe doesn't have even that (should we need this trick there as > well) > > AFAIU the fb is not yet pinned in terms of kernel structures, so it > doesn't seems to be an issue. Hmm. Yeah, I suppose we don't really care where in the ggtt the thing is bound as we rewrite the PTEs anyway when we bind the vma. So whether we bind it into its current place or not doesn't really matter. Well, assuming we move the thing to a non-overlapping section of the ggtt. Any overlap with the current positon would clobber the currently used PTEs and cause display corruption. If we did try this we'd also have to think where to move it. Due the requirement of no overlaps I suppose offset 0 might be the most obvious choice. That would also avoid permanently fragmentating the ggtt. Though I suppose one has to wonder why the GOP doesn't just use offset 0 anyway. Is there perhaps some other weird requirement that needs the bottom end of ggtt to be available for something else? > Of course there is problem with PLANE_SURF still pointing to old VA, it > should be replaced with new VA before ggtt will be populated with new stuff. > > > > > b) flip the plane(s) over to the relocated vma > > c) wait for vblank(s) > > d) discard the original vma > > e) all of that would need to happen pretty early so we may not have > > a lot of the required normal machinery fully up and running yet > > Async update to PLANE_SURF shouldn't be enough? Async flips: - Can't be assume to be universally available - Don't really help anyway as they don't happen instaneously. Ie. you still need to wait for them to have happened before clobbering old the PTEs. -- Ville Syrjälä Intel
