Menu
▾
▴
[PATCH] drm: Copy cliprects from user outside struct_mutex.
|
From: <kr...@bi...> - 2009-02-17 23:36:31
|
From: Kristian Høgsberg <kr...@re...>
This fixes a potential dead lock similar to the one where we copy the
new buffer offsets back to userspace, fixed in
0e2f967303023f7f82a0963b7f7f3980d4c08f20. In this case the lock order
violation occurs when i915_emit_box() copies the cliprects from
userspace from i915_dispatch_gem_execbuffer().
The fix in this case is a little more involved since we can't just
copy the cliprects from userspace up front without either limiting the
number of cliprects or allocating a buffer to hold them. We also can't
lift the struct_mutex lock to copy them as that would allow other GEM
users to pre-empt the batch between setting up the domains and
submitting the commands.
What this patch does is loop through the cliprects and call
i915_gem_execbuffer() per cliprect. This will only affect legacy DRI
rendering and even then it's not expected to be a significant slow
down. Most of the overhead will be in the first invocation of
i915_gem_execbuffer() and subsequent invocations will find the buffer
objects already in the right cache domains.
Signed-off-by: Kristian Høgsberg <kr...@re...>
---
drivers/gpu/drm/i915/i915_dma.c | 68 ++++++++-------
drivers/gpu/drm/i915/i915_drv.h | 7 +-
drivers/gpu/drm/i915/i915_gem.c | 180 ++++++++++++++++++++++-----------------
3 files changed, 144 insertions(+), 111 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 81f1cff..c27b09e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -397,36 +397,30 @@ static int i915_emit_cmds(struct drm_device * dev, int __user * buffer, int dwor
int
i915_emit_box(struct drm_device *dev,
- struct drm_clip_rect __user *boxes,
- int i, int DR1, int DR4)
+ struct drm_clip_rect *box, int DR1, int DR4)
{
drm_i915_private_t *dev_priv = dev->dev_private;
- struct drm_clip_rect box;
RING_LOCALS;
- if (DRM_COPY_FROM_USER_UNCHECKED(&box, &boxes[i], sizeof(box))) {
- return -EFAULT;
- }
-
- if (box.y2 <= box.y1 || box.x2 <= box.x1 || box.y2 <= 0 || box.x2 <= 0) {
+ if (box->y2 <= box->y1 || box->x2 <= box->x1 || box->y2 <= 0 || box->x2 <= 0) {
DRM_ERROR("Bad box %d,%d..%d,%d\n",
- box.x1, box.y1, box.x2, box.y2);
+ box->x1, box->y1, box->x2, box->y2);
return -EINVAL;
}
if (IS_I965G(dev)) {
BEGIN_LP_RING(4);
OUT_RING(GFX_OP_DRAWRECT_INFO_I965);
- OUT_RING((box.x1 & 0xffff) | (box.y1 << 16));
- OUT_RING(((box.x2 - 1) & 0xffff) | ((box.y2 - 1) << 16));
+ OUT_RING((box->x1 & 0xffff) | (box->y1 << 16));
+ OUT_RING(((box->x2 - 1) & 0xffff) | ((box->y2 - 1) << 16));
OUT_RING(DR4);
ADVANCE_LP_RING();
} else {
BEGIN_LP_RING(6);
OUT_RING(GFX_OP_DRAWRECT_INFO);
OUT_RING(DR1);
- OUT_RING((box.x1 & 0xffff) | (box.y1 << 16));
- OUT_RING(((box.x2 - 1) & 0xffff) | ((box.y2 - 1) << 16));
+ OUT_RING((box->x1 & 0xffff) | (box->y1 << 16));
+ OUT_RING(((box->x2 - 1) & 0xffff) | ((box->y2 - 1) << 16));
OUT_RING(DR4);
OUT_RING(0);
ADVANCE_LP_RING();
@@ -462,6 +456,7 @@ static void i915_emit_breadcrumb(struct drm_device *dev)
static int i915_dispatch_cmdbuffer(struct drm_device * dev,
drm_i915_cmdbuffer_t * cmd)
{
+ struct drm_clip_rect box;
int nbox = cmd->num_cliprects;
int i = 0, count, ret;
@@ -476,13 +471,20 @@ static int i915_dispatch_cmdbuffer(struct drm_device * dev,
for (i = 0; i < count; i++) {
if (i < nbox) {
- ret = i915_emit_box(dev, cmd->cliprects, i,
- cmd->DR1, cmd->DR4);
- if (ret)
- return ret;
+ if (DRM_COPY_FROM_USER_UNCHECKED(&box, &cmd->cliprects[i], sizeof(box))) {
+ return -EFAULT;
+ }
}
- ret = i915_emit_cmds(dev, (int __user *)cmd->buf, cmd->sz / 4);
+ mutex_lock(&dev->struct_mutex);
+
+ if (i < nbox)
+ ret = i915_emit_box(dev, &box, cmd->DR1, cmd->DR4);
+ if (!ret)
+ ret = i915_emit_cmds(dev, (int __user *)cmd->buf, cmd->sz / 4);
+
+ mutex_unlock(&dev->struct_mutex);
+
if (ret)
return ret;
}
@@ -496,8 +498,9 @@ static int i915_dispatch_batchbuffer(struct drm_device * dev,
{
drm_i915_private_t *dev_priv = dev->dev_private;
struct drm_clip_rect __user *boxes = batch->cliprects;
+ struct drm_clip_rect box;
int nbox = batch->num_cliprects;
- int i = 0, count;
+ int i = 0, count, ret;
RING_LOCALS;
if ((batch->start | batch->used) & 0x7) {
@@ -511,13 +514,17 @@ static int i915_dispatch_batchbuffer(struct drm_device * dev,
for (i = 0; i < count; i++) {
if (i < nbox) {
- int ret = i915_emit_box(dev, boxes, i,
- batch->DR1, batch->DR4);
- if (ret)
- return ret;
+ if (DRM_COPY_FROM_USER_UNCHECKED(&box, &boxes[i], sizeof(box))) {
+ return -EFAULT;
+ }
}
- if (!IS_I830(dev) && !IS_845G(dev)) {
+ mutex_lock(&dev->struct_mutex);
+
+ if (i < nbox)
+ ret = i915_emit_box(dev, &box, batch->DR1, batch->DR4);
+
+ if (!IS_I830(dev) && !IS_845G(dev) && !ret) {
BEGIN_LP_RING(2);
if (IS_I965G(dev)) {
OUT_RING(MI_BATCH_BUFFER_START | (2 << 6) | MI_BATCH_NON_SECURE_I965);
@@ -527,7 +534,7 @@ static int i915_dispatch_batchbuffer(struct drm_device * dev,
OUT_RING(batch->start | MI_BATCH_NON_SECURE);
}
ADVANCE_LP_RING();
- } else {
+ } else if (!ret) {
BEGIN_LP_RING(4);
OUT_RING(MI_BATCH_BUFFER);
OUT_RING(batch->start | MI_BATCH_NON_SECURE);
@@ -535,6 +542,11 @@ static int i915_dispatch_batchbuffer(struct drm_device * dev,
OUT_RING(0);
ADVANCE_LP_RING();
}
+
+ mutex_unlock(&dev->struct_mutex);
+
+ if (ret)
+ return ret;
}
i915_emit_breadcrumb(dev);
@@ -642,9 +654,7 @@ static int i915_batchbuffer(struct drm_device *dev, void *data,
sizeof(struct drm_clip_rect)))
return -EFAULT;
- mutex_lock(&dev->struct_mutex);
ret = i915_dispatch_batchbuffer(dev, batch);
- mutex_unlock(&dev->struct_mutex);
if (sarea_priv)
sarea_priv->last_dispatch = READ_BREADCRUMB(dev_priv);
@@ -674,9 +684,7 @@ static int i915_cmdbuffer(struct drm_device *dev, void *data,
return -EFAULT;
}
- mutex_lock(&dev->struct_mutex);
ret = i915_dispatch_cmdbuffer(dev, cmdbuf);
- mutex_unlock(&dev->struct_mutex);
if (ret) {
DRM_ERROR("i915_dispatch_cmdbuffer failed\n");
return ret;
@@ -1290,7 +1298,7 @@ struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF(DRM_I915_VBLANK_SWAP, i915_vblank_swap, DRM_AUTH),
DRM_IOCTL_DEF(DRM_I915_HWS_ADDR, i915_set_status_page, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
DRM_IOCTL_DEF(DRM_I915_GEM_INIT, i915_gem_init_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
- DRM_IOCTL_DEF(DRM_I915_GEM_EXECBUFFER, i915_gem_execbuffer, DRM_AUTH),
+ DRM_IOCTL_DEF(DRM_I915_GEM_EXECBUFFER, i915_gem_execbuffer_ioctl, DRM_AUTH),
DRM_IOCTL_DEF(DRM_I915_GEM_PIN, i915_gem_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY),
DRM_IOCTL_DEF(DRM_I915_GEM_UNPIN, i915_gem_unpin_ioctl, DRM_AUTH|DRM_ROOT_ONLY),
DRM_IOCTL_DEF(DRM_I915_GEM_BUSY, i915_gem_busy_ioctl, DRM_AUTH),
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7325363..fe12a94 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -512,8 +512,7 @@ extern int i915_driver_device_is_agp(struct drm_device * dev);
extern long i915_compat_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg);
extern int i915_emit_box(struct drm_device *dev,
- struct drm_clip_rect __user *boxes,
- int i, int DR1, int DR4);
+ struct drm_clip_rect *box, int DR1, int DR4);
/* i915_irq.c */
extern int i915_irq_emit(struct drm_device *dev, void *data,
@@ -576,8 +575,8 @@ int i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
int i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
-int i915_gem_execbuffer(struct drm_device *dev, void *data,
- struct drm_file *file_priv);
+int i915_gem_execbuffer_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv);
int i915_gem_pin_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
int i915_gem_unpin_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0788581..68037cf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2350,15 +2350,13 @@ i915_gem_object_pin_and_relocate(struct drm_gem_object *obj,
*/
static int
i915_dispatch_gem_execbuffer(struct drm_device *dev,
- struct drm_i915_gem_execbuffer *exec,
- uint64_t exec_offset)
+ struct drm_i915_gem_execbuffer *exec,
+ uint64_t exec_offset,
+ struct drm_clip_rect *box)
{
drm_i915_private_t *dev_priv = dev->dev_private;
- struct drm_clip_rect __user *boxes = (struct drm_clip_rect __user *)
- (uintptr_t) exec->cliprects_ptr;
- int nbox = exec->num_cliprects;
- int i = 0, count;
- uint32_t exec_start, exec_len;
+ uint32_t exec_start, exec_len;
+ int ret;
RING_LOCALS;
exec_start = (uint32_t) exec_offset + exec->batch_start_offset;
@@ -2372,37 +2370,32 @@ i915_dispatch_gem_execbuffer(struct drm_device *dev,
if (!exec_start)
return -EINVAL;
- count = nbox ? nbox : 1;
-
- for (i = 0; i < count; i++) {
- if (i < nbox) {
- int ret = i915_emit_box(dev, boxes, i,
- exec->DR1, exec->DR4);
- if (ret)
- return ret;
- }
+ if (box) {
+ ret = i915_emit_box(dev, box, exec->DR1, exec->DR4);
+ if (ret)
+ return ret;
+ }
- if (IS_I830(dev) || IS_845G(dev)) {
- BEGIN_LP_RING(4);
- OUT_RING(MI_BATCH_BUFFER);
- OUT_RING(exec_start | MI_BATCH_NON_SECURE);
- OUT_RING(exec_start + exec_len - 4);
- OUT_RING(0);
- ADVANCE_LP_RING();
+ if (IS_I830(dev) || IS_845G(dev)) {
+ BEGIN_LP_RING(4);
+ OUT_RING(MI_BATCH_BUFFER);
+ OUT_RING(exec_start | MI_BATCH_NON_SECURE);
+ OUT_RING(exec_start + exec_len - 4);
+ OUT_RING(0);
+ ADVANCE_LP_RING();
+ } else {
+ BEGIN_LP_RING(2);
+ if (IS_I965G(dev)) {
+ OUT_RING(MI_BATCH_BUFFER_START |
+ (2 << 6) |
+ MI_BATCH_NON_SECURE_I965);
+ OUT_RING(exec_start);
} else {
- BEGIN_LP_RING(2);
- if (IS_I965G(dev)) {
- OUT_RING(MI_BATCH_BUFFER_START |
- (2 << 6) |
- MI_BATCH_NON_SECURE_I965);
- OUT_RING(exec_start);
- } else {
- OUT_RING(MI_BATCH_BUFFER_START |
- (2 << 6));
- OUT_RING(exec_start | MI_BATCH_NON_SECURE);
- }
- ADVANCE_LP_RING();
+ OUT_RING(MI_BATCH_BUFFER_START |
+ (2 << 6));
+ OUT_RING(exec_start | MI_BATCH_NON_SECURE);
}
+ ADVANCE_LP_RING();
}
/* XXX breadcrumb */
@@ -2432,52 +2425,22 @@ i915_gem_ring_throttle(struct drm_device *dev, struct drm_file *file_priv)
return ret;
}
-int
-i915_gem_execbuffer(struct drm_device *dev, void *data,
+static int
+i915_gem_execbuffer(struct drm_device *dev,
+ struct drm_i915_gem_execbuffer *args,
+ struct drm_i915_gem_exec_object *exec_list,
+ struct drm_gem_object **object_list,
+ struct drm_clip_rect *box,
struct drm_file *file_priv)
{
drm_i915_private_t *dev_priv = dev->dev_private;
struct drm_i915_file_private *i915_file_priv = file_priv->driver_priv;
- struct drm_i915_gem_execbuffer *args = data;
- struct drm_i915_gem_exec_object *exec_list = NULL;
- struct drm_gem_object **object_list = NULL;
struct drm_gem_object *batch_obj;
int ret, i, pinned = 0;
uint64_t exec_offset;
uint32_t seqno, flush_domains;
int pin_tries;
-#if WATCH_EXEC
- DRM_INFO("buffers_ptr %d buffer_count %d len %08x\n",
- (int) args->buffers_ptr, args->buffer_count, args->batch_len);
-#endif
-
- if (args->buffer_count < 1) {
- DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
- return -EINVAL;
- }
- /* Copy in the exec list from userland */
- exec_list = drm_calloc(sizeof(*exec_list), args->buffer_count,
- DRM_MEM_DRIVER);
- object_list = drm_calloc(sizeof(*object_list), args->buffer_count,
- DRM_MEM_DRIVER);
- if (exec_list == NULL || object_list == NULL) {
- DRM_ERROR("Failed to allocate exec or object list "
- "for %d buffers\n",
- args->buffer_count);
- ret = -ENOMEM;
- goto pre_mutex_err;
- }
- ret = copy_from_user(exec_list,
- (struct drm_i915_relocation_entry __user *)
- (uintptr_t) args->buffers_ptr,
- sizeof(*exec_list) * args->buffer_count);
- if (ret != 0) {
- DRM_ERROR("copy %d exec entries failed %d\n",
- args->buffer_count, ret);
- goto pre_mutex_err;
- }
-
mutex_lock(&dev->struct_mutex);
i915_verify_inactive(dev, __FILE__, __LINE__);
@@ -2485,15 +2448,13 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
if (dev_priv->mm.wedged) {
DRM_ERROR("Execbuf while wedged\n");
mutex_unlock(&dev->struct_mutex);
- ret = -EIO;
- goto pre_mutex_err;
+ return -EIO;
}
if (dev_priv->mm.suspended) {
DRM_ERROR("Execbuf while VT-switched.\n");
mutex_unlock(&dev->struct_mutex);
- ret = -EBUSY;
- goto pre_mutex_err;
+ return -EBUSY;
}
/* Look up object handles */
@@ -2601,7 +2562,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
#endif
/* Exec the batchbuffer */
- ret = i915_dispatch_gem_execbuffer(dev, args, exec_offset);
+ ret = i915_dispatch_gem_execbuffer(dev, args, exec_offset, box);
if (ret) {
DRM_ERROR("dispatch failed %d\n", ret);
goto err;
@@ -2648,6 +2609,71 @@ err:
mutex_unlock(&dev->struct_mutex);
+ return ret;
+}
+
+int
+i915_gem_execbuffer_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv)
+{
+ struct drm_i915_gem_execbuffer *args = data;
+ struct drm_i915_gem_exec_object *exec_list;
+ struct drm_gem_object **object_list;
+ struct drm_clip_rect box;
+ struct drm_clip_rect __user *boxes =
+ (struct drm_clip_rect __user *) (uintptr_t) args->cliprects_ptr;
+ int i, ret;
+
+#if WATCH_EXEC
+ DRM_INFO("buffers_ptr %d buffer_count %d len %08x\n",
+ (int) args->buffers_ptr, args->buffer_count, args->batch_len);
+#endif
+
+ if (args->buffer_count < 1) {
+ DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
+ return -EINVAL;
+ }
+ /* Copy in the exec list from userland */
+ exec_list = drm_calloc(sizeof(*exec_list), args->buffer_count,
+ DRM_MEM_DRIVER);
+ object_list = drm_calloc(sizeof(*object_list), args->buffer_count,
+ DRM_MEM_DRIVER);
+ if (exec_list == NULL || object_list == NULL) {
+ DRM_ERROR("Failed to allocate exec or object list "
+ "for %d buffers\n",
+ args->buffer_count);
+ ret = -ENOMEM;
+ goto fail;
+ }
+ ret = copy_from_user(exec_list,
+ (struct drm_i915_relocation_entry __user *)
+ (uintptr_t) args->buffers_ptr,
+ sizeof(*exec_list) * args->buffer_count);
+ if (ret != 0) {
+ DRM_ERROR("copy %d exec entries failed %d\n",
+ args->buffer_count, ret);
+ goto fail;
+ }
+
+ if (args->num_cliprects == 0) {
+ ret = i915_gem_execbuffer(dev, args,
+ exec_list, object_list,
+ NULL, file_priv);
+ } else {
+ for (i = 0; i < args->num_cliprects; i++) {
+ if (copy_from_user(&box, &boxes[i], sizeof(box))) {
+ ret = -EFAULT;
+ break;
+ }
+
+ ret = i915_gem_execbuffer(dev, args,
+ exec_list, object_list,
+ &box, file_priv);
+ if (ret)
+ break;
+ }
+ }
+
if (!ret) {
/* Copy the new buffer offsets back to the user's exec list. */
ret = copy_to_user((struct drm_i915_relocation_entry __user *)
@@ -2660,7 +2686,7 @@ err:
args->buffer_count, ret);
}
-pre_mutex_err:
+ fail:
drm_free(object_list, sizeof(*object_list) * args->buffer_count,
DRM_MEM_DRIVER);
drm_free(exec_list, sizeof(*exec_list) * args->buffer_count,
--
1.6.1.3
|