[PATCH 5/5] drm/radeon: WIP remove vmram_mutex
Christian König
deathsimple at vodafone.de
Fri May 11 08:12:11 PDT 2012
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.
More information about the dri-devel
mailing list