Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
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 +, Matthew Brost wrote:
> > > On Tue, Aug 13, 2024 at 07:08:02PM +, 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 +, 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 ins
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
Hi Sima,
On Mon, Aug 19, 2024 at 04:17:01PM +0200, Daniel Vetter wrote:
> On Wed, Aug 14, 2024 at 02:08:49AM +, Matthew Brost wrote:
> > On Tue, Aug 13, 2024 at 07:08:02PM +, 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 +, 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
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
On Wed, Aug 14, 2024 at 02:08:49AM +, Matthew Brost wrote:
> On Tue, Aug 13, 2024 at 07:08:02PM +, 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 +, 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 :-)
> > >
> >
> > 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
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
On Tue, Aug 13, 2024 at 07:08:02PM +, 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 +, 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 :-)
> >
>
> 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 bu
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
On Tue, Aug 13, 2024 at 04:09:55PM +0200, Daniel Vetter wrote:
> On Tue, Aug 13, 2024 at 02:54:31AM +, 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 :-)
>
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.
Matt
> Cheers, Sima
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
On Tue, Aug 13, 2024 at 02:54:31AM +, 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
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
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.
We don't have IGT test cases for this in Xe though, we likely should add
some if mesa is doing this.
Matt
> needs igt testcases, and it needs a solid way to detect whether the
> kernel supports this feature or not. But unless other drivers are doing
> this too, I have some big questions why i915-gem needs this.
>
> > > Also, does xe also support this? If we only add this to i915-gem but xe
> > > doesn't have it, it doesn't make much sense imo.
> >
> > I don't know about. Lionel, Do you have anything to add here from
> > your side?
>
> "I don't know" is not an acceptable answer for uapi work.
> -Sima
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
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
needs igt testcases, and it needs a solid way to detect whether the
kernel supports this feature or not. But unless other drivers are doing
this too, I have some big questions why i915-gem needs this.
> > Also, does xe also support this? If we only add this to i915-gem but xe
> > doesn't have it, it doesn't make much sense imo.
>
> I don't know about. Lionel, Do you have anything to add here from
> your side?
"I don't know" is not an acceptable answer for uapi work.
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
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.
> Also, does xe also support this? If we only add this to i915-gem but xe
> doesn't have it, it doesn't make much sense imo.
I don't know about. Lionel, Do you have anything to add here from
your side?
Thanks,
Andi
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
On Fri, Aug 09, 2024 at 11:20:56AM +0100, Andi Shyti wrote:
> Hi Sima,
>
> 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.
Also, does xe also support this? If we only add this to i915-gem but xe
doesn't have it, it doesn't make much sense imo.
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
Hi Sima,
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.
Thanks,
Andi
Re: [PATCH 0/2] Allow partial memory mapping for cpu memory
On Wed, Aug 07, 2024 at 11:05:19AM +0100, Andi Shyti wrote:
> Hi,
>
> 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.
-Sima
>
> Andi
>
> Andi Shyti (2):
> drm/i915/gem: Do not look for the exact address in node
> drm/i915/gem: Calculate object page offset for partial memory mapping
>
> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 10 ++
> drivers/gpu/drm/i915/i915_mm.c | 12 +++-
> drivers/gpu/drm/i915/i915_mm.h | 3 ++-
> 3 files changed, 19 insertions(+), 6 deletions(-)
>
> --
> 2.45.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
