On 11.05.2012 16:44, Jerome Glisse wrote:
> On Fri, May 11, 2012 at 10:41 AM, Jerome Glisse<j.glisse at gmail.com> wrote:
>> On Fri, May 11, 2012 at 6:10 AM, Christian K?nig
>> <deathsimple at vodafone.de> 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 at vodafone.de>
>>> ---
>>> 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.
