| Message ID | 20240814134837.116498-1-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Allow partial memory mapping for cpu memory | expand |
Argh... sorry, I messed up with format-patch and send-mail and sent it twice. Maybe I need to resend it if CI complains. Andi On Wed, Aug 14, 2024 at 03:48:32PM +0200, Andi Shyti wrote: > Hi, > > I am resending this patch series, not to disregard the previous > discussions, but to ensure it gets tested with the IGTs that > Krzysztof has provided. > > This patch series finalizes the memory mapping fixes and > improvements by enabling partial memory mapping for CPU memory as > well. > > The concept of partial memory mapping, achieved by adding an > object offset, was implicitly introduced in commit 8bdd9ef7e9b1 > ("drm/i915/gem: Fix Virtual Memory mapping boundaries > calculation") for GTT memory. > > To address a previous discussion with Sima and Matt, this feature > is used by Mesa and is required across all platforms utilizing > Mesa. Although Nirmoy suggested using the Fixes tag to backport > this to previous kernels, I view this as a new feature rather > than a fix. > > Lionel, please let me know if you have a different perspective > and believe this should be treated as a bug fix, requiring it > to be backported to stable kernels. > > The IGTs have been developed in collaboration with the Mesa team > to replicate the exact Mesa use case[*]. > > Thanks Chris for the support, thanks Krzysztof for taking care of > the IGT tests, thanks Nirmoy for your reviews and thanks Sima and > Matt for the discussion on this series. > > Andi > > [*] https://patchwork.freedesktop.org/patch/608232/?series=137303&rev=1 > > Test-with: 20240814132404.18392-1-krzysztof.niemiec@intel.com > > Changelog: > ========== > v1 -> v2 > - Added Nirmoy's tags. > > 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
On Wed, Aug 14, 2024 at 03:48:32PM +0200, Andi Shyti wrote: > Hi, > > I am resending this patch series, not to disregard the previous > discussions, but to ensure it gets tested with the IGTs that > Krzysztof has provided. > > This patch series finalizes the memory mapping fixes and > improvements by enabling partial memory mapping for CPU memory as > well. > > The concept of partial memory mapping, achieved by adding an > object offset, was implicitly introduced in commit 8bdd9ef7e9b1 > ("drm/i915/gem: Fix Virtual Memory mapping boundaries > calculation") for GTT memory. > > To address a previous discussion with Sima and Matt, this feature > is used by Mesa and is required across all platforms utilizing > Mesa. Although Nirmoy suggested using the Fixes tag to backport Other vendors than Intel too? > this to previous kernels, I view this as a new feature rather > than a fix. > > Lionel, please let me know if you have a different perspective > and believe this should be treated as a bug fix, requiring it > to be backported to stable kernels. > > The IGTs have been developed in collaboration with the Mesa team > to replicate the exact Mesa use case[*]. > > Thanks Chris for the support, thanks Krzysztof for taking care of > the IGT tests, thanks Nirmoy for your reviews and thanks Sima and > Matt for the discussion on this series. > > Andi > > [*] https://patchwork.freedesktop.org/patch/608232/?series=137303&rev=1 So here is really quick test [1] which I put together in Xe to test partial mmaps, not as complete as the i915 one though. It fails on the Xe baseline. It pass if with [2] in drm_gem.c:drm_gem_mmap. Blindly changing that function might not be the correct solution but thought I'd share as a reference. Matt [1] https://patchwork.freedesktop.org/patch/608268/?series=137313&rev=1 [2] s/drm_vma_offset_exact_lookup_locked/drm_vma_offset_lookup_locked > > Test-with: 20240814132404.18392-1-krzysztof.niemiec@intel.com > > Changelog: > ========== > v1 -> v2 > - Added Nirmoy's tags. > > 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 Matt, On Wed, Aug 14, 2024 at 04:07:02PM +0000, Matthew Brost wrote: > On Wed, Aug 14, 2024 at 03:48:32PM +0200, Andi Shyti wrote: > > I am resending this patch series, not to disregard the previous > > discussions, but to ensure it gets tested with the IGTs that > > Krzysztof has provided. > > > > This patch series finalizes the memory mapping fixes and > > improvements by enabling partial memory mapping for CPU memory as > > well. > > > > The concept of partial memory mapping, achieved by adding an > > object offset, was implicitly introduced in commit 8bdd9ef7e9b1 > > ("drm/i915/gem: Fix Virtual Memory mapping boundaries > > calculation") for GTT memory. > > > > To address a previous discussion with Sima and Matt, this feature > > is used by Mesa and is required across all platforms utilizing > > Mesa. Although Nirmoy suggested using the Fixes tag to backport > > Other vendors than Intel too? Yes, that's what I understood. I hope Lionel can jump in and explain the use cases from Mesa perspective. > > this to previous kernels, I view this as a new feature rather > > than a fix. > > > > Lionel, please let me know if you have a different perspective > > and believe this should be treated as a bug fix, requiring it > > to be backported to stable kernels. > > > > The IGTs have been developed in collaboration with the Mesa team > > to replicate the exact Mesa use case[*]. > > > > Thanks Chris for the support, thanks Krzysztof for taking care of > > the IGT tests, thanks Nirmoy for your reviews and thanks Sima and > > Matt for the discussion on this series. > > > > Andi > > > > [*] https://patchwork.freedesktop.org/patch/608232/?series=137303&rev=1 > > So here is really quick test [1] which I put together in Xe to test > partial mmaps, not as complete as the i915 one though. > > It fails on the Xe baseline. > > It pass if with [2] in drm_gem.c:drm_gem_mmap. Blindly changing that > function might not be the correct solution but thought I'd share as a > reference. Thanks for sharing it. I took a quick look and I think there are a few things missing there. If you want and if this is not in anyone's task list, I can try to "port" this in XE. Is there any other objection to getting this merged into i915? Andi
On Mon, Aug 19, 2024 at 05:16:09PM +0200, Andi Shyti wrote: > Hi Matt, > > On Wed, Aug 14, 2024 at 04:07:02PM +0000, Matthew Brost wrote: > > On Wed, Aug 14, 2024 at 03:48:32PM +0200, Andi Shyti wrote: > > > I am resending this patch series, not to disregard the previous > > > discussions, but to ensure it gets tested with the IGTs that > > > Krzysztof has provided. > > > > > > This patch series finalizes the memory mapping fixes and > > > improvements by enabling partial memory mapping for CPU memory as > > > well. > > > > > > The concept of partial memory mapping, achieved by adding an > > > object offset, was implicitly introduced in commit 8bdd9ef7e9b1 > > > ("drm/i915/gem: Fix Virtual Memory mapping boundaries > > > calculation") for GTT memory. > > > > > > To address a previous discussion with Sima and Matt, this feature > > > is used by Mesa and is required across all platforms utilizing > > > Mesa. Although Nirmoy suggested using the Fixes tag to backport > > > > Other vendors than Intel too? > > Yes, that's what I understood. > > I hope Lionel can jump in and explain the use cases from Mesa > perspective. > Hearing from Lionel would be helpful. > > > this to previous kernels, I view this as a new feature rather > > > than a fix. > > > > > > Lionel, please let me know if you have a different perspective > > > and believe this should be treated as a bug fix, requiring it > > > to be backported to stable kernels. > > > > > > The IGTs have been developed in collaboration with the Mesa team > > > to replicate the exact Mesa use case[*]. > > > > > > Thanks Chris for the support, thanks Krzysztof for taking care of > > > the IGT tests, thanks Nirmoy for your reviews and thanks Sima and > > > Matt for the discussion on this series. > > > > > > Andi > > > > > > [*] https://patchwork.freedesktop.org/patch/608232/?series=137303&rev=1 > > > > So here is really quick test [1] which I put together in Xe to test > > partial mmaps, not as complete as the i915 one though. > > > > It fails on the Xe baseline. > > > > It pass if with [2] in drm_gem.c:drm_gem_mmap. Blindly changing that > > function might not be the correct solution but thought I'd share as a > > reference. > > Thanks for sharing it. I took a quick look and I think there are > a few things missing there. If you want and if this is not in > anyone's task list, I can try to "port" this in XE. > That would be great as I'm sure you undertstand what needs to be done the best. Thanks for volunteering here. > Is there any other objection to getting this merged into i915? > No as long as sima is ok with it, and we prioritize this for Xe as I don't want to remove force probe with an incongruence in behavior from the i915 or have a mesa use case broken. Matt > Andi
On Mon, Aug 19, 2024 at 04:53:41PM +0000, Matthew Brost wrote: > On Mon, Aug 19, 2024 at 05:16:09PM +0200, Andi Shyti wrote: > > Hi Matt, > > > > On Wed, Aug 14, 2024 at 04:07:02PM +0000, Matthew Brost wrote: > > > On Wed, Aug 14, 2024 at 03:48:32PM +0200, Andi Shyti wrote: > > > > I am resending this patch series, not to disregard the previous > > > > discussions, but to ensure it gets tested with the IGTs that > > > > Krzysztof has provided. > > > > > > > > This patch series finalizes the memory mapping fixes and > > > > improvements by enabling partial memory mapping for CPU memory as > > > > well. > > > > > > > > The concept of partial memory mapping, achieved by adding an > > > > object offset, was implicitly introduced in commit 8bdd9ef7e9b1 > > > > ("drm/i915/gem: Fix Virtual Memory mapping boundaries > > > > calculation") for GTT memory. > > > > > > > > To address a previous discussion with Sima and Matt, this feature > > > > is used by Mesa and is required across all platforms utilizing > > > > Mesa. Although Nirmoy suggested using the Fixes tag to backport > > > > > > Other vendors than Intel too? > > > > Yes, that's what I understood. > > > > I hope Lionel can jump in and explain the use cases from Mesa > > perspective. > > > > Hearing from Lionel would be helpful. I'm pretty sure there's no other driver doing this except i915-gem. > > > > this to previous kernels, I view this as a new feature rather > > > > than a fix. > > > > > > > > Lionel, please let me know if you have a different perspective > > > > and believe this should be treated as a bug fix, requiring it > > > > to be backported to stable kernels. > > > > > > > > The IGTs have been developed in collaboration with the Mesa team > > > > to replicate the exact Mesa use case[*]. > > > > > > > > Thanks Chris for the support, thanks Krzysztof for taking care of > > > > the IGT tests, thanks Nirmoy for your reviews and thanks Sima and > > > > Matt for the discussion on this series. > > > > > > > > Andi > > > > > > > > [*] https://patchwork.freedesktop.org/patch/608232/?series=137303&rev=1 > > > > > > So here is really quick test [1] which I put together in Xe to test > > > partial mmaps, not as complete as the i915 one though. > > > > > > It fails on the Xe baseline. > > > > > > It pass if with [2] in drm_gem.c:drm_gem_mmap. Blindly changing that > > > function might not be the correct solution but thought I'd share as a > > > reference. > > > > Thanks for sharing it. I took a quick look and I think there are > > a few things missing there. If you want and if this is not in > > anyone's task list, I can try to "port" this in XE. > > > > That would be great as I'm sure you undertstand what needs to be done > the best. Thanks for volunteering here. > > > Is there any other objection to getting this merged into i915? > > > > No as long as sima is ok with it, and we prioritize this for Xe as I > don't want to remove force probe with an incongruence in behavior from > the i915 or have a mesa use case broken. I've actually gone back to the ggtt fix, and I don't see the security aspect. Like sure if you do a partial unmap you screw up the offset calculation and get unexpected aliasing and a mess and the igt you've linked will fail. But I don't see how that's a security bug? And if it is, then it's a drm wide issue, because they all work like that. And so probably we need a drm wide fix first. So yeah I'm still stuck on the fundamentals here of why this is even done. And for the uapi extension of allowing partial mmaps, we need the full uapi dance. Which means some driver flag so userspace can figure out this is supported, and a link to the mesa MR that uses this all. Without a link to a mesa MR this won't go anywhere, because that's the rules for new uapi. -Sima
Hi, I am resending this patch series, not to disregard the previous discussions, but to ensure it gets tested with the IGTs that Krzysztof has provided. This patch series finalizes the memory mapping fixes and improvements by enabling partial memory mapping for CPU memory as well. The concept of partial memory mapping, achieved by adding an object offset, was implicitly introduced in commit 8bdd9ef7e9b1 ("drm/i915/gem: Fix Virtual Memory mapping boundaries calculation") for GTT memory. To address a previous discussion with Sima and Matt, this feature is used by Mesa and is required across all platforms utilizing Mesa. Although Nirmoy suggested using the Fixes tag to backport this to previous kernels, I view this as a new feature rather than a fix. Lionel, please let me know if you have a different perspective and believe this should be treated as a bug fix, requiring it to be backported to stable kernels. The IGTs have been developed in collaboration with the Mesa team to replicate the exact Mesa use case[*]. Thanks Chris for the support, thanks Krzysztof for taking care of the IGT tests, thanks Nirmoy for your reviews and thanks Sima and Matt for the discussion on this series. Andi [*] https://patchwork.freedesktop.org/patch/608232/?series=137303&rev=1 Test-with: 20240814132404.18392-1-krzysztof.niemiec@intel.com Changelog: ========== v1 -> v2 - Added Nirmoy's tags. 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(-)