From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: VM lockdep warning Date: Sat, 21 Apr 2012 18:32:50 +0200 Message-ID: <4F92E132.6050600@vodafone.de> References: <4F92A9AA.4030607@vodafone.de> <4F92C137.6050403@vodafone.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------030900030703060205040108" Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id CE0D89E864 for ; Sat, 21 Apr 2012 09:32:55 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Dave Airlie Cc: dri-devel List-Id: dri-devel@lists.freedesktop.org This is a multi-part message in MIME format. --------------030900030703060205040108 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable On 21.04.2012 17:57, Dave Airlie wrote: > 2012/4/21 Jerome Glisse: >> 2012/4/21 Christian K=F6nig: >>> On 21.04.2012 16:08, Jerome Glisse wrote: >>>> 2012/4/21 Christian K=F6nig: >>>>> Interesting, I'm pretty sure that I haven't touched the locking ord= er of >>>>> the >>>>> cs_mutex vs. vm_mutex. >>>>> >>>>> Maybe it is just some kind of side effect, going to locking into it >>>>> anyway. >>>>> >>>>> Christian. >>>>> >>>> It's the using, init path take lock in different order than cs path >>> Well, could you explain to me why the vm code takes cs mutex in the f= irst >>> place? >>> >>> It clearly has it's own mutex and it doesn't looks like that it deals= with >>> any cs related data anyway. >>> >>> Christian. >> Lock simplification is on my todo. The issue is that vm manager is pro= tected by >> cs_mutex The vm.mutex is specific to each vm it doesn't protect the gl= obal vm >> management. I didn't wanted to introduce a new global vm mutex as vm a= ctivity >> is mostly trigger on behalf of cs so i dediced to use the cs mutex. >> >> That's why non cs path of vm need to take the cs mutex. > So if one app is adding a bo, and another doing CS, isn't deadlock a > real possibility? Yeah, I think so. > I expect the VM code need to take CS mutex earlier then. I would strongly suggest to give the vm code their own global mutex and=20 remove the per vm mutex, cause the later is pretty superfluous if the=20 cs_mutex is also taken most of the time. The attached patch is against drm-fixes and does exactly that. Christian. --------------030900030703060205040108 Content-Type: text/x-patch; name="0001-drm-radeon-use-a-global-mutex-instead-of-per-vm-one.patch" Content-Disposition: attachment; filename*0="0001-drm-radeon-use-a-global-mutex-instead-of-per-vm-one.pat"; filename*1="ch" Content-Transfer-Encoding: quoted-printable >>From b6a79c2e54f8200e770c25e930b0784343105a2b Mon Sep 17 00:00:00 2001 From: =3D?UTF-8?q?Christian=3D20K=3DC3=3DB6nig?=3D Date: Sat, 21 Apr 2012 18:29:34 +0200 Subject: [PATCH] drm/radeon: use a global mutex instead of per vm one MIME-Version: 1.0 Content-Type: text/plain; charset=3DUTF-8 Content-Transfer-Encoding: 8bit Resolving deadlock problems with the cs_mutex. Signed-off-by: Christian K=C3=B6nig --- drivers/gpu/drm/radeon/radeon.h | 2 +- drivers/gpu/drm/radeon/radeon_device.c | 1 + drivers/gpu/drm/radeon/radeon_gart.c | 25 +++++++++---------------- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/rad= eon.h index 138b952..f35957d 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -680,7 +680,6 @@ struct radeon_vm { u64 pt_gpu_addr; u64 *pt; struct radeon_sa_bo sa_bo; - struct mutex mutex; /* last fence for cs using this vm */ struct radeon_fence *fence; }; @@ -1527,6 +1526,7 @@ struct radeon_device { struct radeon_pm pm; uint32_t bios_scratch[RADEON_BIOS_NUM_SCRATCH]; struct radeon_mutex cs_mutex; + struct mutex vm_mutex; struct radeon_wb wb; struct radeon_dummy_page dummy_page; bool gpu_lockup; diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/rad= eon/radeon_device.c index ea7df16..cecb785 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -725,6 +725,7 @@ int radeon_device_init(struct radeon_device *rdev, * can recall function without having locking issues */ radeon_mutex_init(&rdev->cs_mutex); radeon_mutex_init(&rdev->ib_pool.mutex); + mutex_init(&rdev->vm_mutex); for (i =3D 0; i < RADEON_NUM_RINGS; ++i) mutex_init(&rdev->ring[i].mutex); mutex_init(&rdev->dc_hw_i2c_mutex); diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeo= n/radeon_gart.c index c58a036..1b4933b 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -356,13 +356,13 @@ int radeon_vm_manager_suspend(struct radeon_device = *rdev) { struct radeon_vm *vm, *tmp; =20 - radeon_mutex_lock(&rdev->cs_mutex); + mutex_lock(&rdev->vm_mutex); /* unbind all active vm */ list_for_each_entry_safe(vm, tmp, &rdev->vm_manager.lru_vm, list) { radeon_vm_unbind_locked(rdev, vm); } rdev->vm_manager.funcs->fini(rdev); - radeon_mutex_unlock(&rdev->cs_mutex); + mutex_unlock(&rdev->vm_mutex); return radeon_sa_bo_manager_suspend(rdev, &rdev->vm_manager.sa_manager)= ; } =20 @@ -476,13 +476,11 @@ int radeon_vm_bo_add(struct radeon_device *rdev, return -EINVAL; } =20 - mutex_lock(&vm->mutex); + mutex_lock(&rdev->vm_mutex); if (last_pfn > vm->last_pfn) { /* grow va space 32M by 32M */ unsigned align =3D ((32 << 20) >> 12) - 1; - radeon_mutex_lock(&rdev->cs_mutex); radeon_vm_unbind_locked(rdev, vm); - radeon_mutex_unlock(&rdev->cs_mutex); vm->last_pfn =3D (last_pfn + align) & ~align; } head =3D &vm->va; @@ -498,7 +496,7 @@ int radeon_vm_bo_add(struct radeon_device *rdev, bo, (unsigned)bo_va->soffset, tmp->bo, (unsigned)tmp->soffset, (unsigned)tmp->eoffset); kfree(bo_va); - mutex_unlock(&vm->mutex); + mutex_unlock(&rdev->vm_mutex); return -EINVAL; } last_offset =3D tmp->eoffset; @@ -506,7 +504,7 @@ int radeon_vm_bo_add(struct radeon_device *rdev, } list_add(&bo_va->vm_list, head); list_add_tail(&bo_va->bo_list, &bo->va); - mutex_unlock(&vm->mutex); + mutex_unlock(&rdev->vm_mutex); return 0; } =20 @@ -597,13 +595,11 @@ int radeon_vm_bo_rmv(struct radeon_device *rdev, if (bo_va =3D=3D NULL) return 0; =20 - mutex_lock(&vm->mutex); - radeon_mutex_lock(&rdev->cs_mutex); + mutex_lock(&rdev->vm_mutex); radeon_vm_bo_update_pte(rdev, vm, bo, NULL); - radeon_mutex_unlock(&rdev->cs_mutex); list_del(&bo_va->vm_list); - mutex_unlock(&vm->mutex); list_del(&bo_va->bo_list); + mutex_unlock(&rdev->vm_mutex); =20 kfree(bo_va); return 0; @@ -643,11 +639,8 @@ void radeon_vm_fini(struct radeon_device *rdev, stru= ct radeon_vm *vm) struct radeon_bo_va *bo_va, *tmp; int r; =20 - mutex_lock(&vm->mutex); - - radeon_mutex_lock(&rdev->cs_mutex); + mutex_lock(&rdev->vm_mutex); radeon_vm_unbind_locked(rdev, vm); - radeon_mutex_unlock(&rdev->cs_mutex); =20 /* remove all bo */ r =3D radeon_bo_reserve(rdev->ib_pool.sa_manager.bo, false); @@ -670,5 +663,5 @@ void radeon_vm_fini(struct radeon_device *rdev, struc= t radeon_vm *vm) kfree(bo_va); } } - mutex_unlock(&vm->mutex); + mutex_unlock(&rdev->vm_mutex); } --=20 1.7.5.4 --------------030900030703060205040108 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --------------030900030703060205040108--