On 2017-05-08 11:51 PM, Liu, Monk wrote: >> Can you explain your reasoning behind your current position that the KIQ shouldn't be used by baremetal amdgpu? > > [ML] I didn't mean KIQ shouldn't leveraged by bare-metal, instead how it is used by bare-metal is none of my interest ... > I mean it better not be used under SR-IOV case by other client except original two (RLCV, register access in KMD) > I assume you try to use kiq for KFD under SRIOV case, right ? Can you elaborate what kind of jobs you are going to submit to kiq for kfd? I already get one that you want to write register in INTR context . > This is not for KFD. This is to enable high priority queues for vulkan applications. These high priority queues are used by vulkan based VR compositors to enable low latency frame reprojection. There are certain resources that are reserved by the high priority queue, and these reservations need to be dropped as soon as possible to allow GFX (or other queues) to use these resources again with minimal impact. This is why the resource release is performed in the IRQ. Regards, Andres > THANKS & BR > > -----Original Message----- > From: Andres Rodriguez [mailto:andresr at valvesoftware.com] > Sent: Monday, May 08, 2017 11:42 PM > To: Liu, Monk <Monk.Liu at amd.com>; Andres Rodriguez <andresx7 at gmail.com>; amd-gfx at lists.freedesktop.org > Subject: RE: [PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a spinlock > > > > On 2017-05-08 02:08 AM, Liu, Monk wrote: >> Andres >> >> Some previous patches like move KIQ mutex-lock from amdgpu_virt to >> common place jumped my NAK, but from technique perspective it's no >> matter anyway, But this patch and the following patches are go to a >> dead end, >> >> 1, Don't use KIQ to access register inside INTR context ... > > This is explained below. > >> 2, Don't submit CP/hw blocking jobs on KIQ since they will prevent >> world switch, > > Only WRITE_DATA packets are being submitted. There is no blocking work that would prevent a worldswitch. > >> >> Besides, why not use MEC2 PIPE1 queue1 to serve your purpose ? if you insist to do above things in KIQ? >> Any reason you must use KIQ ? > > Having a separate KIQ for bare-metal amdgpu and virtualized amdgpu sounds good at face-value. However, I'm not sure if it would introduce subtle synchronization bugs if both pipes attempt to touch the same HW resources simultaneously. > > Can you explain your reasoning behind your current position that the KIQ shouldn't be used by baremetal amdgpu? > >> >> >> BR >> -----Original Message----- >> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf >> Of Liu, Monk >> Sent: Monday, May 08, 2017 1:59 PM >> To: Andres Rodriguez <andresx7 at gmail.com>; >> amd-gfx at lists.freedesktop.org >> Subject: RE: [PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a >> spinlock >> >> Clearly NAK >> >> mutex_lock(&kiq->ring_mutex); >> amdgpu_ring_alloc(ring, 32); >> amdgpu_ring_emit_rreg(ring, reg); >> amdgpu_fence_emit(ring, &f); >> amdgpu_ring_commit(ring); >> mutex_unlock(&kiq->ring_mutex); >> >> >> be aware that amdgpu_fence_emit() introduces fence_wait() operation, > > Please see the following patch: > [PATCH 6/8] drm/amdgpu: add macro for asynchronous KIQ register writes > > This is why this patch is described as "step one" > >> spin_lock is forbidden and why you want to use spinlock instead of >> mutex, please give the explanation > > I don't understand what you mean by spin_lock being forbidden. The spin_lock is a standard kernel construct specifically designed to address mutual exclusion in contexts that disallow sleeping. That is exactly what we are trying to do here, as described in the commit message. > > Regards, > Andres > >> >> BR Monk >> >> -----Original Message----- >> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf >> Of Andres Rodriguez >> Sent: Saturday, May 06, 2017 1:10 AM >> To: amd-gfx at lists.freedesktop.org >> Cc: andresx7 at gmail.com >> Subject: [PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a spinlock >> >> First step towards enabling kiq register operations from an interrupt >> handler >> >> Signed-off-by: Andres Rodriguez <andresx7 at gmail.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +- >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +++++----- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> index 496ad66..1d987e3 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> @@ -941,7 +941,7 @@ struct amdgpu_kiq { >> struct amdgpu_ring ring; >> struct amdgpu_irq_src irq; >> uint32_t reg_val_offs; >> - struct mutex ring_mutex; >> + spinlock_t ring_lock; >> }; >> >> /* >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index ef3a46d..ce22f04 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -169,12 +169,12 @@ uint32_t amdgpu_kiq_rreg(struct amdgpu_device >> *adev, uint32_t reg) >> >> BUG_ON(!ring->funcs->emit_rreg); >> >> - mutex_lock(&kiq->ring_mutex); >> + spin_lock(&kiq->ring_lock); >> amdgpu_ring_alloc(ring, 32); >> amdgpu_ring_emit_rreg(ring, reg); >> amdgpu_fence_emit(ring, &f); >> amdgpu_ring_commit(ring); >> - mutex_unlock(&kiq->ring_mutex); >> + spin_unlock(&kiq->ring_lock); >> >> r = dma_fence_wait(f, false); >> if (r) >> @@ -195,12 +195,12 @@ void amdgpu_kiq_wreg(struct amdgpu_device *adev, >> uint32_t reg, uint32_t v) >> >> BUG_ON(!ring->funcs->emit_wreg); >> >> - mutex_lock(&kiq->ring_mutex); >> + spin_lock(&kiq->ring_lock); >> amdgpu_ring_alloc(ring, 32); >> amdgpu_ring_emit_wreg(ring, reg, v); >> amdgpu_fence_emit(ring, &f); >> amdgpu_ring_commit(ring); >> - mutex_unlock(&kiq->ring_mutex); >> + spin_unlock(&kiq->ring_lock); >> >> r = dma_fence_wait(f, false); >> if (r) >> @@ -1903,7 +1903,6 @@ int amdgpu_device_init(struct amdgpu_device *adev, >> mutex_init(&adev->firmware.mutex); >> mutex_init(&adev->pm.mutex); >> mutex_init(&adev->gfx.gpu_clock_mutex); >> - mutex_init(&adev->gfx.kiq.ring_mutex); >> mutex_init(&adev->srbm_mutex); >> mutex_init(&adev->grbm_idx_mutex); >> mutex_init(&adev->mn_lock); >> @@ -1921,6 +1920,7 @@ int amdgpu_device_init(struct amdgpu_device *adev, >> spin_lock_init(&adev->gc_cac_idx_lock); >> spin_lock_init(&adev->audio_endpt_idx_lock); >> spin_lock_init(&adev->mm_stats.lock); >> + spin_lock_init(&adev->gfx.kiq.ring_lock); >> >> INIT_LIST_HEAD(&adev->shadow_list); >> mutex_init(&adev->shadow_list_lock); >> -- >> 2.9.3 >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> > >
