On Mon, Aug 19, 2024 at 05:31:36PM +0200, Andi Shyti wrote:
> Hi Sima,
>
> On Mon, Aug 19, 2024 at 04:17:01PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 14, 2024 at 02:08:49AM +0000, Matthew Brost wrote:
> > > On Tue, Aug 13, 2024 at 07:08:02PM +0000, Matthew Brost wrote:
> > > > On Tue, Aug 13, 2024 at 04:09:55PM +0200, Daniel Vetter wrote:
> > > > > 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:
> > > > > > > > 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 :-)
> > > > >
> > > >
> > > > Not blind.
> > > >
> > > > > > 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 ...
> > > > >
> > > >
> > > > Just wrote one, it fails.
> > > >
> > > > So agree with everything you are saying. Sorry for the noise.
> > >
> > > To be clear what I agree with:
> > >
> > > - Xe doesn't support partial mmaps, if the i915 / Mesa needs to support
> > > partial mmaps Xe needs changed in step with the i915 (it is a 1 line
> > > change in drm_gem_mmap which then will a community ack too)
> > > - We need IGTs for partial mmaps in both i915 and Xe
> > > - The Mesa use needs to be understood ensuring this isn't a bug on their
> > > end and this actually required
> > > - If partial mmaps are actually required, I'd like to see this in 6.12
> > > for Xe as we are about to remove force probe
> >
> > Fwiw I concur on this all. Maybe one thing to add is that if i915/mesa do
> > not need partial mmaps, then we should also again removed them for gtt
> > mmaps since that patch already slipped in.
>
> I'm sorry, but I am not understanding why i915 should
> deliberately stop supporting Mesa? This was requested by them
> because there were bug reports filed.
>
> There is a test developed by Lionel[1] that is now taken into
> igt[2].
>
> The gtt partial mapping did not slip in accidentaly, we fixed a
> security issue where the partial mapping needed to be taken into
> account.
Yeah the partial unmap looks like it could wrong, would be really good to
make sure we validate that on xe too (and any other mapping type i915
hase).
But I thought this was about allowing partial mmap to begin with, which is
a different beast?
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
