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@xxxxxxxxx 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@xxxxxxxxx>
--
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
