drm/i915: Move object to GPU domains after dispatching execbuffer

In the event that we fail to dispatch the execbuffer, for example if
there is insufficient space on the ring, we were leaving the objects in
an inconsistent state. Notably they were marked as being in the GPU
write domain, but were not added to the ring or any list. This would
lead to inevitable oops:

[ 1010.522940] [drm:i915_gem_do_execbuffer] *ERROR* dispatch failed -16
[ 1010.523055] BUG: unable to handle kernel NULL pointer dereference at
0000000000000088
[ 1010.523097] IP: [<ffffffff8122d006>] i915_gem_flush_ring+0x26/0x140
[ 1010.523120] PGD 14cf2f067 PUD 14ce04067 PMD 0
[ 1010.523140] Oops: 0000 [#1] SMP
[ 1010.523154] last sysfs file: /sys/devices/virtual/vc/vcsa2/uevent
[ 1010.523173] CPU 0
[ 1010.523183] Pid: 716, comm: X Not tainted 2.6.36+ #34 LosLunas
CRB/SandyBridge Platform
[ 1010.523206] RIP: 0010:[<ffffffff8122d006>]  [<ffffffff8122d006>]
i915_gem_flush_ring+0x26/0x140
[ 1010.523233] RSP: 0018:ffff88014bf97cd8  EFLAGS: 00010296
[ 1010.523249] RAX: ffff88014e2d1808 RBX: 0000000000000000 RCX: 0000000000000000
[ 1010.523270] RDX: 0000000000000002 RSI: 0000000000000000 RDI: 0000000000000000
[ 1010.523290] RBP: ffff88014e2d1000 R08: 0000000000000002 R09: 00000000400c645f
[ 1010.523311] R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000002
[ 1010.523331] R13: ffff88014e29a000 R14: 00000000000000c8 R15: ffffffff8162eb28
[ 1010.523352] FS:  00007fc62379d700(0000) GS:ffff88001fc00000(0000) knlGS:0000000000000000
[ 1010.523375] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1010.523392] CR2: 0000000000000088 CR3: 000000014bf87000 CR4: 00000000000406f0
[ 1010.523412] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1010.523433] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 1010.523454] Process X (pid: 716, threadinfo ffff88014bf96000, task ffff88014cc1ee40)
[ 1010.523475] Stack:
[ 1010.523483]  ffff88014d5199c0 0000000000000200 0000000000000000 ffff88014bcc6400
[ 1010.523509] <0> 0000000000000000 0000000000000001 ffff88014e29a000 ffff88014bcc6400
[ 1010.523537] <0> ffffffff8162eb28 ffffffff8122faa8 ffff88014e29a000 ffff88014bcc6400
[ 1010.523568] Call Trace:
[ 1010.523578]  [<ffffffff8122faa8>] ?  i915_gem_object_flush_gpu_write_domain+0x48/0x80
[ 1010.523601]  [<ffffffff8122fb8e>] ?  i915_gem_object_set_to_gtt_domain+0x2e/0xb0
[ 1010.523623]  [<ffffffff8123113b>] ?  i915_gem_set_domain_ioctl+0xdb/0x1f0
[ 1010.523644]  [<ffffffff8120a3f1>] ? drm_ioctl+0x3d1/0x460
[ 1010.523660]  [<ffffffff81231060>] ?  i915_gem_set_domain_ioctl+0x0/0x1f0
[ 1010.523682]  [<ffffffff81092618>] ?  vma_prio_tree_insert+0x28/0x120
[ 1010.523701]  [<ffffffff8109f379>] ? vma_link+0x99/0xf0
[ 1010.523717]  [<ffffffff810a111d>] ? mmap_region+0x1ed/0x4f0
[ 1010.523734]  [<ffffffff810c306f>] ? do_vfs_ioctl+0x9f/0x580
[ 1010.523750]  [<ffffffff810c3599>] ? sys_ioctl+0x49/0x80
[ 1010.523767]  [<ffffffff810022eb>] ?  system_call_fastpath+0x16/0x1b
[ 1010.523785] Code: 00 00 00 00 00 41 57 89 ce 41 56 41 55 41 54 45 89 c4 55 48 89 fd 53 48 89 d3 44 89 c2 48 89 df 4c 8d b3 c8 00 00 00 48 83 ec 18 <ff> 93 88 00 00 00 48 8b 83 c8 00 00 00 4c 8b bd 30 03 00 00 48
[ 1010.523946] RIP  [<ffffffff8122d006>] i915_gem_flush_ring+0x26/0x140
[ 1010.523966]  RSP <ffff88014bf97cd8>
[ 1010.523977] CR2: 0000000000000088

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 00e901483ba5d6172fb43edc0a7176576bfcb253..580244c1e7932b93afe14032330e75e72be356c8 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3085,9 +3085,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_gem_object *obj,
        struct drm_i915_gem_object      *obj_priv = to_intel_bo(obj);
        uint32_t                        invalidate_domains = 0;
        uint32_t                        flush_domains = 0;
-       uint32_t                        old_read_domains;
-
-       intel_mark_busy(dev, obj);
 
        /*
         * If the object isn't moving to a new write domain,
@@ -3095,8 +3092,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_gem_object *obj,
         */
        if (obj->pending_write_domain == 0)
                obj->pending_read_domains |= obj->read_domains;
-       else
-               obj_priv->dirty = 1;
 
        /*
         * Flush the current write domain if
@@ -3118,8 +3113,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_gem_object *obj,
        if ((flush_domains | invalidate_domains) & I915_GEM_DOMAIN_CPU)
                i915_gem_clflush_object(obj);
 
-       old_read_domains = obj->read_domains;
-
        /* The actual obj->write_domain will be updated with
         * pending_write_domain after we emit the accumulated flush for all
         * of our domain changes in execbuffers (which clears objects'
@@ -3128,7 +3121,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_gem_object *obj,
         */
        if (flush_domains == 0 && obj->pending_write_domain == 0)
                obj->pending_write_domain = obj->write_domain;
-       obj->read_domains = obj->pending_read_domains;
 
        dev->invalidate_domains |= invalidate_domains;
        dev->flush_domains |= flush_domains;
@@ -3136,10 +3128,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_gem_object *obj,
                dev_priv->mm.flush_rings |= obj_priv->ring->id;
        if (invalidate_domains & I915_GEM_GPU_DOMAINS)
                dev_priv->mm.flush_rings |= ring->id;
-
-       trace_i915_gem_object_change_domain(obj,
-                                           old_read_domains,
-                                           obj->write_domain);
 }
 
 /**
@@ -3602,7 +3590,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
        drm_i915_private_t *dev_priv = dev->dev_private;
        struct drm_gem_object **object_list = NULL;
        struct drm_gem_object *batch_obj;
-       struct drm_i915_gem_object *obj_priv;
        struct drm_clip_rect *cliprects = NULL;
        struct drm_i915_gem_request *request = NULL;
        int ret, i, flips;
@@ -3697,6 +3684,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
        /* Look up object handles */
        for (i = 0; i < args->buffer_count; i++) {
+               struct drm_i915_gem_object *obj_priv;
+
                object_list[i] = drm_gem_object_lookup(dev, file,
                                                       exec_list[i].handle);
                if (object_list[i] == NULL) {
@@ -3761,13 +3750,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
        dev->invalidate_domains = 0;
        dev->flush_domains = 0;
        dev_priv->mm.flush_rings = 0;
-
-       for (i = 0; i < args->buffer_count; i++) {
-               struct drm_gem_object *obj = object_list[i];
-
-               /* Compute new gpu domains and update invalidate/flush */
-               i915_gem_object_set_to_gpu_domain(obj, ring);
-       }
+       for (i = 0; i < args->buffer_count; i++)
+               i915_gem_object_set_to_gpu_domain(object_list[i], ring);
 
        if (dev->invalidate_domains | dev->flush_domains) {
 #if WATCH_EXEC
@@ -3782,15 +3766,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
                               dev_priv->mm.flush_rings);
        }
 
-       for (i = 0; i < args->buffer_count; i++) {
-               struct drm_gem_object *obj = object_list[i];
-               uint32_t old_write_domain = obj->write_domain;
-               obj->write_domain = obj->pending_write_domain;
-               trace_i915_gem_object_change_domain(obj,
-                                                   obj->read_domains,
-                                                   old_write_domain);
-       }
-
 #if WATCH_COHERENCY
        for (i = 0; i < args->buffer_count; i++) {
                i915_gem_object_check_coherency(object_list[i],
@@ -3843,30 +3818,41 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
                goto err;
        }
 
-       /*
-        * Ensure that the commands in the batch buffer are
-        * finished before the interrupt fires
-        */
-       i915_retire_commands(dev, ring);
-
        for (i = 0; i < args->buffer_count; i++) {
                struct drm_gem_object *obj = object_list[i];
 
+               obj->read_domains = obj->pending_read_domains;
+               obj->write_domain = obj->pending_write_domain;
+
                i915_gem_object_move_to_active(obj, ring);
-               if (obj->write_domain)
-                       list_move_tail(&to_intel_bo(obj)->gpu_write_list,
+               if (obj->write_domain) {
+                       struct drm_i915_gem_object *obj_priv = to_intel_bo(obj);
+                       obj_priv->dirty = 1;
+                       list_move_tail(&obj_priv->gpu_write_list,
                                       &ring->gpu_write_list);
+                       intel_mark_busy(dev, obj);
+               }
+
+               trace_i915_gem_object_change_domain(obj,
+                                                   obj->read_domains,
+                                                   obj->write_domain);
        }
 
+       /*
+        * Ensure that the commands in the batch buffer are
+        * finished before the interrupt fires
+        */
+       i915_retire_commands(dev, ring);
+
        i915_add_request(dev, file, request, ring);
        request = NULL;
 
 err:
        for (i = 0; i < args->buffer_count; i++) {
-               if (object_list[i]) {
-                       obj_priv = to_intel_bo(object_list[i]);
-                       obj_priv->in_execbuffer = false;
-               }
+               if (object_list[i] == NULL)
+                   break;
+
+               to_intel_bo(object_list[i])->in_execbuffer = false;
                drm_gem_object_unreference(object_list[i]);
        }