On Tue, Aug 13, 2024 at 02:54:31AM +0000, Matthew Brost wrote:
> On Mon, Aug 12, 2024 at 04:45:32PM +0200, Daniel Vetter wrote:
> > On Mon, Aug 12, 2024 at 01:51:30PM +0200, Andi Shyti wrote:
> > > Hi Daniel,
> > >
> > > On Mon, Aug 12, 2024 at 11:11:21AM +0200, Daniel Vetter wrote:
> > > > On Fri, Aug 09, 2024 at 11:20:56AM +0100, Andi Shyti wrote:
> > > > > On Fri, Aug 09, 2024 at 10:53:38AM +0200, Daniel Vetter wrote:
> > > > > > On Wed, Aug 07, 2024 at 11:05:19AM +0100, Andi Shyti wrote:
> > > > > > > This patch series concludes on the memory mapping fixes and
> > > > > > > improvements by allowing partial memory mapping for the cpu
> > > > > > > memory as well.
> > > > > > >
> > > > > > > The partial memory mapping by adding an object offset was
> > > > > > > implicitely included in commit 8bdd9ef7e9b1 ("drm/i915/gem: Fix
> > > > > > > Virtual Memory mapping boundaries calculation") for the gtt
> > > > > > > memory.
> > > > > >
> > > > > > Does userspace actually care? Do we have a flag or something, so that
> > > > > > userspace can discover this?
> > > > > >
> > > > > > Adding complexity of any kind is absolute no-go, unless there's a
> > > > > > userspace need. This also includes the gtt accidental fix.
> > > > >
> > > > > Actually this missing functionality was initially filed as a bug
> > > > > by mesa folks. So that this patch was requested by them (Lionel
> > > > > is Cc'ed).
> > > > >
> > > > > The tests cases that have been sent previously and I'm going to
> > > > > send again, are directly taken from mesa use cases.
> > > >
> > > > Please add the relevant mesa MR to this patch then, and some relevant
> > > > explanations for how userspace detects this all and decides to use it.
> > >
> > > AFAIK, there is no Mesa MR. We are adding a feature that was
> > > missing, but Mesa already supported it (indeed, Nimroy suggested
> > > adding the Fixes tag for this).
> > >
> > > Also because, Mesa was receiving an invalid address error and
> > > asked to support the partial mapping of the memory.
> >
> > Uh this sounds a bit too much like just yolo'ing uabi. There's two cases:
> >
> > - Either this is a regression, it worked previously, mesa is now angry.
> > Then we absolutely need a Fixes: tag, and we also need that for the
> > preceeding work to re-enable this for gtt mappings.
> >
> > - Or mesa is just plain wrong here, which is what my guess is. Because bo
> > mappings have always been full-object (except for the old-style shm
> > mmaps). In that case mesa needs to be fixed (because we're not going to
> > backport old uapi).
> >
> > Also in that case, _if_ (and that's a really big if) we really want this
> > uapi, we need it in xe too, it needs a proper mesa MR to use it, it
>
> I looked at this code from Xe PoV to see if we support this and I think
> we actually do as our CPU fault handler more or less just calls
> ttm_bo_vm_fault_reserved which has similar code to this patch. So I
> think this actually a valid fix. Can't be 100% sure though as I quickly
> just reversed engineered this code and TTM.
That's the fault path, which isn't relevant here since you already have
the vma set up. The relevant path is the mmap path, which is common for
all gem drivers nowadays and the lookup handled entirely in the core. Well
except for i915-gem being absolutely massively over the top special in
everything. i915-gem being extremely special here is also why this patch
caught my attention, because it just furthers the divergence instead of at
least stopping. Since we've given up on trying to get i915-gem onto common
code and patterns that's not an option, and the reason why xe exists ...
Anyway that common gem bo mmap code code is in drm_gem_mmap and still only
allows exact matches.
Unless I'm completely blind, it does happen :-)
> We don't have IGT test cases for this in Xe though, we likely should add
> some if mesa is doing this.
I'd expect they would fail ...
Cheers, Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
