From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 09/29] drm/i915: thread address space through execbuf Date: Mon, 5 Aug 2013 11:39:24 +0200 Message-ID: <20130805093924.GF22035@phenom.ffwll.local> References: <1375315222-4785-1-git-send-email-ben@bwidawsk.net> <1375315222-4785-10-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 mail-we0-f176.google.com (mail-we0-f176.google.com [74.125.82.176]) by gabe.freedesktop.org (Postfix) with ESMTP id 56410E6329 for ; Mon, 5 Aug 2013 02:39:18 -0700 (PDT) Received: by mail-we0-f176.google.com with SMTP id q56so2209752wes.7 for ; Mon, 05 Aug 2013 02:39:17 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1375315222-4785-10-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: Ben Widawsky Cc: Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Wed, Jul 31, 2013 at 05:00:02PM -0700, Ben Widawsky wrote: > This represents the first half of hooking up VMs to execbuf. Here we > basically pass an address space all around to the different internal > functions. It should be much more readable, and have less risk than the > second half, which begins switching over to using VMAs instead of an > obj,vm. > > The overall series echoes this style of, "add a VM, then make it smart > later" > > Signed-off-by: Ben Widawsky > --- [snip] > @@ -477,6 +483,7 @@ i915_gem_execbuffer_unreserve_object(struct drm_i915_gem_object *obj) > static int > i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring, > struct list_head *objects, > + struct i915_address_space *vm, > bool *need_relocs) > { > struct drm_i915_gem_object *obj; > @@ -531,32 +538,37 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring, > list_for_each_entry(obj, objects, exec_list) { > struct drm_i915_gem_exec_object2 *entry = obj->exec_entry; > bool need_fence, need_mappable; > + u32 obj_offset; > > - if (!i915_gem_obj_ggtt_bound(obj)) > + if (!i915_gem_obj_bound(obj, vm)) > continue; > > + obj_offset = i915_gem_obj_offset(obj, vm); > need_fence = > has_fenced_gpu_access && > entry->flags & EXEC_OBJECT_NEEDS_FENCE && > obj->tiling_mode != I915_TILING_NONE; > need_mappable = need_fence || need_reloc_mappable(obj); > > + BUG_ON((need_mappable || need_fence) && > + !i915_is_ggtt(vm)); No BUG_ON if the error isn't fatal, please. I've fixed this up to a WARN_ON while applying. I know that you prefer BUG_ON for developing, but when we hit one of these in the wild it's a royal pain to debug. But please either switch to WARNs when submitting the patches or (imo preferred) get into the habit of grepping dmesg for backtraces when testing. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch