Re: [PATCH] drm/i915: Support to create uncached user mapping for a Gem object

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Oct 23, 2014 at 04:03:56PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> This patch provides support to create uncached virtual mappings for a Gem
> object. It intends to provide the same funtionality of 'mmap_gtt' interface
> without the constraints of a limited aperture space, but provided clients
> handles the linear to tile conversion on their own.
> This is for improving the CPU write operation performance, as with such
> mapping, writes are almost 50% faster than with mmap_gtt. Also it avoids the
> Cache flush after update from CPU side, when object is passed onto GPU, which
> will be the case if regular mmap ioctl interface is used.
> This type of mapping is specially useful in case of sub-region update,
> i.e. when only a portion of the object is to be updated.
> To ensure the cache coherency, before using this mapping, the GTT domain has
> been reused here. This provides the required Cache flush if the object is in
> CPU domain or synchronization against the concurrent rendering. Although the
> access through an uncached mmap shall automatically invalidate the cache lines,
> but this may not be true for non temporal write instructions and also not all
> pages of the object be updated at any given point of time through this mapping.
> Having a call to get_pages in set_to_gtt_domain function, as added by Chris in
> the earlier patch, would guarantee the clflush and so there will be no cache-
> lines holding the data for the object before it is accessed through this map.
> A new field 'flags' has been added to 'drm_i915_gem_mmap' structure, used in
> gem_mmap ioctl, which allows to convey the required mapping type as uncached.
> User can query the driver for the support of this mapping through the
> get_params. For that a new define I915_PARAM_HAS_UC_MMAP has been added.
> 
> Change-Id: Ie883942f9e689525f72fe9a8d3780c3a9faa769a
> Signed-off-by: Akash Goel <akash.goel@intel.com>

Yeah, I like this. And Chris has already gone ahead and added all the
functional tests. So here's what seems to still be missing:
- Reviewing Chris' patches. Akash, can you please do that? This kind of
  cross-review usually works best.
- Polish for Akash' patch according to Chris' review. Commit message also
  needs to gain the performance data. And we need to have a "does PAT
  work" test I think like Chris suggested.
- ioctl argument check tests for the cpu mmap ioctl. We currently have
  absotutely nothing here (one of the very few cases left). So Akash, can
  you please take the AR to write a new igt testcase which does all the
  usual argument checking test like invalid buffer, invalid flags?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch