From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 3/6] drm/i915: Support write only mappings Date: Tue, 20 Sep 2011 09:30:27 +0100 Message-ID: References: <1316492706-31081-1-git-send-email-ben@bwidawsk.net> <1316492706-31081-4-git-send-email-ben@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 04A5FA0AA5 for ; Tue, 20 Sep 2011 01:30:31 -0700 (PDT) In-Reply-To: <1316492706-31081-4-git-send-email-ben@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: intel-gfx@lists.freedesktop.org Cc: Daniel Vetter , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Mon, 19 Sep 2011 21:25:03 -0700, Ben Widawsky wrote: > When doing a GTT mapping, the pages are not backed until taking a fault. > If we know that the object is write only, when the fault happens we do not need > to make the pages coherent with the CPU. This allows for semi fast prefaults > to occur in libdrm at map time. > > For the non-GTT case, there isn't much to do except make sure we handle things > properly we we move an object out of the GTT. > > Finally, modify the set domain IOCTL to actually mark objects as write only. > Current limitation in that once an object is marked write only, it cannot be > readable. I couldn't see any reason for this restriction and it does impose a significant burden on the userspace cache; any buffer used for write-only mappings must be purged and can't be reused. > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_gem.c | 32 ++++++++++++++++++++++++-------- > 1 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index d4de7f7..48ea4bc 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1047,6 +1047,7 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > struct drm_i915_gem_object *obj; > uint32_t read_domains = args->read_domains; > uint32_t write_domain = args->write_domain; > + int write_only = 0; > int ret; > > if (!(dev->driver->driver_features & DRIVER_GEM)) > @@ -1059,11 +1060,15 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > if (read_domains & I915_GEM_GPU_DOMAINS) > return -EINVAL; > > - /* Having something in the write domain implies it's in the read > - * domain, and only that read domain. Enforce that in the request. > - */ > - if (write_domain != 0 && read_domains != write_domain) > + if (write_domain != 0 && read_domains == 0) { Neat extension of the existing ioctl. Just a little wary of the potential for mass foot shootings - but this whole extension is about giving them a machinegun. > + write_only = 1; > + } else if (write_domain != 0 && read_domains != write_domain) { > + /* Having something in the write domain implies it's in the read > + * domain, and only that read domain. Enforce that in the > + * request. > + */ > return -EINVAL; > + } > > ret = i915_mutex_lock_interruptible(dev); > if (ret) > @@ -1084,6 +1089,13 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, > */ > if (ret == -EINVAL) > ret = 0; > + } else if (obj->cpu_write_only && !write_only) { > + DRM_ERROR("Not yet supported\n"); > + ret = -EINVAL; > + goto unlock; > + } else if (write_only) { > + atomic_set(&obj->cpu_write_only, 1); Atomic? Did you intend for obj->cpu_write_only be atomic_t? > + obj->base.read_domains = 0; > } else { > ret = i915_gem_object_set_to_cpu_domain(obj, write_domain != 0); > } > @@ -1217,9 +1229,13 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf) > if (ret) > goto unlock; > > - ret = i915_gem_object_set_to_gtt_domain(obj, write); > - if (ret) > - goto unlock; > + if (obj->cpu_write_only) { > + i915_gem_object_flush_cpu_write_domain(obj); > + } else { > + ret = i915_gem_object_set_to_gtt_domain(obj, write); > + if (ret) > + goto unlock; > + } > } > > if (obj->tiling_mode == I915_TILING_NONE) > @@ -1586,7 +1602,7 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) > obj->dirty = 0; > > for (i = 0; i < page_count; i++) { > - if (obj->dirty) > + if (obj->dirty || obj->cpu_write_only) Could this be simplified by marking the bo as dirty upon setting the object write-only? -Chris -- Chris Wilson, Intel Open Source Technology Centre