>-----Original Message----- >From: Bridgman, John >Sent: Friday, July 11, 2014 1:48 PM >To: 'Jerome Glisse'; Oded Gabbay >Cc: David Airlie; Deucher, Alexander; linux-kernel@xxxxxxxxxxxxxxx; dri- >devel@xxxxxxxxxxxxxxxxxxxxx; Lewycky, Andrew; Joerg Roedel; Gabbay, Oded; >Koenig, Christian >Subject: RE: [PATCH 07/83] drm/radeon: Add kfd-->kgd interface of locking >srbm_gfx_cntl register > >Checking... we shouldn't need to call the lock from kfd any more.We should >be able to do any required locking in radeon kgd code. Confirmed. The locking functions are removed from the interface in commit 82 : [PATCH 82/83] drm/radeon: Remove lock functions from kfd2kgd interface There is an elegant symmetry there, but yeah we need to find a way to make this less awkward to review without screwing up all the work you've done so far. It's not obvious how to do that though. I looked at squashing into a smaller number of big commits earlier on but unless we completely rip the code out and recreate from scratch I don't see anything better than : - a few foundation commits - a big code dump that covers everything up to ~patch 54 (with 71 squashed in) - remaining commits squashed a bit to combine fixes with initial code Is that what you had in mind when you said ~10 big commits ? Our feeling was that the need to skip over the original scheduler would make it more like "one really big commit and 10-20 smaller ones", and I think we all felt that the "big code dump" required to skip over the original scheduler would be a non-starter. I guess there is another option, and maybe that's what you had in mind -- breaking the "big code dump" into smaller commits would be possible if we were willing to not have working code until we got to the equivalent of ~patch 54 (+71) when all the new scheduler bits were in. Maybe that would still be an improvement ? Thanks, JB > >>-----Original Message----- >>From: Jerome Glisse [mailto:j.glisse@xxxxxxxxx] >>Sent: Friday, July 11, 2014 12:35 PM >>To: Oded Gabbay >>Cc: David Airlie; Deucher, Alexander; linux-kernel@xxxxxxxxxxxxxxx; >>dri- devel@xxxxxxxxxxxxxxxxxxxxx; Bridgman, John; Lewycky, Andrew; >>Joerg Roedel; Gabbay, Oded; Koenig, Christian >>Subject: Re: [PATCH 07/83] drm/radeon: Add kfd-->kgd interface of >>locking srbm_gfx_cntl register >> >>On Fri, Jul 11, 2014 at 12:50:07AM +0300, Oded Gabbay wrote: >>> This patch adds a new interface to kfd2kgd_calls structure, which >>> allows the kfd to lock and unlock the srbm_gfx_cntl register >> >>Why does kfd needs to lock this register if kfd can not access any of >>those register ? This sounds broken to me, exposing a driver internal >>mutex to another driver is not something i am fan of. >> >>Cheers, >>Jérôme >> >>> >>> Signed-off-by: Oded Gabbay <oded.gabbay@xxxxxxx> >>> --- >>> drivers/gpu/drm/radeon/radeon_kfd.c | 20 ++++++++++++++++++++ >>> include/linux/radeon_kfd.h | 4 ++++ >>> 2 files changed, 24 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/radeon/radeon_kfd.c >>> b/drivers/gpu/drm/radeon/radeon_kfd.c >>> index 66ee36b..594020e 100644 >>> --- a/drivers/gpu/drm/radeon/radeon_kfd.c >>> +++ b/drivers/gpu/drm/radeon/radeon_kfd.c >>> @@ -43,6 +43,10 @@ static void unkmap_mem(struct kgd_dev *kgd, >struct >>> kgd_mem *mem); >>> >>> static uint64_t get_vmem_size(struct kgd_dev *kgd); >>> >>> +static void lock_srbm_gfx_cntl(struct kgd_dev *kgd); static void >>> +unlock_srbm_gfx_cntl(struct kgd_dev *kgd); >>> + >>> + >>> static const struct kfd2kgd_calls kfd2kgd = { >>> .allocate_mem = allocate_mem, >>> .free_mem = free_mem, >>> @@ -51,6 +55,8 @@ static const struct kfd2kgd_calls kfd2kgd = { >>> .kmap_mem = kmap_mem, >>> .unkmap_mem = unkmap_mem, >>> .get_vmem_size = get_vmem_size, >>> + .lock_srbm_gfx_cntl = lock_srbm_gfx_cntl, >>> + .unlock_srbm_gfx_cntl = unlock_srbm_gfx_cntl, >>> }; >>> >>> static const struct kgd2kfd_calls *kgd2kfd; @@ -233,3 +239,17 @@ >>> static uint64_t get_vmem_size(struct kgd_dev *kgd) >>> >>> return rdev->mc.real_vram_size; >>> } >>> + >>> +static void lock_srbm_gfx_cntl(struct kgd_dev *kgd) { >>> + struct radeon_device *rdev = (struct radeon_device *)kgd; >>> + >>> + mutex_lock(&rdev->srbm_mutex); >>> +} >>> + >>> +static void unlock_srbm_gfx_cntl(struct kgd_dev *kgd) { >>> + struct radeon_device *rdev = (struct radeon_device *)kgd; >>> + >>> + mutex_unlock(&rdev->srbm_mutex); >>> +} >>> diff --git a/include/linux/radeon_kfd.h b/include/linux/radeon_kfd.h >>> index c7997d4..40b691c 100644 >>> --- a/include/linux/radeon_kfd.h >>> +++ b/include/linux/radeon_kfd.h >>> @@ -81,6 +81,10 @@ struct kfd2kgd_calls { >>> void (*unkmap_mem)(struct kgd_dev *kgd, struct kgd_mem *mem); >>> >>> uint64_t (*get_vmem_size)(struct kgd_dev *kgd); >>> + >>> + /* SRBM_GFX_CNTL mutex */ >>> + void (*lock_srbm_gfx_cntl)(struct kgd_dev *kgd); >>> + void (*unlock_srbm_gfx_cntl)(struct kgd_dev *kgd); >>> }; >>> >>> bool kgd2kfd_init(unsigned interface_version, >>> -- >>> 1.9.1 >>> _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
