Re: [PATCH v2 29/37] drm/i915: Introduce DRM_I915_GEM_MMAP_OFFSET

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 30, 2019 at 03:28:11PM +0100, Matthew Auld wrote:
> On 30/07/2019 10:49, Daniel Vetter wrote:
> > On Thu, Jun 27, 2019 at 09:56:25PM +0100, Matthew Auld wrote:
> > > From: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > > 
> > > Add a new CPU mmap implementation that allows multiple fault handlers
> > > that depends on the object's backing pages.
> > > 
> > > Note that we multiplex mmap_gtt and mmap_offset through the same ioctl,
> > > and use the zero extending behaviour of drm to differentiate between
> > > them, when we inspect the flags.
> > > 
> > > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
> > So I thought that the plan is to reject invalid mmaps, i.e. mmap modes
> > which are not compatibale with all placement options. Given that, why do
> > we need this?
> 
> We are meant to reject anything !wc for LMEM. There were some patches for
> that but I guess got lost under the radar...
> 
> > 
> > - cpu mmap with all the flags still keep working, as long as the only
> >    placement you select is smem.
> > 
> > - for lmem/stolen the only option we have is a wc mapping, either through
> >    the pci bar or through the gtt. So for objects only sitting in there
> >    also no problem, we can just keep using the current gtt mmap stuff (but
> >    redirect it internally).
> > 
> > - that leaves us with objects which can move around. Only option allows is
> >    WC, and the gtt mmap ioctl does that already. When the object is in smem
> >    we'll need to redirect it to a cpu wc mmap, but I think we need to do
> >    that anyway.
> 
> So for legacy, gtt_mmap will still go through the aperture, otherwise if
> LMEM is supported then there is no aperture, so we just wc mmap via cpu or
> LMEMBAR depending on the final object placement. And cpu_mmap still works if
> we don't care about LMEM. Hmm, so do we even need most of the previous patch
> then? ALso does that mean we also have to track the placement of an object
> in igt?
> 
> gem_mmap__wc:
> 
> if (supports_lmem(dev))
> 	gtt_mmap();
> else
> 	gem_mmap(wc);
> 
> gem_mmap__wc:
> 
> if (placement_contains(obj, LMEM))
> 	gtt_mmap();
> else
> 	gem_mmap(wc);
> 
> ?

Well if you want cpu wc mmaps, then just allocate it as smem ... we might
need a new gem_mmap__lmem I guess to exercise all the possible ways to get
at stuff in lmem (including when it migrates around underneath us while we
access it through the mmap). I wouldn't try too hard to smash all these
use/testcases into one.

> > So not really seeing what the uapi problem is you're trying to solve here?
> > 
> > Can you pls explain why we need this?
> 
> The naming of gtt_mmap seemed confusing, since there is no aperture, and
> having one mmap ioctl to cover both smem and lmem seemed like a nice
> idea...also I think umd's stopped using gtt_mmap(or were told to?) but maybe
> those aren't good enough reasons.

We stopped using gtt mmap because for many cases cpu WC mmap is faster.

Wrt having a clean slate: Not sure why this would benefit us, we just
diverge a bit more from how this works on !lmem, so a bit more complexity
(not much) everywhere for not much gain.

I'm also not sure whether there will be a whole lot of uses of such a
magic LMEMBAR wc mapping. It's probably slow for the exact same reasons
gtt mmap is slow.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx