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

Matthew Auld
2019-07-30T14:28:14Z

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);

?

>
> 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.