Re: [PATCH v2 0/2] Allow partial memory mapping for cpu memory

[Daniel Vetter <daniel.vetter@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch