On 11.05.2012 16:44, Jerome Glisse wrote:
On Fri, May 11, 2012 at 10:41 AM, Jerome Glisse<j.glisse@xxxxxxxxx> wrote:
On Fri, May 11, 2012 at 6:10 AM, Christian König
<deathsimple@xxxxxxxxxxx> wrote:
Even more heretic than the last one. The mutex is
probably good for something, I just can't see what
that is at the moment.
Signed-off-by: Christian König<deathsimple@xxxxxxxxxxx>
---
drivers/gpu/drm/radeon/radeon.h | 1 -
drivers/gpu/drm/radeon/radeon_device.c | 1 -
drivers/gpu/drm/radeon/radeon_object.c | 4 ----
drivers/gpu/drm/radeon/radeon_pm.c | 2 --
drivers/gpu/drm/radeon/radeon_ttm.c | 26 --------------------------
5 files changed, 34 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 8769217..c2753e7 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -1509,7 +1509,6 @@ struct radeon_device {
struct work_struct audio_work;
int num_crtc; /* number of crtcs */
struct mutex dc_hw_i2c_mutex; /* display controller hw i2c mutex */
- struct mutex vram_mutex;
struct r600_audio audio; /* audio stuff */
struct notifier_block acpi_nb;
/* only one userspace can use Hyperz features or CMASK at a time */
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 7ddab8b..24e185c 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -729,7 +729,6 @@ int radeon_device_init(struct radeon_device *rdev,
spin_lock_init(&rdev->ih.lock);
mutex_init(&rdev->gem.mutex);
mutex_init(&rdev->pm.mutex);
- mutex_init(&rdev->vram_mutex);
INIT_LIST_HEAD(&rdev->gem.objects);
init_waitqueue_head(&rdev->irq.vblank_queue);
init_waitqueue_head(&rdev->irq.idle_queue);
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index df6a4db..5fa2b1b 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -152,11 +152,9 @@ retry:
INIT_LIST_HEAD(&bo->va);
radeon_ttm_placement_from_domain(bo, domain);
/* Kernel allocation are uninterruptible */
- mutex_lock(&rdev->vram_mutex);
r = ttm_bo_init(&rdev->mman.bdev,&bo->tbo, size, type,
&bo->placement, page_align, 0, !kernel, NULL,
acc_size,&radeon_ttm_bo_destroy);
- mutex_unlock(&rdev->vram_mutex);
if (unlikely(r != 0)) {
if (r != -ERESTARTSYS) {
if (domain == RADEON_GEM_DOMAIN_VRAM) {
@@ -217,9 +215,7 @@ void radeon_bo_unref(struct radeon_bo **bo)
return;
rdev = (*bo)->rdev;
tbo =&((*bo)->tbo);
- mutex_lock(&rdev->vram_mutex);
ttm_bo_unref(&tbo);
- mutex_unlock(&rdev->vram_mutex);
if (tbo == NULL)
*bo = NULL;
}
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 0882554..e8fba26 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -251,7 +251,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
return;
mutex_lock(&rdev->ddev->struct_mutex);
- mutex_lock(&rdev->vram_mutex);
mutex_lock(&rdev->ring_lock);
/* gui idle int has issues on older chips it seems */
@@ -303,7 +302,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
mutex_unlock(&rdev->ring_lock);
- mutex_unlock(&rdev->vram_mutex);
mutex_unlock(&rdev->ddev->struct_mutex);
}
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index a7f9007..c0a8647 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -771,26 +771,6 @@ void radeon_ttm_set_active_vram_size(struct radeon_device *rdev, u64 size)
man->size = size>> PAGE_SHIFT;
}
-static struct vm_operations_struct radeon_ttm_vm_ops;
-static const struct vm_operations_struct *ttm_vm_ops = NULL;
-
-static int radeon_ttm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
-{
- struct ttm_buffer_object *bo;
- struct radeon_device *rdev;
- int r;
-
- bo = (struct ttm_buffer_object *)vma->vm_private_data;
- if (bo == NULL) {
- return VM_FAULT_NOPAGE;
- }
- rdev = radeon_get_rdev(bo->bdev);
- mutex_lock(&rdev->vram_mutex);
- r = ttm_vm_ops->fault(vma, vmf);
- mutex_unlock(&rdev->vram_mutex);
- return r;
-}
-
int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
{
struct drm_file *file_priv;
@@ -810,12 +790,6 @@ int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
if (unlikely(r != 0)) {
return r;
}
- if (unlikely(ttm_vm_ops == NULL)) {
- ttm_vm_ops = vma->vm_ops;
- radeon_ttm_vm_ops = *ttm_vm_ops;
- radeon_ttm_vm_ops.fault =&radeon_ttm_fault;
- }
- vma->vm_ops =&radeon_ttm_vm_ops;
return 0;
}
Why are you removing the ttm fault stuff ? And does the driver keep
working without this ? I would be surprise.
Cheers,
Jerome
Oh i forgot ttm already fill the vma with its own callback.
Cheers,
Jerome
Removed it to just figure out why it is there in the first place, Dave
already explained it as a protection for accessing VRAM while we change
the memory clock. Currently just trying to look into all the parts of
the driver I still doesn't understand completely.
Anyway, I would suggest to change it to a rw_semaphore and read lock it
in most cases, and just write lock it while changing the memory clock,
see the attached patch for this.
Cheers,
Christian.
>From 2f8068d07e8dec198b61f096f5300a477a67a5b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <deathsimple@xxxxxxxxxxx>
Date: Fri, 11 May 2012 14:57:18 +0200
Subject: [PATCH 1/2] drm/radeon: replace vmram_mutex with mclk_lock
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
It is a rw_semaphore now and only write locked
while changing the clock. Also the lock is renamed
to better reflect what it is protecting.
Signed-off-by: Christian König <deathsimple@xxxxxxxxxxx>
---
drivers/gpu/drm/radeon/radeon.h | 3 ++-
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
drivers/gpu/drm/radeon/radeon_object.c | 8 ++++----
drivers/gpu/drm/radeon/radeon_pm.c | 4 ++--
drivers/gpu/drm/radeon/radeon_ttm.c | 6 +++---
5 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index e00c50d..2417327 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -1055,6 +1055,8 @@ struct radeon_power_state {
struct radeon_pm {
struct mutex mutex;
+ /* write locked while reprogramming mclk */
+ struct rw_semaphore mclk_lock;
u32 active_crtcs;
int active_crtc_count;
int req_vblank;
@@ -1552,7 +1554,6 @@ struct radeon_device {
struct work_struct audio_work;
int num_crtc; /* number of crtcs */
struct mutex dc_hw_i2c_mutex; /* display controller hw i2c mutex */
- struct mutex vram_mutex;
struct r600_audio audio; /* audio stuff */
struct notifier_block acpi_nb;
/* only one userspace can use Hyperz features or CMASK at a time */
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index e1bc7e9..c74f770 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -730,7 +730,7 @@ int radeon_device_init(struct radeon_device *rdev,
spin_lock_init(&rdev->ih.lock);
mutex_init(&rdev->gem.mutex);
mutex_init(&rdev->pm.mutex);
- mutex_init(&rdev->vram_mutex);
+ init_rwsem(&rdev->pm.mclk_lock);
INIT_LIST_HEAD(&rdev->gem.objects);
init_waitqueue_head(&rdev->irq.vblank_queue);
init_waitqueue_head(&rdev->irq.idle_queue);
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index df6a4db..bc814e7 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -152,11 +152,11 @@ retry:
INIT_LIST_HEAD(&bo->va);
radeon_ttm_placement_from_domain(bo, domain);
/* Kernel allocation are uninterruptible */
- mutex_lock(&rdev->vram_mutex);
+ down_read(&rdev->pm.mclk_lock);
r = ttm_bo_init(&rdev->mman.bdev, &bo->tbo, size, type,
&bo->placement, page_align, 0, !kernel, NULL,
acc_size, &radeon_ttm_bo_destroy);
- mutex_unlock(&rdev->vram_mutex);
+ up_read(&rdev->pm.mclk_lock);
if (unlikely(r != 0)) {
if (r != -ERESTARTSYS) {
if (domain == RADEON_GEM_DOMAIN_VRAM) {
@@ -217,9 +217,9 @@ void radeon_bo_unref(struct radeon_bo **bo)
return;
rdev = (*bo)->rdev;
tbo = &((*bo)->tbo);
- mutex_lock(&rdev->vram_mutex);
+ down_read(&rdev->pm.mclk_lock);
ttm_bo_unref(&tbo);
- mutex_unlock(&rdev->vram_mutex);
+ up_read(&rdev->pm.mclk_lock);
if (tbo == NULL)
*bo = NULL;
}
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 0882554..d13b6ae 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -251,7 +251,7 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
return;
mutex_lock(&rdev->ddev->struct_mutex);
- mutex_lock(&rdev->vram_mutex);
+ down_write(&rdev->pm.mclk_lock);
mutex_lock(&rdev->ring_lock);
/* gui idle int has issues on older chips it seems */
@@ -303,7 +303,7 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
rdev->pm.dynpm_planned_action = DYNPM_ACTION_NONE;
mutex_unlock(&rdev->ring_lock);
- mutex_unlock(&rdev->vram_mutex);
+ up_write(&rdev->pm.mclk_lock);
mutex_unlock(&rdev->ddev->struct_mutex);
}
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index a7f9007..fc05ae3 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -785,9 +785,9 @@ static int radeon_ttm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
return VM_FAULT_NOPAGE;
}
rdev = radeon_get_rdev(bo->bdev);
- mutex_lock(&rdev->vram_mutex);
+ down_read(&rdev->pm.mclk_lock);
r = ttm_vm_ops->fault(vma, vmf);
- mutex_unlock(&rdev->vram_mutex);
+ up_read(&rdev->pm.mclk_lock);
return r;
}
@@ -810,7 +810,7 @@ int radeon_mmap(struct file *filp, struct vm_area_struct *vma)
if (unlikely(r != 0)) {
return r;
}
- if (unlikely(ttm_vm_ops == NULL)) {
+ if (unlikely(ttm_vm_ops == NULL && !(rdev->flags & RADEON_IS_IGP))) {
ttm_vm_ops = vma->vm_ops;
radeon_ttm_vm_ops = *ttm_vm_ops;
radeon_ttm_vm_ops.fault = &radeon_ttm_fault;
--
1.7.9.5
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
