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

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

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