From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 5/5] drm/radeon: WIP remove vmram_mutex Date: Fri, 11 May 2012 17:12:11 +0200 Message-ID: <4FAD2C4B.7000308@vodafone.de> References: <1336731029-30365-1-git-send-email-deathsimple@vodafone.de> <1336731029-30365-6-git-send-email-deathsimple@vodafone.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------050900000904080402060909" Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 552889E7D2 for ; Fri, 11 May 2012 08:12:15 -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: Jerome Glisse Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org This is a multi-part message in MIME format. --------------050900000904080402060909 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable On 11.05.2012 16:44, Jerome Glisse wrote: > On Fri, May 11, 2012 at 10:41 AM, Jerome Glisse wr= ote: >> On Fri, May 11, 2012 at 6:10 AM, Christian K=F6nig >> 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=F6nig >>> --- >>> 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 mu= tex */ >>> - 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 t= ime */ >>> 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 =3D 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 !=3D 0)) { >>> if (r !=3D -ERESTARTSYS) { >>> if (domain =3D=3D RADEON_GEM_DOMAIN_VRAM) { >>> @@ -217,9 +215,7 @@ void radeon_bo_unref(struct radeon_bo **bo) >>> return; >>> rdev =3D (*bo)->rdev; >>> tbo =3D&((*bo)->tbo); >>> - mutex_lock(&rdev->vram_mutex); >>> ttm_bo_unref(&tbo); >>> - mutex_unlock(&rdev->vram_mutex); >>> if (tbo =3D=3D NULL) >>> *bo =3D NULL; >>> } >>> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/rad= eon/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_de= vice *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_de= vice *rdev) >>> rdev->pm.dynpm_planned_action =3D 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/ra= deon/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 rade= on_device *rdev, u64 size) >>> man->size =3D size>> PAGE_SHIFT; >>> } >>> >>> -static struct vm_operations_struct radeon_ttm_vm_ops; >>> -static const struct vm_operations_struct *ttm_vm_ops =3D NULL; >>> - >>> -static int radeon_ttm_fault(struct vm_area_struct *vma, struct vm_fa= ult *vmf) >>> -{ >>> - struct ttm_buffer_object *bo; >>> - struct radeon_device *rdev; >>> - int r; >>> - >>> - bo =3D (struct ttm_buffer_object *)vma->vm_private_data; >>> - if (bo =3D=3D NULL) { >>> - return VM_FAULT_NOPAGE; >>> - } >>> - rdev =3D radeon_get_rdev(bo->bdev); >>> - mutex_lock(&rdev->vram_mutex); >>> - r =3D 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_are= a_struct *vma) >>> if (unlikely(r !=3D 0)) { >>> return r; >>> } >>> - if (unlikely(ttm_vm_ops =3D=3D NULL)) { >>> - ttm_vm_ops =3D vma->vm_ops; >>> - radeon_ttm_vm_ops =3D *ttm_vm_ops; >>> - radeon_ttm_vm_ops.fault =3D&radeon_ttm_fault; >>> - } >>> - vma->vm_ops =3D&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=20 already explained it as a protection for accessing VRAM while we change=20 the memory clock. Currently just trying to look into all the parts of=20 the driver I still doesn't understand completely. Anyway, I would suggest to change it to a rw_semaphore and read lock it=20 in most cases, and just write lock it while changing the memory clock,=20 see the attached patch for this. Cheers, Christian. --------------050900000904080402060909 Content-Type: text/x-patch; name="0001-drm-radeon-replace-vmram_mutex-with-mclk_lock.patch" Content-Disposition: attachment; filename*0="0001-drm-radeon-replace-vmram_mutex-with-mclk_lock.patch" Content-Transfer-Encoding: quoted-printable >>From 2f8068d07e8dec198b61f096f5300a477a67a5b1 Mon Sep 17 00:00:00 2001 From: =3D?UTF-8?q?Christian=3D20K=3DC3=3DB6nig?=3D 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=3DUTF-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=C3=B6nig --- 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/rad= eon.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 { =20 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/rad= eon/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/rad= eon/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 =3D 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 !=3D 0)) { if (r !=3D -ERESTARTSYS) { if (domain =3D=3D RADEON_GEM_DOMAIN_VRAM) { @@ -217,9 +217,9 @@ void radeon_bo_unref(struct radeon_bo **bo) return; rdev =3D (*bo)->rdev; tbo =3D &((*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 =3D=3D NULL) *bo =3D 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; =20 mutex_lock(&rdev->ddev->struct_mutex); - mutex_lock(&rdev->vram_mutex); + down_write(&rdev->pm.mclk_lock); mutex_lock(&rdev->ring_lock); =20 /* 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 =3D DYNPM_ACTION_NONE; =20 mutex_unlock(&rdev->ring_lock); - mutex_unlock(&rdev->vram_mutex); + up_write(&rdev->pm.mclk_lock); mutex_unlock(&rdev->ddev->struct_mutex); } =20 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 *vm= a, struct vm_fault *vmf) return VM_FAULT_NOPAGE; } rdev =3D radeon_get_rdev(bo->bdev); - mutex_lock(&rdev->vram_mutex); + down_read(&rdev->pm.mclk_lock); r =3D ttm_vm_ops->fault(vma, vmf); - mutex_unlock(&rdev->vram_mutex); + up_read(&rdev->pm.mclk_lock); return r; } =20 @@ -810,7 +810,7 @@ int radeon_mmap(struct file *filp, struct vm_area_str= uct *vma) if (unlikely(r !=3D 0)) { return r; } - if (unlikely(ttm_vm_ops =3D=3D NULL)) { + if (unlikely(ttm_vm_ops =3D=3D NULL && !(rdev->flags & RADEON_IS_IGP)))= { ttm_vm_ops =3D vma->vm_ops; radeon_ttm_vm_ops =3D *ttm_vm_ops; radeon_ttm_vm_ops.fault =3D &radeon_ttm_fault; --=20 1.7.9.5 --------------050900000904080402060909 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 --------------050900000904080402060909--