From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 3/5] drm/i915: fall through pwrite_gtt_slow to the shmem slow path Date: Sat, 17 Sep 2011 21:57:05 +0100 Message-ID: References: <1316285749-30130-1-git-send-email-daniel.vetter@ffwll.ch> <1316285749-30130-4-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id EC2A29E77C for ; Sat, 17 Sep 2011 13:57:09 -0700 (PDT) In-Reply-To: <1316285749-30130-4-git-send-email-daniel.vetter@ffwll.ch> 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 Cc: Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org On Sat, 17 Sep 2011 20:55:47 +0200, Daniel Vetter wrote: > The gtt_pwrite slowpath grabs the userspace memory with > get_user_pages. This will not work for non-page backed memory, like a > gtt mmapped gem object. Hence fall throuh to the shmem paths if we hit > -EFAULT in the gtt paths. > > Now the shmem paths have exactly the same problem, but this way we > only need to rearrange the code in one write path. > > Signed-Off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem.c | 24 +++++++++++++++--------- > 1 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index e0475ca..9c28d48 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1018,18 +1018,24 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, > > out_unpin: > i915_gem_object_unpin(obj); > - } else { > - ret = i915_gem_object_set_to_cpu_domain(obj, 1); > - if (ret) > - goto out; > > - ret = -EFAULT; > - if (!i915_gem_object_needs_bit17_swizzle(obj)) > - ret = i915_gem_shmem_pwrite_fast(dev, obj, args, file); > - if (ret == -EFAULT) > - ret = i915_gem_shmem_pwrite_slow(dev, obj, args, file); > + if (ret != -EFAULT) > + goto out; > + /* Fall through to the shmfs paths because the gtt paths might > + * fail with non-page-backed user pointers (e.g. gtt mappings > + * when moving data between textures). */ > } I'd actually prefer to have an if (ret == -EFAULT) { and comments here in order to mark the shmem paths as a logical unit to the reader. Better yet would be to move the two paths (GTT/CPU) into their own routines: ret = -EFAULT; if (can_gtt) ret = i915_gem_gtt_pwrite(); /* Fallback to the slow shmem paths if we need to handle * GTT mapped user pointers, or otherwise can not do the upload in * place. */ if (ret == -EFAULT) ret = i915_gem_cpu_pwrite(); And whilst you are here, can you incorporate this patch? else if (obj->gtt_space && + obj->map_and_fenceable && obj->base.write_domain != I915_GEM_DOMAIN_CPU) { ret = i915_gem_object_pin(obj, 0, true); -Chris -- Chris Wilson, Intel Open Source Technology Centre