From 56bb29cf37c27b283efcd1a32d3583393e5208ea Mon Sep 17 00:00:00 2001 From: Thomas Hellstrom Date: Tue, 26 Feb 2008 00:01:09 +0100 Subject: Make the execbuffer code reasonably safe against errors. In particular -EAGAINs, which should be common during Xserver operation. Also handle the fence creation failure case. --- shared-core/i915_dma.c | 244 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 180 insertions(+), 64 deletions(-) diff --git a/shared-core/i915_dma.c b/shared-core/i915_dma.c index 7da8d55c..4f41a688 100644 --- a/shared-core/i915_dma.c +++ b/shared-core/i915_dma.c @@ -809,7 +809,10 @@ struct i915_relocatee_info { struct drm_i915_validate_buffer { struct drm_buffer_object *buffer; + struct drm_bo_info_rep rep; int presumed_offset_correct; + void __user *data; + int ret; }; static void i915_dereference_buffers_locked(struct drm_i915_validate_buffer *buffers, @@ -1012,6 +1015,40 @@ out_err: return ret; } +static int i915_check_presumed(struct drm_i915_op_arg *arg, + struct drm_buffer_object *bo, + uint32_t __user *data, + int *presumed_ok) +{ + struct drm_bo_op_req *req = &arg->d.req; + uint32_t hint_offset; + uint32_t hint = req->bo_req.hint; + + *presumed_ok = 0; + + if (!(hint & DRM_BO_HINT_PRESUMED_OFFSET)) + return 0; + if (bo->offset == req->bo_req.presumed_offset) { + *presumed_ok = 1; + return 0; + } + + /* + * We need to turn off the HINT_PRESUMED_OFFSET for this buffer in + * the user-space IOCTL argument list, since the buffer has moved, + * we're about to apply relocations and we might subsequently + * hit an -EAGAIN. In that case the argument list will be reused by + * user-space, but the presumed offset is no longer valid. + * + * Needless to say, this is a bit ugly. + */ + + hint_offset = (uint32_t *)&req->bo_req.hint - (uint32_t *)arg; + hint &= ~DRM_BO_HINT_PRESUMED_OFFSET; + return __put_user(hint, data + hint_offset); +} + + /* * Validate, add fence and relocate a block of bos from a userspace list */ @@ -1022,13 +1059,11 @@ int i915_validate_buffer_list(struct drm_file *file_priv, { struct drm_i915_op_arg arg; struct drm_bo_op_req *req = &arg.d.req; - struct drm_bo_arg_rep rep; - unsigned long next = 0; int ret = 0; unsigned buf_count = 0; - struct drm_device *dev = file_priv->head->dev; uint32_t buf_handle; uint32_t __user *reloc_user_ptr; + struct drm_i915_validate_buffer *item = buffers; do { if (buf_count >= *num_buffers) { @@ -1036,31 +1071,26 @@ int i915_validate_buffer_list(struct drm_file *file_priv, ret = -EINVAL; goto out_err; } + item = buffers + buf_count; + item->buffer = NULL; + item->presumed_offset_correct = 0; buffers[buf_count].buffer = NULL; - buffers[buf_count].presumed_offset_correct = 0; if (copy_from_user(&arg, (void __user *)(unsigned long)data, sizeof(arg))) { ret = -EFAULT; goto out_err; } - if (arg.handled) { - data = arg.next; - mutex_lock(&dev->struct_mutex); - buffers[buf_count].buffer = drm_lookup_buffer_object(file_priv, req->arg_handle, 1); - mutex_unlock(&dev->struct_mutex); - buf_count++; - continue; - } - - rep.ret = 0; + ret = 0; if (req->op != drm_bo_validate) { DRM_ERROR ("Buffer object operation wasn't \"validate\".\n"); - rep.ret = -EINVAL; + ret = -EINVAL; goto out_err; } + item->ret = 0; + item->data = (void __user *) (unsigned long) data; buf_handle = req->bo_req.handle; reloc_user_ptr = (uint32_t *)(unsigned long)arg.reloc_ptr; @@ -1072,48 +1102,149 @@ int i915_validate_buffer_list(struct drm_file *file_priv, DRM_MEMORYBARRIER(); } - rep.ret = drm_bo_handle_validate(file_priv, req->bo_req.handle, - req->bo_req.flags, req->bo_req.mask, - req->bo_req.hint, - req->bo_req.fence_class, 0, - &rep.bo_info, - &buffers[buf_count].buffer); + ret = drm_bo_handle_validate(file_priv, req->bo_req.handle, + req->bo_req.flags, req->bo_req.mask, + req->bo_req.hint, + req->bo_req.fence_class, 0, + &item->rep, + &item->buffer); + + if (ret) { + DRM_ERROR("error on handle validate %d\n", ret); + goto out_err; + } + + buf_count++; - if (rep.ret) { - DRM_ERROR("error on handle validate %d\n", rep.ret); + ret = i915_check_presumed(&arg, item->buffer, + (uint32_t __user *) + (unsigned long) data, + &item->presumed_offset_correct); + if (ret) goto out_err; + + data = arg.next; + } while (data != 0); +out_err: + *num_buffers = buf_count; + item->ret = (ret != -EAGAIN) ? ret : 0; + return ret; +} + + +/* + * Remove all buffers from the unfenced list. + * If the execbuffer operation was aborted, for example due to a signal, + * this also make sure that buffers retain their original state and + * fence pointers. + * Copy back buffer information to user-space unless we were interrupted + * by a signal. In which case the IOCTL must be rerun. + */ + +static int i915_handle_copyback(struct drm_device *dev, + struct drm_i915_validate_buffer *buffers, + unsigned int num_buffers, int ret) +{ + int err = ret; + int i; + struct drm_i915_op_arg arg; + + if (ret) + drm_putback_buffer_objects(dev); + + if (ret != -EAGAIN) { + for (i = 0; i < num_buffers; ++i) { + arg.handled = 1; + arg.d.rep.ret = buffers->ret; + arg.d.rep.bo_info = buffers->rep; + if (__copy_to_user(buffers->data, &arg, sizeof(arg))) + err = -EFAULT; + buffers++; } + } + + return err; +} + +/* + * Create a fence object, and if that fails, pretend that everything is + * OK and just idle the GPU. + */ + +void i915_fence_or_sync(struct drm_file *file_priv, + uint32_t fence_flags, + struct drm_fence_arg *fence_arg, + struct drm_fence_object **fence_p) +{ + struct drm_device *dev = file_priv->head->dev; + int ret; + struct drm_fence_object *fence; + + ret = drm_fence_buffer_objects(dev, NULL, fence_flags, + NULL, &fence); + + if (ret) { + /* - * If the user provided a presumed offset hint, check whether - * the buffer is in the same place, if so, relocations relative to - * this buffer need not be performed + * Fence creation failed. + * Fall back to synchronous operation and idle the engine. */ - if ((req->bo_req.hint & DRM_BO_HINT_PRESUMED_OFFSET) && - buffers[buf_count].buffer->offset == req->bo_req.presumed_offset) { - buffers[buf_count].presumed_offset_correct = 1; - } - next = arg.next; - arg.handled = 1; - arg.d.rep = rep; + (void) i915_quiescent(dev); - if (copy_to_user((void __user *)(unsigned long)data, &arg, sizeof(arg))) - return -EFAULT; + /* + * FIXME: Might need a sync flush here. + */ - data = next; - buf_count++; + if (!(fence_flags & DRM_FENCE_FLAG_NO_USER)) { - } while (next != 0); - *num_buffers = buf_count; - return 0; -out_err: - mutex_lock(&dev->struct_mutex); - i915_dereference_buffers_locked(buffers, buf_count); - mutex_unlock(&dev->struct_mutex); - *num_buffers = 0; - return (ret) ? ret : rep.ret; + /* + * Communicate to user-space that + * fence creation has failed and that + * the engine is idle. + */ + + fence_arg->handle = ~0; + fence_arg->error = ret; + } + + drm_putback_buffer_objects(dev); + if (fence_p) + *fence_p = NULL; + return; + } + + if (!(fence_flags & DRM_FENCE_FLAG_NO_USER)) { + + ret = drm_fence_add_user_object(file_priv, fence, + fence_flags & + DRM_FENCE_FLAG_SHAREABLE); + if (!ret) + drm_fence_fill_arg(fence, fence_arg); + else { + /* + * Fence user object creation failed. + * We must idle the engine here as well, as user- + * space expects a fence object to wait on. Since we + * have a fence object we wait for it to signal + * to indicate engine "sufficiently" idle. + */ + + (void) drm_fence_object_wait(fence, 0, 1, + fence->type); + drm_fence_usage_deref_unlocked(&fence); + fence_arg->handle = ~0; + fence_arg->error = ret; + } + } + + if (fence_p) + *fence_p = fence; + else if (fence) + drm_fence_usage_deref_unlocked(&fence); } + static int i915_execbuffer(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -1126,7 +1257,6 @@ static int i915_execbuffer(struct drm_device *dev, void *data, int num_buffers; int ret; struct drm_i915_validate_buffer *buffers; - struct drm_fence_object *fence; if (!dev_priv->allow_batchbuffer) { DRM_ERROR("Batchbuffer ioctl disabled\n"); @@ -1171,7 +1301,7 @@ static int i915_execbuffer(struct drm_device *dev, void *data, ret = i915_validate_buffer_list(file_priv, 0, exec_buf->ops_list, buffers, &num_buffers); if (ret) - goto out_free; + goto out_err0; /* make sure all previous memory operations have passed */ DRM_MEMORYBARRIER(); @@ -1190,30 +1320,16 @@ static int i915_execbuffer(struct drm_device *dev, void *data, if (sarea_priv) sarea_priv->last_dispatch = READ_BREADCRUMB(dev_priv); - /* fence */ - ret = drm_fence_buffer_objects(dev, NULL, fence_arg->flags, - NULL, &fence); - if (ret) - goto out_err0; + i915_fence_or_sync(file_priv, fence_arg->flags, fence_arg, NULL); - if (!(fence_arg->flags & DRM_FENCE_FLAG_NO_USER)) { - ret = drm_fence_add_user_object(file_priv, fence, fence_arg->flags & DRM_FENCE_FLAG_SHAREABLE); - if (!ret) { - fence_arg->handle = fence->base.hash.key; - fence_arg->fence_class = fence->fence_class; - fence_arg->type = fence->type; - fence_arg->signaled = fence->signaled_types; - } - } - drm_fence_usage_deref_unlocked(&fence); out_err0: /* handle errors */ + ret = i915_handle_copyback(dev, buffers, num_buffers, ret); mutex_lock(&dev->struct_mutex); i915_dereference_buffers_locked(buffers, num_buffers); mutex_unlock(&dev->struct_mutex); -out_free: drm_free(buffers, (exec_buf->num_buffers * sizeof(struct drm_buffer_object *)), DRM_MEM_DRIVER); mutex_unlock(&dev_priv->cmdbuf_mutex); -- cgit v1.2.3