Re: [PATCH v3] intel: New libdrm interface to create unbound wc user mappings for objects

[Damien Lespiau <damien.lespiau@intel.com>]

On Tue, Dec 09, 2014 at 10:54:25AM +0000, Chris Wilson wrote:
> On Wed, Dec 03, 2014 at 02:13:58PM +0000, Damien Lespiau wrote:
> > On Tue, Oct 28, 2014 at 06:39:27PM +0530, akash.goel@intel.com wrote:
> > > @@ -1126,6 +1136,8 @@ static void drm_intel_gem_bo_purge_vma_cache(drm_intel_bufmgr_gem *bufmgr_gem)
> > >  
> > >  	/* We may need to evict a few entries in order to create new mmaps */
> > >  	limit = bufmgr_gem->vma_max - 2*bufmgr_gem->vma_open;
> > > +	if (bufmgr_gem->has_ext_mmap)
> > > +		limit -= bufmgr_gem->vma_open;
> > >  	if (limit < 0)
> > >  		limit = 0;
> > >  
> > 
> > Aren't we being overly aggressive in purging the vma cache with this new
> > code? I guess it depends on the workload but I don't think the WC
> > mapping is likely to be used in addition to the other two often enough
> > to warrant a max - 3 * open;
> 
> And if they do, you've just exceeded their limit. And it conceivably
> that you will have bo with all 3 vma mappings, though I only expect 2 to
> be active in any single lifetime. The patch is correct regarding the code
> as it exists, you may want to argue for finer tracking of vma_open and
> vma_cached, but that would require a different approach.
>  
> > > +	/* Now move it to the GTT domain so that the GPU and CPU
> > > +	 * caches are flushed and the GPU isn't actively using the
> > > +	 * buffer.
> > > +	 *
> > > +	 * The domain change is done even for the objects which
> > > +	 * are not bounded. For them first the pages are acquired,
> > > +	 * before the domain change.
> > > +	 */
> > > +	VG_CLEAR(set_domain);
> > > +	set_domain.handle = bo_gem->gem_handle;
> > > +	set_domain.read_domains = I915_GEM_DOMAIN_GTT;
> > > +	set_domain.write_domain = I915_GEM_DOMAIN_GTT;
> > > +	ret = drmIoctl(bufmgr_gem->fd,
> > > +		       DRM_IOCTL_I915_GEM_SET_DOMAIN,
> > > +		       &set_domain);
> > > +	if (ret != 0) {
> > > +		DBG("%s:%d: Error setting domain %d: %s\n",
> > > +		    __FILE__, __LINE__, bo_gem->gem_handle,
> > > +		    strerror(errno));
> > > +	}
> > 
> > Why move the buffer to the GTT domain and not the CPU domain here?
> 
> There are 3 cache domains: GPU, CPU and NONE (aka GTT). The purpose of
> mmap(wc) is to be able to access the buffer without going through the CPU
> cache (and it should be out of the GPU cache to maintain coherency). The
> NONE cache domain is exactly what we want, or else you end up in clflush
> misery.

Right that leaves the last point in my answer to v3. With that addressed
this is:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

--
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx