This fixes up
commit e8e89622ed361c46bf90ba4828e685a8b603f7e5 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Tue Dec 18 22:25:11 2012 +0100
drm/ttm: fix fence locking in ttm_buffer_object_transfer
which leaves behind a might_sleep in atomic context, since the fence_lock spinlock is held over a kmalloc(GFP_KERNEL) call. The fix is to revert the above commit and only take the lock where we need it, around the call to ->sync_obj_ref.
Reported-by: Dave Airlie airlied@gmail.com Cc: Jerome Glisse jglisse@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@canonical.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ttm/ttm_bo_util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d73d6e3..d4bb792 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -441,7 +441,9 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, fbo->vm_node = NULL; atomic_set(&fbo->cpu_writers, 0);
+ spin_unlock(&bdev->fence_lock); fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj); + spin_lock(&bdev->fence_lock); kref_init(&fbo->list_kref); kref_init(&fbo->kref); fbo->destroy = &ttm_transfered_destroy; @@ -654,13 +656,11 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo, */
set_bit(TTM_BO_PRIV_FLAG_MOVING, &bo->priv_flags); - - /* ttm_buffer_object_transfer accesses bo->sync_obj */ - ret = ttm_buffer_object_transfer(bo, &ghost_obj); spin_unlock(&bdev->fence_lock); if (tmp_obj) driver->sync_obj_unref(&tmp_obj);
+ ret = ttm_buffer_object_transfer(bo, &ghost_obj); if (ret) return ret;
Op 14-01-13 11:03, Daniel Vetter schreef:
This fixes up
commit e8e89622ed361c46bf90ba4828e685a8b603f7e5 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Tue Dec 18 22:25:11 2012 +0100
drm/ttm: fix fence locking in ttm_buffer_object_transfer
which leaves behind a might_sleep in atomic context, since the fence_lock spinlock is held over a kmalloc(GFP_KERNEL) call. The fix is to revert the above commit and only take the lock where we need it, around the call to ->sync_obj_ref.
Reported-by: Dave Airlie airlied@gmail.com Cc: Jerome Glisse jglisse@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@canonical.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/ttm/ttm_bo_util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d73d6e3..d4bb792 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -441,7 +441,9 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, fbo->vm_node = NULL; atomic_set(&fbo->cpu_writers, 0);
- spin_unlock(&bdev->fence_lock); fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj);
- spin_lock(&bdev->fence_lock); kref_init(&fbo->list_kref); kref_init(&fbo->kref); fbo->destroy = &ttm_transfered_destroy;
Methinks you messed up spin_lock and spin_unlock here.
@@ -654,13 +656,11 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo, */
set_bit(TTM_BO_PRIV_FLAG_MOVING, &bo->priv_flags);
/* ttm_buffer_object_transfer accesses bo->sync_obj */
spin_unlock(&bdev->fence_lock); if (tmp_obj) driver->sync_obj_unref(&tmp_obj);ret = ttm_buffer_object_transfer(bo, &ghost_obj);
if (ret) return ret;ret = ttm_buffer_object_transfer(bo, &ghost_obj);
~Maarten
This fixes up
commit e8e89622ed361c46bf90ba4828e685a8b603f7e5 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Tue Dec 18 22:25:11 2012 +0100
drm/ttm: fix fence locking in ttm_buffer_object_transfer
which leaves behind a might_sleep in atomic context, since the fence_lock spinlock is held over a kmalloc(GFP_KERNEL) call. The fix is to revert the above commit and only take the lock where we need it, around the call to ->sync_obj_ref.
v2: Fixup things noticed by Maarten Lankhorst: - Brown paper bag locking bug. - No need for kzalloc if we clear the entire thing on the next line. - check for bo->sync_obj (totally unlikely race, but still someone else could have snuck in) and clear fbo->sync_obj if it's cleared already.
Reported-by: Dave Airlie airlied@gmail.com Cc: Jerome Glisse jglisse@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@canonical.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/ttm/ttm_bo_util.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d73d6e3..544e407 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -422,7 +422,7 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, struct ttm_bo_device *bdev = bo->bdev; struct ttm_bo_driver *driver = bdev->driver;
- fbo = kzalloc(sizeof(*fbo), GFP_KERNEL); + fbo = kmalloc(sizeof(*fbo), GFP_KERNEL); if (!fbo) return -ENOMEM;
@@ -441,7 +441,12 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, fbo->vm_node = NULL; atomic_set(&fbo->cpu_writers, 0);
- fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj); + spin_lock(&bdev->fence_lock); + if (bo->sync_obj) + fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj); + else + fbo->sync_obj = NULL; + spin_unlock(&bdev->fence_lock); kref_init(&fbo->list_kref); kref_init(&fbo->kref); fbo->destroy = &ttm_transfered_destroy; @@ -654,13 +659,11 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo, */
set_bit(TTM_BO_PRIV_FLAG_MOVING, &bo->priv_flags); - - /* ttm_buffer_object_transfer accesses bo->sync_obj */ - ret = ttm_buffer_object_transfer(bo, &ghost_obj); spin_unlock(&bdev->fence_lock); if (tmp_obj) driver->sync_obj_unref(&tmp_obj);
+ ret = ttm_buffer_object_transfer(bo, &ghost_obj); if (ret) return ret;
Op 14-01-13 15:08, Daniel Vetter schreef:
This fixes up
commit e8e89622ed361c46bf90ba4828e685a8b603f7e5 Author: Daniel Vetter daniel.vetter@ffwll.ch Date: Tue Dec 18 22:25:11 2012 +0100
drm/ttm: fix fence locking in ttm_buffer_object_transfer
which leaves behind a might_sleep in atomic context, since the fence_lock spinlock is held over a kmalloc(GFP_KERNEL) call. The fix is to revert the above commit and only take the lock where we need it, around the call to ->sync_obj_ref.
v2: Fixup things noticed by Maarten Lankhorst:
- Brown paper bag locking bug.
- No need for kzalloc if we clear the entire thing on the next line.
- check for bo->sync_obj (totally unlikely race, but still someone else could have snuck in) and clear fbo->sync_obj if it's cleared already.
Reported-by: Dave Airlie airlied@gmail.com Cc: Jerome Glisse jglisse@redhat.com Cc: Maarten Lankhorst maarten.lankhorst@canonical.com
Reviewed-by: Maarten Lankhorst maarten.lankhorst@canonical.com
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/ttm/ttm_bo_util.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d73d6e3..544e407 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -422,7 +422,7 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, struct ttm_bo_device *bdev = bo->bdev; struct ttm_bo_driver *driver = bdev->driver;
- fbo = kzalloc(sizeof(*fbo), GFP_KERNEL);
- fbo = kmalloc(sizeof(*fbo), GFP_KERNEL); if (!fbo) return -ENOMEM;
@@ -441,7 +441,12 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, fbo->vm_node = NULL; atomic_set(&fbo->cpu_writers, 0);
- fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj);
- spin_lock(&bdev->fence_lock);
- if (bo->sync_obj)
fbo->sync_obj = driver->sync_obj_ref(bo->sync_obj);
- else
fbo->sync_obj = NULL;
- spin_unlock(&bdev->fence_lock); kref_init(&fbo->list_kref); kref_init(&fbo->kref); fbo->destroy = &ttm_transfered_destroy;
@@ -654,13 +659,11 @@ int ttm_bo_move_accel_cleanup(struct ttm_buffer_object *bo, */
set_bit(TTM_BO_PRIV_FLAG_MOVING, &bo->priv_flags);
/* ttm_buffer_object_transfer accesses bo->sync_obj */
spin_unlock(&bdev->fence_lock); if (tmp_obj) driver->sync_obj_unref(&tmp_obj);ret = ttm_buffer_object_transfer(bo, &ghost_obj);
if (ret) return ret;ret = ttm_buffer_object_transfer(bo, &ghost_obj);
dri-devel@lists.freedesktop.org