| Message ID | 20240807100521.478266-1-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Allow partial memory mapping for cpu memory | expand |
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 >
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
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
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
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
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
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
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: > > > > 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
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: > > > > > 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 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 Also thanks catching this Sima. Matt > > Matt > > > Cheers, Sima > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
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: > > > > > > 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 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. Cheers, Sima
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. Thanks, Andi [1] https://gitlab.freedesktop.org/llandwerlin/igt-gpu-tools/-/commits/wip/gem_mmap_offset-partial-unmap [2] https://patchwork.freedesktop.org/series/137303/
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
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. 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(-)