Re: [PATCH 09/29] drm/i915: thread address space through execbuf

Daniel Vetter
2013-08-05T09:39:18Z

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 <ben@bwidawsk.net>
> ---

[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