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 >
