[RFC] Problems with SRBM select on KIQ

[RFC] Problems with SRBM select on KIQ

[Andres Rodriguez]

Hey Everyone,

On one of the previous comments for this series I received feedback that
the register programming should be done through the KIQ. This series has
the relevant changes.

However, register writes that require an srbm_select are not working
correctly. The effect is as if the write never happened. 

Maybe I'm just doing something silly or dumb in here, but I can't really
see it. Possibly the way I try to perform the srbm_select for KIQ?

Any feedback on this issue is appreciated.

Regards,
Andres

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/8] drm/amdgpu: add parameter to allocate high priority contexts v9

[Andres Rodriguez]

Add a new context creation parameter to express a global context priority.

The priority ranking in descending order is as follows:
 * AMDGPU_CTX_PRIORITY_HIGH
 * AMDGPU_CTX_PRIORITY_NORMAL
 * AMDGPU_CTX_PRIORITY_LOW

The driver will attempt to schedule work to the hardware according to
the priorities. No latency or throughput guarantees are provided by
this patch.

This interface intends to service the EGL_IMG_context_priority
extension, and vulkan equivalents.

v2: Instead of using flags, repurpose __pad
v3: Swap enum values of _NORMAL _HIGH for backwards compatibility
v4: Validate usermode priority and store it
v5: Move priority validation into amdgpu_ctx_ioctl(), headline reword
v6: add UAPI note regarding priorities requiring CAP_SYS_ADMIN
v7: remove ctx->priority
v8: added AMDGPU_CTX_PRIORITY_LOW, s/CAP_SYS_ADMIN/CAP_SYS_NICE
v9: change the priority parameter to __s32

Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c       | 38 ++++++++++++++++++++++++---
 drivers/gpu/drm/amd/scheduler/gpu_scheduler.h |  4 ++-
 include/uapi/drm/amdgpu_drm.h                 |  8 +++++-
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 1969f27..e0bb76a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -25,11 +25,19 @@
 #include <drm/drmP.h>
 #include "amdgpu.h"
 
-static int amdgpu_ctx_init(struct amdgpu_device *adev, struct amdgpu_ctx *ctx)
+static int amdgpu_ctx_init(struct amdgpu_device *adev,
+			   enum amd_sched_priority priority,
+			   struct amdgpu_ctx *ctx)
 {
 	unsigned i, j;
 	int r;
 
+	if (priority < 0 || priority >= AMD_SCHED_PRIORITY_MAX)
+		return -EINVAL;
+
+	if (priority >= AMD_SCHED_PRIORITY_HIGH && !capable(CAP_SYS_NICE))
+		return -EACCES;
+
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->adev = adev;
 	kref_init(&ctx->refcount);
@@ -51,7 +59,7 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev, struct amdgpu_ctx *ctx)
 		struct amdgpu_ring *ring = adev->rings[i];
 		struct amd_sched_rq *rq;
 
-		rq = &ring->sched.sched_rq[AMD_SCHED_PRIORITY_NORMAL];
+		rq = &ring->sched.sched_rq[priority];
 		r = amd_sched_entity_init(&ring->sched, &ctx->rings[i].entity,
 					  rq, amdgpu_sched_jobs);
 		if (r)
@@ -96,6 +104,7 @@ static void amdgpu_ctx_fini(struct amdgpu_ctx *ctx)
 
 static int amdgpu_ctx_alloc(struct amdgpu_device *adev,
 			    struct amdgpu_fpriv *fpriv,
+			    enum amd_sched_priority priority,
 			    uint32_t *id)
 {
 	struct amdgpu_ctx_mgr *mgr = &fpriv->ctx_mgr;
@@ -113,8 +122,9 @@ static int amdgpu_ctx_alloc(struct amdgpu_device *adev,
 		kfree(ctx);
 		return r;
 	}
+
 	*id = (uint32_t)r;
-	r = amdgpu_ctx_init(adev, ctx);
+	r = amdgpu_ctx_init(adev, priority, ctx);
 	if (r) {
 		idr_remove(&mgr->ctx_handles, *id);
 		*id = 0;
@@ -184,11 +194,27 @@ static int amdgpu_ctx_query(struct amdgpu_device *adev,
 	return 0;
 }
 
+static enum amd_sched_priority amdgpu_to_sched_priority(int amdgpu_priority)
+{
+	switch (amdgpu_priority) {
+	case AMDGPU_CTX_PRIORITY_HIGH:
+		return AMD_SCHED_PRIORITY_HIGH;
+	case AMDGPU_CTX_PRIORITY_NORMAL:
+		return AMD_SCHED_PRIORITY_NORMAL;
+	case AMDGPU_CTX_PRIORITY_LOW:
+		return AMD_SCHED_PRIORITY_LOW;
+	default:
+		WARN(1, "Invalid context priority %d\n", amdgpu_priority);
+		return AMD_SCHED_PRIORITY_NORMAL;
+	}
+}
+
 int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *filp)
 {
 	int r;
 	uint32_t id;
+	enum amd_sched_priority priority;
 
 	union drm_amdgpu_ctx *args = data;
 	struct amdgpu_device *adev = dev->dev_private;
@@ -196,10 +222,14 @@ int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
 
 	r = 0;
 	id = args->in.ctx_id;
+	priority = amdgpu_to_sched_priority(args->in.priority);
+
+	if (priority >= AMD_SCHED_PRIORITY_MAX)
+		return -EINVAL;
 
 	switch (args->in.op) {
 	case AMDGPU_CTX_OP_ALLOC_CTX:
-		r = amdgpu_ctx_alloc(adev, fpriv, &id);
+		r = amdgpu_ctx_alloc(adev, fpriv, priority, &id);
 		args->out.alloc.ctx_id = id;
 		break;
 	case AMDGPU_CTX_OP_FREE_CTX:
diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
index 0255c7f..fc2d4bd 100644
--- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
+++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
@@ -109,7 +109,9 @@ struct amd_sched_backend_ops {
 
 enum amd_sched_priority {
 	AMD_SCHED_PRIORITY_MIN,
-	AMD_SCHED_PRIORITY_NORMAL = AMD_SCHED_PRIORITY_MIN,
+	AMD_SCHED_PRIORITY_LOW = AMD_SCHED_PRIORITY_MIN,
+	AMD_SCHED_PRIORITY_NORMAL,
+	AMD_SCHED_PRIORITY_HIGH,
 	AMD_SCHED_PRIORITY_KERNEL,
 	AMD_SCHED_PRIORITY_MAX
 };
diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 56ceb3d..df1f4cb 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -162,13 +162,19 @@ union drm_amdgpu_bo_list {
 /* unknown cause */
 #define AMDGPU_CTX_UNKNOWN_RESET	3
 
+/* Context priority level */
+#define AMDGPU_CTX_PRIORITY_LOW         -1023
+#define AMDGPU_CTX_PRIORITY_NORMAL      0
+/* Selecting a priority above NORMAL requires CAP_SYS_ADMIN */
+#define AMDGPU_CTX_PRIORITY_HIGH        1023
+
 struct drm_amdgpu_ctx_in {
 	/** AMDGPU_CTX_OP_* */
 	__u32	op;
 	/** For future use, no flags defined so far */
 	__u32	flags;
 	__u32	ctx_id;
-	__u32	_pad;
+	__s32	priority;
 };
 
 union drm_amdgpu_ctx_out {
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/8] drm/amdgpu: add framework for HW specific priority settings v6

[Andres Rodriguez]

Add an initial framework for changing the HW priorities of rings. The
framework allows requesting priority changes for the lifetime of an
amdgpu_job. After the job completes the priority will decay to the next
lowest priority for which a request is still valid.

A new ring function set_priority() can now be populated to take care of
the HW specific programming sequence for priority changes.

v2: set priority before emitting IB, and take a ref on amdgpu_job
v3: use AMD_SCHED_PRIORITY_* instead of AMDGPU_CTX_PRIORITY_*
v4: plug amdgpu_ring_restore_priority_cb into amdgpu_job_free_cb
v5: use atomic for tracking job priorities instead of last_job
v6: rename amdgpu_ring_priority_[get/put]() and align parameters

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c       |  7 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c      | 78 ++++++++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h      | 15 ++++++
 drivers/gpu/drm/amd/scheduler/gpu_scheduler.h |  7 +++
 4 files changed, 106 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index c3cfeb3..42f60d5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -95,6 +95,7 @@ static void amdgpu_job_free_cb(struct amd_sched_job *s_job)
 {
 	struct amdgpu_job *job = container_of(s_job, struct amdgpu_job, base);
 
+	amdgpu_ring_priority_put(job->ring, amd_sched_get_job_priority(s_job));
 	dma_fence_put(job->fence);
 	amdgpu_sync_free(&job->sync);
 	kfree(job);
@@ -169,6 +170,11 @@ static struct dma_fence *amdgpu_job_run(struct amd_sched_job *sched_job)
 
 	BUG_ON(amdgpu_sync_peek_fence(&job->sync, NULL));
 
+	r = amdgpu_ring_priority_get(job->ring,
+				     amd_sched_get_job_priority(&job->base));
+	if (r)
+		DRM_ERROR("Failed to set job priority (%d)\n", r);
+
 	trace_amdgpu_sched_run_job(job);
 	r = amdgpu_ib_schedule(job->ring, job->num_ibs, job->ibs, job, &fence);
 	if (r)
@@ -177,6 +183,7 @@ static struct dma_fence *amdgpu_job_run(struct amd_sched_job *sched_job)
 	/* if gpu reset, hw fence will be replaced here */
 	dma_fence_put(job->fence);
 	job->fence = dma_fence_get(fence);
+
 	amdgpu_job_free_resources(job);
 	return fence;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 7486277..09fa8f7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -200,6 +200,77 @@ void amdgpu_ring_undo(struct amdgpu_ring *ring)
 }
 
 /**
+ * amdgpu_ring_priority_put - restore a ring's priority
+ *
+ * @ring: amdgpu_ring structure holding the information
+ * @priority: target priority
+ *
+ * Release a request for executing at @priority
+ */
+void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
+			      enum amd_sched_priority priority)
+{
+	int i;
+
+	if (!ring->funcs->set_priority)
+		return;
+
+	if (atomic_dec_return(&ring->num_jobs[priority]) > 0)
+		return;
+
+	/* no need to restore if the job is already at the lowest priority */
+	if (priority == AMD_SCHED_PRIORITY_NORMAL)
+		return;
+
+	spin_lock(&ring->priority_lock);
+	/* something higher prio is executing, no need to decay */
+	if (ring->priority > priority)
+		goto out_unlock;
+
+	/* decay priority to the next level with a job available */
+	for (i = priority; i >= AMD_SCHED_PRIORITY_MIN; i--) {
+		if (i == AMD_SCHED_PRIORITY_NORMAL
+				|| atomic_read(&ring->num_jobs[i])) {
+			ring->priority = i;
+			ring->funcs->set_priority(ring, i);
+			break;
+		}
+	}
+
+out_unlock:
+	spin_unlock(&ring->priority_lock);
+}
+
+/**
+ * amdgpu_ring_priority_get - change the ring's priority
+ *
+ * @ring: amdgpu_ring structure holding the information
+ * @priority: target priority
+ *
+ * Request a ring's priority to be raised to @priority (refcounted).
+ * Returns 0 on success, error otherwise
+ */
+int amdgpu_ring_priority_get(struct amdgpu_ring *ring,
+			     enum amd_sched_priority priority)
+{
+	if (!ring->funcs->set_priority)
+		return 0;
+
+	atomic_inc(&ring->num_jobs[priority]);
+
+	spin_lock(&ring->priority_lock);
+	if (priority <= ring->priority)
+		goto out_unlock;
+
+	ring->priority = priority;
+	ring->funcs->set_priority(ring, priority);
+
+out_unlock:
+	spin_unlock(&ring->priority_lock);
+	return 0;
+}
+
+/**
  * amdgpu_ring_init - init driver ring struct.
  *
  * @adev: amdgpu_device pointer
@@ -214,7 +285,7 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
 		     unsigned max_dw, struct amdgpu_irq_src *irq_src,
 		     unsigned irq_type)
 {
-	int r;
+	int r, i;
 
 	if (ring->adev == NULL) {
 		if (adev->num_rings >= AMDGPU_MAX_RINGS)
@@ -300,9 +371,14 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
 	}
 
 	ring->max_dw = max_dw;
+	ring->priority = AMD_SCHED_PRIORITY_NORMAL;
+	spin_lock_init(&ring->priority_lock);
 	INIT_LIST_HEAD(&ring->lru_list);
 	amdgpu_ring_lru_touch(adev, ring);
 
+	for (i = 0; i < AMD_SCHED_PRIORITY_MAX; ++i)
+		atomic_set(&ring->num_jobs[i], 0);
+
 	if (amdgpu_debugfs_ring_init(adev, ring)) {
 		DRM_ERROR("Failed to register debugfs file for rings !\n");
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 10d6692..885d3b5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -24,6 +24,7 @@
 #ifndef __AMDGPU_RING_H__
 #define __AMDGPU_RING_H__
 
+#include <drm/amdgpu_drm.h>
 #include "gpu_scheduler.h"
 
 /* max number of rings */
@@ -54,6 +55,7 @@ struct amdgpu_device;
 struct amdgpu_ring;
 struct amdgpu_ib;
 struct amdgpu_cs_parser;
+struct amdgpu_job;
 
 /*
  * Fences.
@@ -142,6 +144,9 @@ struct amdgpu_ring_funcs {
 	void (*emit_cntxcntl) (struct amdgpu_ring *ring, uint32_t flags);
 	void (*emit_rreg)(struct amdgpu_ring *ring, uint32_t reg);
 	void (*emit_wreg)(struct amdgpu_ring *ring, uint32_t reg, uint32_t val);
+	/* priority functions */
+	void (*set_priority) (struct amdgpu_ring *ring,
+			      enum amd_sched_priority priority);
 };
 
 struct amdgpu_ring {
@@ -181,6 +186,12 @@ struct amdgpu_ring {
 	u64			cond_exe_gpu_addr;
 	volatile u32		*cond_exe_cpu_addr;
 	unsigned		vm_inv_eng;
+
+	atomic_t		num_jobs[AMD_SCHED_PRIORITY_MAX];
+	spinlock_t		priority_lock;
+	/* protected by priority_lock */
+	int			priority;
+
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *ent;
 #endif
@@ -193,6 +204,10 @@ void amdgpu_ring_insert_nop(struct amdgpu_ring *ring, uint32_t count);
 void amdgpu_ring_generic_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib);
 void amdgpu_ring_commit(struct amdgpu_ring *ring);
 void amdgpu_ring_undo(struct amdgpu_ring *ring);
+int amdgpu_ring_priority_get(struct amdgpu_ring *ring,
+			     enum amd_sched_priority priority);
+void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
+			      enum amd_sched_priority priority);
 int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
 		     unsigned ring_size, struct amdgpu_irq_src *irq_src,
 		     unsigned irq_type);
diff --git a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
index fc2d4bd..9dc19ed 100644
--- a/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
+++ b/drivers/gpu/drm/amd/scheduler/gpu_scheduler.h
@@ -160,4 +160,11 @@ int amd_sched_job_init(struct amd_sched_job *job,
 		       void *owner);
 void amd_sched_hw_job_reset(struct amd_gpu_scheduler *sched);
 void amd_sched_job_recovery(struct amd_gpu_scheduler *sched);
+
+static inline enum amd_sched_priority
+amd_sched_get_job_priority(struct amd_sched_job *job)
+{
+	return (job->s_entity->rq - job->sched->sched_rq);
+}
+
 #endif
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/8] drm/amdgpu: separate KIQ operations from amdgpu_virt

[Andres Rodriguez]

KIQ is now used for baremetal compute initialization. Since it is no
longer compute specific, move the KIQ reg opts to a more general
location.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  4 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 55 ++++++++++++++++++++++++++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c   | 50 ---------------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h   |  4 ---
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c      |  8 ++---
 drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c      |  8 ++---
 6 files changed, 65 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 8ae8671..496ad66 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -940,6 +940,8 @@ struct amdgpu_kiq {
 	struct amdgpu_bo	*eop_obj;
 	struct amdgpu_ring	ring;
 	struct amdgpu_irq_src	irq;
+	uint32_t		reg_val_offs;
+	struct mutex		ring_mutex;
 };
 
 /*
@@ -1666,6 +1668,8 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
 		    uint32_t acc_flags);
 u32 amdgpu_io_rreg(struct amdgpu_device *adev, u32 reg);
 void amdgpu_io_wreg(struct amdgpu_device *adev, u32 reg, u32 v);
+uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg);
+void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v);
 
 u32 amdgpu_mm_rdoorbell(struct amdgpu_device *adev, u32 index);
 void amdgpu_mm_wdoorbell(struct amdgpu_device *adev, u32 index, u32 v);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c93516a..ef3a46d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -99,7 +99,7 @@ uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
 
 	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
 		BUG_ON(in_interrupt());
-		return amdgpu_virt_kiq_rreg(adev, reg);
+		return amdgpu_kiq_rreg(adev, reg);
 	}
 
 	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
@@ -123,7 +123,7 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
 
 	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
 		BUG_ON(in_interrupt());
-		return amdgpu_virt_kiq_wreg(adev, reg, v);
+		return amdgpu_kiq_wreg(adev, reg, v);
 	}
 
 	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
@@ -159,6 +159,56 @@ void amdgpu_io_wreg(struct amdgpu_device *adev, u32 reg, u32 v)
 	}
 }
 
+uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
+{
+	signed long r;
+	uint32_t val;
+	struct dma_fence *f;
+	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
+	struct amdgpu_ring *ring = &kiq->ring;
+
+	BUG_ON(!ring->funcs->emit_rreg);
+
+	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);
+
+	r = dma_fence_wait(f, false);
+	if (r)
+		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
+	dma_fence_put(f);
+
+	val = adev->wb.wb[kiq->reg_val_offs];
+
+	return val;
+}
+
+void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
+{
+	signed long r;
+	struct dma_fence *f;
+	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
+	struct amdgpu_ring *ring = &kiq->ring;
+
+	BUG_ON(!ring->funcs->emit_wreg);
+
+	mutex_lock(&kiq->ring_mutex);
+	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);
+
+	r = dma_fence_wait(f, false);
+	if (r)
+		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
+	dma_fence_put(f);
+}
+
+
 /**
  * amdgpu_mm_rdoorbell - read a doorbell dword
  *
@@ -1853,6 +1903,7 @@ 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);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index bd422f0..7658522 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -108,59 +108,9 @@ void amdgpu_virt_init_setting(struct amdgpu_device *adev)
 	adev->cg_flags = 0;
 	adev->pg_flags = 0;
 
-	mutex_init(&adev->virt.lock_kiq);
 	mutex_init(&adev->virt.lock_reset);
 }
 
-uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
-{
-	signed long r;
-	uint32_t val;
-	struct dma_fence *f;
-	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
-	struct amdgpu_ring *ring = &kiq->ring;
-
-	BUG_ON(!ring->funcs->emit_rreg);
-
-	mutex_lock(&adev->virt.lock_kiq);
-	amdgpu_ring_alloc(ring, 32);
-	amdgpu_ring_emit_rreg(ring, reg);
-	amdgpu_fence_emit(ring, &f);
-	amdgpu_ring_commit(ring);
-	mutex_unlock(&adev->virt.lock_kiq);
-
-	r = dma_fence_wait(f, false);
-	if (r)
-		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
-	dma_fence_put(f);
-
-	val = adev->wb.wb[adev->virt.reg_val_offs];
-
-	return val;
-}
-
-void amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
-{
-	signed long r;
-	struct dma_fence *f;
-	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
-	struct amdgpu_ring *ring = &kiq->ring;
-
-	BUG_ON(!ring->funcs->emit_wreg);
-
-	mutex_lock(&adev->virt.lock_kiq);
-	amdgpu_ring_alloc(ring, 32);
-	amdgpu_ring_emit_wreg(ring, reg, v);
-	amdgpu_fence_emit(ring, &f);
-	amdgpu_ring_commit(ring);
-	mutex_unlock(&adev->virt.lock_kiq);
-
-	r = dma_fence_wait(f, false);
-	if (r)
-		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
-	dma_fence_put(f);
-}
-
 /**
  * amdgpu_virt_request_full_gpu() - request full gpu access
  * @amdgpu:	amdgpu device.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
index a8ed162..2e67db9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
@@ -51,8 +51,6 @@ struct amdgpu_virt {
 	struct amdgpu_bo		*csa_obj;
 	uint64_t			csa_vmid0_addr;
 	bool chained_ib_support;
-	uint32_t			reg_val_offs;
-	struct mutex			lock_kiq;
 	struct mutex                    lock_reset;
 	struct amdgpu_irq_src		ack_irq;
 	struct amdgpu_irq_src		rcv_irq;
@@ -92,8 +90,6 @@ struct amdgpu_vm;
 int amdgpu_allocate_static_csa(struct amdgpu_device *adev);
 int amdgpu_map_static_csa(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 void amdgpu_virt_init_setting(struct amdgpu_device *adev);
-uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg);
-void amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v);
 int amdgpu_virt_request_full_gpu(struct amdgpu_device *adev, bool init);
 int amdgpu_virt_release_full_gpu(struct amdgpu_device *adev, bool init);
 int amdgpu_virt_reset_gpu(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 4ba2aeb..976ee0b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -1416,7 +1416,7 @@ static int gfx_v8_0_kiq_init_ring(struct amdgpu_device *adev,
 	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
 	int r = 0;
 
-	r = amdgpu_wb_get(adev, &adev->virt.reg_val_offs);
+	r = amdgpu_wb_get(adev, &kiq->reg_val_offs);
 	if (r)
 		return r;
 
@@ -1442,7 +1442,7 @@ static int gfx_v8_0_kiq_init_ring(struct amdgpu_device *adev,
 static void gfx_v8_0_kiq_free_ring(struct amdgpu_ring *ring,
 				   struct amdgpu_irq_src *irq)
 {
-	amdgpu_wb_free(ring->adev, ring->adev->virt.reg_val_offs);
+	amdgpu_wb_free(ring->adev, ring->adev->gfx.kiq.reg_val_offs);
 	amdgpu_ring_fini(ring);
 }
 
@@ -6778,9 +6778,9 @@ static void gfx_v8_0_ring_emit_rreg(struct amdgpu_ring *ring, uint32_t reg)
 	amdgpu_ring_write(ring, reg);
 	amdgpu_ring_write(ring, 0);
 	amdgpu_ring_write(ring, lower_32_bits(adev->wb.gpu_addr +
-				adev->virt.reg_val_offs * 4));
+				adev->gfx.kiq.reg_val_offs * 4));
 	amdgpu_ring_write(ring, upper_32_bits(adev->wb.gpu_addr +
-				adev->virt.reg_val_offs * 4));
+				adev->gfx.kiq.reg_val_offs * 4));
 }
 
 static void gfx_v8_0_ring_emit_wreg(struct amdgpu_ring *ring, uint32_t reg,
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index a0c04b0c..f2054c8 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -686,7 +686,7 @@ static int gfx_v9_0_kiq_init_ring(struct amdgpu_device *adev,
 	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
 	int r = 0;
 
-	r = amdgpu_wb_get(adev, &adev->virt.reg_val_offs);
+	r = amdgpu_wb_get(adev, &kiq->reg_val_offs);
 	if (r)
 		return r;
 
@@ -712,7 +712,7 @@ static int gfx_v9_0_kiq_init_ring(struct amdgpu_device *adev,
 static void gfx_v9_0_kiq_free_ring(struct amdgpu_ring *ring,
 				   struct amdgpu_irq_src *irq)
 {
-	amdgpu_wb_free(ring->adev, ring->adev->virt.reg_val_offs);
+	amdgpu_wb_free(ring->adev, ring->adev->gfx.kiq.reg_val_offs);
 	amdgpu_ring_fini(ring);
 }
 
@@ -3291,9 +3291,9 @@ static void gfx_v9_0_ring_emit_rreg(struct amdgpu_ring *ring, uint32_t reg)
 	amdgpu_ring_write(ring, reg);
 	amdgpu_ring_write(ring, 0);
 	amdgpu_ring_write(ring, lower_32_bits(adev->wb.gpu_addr +
-				adev->virt.reg_val_offs * 4));
+				adev->gfx.kiq.reg_val_offs * 4));
 	amdgpu_ring_write(ring, upper_32_bits(adev->wb.gpu_addr +
-				adev->virt.reg_val_offs * 4));
+				adev->gfx.kiq.reg_val_offs * 4));
 }
 
 static void gfx_v9_0_ring_emit_wreg(struct amdgpu_ring *ring, uint32_t reg,
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a spinlock

[Andres Rodriguez]

First step towards enabling kiq register operations from an interrupt
handler

Signed-off-by: Andres Rodriguez <andresx7@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a spinlock

[Liu, Monk]

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, spin_lock is forbidden
and why you want to use spinlock instead of mutex, please give the explanation 

BR Monk

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Andres Rodriguez
Sent: Saturday, May 06, 2017 1:10 AM
To: amd-gfx@lists.freedesktop.org
Cc: andresx7@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@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a spinlock

[Liu, Monk]

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 ....
2, Don't submit CP/hw blocking jobs on KIQ since they will prevent world switch,

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 ?


BR
-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Liu, Monk
Sent: Monday, May 08, 2017 1:59 PM
To: Andres Rodriguez <andresx7@gmail.com>; amd-gfx@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, spin_lock is forbidden and why you want to use spinlock instead of mutex, please give the explanation 

BR Monk

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf Of Andres Rodriguez
Sent: Saturday, May 06, 2017 1:10 AM
To: amd-gfx@lists.freedesktop.org
Cc: andresx7@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@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a spinlock

[Andres Rodriguez]

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@lists.freedesktop.org] On Behalf Of Liu, Monk
> Sent: Monday, May 08, 2017 1:59 PM
> To: Andres Rodriguez <andresx7@gmail.com>; amd-gfx@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@lists.freedesktop.org] On Behalf Of Andres Rodriguez
> Sent: Saturday, May 06, 2017 1:10 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: andresx7@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a spinlock

[Liu, Monk]

>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 .

THANKS & BR

-----Original Message-----
From: Andres Rodriguez [mailto:andresr@valvesoftware.com] 
Sent: Monday, May 08, 2017 11:42 PM
To: Liu, Monk <Monk.Liu@amd.com>; Andres Rodriguez <andresx7@gmail.com>; amd-gfx@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@lists.freedesktop.org] On Behalf 
> Of Liu, Monk
> Sent: Monday, May 08, 2017 1:59 PM
> To: Andres Rodriguez <andresx7@gmail.com>; 
> amd-gfx@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@lists.freedesktop.org] On Behalf 
> Of Andres Rodriguez
> Sent: Saturday, May 06, 2017 1:10 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: andresx7@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/8] drm/amdgpu: convert kiq ring_mutex to a spinlock

[Andres Rodriguez]

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@valvesoftware.com]
> Sent: Monday, May 08, 2017 11:42 PM
> To: Liu, Monk <Monk.Liu@amd.com>; Andres Rodriguez <andresx7@gmail.com>; amd-gfx@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@lists.freedesktop.org] On Behalf
>> Of Liu, Monk
>> Sent: Monday, May 08, 2017 1:59 PM
>> To: Andres Rodriguez <andresx7@gmail.com>;
>> amd-gfx@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@lists.freedesktop.org] On Behalf
>> Of Andres Rodriguez
>> Sent: Saturday, May 06, 2017 1:10 AM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: andresx7@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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/8] drm/amdgpu: add option to force register ops through kiq

[Andres Rodriguez]

Use the [WR]REG32_KIQ() macros when a register operation should go
through the kiq.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 3 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 1d987e3..ca040e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1682,9 +1682,12 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v);
 
 #define AMDGPU_REGS_IDX       (1<<0)
 #define AMDGPU_REGS_NO_KIQ    (1<<1)
+#define AMDGPU_REGS_FORCE_KIQ (1<<2)
 
 #define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg), AMDGPU_REGS_NO_KIQ)
 #define WREG32_NO_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
+#define RREG32_KIQ(reg) amdgpu_mm_rreg(adev, (reg), AMDGPU_REGS_FORCE_KIQ)
+#define WREG32_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_FORCE_KIQ)
 
 #define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0)
 #define RREG32_IDX(reg) amdgpu_mm_rreg(adev, (reg), AMDGPU_REGS_IDX)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index ce22f04..6584d7e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -97,7 +97,8 @@ uint32_t amdgpu_mm_rreg(struct amdgpu_device *adev, uint32_t reg,
 {
 	uint32_t ret;
 
-	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
+	if ((!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
+			|| acc_flags & AMDGPU_REGS_FORCE_KIQ) {
 		BUG_ON(in_interrupt());
 		return amdgpu_kiq_rreg(adev, reg);
 	}
@@ -121,7 +122,8 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
 {
 	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
 
-	if (!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev)) {
+	if ((!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
+			|| acc_flags & AMDGPU_REGS_FORCE_KIQ) {
 		BUG_ON(in_interrupt());
 		return amdgpu_kiq_wreg(adev, reg, v);
 	}
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/8] drm/amdgpu: add macro for asynchronous KIQ register writes

[Andres Rodriguez]

Add WREG32_KIQ_ASYNC() to allow for a caller to perform a register write
without waiting for the result to be commited.

This allows us to queue register writes from a context that cannot
sleep.

It may also be useful for long sequences of register writes performed
through the kiq. Where only the last write actually needs to be
synchronized.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  5 ++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 26 ++++++++++++++------------
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index ca040e6..50bfead 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1669,7 +1669,8 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
 u32 amdgpu_io_rreg(struct amdgpu_device *adev, u32 reg);
 void amdgpu_io_wreg(struct amdgpu_device *adev, u32 reg, u32 v);
 uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg);
-void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v);
+void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
+		     uint32_t acc_flags);
 
 u32 amdgpu_mm_rdoorbell(struct amdgpu_device *adev, u32 index);
 void amdgpu_mm_wdoorbell(struct amdgpu_device *adev, u32 index, u32 v);
@@ -1683,11 +1684,13 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v);
 #define AMDGPU_REGS_IDX       (1<<0)
 #define AMDGPU_REGS_NO_KIQ    (1<<1)
 #define AMDGPU_REGS_FORCE_KIQ (1<<2)
+#define AMDGPU_REGS_KIQ_ASYNC (1<<3)
 
 #define RREG32_NO_KIQ(reg) amdgpu_mm_rreg(adev, (reg), AMDGPU_REGS_NO_KIQ)
 #define WREG32_NO_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_NO_KIQ)
 #define RREG32_KIQ(reg) amdgpu_mm_rreg(adev, (reg), AMDGPU_REGS_FORCE_KIQ)
 #define WREG32_KIQ(reg, v) amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_FORCE_KIQ)
+#define WREG32_KIQ_ASYNC(reg, v) amdgpu_mm_wreg(adev, (reg), (v), AMDGPU_REGS_FORCE_KIQ | AMDGPU_REGS_KIQ_ASYNC)
 
 #define RREG32(reg) amdgpu_mm_rreg(adev, (reg), 0)
 #define RREG32_IDX(reg) amdgpu_mm_rreg(adev, (reg), AMDGPU_REGS_IDX)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6584d7e..3469b9d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -123,10 +123,8 @@ void amdgpu_mm_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
 	trace_amdgpu_mm_wreg(adev->pdev->device, reg, v);
 
 	if ((!(acc_flags & AMDGPU_REGS_NO_KIQ) && amdgpu_sriov_runtime(adev))
-			|| acc_flags & AMDGPU_REGS_FORCE_KIQ) {
-		BUG_ON(in_interrupt());
-		return amdgpu_kiq_wreg(adev, reg, v);
-	}
+			|| acc_flags & AMDGPU_REGS_FORCE_KIQ)
+		return amdgpu_kiq_wreg(adev, reg, v, acc_flags);
 
 	if ((reg * 4) < adev->rmmio_size && !(acc_flags & AMDGPU_REGS_IDX))
 		writel(v, ((void __iomem *)adev->rmmio) + (reg * 4));
@@ -188,10 +186,11 @@ uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
 	return val;
 }
 
-void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
+void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v,
+		     uint32_t acc_flags)
 {
 	signed long r;
-	struct dma_fence *f;
+	struct dma_fence *f = NULL;
 	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
 	struct amdgpu_ring *ring = &kiq->ring;
 
@@ -200,17 +199,20 @@ void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
 	spin_lock(&kiq->ring_lock);
 	amdgpu_ring_alloc(ring, 32);
 	amdgpu_ring_emit_wreg(ring, reg, v);
-	amdgpu_fence_emit(ring, &f);
+	if (!(acc_flags & AMDGPU_REGS_KIQ_ASYNC))
+		amdgpu_fence_emit(ring, &f);
 	amdgpu_ring_commit(ring);
 	spin_unlock(&kiq->ring_lock);
 
-	r = dma_fence_wait(f, false);
-	if (r)
-		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
-	dma_fence_put(f);
+	if (f) {
+		BUG_ON(in_interrupt());
+		r = dma_fence_wait(f, false);
+		if (r)
+			DRM_ERROR("wait for kiq fence error: %ld.\n", r);
+		dma_fence_put(f);
+	}
 }
 
-
 /**
  * amdgpu_mm_rdoorbell - read a doorbell dword
  *
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 7/8] drm/amdgpu/vi: add mechanism to edit the kiq srbm window

[Andres Rodriguez]

The kiq's srbm window is independent of the MMIO srbm window.

Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  1 +
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c      | 34 +++++++++++++++---------------
 drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c     |  4 ++--
 drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c     |  4 ++--
 drivers/gpu/drm/amd/amdgpu/vi.c            |  9 ++++++--
 drivers/gpu/drm/amd/amdgpu/vi.h            |  2 +-
 7 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 50bfead..9ffe1d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -942,6 +942,7 @@ struct amdgpu_kiq {
 	struct amdgpu_irq_src	irq;
 	uint32_t		reg_val_offs;
 	spinlock_t		ring_lock;
+	spinlock_t		srbm_lock;
 };
 
 /*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 3469b9d..4c8ec52 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1925,6 +1925,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	spin_lock_init(&adev->audio_endpt_idx_lock);
 	spin_lock_init(&adev->mm_stats.lock);
 	spin_lock_init(&adev->gfx.kiq.ring_lock);
+	spin_lock_init(&adev->gfx.kiq.srbm_lock);
 
 	INIT_LIST_HEAD(&adev->shadow_list);
 	mutex_init(&adev->shadow_list_lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 976ee0b..be40fc9 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -3948,14 +3948,14 @@ static void gfx_v8_0_init_compute_vmid(struct amdgpu_device *adev)
 
 	mutex_lock(&adev->srbm_mutex);
 	for (i = FIRST_COMPUTE_VMID; i < LAST_COMPUTE_VMID; i++) {
-		vi_srbm_select(adev, 0, 0, 0, i);
+		vi_srbm_select(adev, 0, 0, 0, i, false);
 		/* CP and shaders */
 		WREG32(mmSH_MEM_CONFIG, sh_mem_config);
 		WREG32(mmSH_MEM_APE1_BASE, 1);
 		WREG32(mmSH_MEM_APE1_LIMIT, 0);
 		WREG32(mmSH_MEM_BASES, sh_mem_bases);
 	}
-	vi_srbm_select(adev, 0, 0, 0, 0);
+	vi_srbm_select(adev, 0, 0, 0, 0, false);
 	mutex_unlock(&adev->srbm_mutex);
 }
 
@@ -3997,7 +3997,7 @@ static void gfx_v8_0_gpu_init(struct amdgpu_device *adev)
 				   INDEX_STRIDE, 3);
 	mutex_lock(&adev->srbm_mutex);
 	for (i = 0; i < adev->vm_manager.id_mgr[0].num_ids; i++) {
-		vi_srbm_select(adev, 0, 0, 0, i);
+		vi_srbm_select(adev, 0, 0, 0, i, false);
 		/* CP and shaders */
 		if (i == 0) {
 			tmp = REG_SET_FIELD(0, SH_MEM_CONFIG, DEFAULT_MTYPE, MTYPE_UC);
@@ -4020,7 +4020,7 @@ static void gfx_v8_0_gpu_init(struct amdgpu_device *adev)
 		WREG32(mmSH_MEM_APE1_LIMIT, 0);
 		WREG32(mmSH_STATIC_MEM_CONFIG, sh_static_mem_cfg);
 	}
-	vi_srbm_select(adev, 0, 0, 0, 0);
+	vi_srbm_select(adev, 0, 0, 0, 0, false);
 	mutex_unlock(&adev->srbm_mutex);
 
 	gfx_v8_0_init_compute_vmid(adev);
@@ -5098,18 +5098,18 @@ static int gfx_v8_0_kiq_init_queue(struct amdgpu_ring *ring)
 		ring->wptr = 0;
 		amdgpu_ring_clear_ring(ring);
 		mutex_lock(&adev->srbm_mutex);
-		vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
+		vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0, false);
 		r = gfx_v8_0_deactivate_hqd(adev, 1);
 		if (r) {
 			dev_err(adev->dev, "failed to deactivate ring %s\n", ring->name);
 			goto out_unlock;
 		}
 		gfx_v8_0_mqd_commit(adev, mqd);
-		vi_srbm_select(adev, 0, 0, 0, 0);
+		vi_srbm_select(adev, 0, 0, 0, 0, false);
 		mutex_unlock(&adev->srbm_mutex);
 	} else {
 		mutex_lock(&adev->srbm_mutex);
-		vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
+		vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0, false);
 		gfx_v8_0_mqd_init(ring);
 		r = gfx_v8_0_deactivate_hqd(adev, 1);
 		if (r) {
@@ -5117,7 +5117,7 @@ static int gfx_v8_0_kiq_init_queue(struct amdgpu_ring *ring)
 			goto out_unlock;
 		}
 		gfx_v8_0_mqd_commit(adev, mqd);
-		vi_srbm_select(adev, 0, 0, 0, 0);
+		vi_srbm_select(adev, 0, 0, 0, 0, false);
 		mutex_unlock(&adev->srbm_mutex);
 
 		if (adev->gfx.mec.mqd_backup[mqd_idx])
@@ -5127,7 +5127,7 @@ static int gfx_v8_0_kiq_init_queue(struct amdgpu_ring *ring)
 	return r;
 
 out_unlock:
-	vi_srbm_select(adev, 0, 0, 0, 0);
+	vi_srbm_select(adev, 0, 0, 0, 0, false);
 	mutex_unlock(&adev->srbm_mutex);
 	return r;
 }
@@ -5140,9 +5140,9 @@ static int gfx_v8_0_kcq_init_queue(struct amdgpu_ring *ring)
 
 	if (!adev->gfx.in_reset && !adev->gfx.in_suspend) {
 		mutex_lock(&adev->srbm_mutex);
-		vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
+		vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0, false);
 		gfx_v8_0_mqd_init(ring);
-		vi_srbm_select(adev, 0, 0, 0, 0);
+		vi_srbm_select(adev, 0, 0, 0, 0, false);
 		mutex_unlock(&adev->srbm_mutex);
 
 		if (adev->gfx.mec.mqd_backup[mqd_idx])
@@ -5473,9 +5473,9 @@ static int gfx_v8_0_pre_soft_reset(void *handle)
 			struct amdgpu_ring *ring = &adev->gfx.compute_ring[i];
 
 			mutex_lock(&adev->srbm_mutex);
-			vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
+			vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0, false);
 			gfx_v8_0_deactivate_hqd(adev, 2);
-			vi_srbm_select(adev, 0, 0, 0, 0);
+			vi_srbm_select(adev, 0, 0, 0, 0, false);
 			mutex_unlock(&adev->srbm_mutex);
 		}
 		/* Disable MEC parsing/prefetching */
@@ -5573,9 +5573,9 @@ static int gfx_v8_0_post_soft_reset(void *handle)
 			struct amdgpu_ring *ring = &adev->gfx.compute_ring[i];
 
 			mutex_lock(&adev->srbm_mutex);
-			vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
+			vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0, false);
 			gfx_v8_0_deactivate_hqd(adev, 2);
-			vi_srbm_select(adev, 0, 0, 0, 0);
+			vi_srbm_select(adev, 0, 0, 0, 0, false);
 			mutex_unlock(&adev->srbm_mutex);
 		}
 		gfx_v8_0_kiq_resume(adev);
@@ -6817,12 +6817,12 @@ static void gfx_v8_0_set_compute_eop_interrupt_state(struct amdgpu_device *adev,
 	}
 
 	mutex_lock(&adev->srbm_mutex);
-	vi_srbm_select(adev, me, pipe, 0, 0);
+	vi_srbm_select(adev, me, pipe, 0, 0, false);
 
 	WREG32_FIELD(CPC_INT_CNTL, TIME_STAMP_INT_ENABLE,
 			state == AMDGPU_IRQ_STATE_DISABLE ? 0 : 1);
 
-	vi_srbm_select(adev, 0, 0, 0, 0);
+	vi_srbm_select(adev, 0, 0, 0, 0, false);
 	mutex_unlock(&adev->srbm_mutex);
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
index f2d0710..0d0e432 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
@@ -424,12 +424,12 @@ static int sdma_v2_4_gfx_resume(struct amdgpu_device *adev)
 
 		mutex_lock(&adev->srbm_mutex);
 		for (j = 0; j < 16; j++) {
-			vi_srbm_select(adev, 0, 0, 0, j);
+			vi_srbm_select(adev, 0, 0, 0, j, false);
 			/* SDMA GFX */
 			WREG32(mmSDMA0_GFX_VIRTUAL_ADDR + sdma_offsets[i], 0);
 			WREG32(mmSDMA0_GFX_APE1_CNTL + sdma_offsets[i], 0);
 		}
-		vi_srbm_select(adev, 0, 0, 0, 0);
+		vi_srbm_select(adev, 0, 0, 0, 0, false);
 		mutex_unlock(&adev->srbm_mutex);
 
 		WREG32(mmSDMA0_TILING_CONFIG + sdma_offsets[i],
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
index a69e5d4..05924a4 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
@@ -618,12 +618,12 @@ static int sdma_v3_0_gfx_resume(struct amdgpu_device *adev)
 
 		mutex_lock(&adev->srbm_mutex);
 		for (j = 0; j < 16; j++) {
-			vi_srbm_select(adev, 0, 0, 0, j);
+			vi_srbm_select(adev, 0, 0, 0, j, false);
 			/* SDMA GFX */
 			WREG32(mmSDMA0_GFX_VIRTUAL_ADDR + sdma_offsets[i], 0);
 			WREG32(mmSDMA0_GFX_APE1_CNTL + sdma_offsets[i], 0);
 		}
-		vi_srbm_select(adev, 0, 0, 0, 0);
+		vi_srbm_select(adev, 0, 0, 0, 0, false);
 		mutex_unlock(&adev->srbm_mutex);
 
 		WREG32(mmSDMA0_TILING_CONFIG + sdma_offsets[i],
diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
index 8217551..d9b7774 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.c
+++ b/drivers/gpu/drm/amd/amdgpu/vi.c
@@ -349,20 +349,25 @@ static u32 vi_get_xclk(struct amdgpu_device *adev)
  * @pipe: pipe
  * @queue: queue
  * @vmid: VMID
+ * @kiq: edit MMIO or KIQ window
  *
  * Switches the currently active registers instances.  Some
  * registers are instanced per VMID, others are instanced per
  * me/pipe/queue combination.
  */
 void vi_srbm_select(struct amdgpu_device *adev,
-		     u32 me, u32 pipe, u32 queue, u32 vmid)
+		     u32 me, u32 pipe, u32 queue, u32 vmid, bool kiq)
 {
 	u32 srbm_gfx_cntl = 0;
 	srbm_gfx_cntl = REG_SET_FIELD(srbm_gfx_cntl, SRBM_GFX_CNTL, PIPEID, pipe);
 	srbm_gfx_cntl = REG_SET_FIELD(srbm_gfx_cntl, SRBM_GFX_CNTL, MEID, me);
 	srbm_gfx_cntl = REG_SET_FIELD(srbm_gfx_cntl, SRBM_GFX_CNTL, VMID, vmid);
 	srbm_gfx_cntl = REG_SET_FIELD(srbm_gfx_cntl, SRBM_GFX_CNTL, QUEUEID, queue);
-	WREG32(mmSRBM_GFX_CNTL, srbm_gfx_cntl);
+
+	if (kiq)
+		WREG32_KIQ_ASYNC(mmSRBM_GFX_CNTL, srbm_gfx_cntl);
+	else
+		WREG32(mmSRBM_GFX_CNTL, srbm_gfx_cntl);
 }
 
 static void vi_vga_set_state(struct amdgpu_device *adev, bool state)
diff --git a/drivers/gpu/drm/amd/amdgpu/vi.h b/drivers/gpu/drm/amd/amdgpu/vi.h
index 575d7ae..f5f3acb 100644
--- a/drivers/gpu/drm/amd/amdgpu/vi.h
+++ b/drivers/gpu/drm/amd/amdgpu/vi.h
@@ -25,7 +25,7 @@
 #define __VI_H__
 
 void vi_srbm_select(struct amdgpu_device *adev,
-		    u32 me, u32 pipe, u32 queue, u32 vmid);
+		    u32 me, u32 pipe, u32 queue, u32 vmid, bool kiq);
 int vi_set_ip_blocks(struct amdgpu_device *adev);
 
 #endif
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 8/8] drm/amdgpu: implement ring set_priority for gfx_v8 compute v7

[Andres Rodriguez]

Programming CP_HQD_QUEUE_PRIORITY enables a queue to take priority over
other queues on the same pipe. Multiple queues on a pipe are timesliced
so this gives us full precedence over other queues.

Programming CP_HQD_PIPE_PRIORITY changes the SPI_ARB_PRIORITY of the
wave as follows:
        0x2: CS_H
        0x1: CS_M
        0x0: CS_L

The SPI block will then dispatch work according to the policy set by
SPI_ARB_PRIORITY. In the current policy CS_H is higher priority than
gfx.

In order to prevent getting stuck in loops of CUs bouncing between GFX
and high priority compute and introducing further latency, we reserve
CUs 2+ for high priority compute on-demand.

v2: fix srbm_select to ring->queue and use ring->funcs->type
v3: use AMD_SCHED_PRIORITY_* instead of AMDGPU_CTX_PRIORITY_*
v4: switch int to enum amd_sched_priority
v5: corresponding changes for srbm_lock
v6: change CU reservation to PIPE_PERCENT allocation
v7: use kiq instead of MMIO

Acked-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Andres Rodriguez <andresx7@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |   4 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |   2 +
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c      | 117 +++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 9ffe1d3..1510a78 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1100,6 +1100,10 @@ struct amdgpu_gfx {
 	bool                            in_suspend;
 	/* NGG */
 	struct amdgpu_ngg		ngg;
+
+	/* pipe reservation */
+	spinlock_t			pipe_reserve_lock;
+	DECLARE_BITMAP			(pipe_reserve_bitmap, AMDGPU_MAX_COMPUTE_QUEUES);
 };
 
 int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm *vm,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 4c8ec52..69df11e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1880,6 +1880,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	adev->vm_manager.vm_pte_num_rings = 0;
 	adev->gart.gart_funcs = NULL;
 	adev->fence_context = dma_fence_context_alloc(AMDGPU_MAX_RINGS);
+	bitmap_zero(adev->gfx.pipe_reserve_bitmap, AMDGPU_MAX_COMPUTE_QUEUES);
 
 	adev->smc_rreg = &amdgpu_invalid_rreg;
 	adev->smc_wreg = &amdgpu_invalid_wreg;
@@ -1926,6 +1927,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	spin_lock_init(&adev->mm_stats.lock);
 	spin_lock_init(&adev->gfx.kiq.ring_lock);
 	spin_lock_init(&adev->gfx.kiq.srbm_lock);
+	spin_lock_init(&adev->gfx.pipe_reserve_lock);
 
 	INIT_LIST_HEAD(&adev->shadow_list);
 	mutex_init(&adev->shadow_list_lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index be40fc9..a043bff 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -53,6 +53,10 @@
 
 #define GFX8_NUM_GFX_RINGS     1
 #define GFX8_MEC_HPD_SIZE 2048
+#define GFX8_CU_RESERVE_RESOURCES 0x45888
+#define GFX8_CU_NUM 8
+#define GFX8_UNRESERVED_CU_NUM 2
+#define GFX8_CU_RESERVE_PIPE_SHIFT 7
 
 #define TOPAZ_GB_ADDR_CONFIG_GOLDEN 0x22010001
 #define CARRIZO_GB_ADDR_CONFIG_GOLDEN 0x22010001
@@ -6649,6 +6653,118 @@ static void gfx_v8_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
 	WDOORBELL32(ring->doorbell_index, lower_32_bits(ring->wptr));
 }
 
+static void gfx_v8_0_ring_set_pipe_percent(struct amdgpu_ring *ring,
+					   bool acquire)
+{
+	struct amdgpu_device *adev = ring->adev;
+	int pipe_num, tmp, reg;
+	int pipe_percent = acquire ? SPI_WCL_PIPE_PERCENT_GFX__VALUE_MASK : 0x1;
+
+	pipe_num = ring->me * adev->gfx.mec.num_pipe_per_mec + ring->pipe;
+
+	/* first me only has 2 entries, GFX and HP3D */
+	if (ring->me > 0)
+		pipe_num -= 2;
+
+	/* There is a bug in the GFX pipes that results in a HS
+	 * deadlock if the pipe is restricted to a percentage
+	 * lower than 17 */
+	if (ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE && pipe_percent < 17)
+		pipe_percent = 17;
+
+	reg = mmSPI_WCL_PIPE_PERCENT_GFX + pipe_num;
+	tmp = RREG32(reg);
+	tmp = REG_SET_FIELD(tmp, SPI_WCL_PIPE_PERCENT_GFX, VALUE, pipe_percent);
+	WREG32_KIQ_ASYNC(reg, tmp);
+}
+
+static void gfx_v8_0_pipe_reserve_resources(struct amdgpu_device *adev,
+					    struct amdgpu_ring *ring,
+					    bool acquire)
+{
+	int i, pipe;
+	bool reserve;
+	struct amdgpu_ring *iring;
+
+	spin_lock(&adev->gfx.pipe_reserve_lock);
+	pipe = amdgpu_queue_to_bit(adev, ring->me, ring->pipe, 0);
+	if (acquire)
+		set_bit(pipe, adev->gfx.pipe_reserve_bitmap);
+	else
+		clear_bit(pipe, adev->gfx.pipe_reserve_bitmap);
+
+	if (!bitmap_weight(adev->gfx.pipe_reserve_bitmap, AMDGPU_MAX_COMPUTE_QUEUES)) {
+		/* Clear all reservations - everyone reacquires all resources */
+		for (i = 0; i < adev->gfx.num_gfx_rings; ++i)
+			gfx_v8_0_ring_set_pipe_percent(&adev->gfx.gfx_ring[i],
+						       true);
+
+		for (i = 0; i < adev->gfx.num_compute_rings; ++i)
+			gfx_v8_0_ring_set_pipe_percent(&adev->gfx.compute_ring[i],
+						       true);
+	} else {
+		/* Lower all pipes without a current reservation */
+		for (i = 0; i < adev->gfx.num_gfx_rings; ++i) {
+			iring = &adev->gfx.gfx_ring[i];
+			pipe = amdgpu_queue_to_bit(adev,
+						   iring->me,
+						   iring->pipe,
+						   0);
+			reserve = test_bit(pipe, adev->gfx.pipe_reserve_bitmap);
+			gfx_v8_0_ring_set_pipe_percent(iring, reserve);
+		}
+
+		for (i = 0; i < adev->gfx.num_compute_rings; ++i) {
+			iring = &adev->gfx.compute_ring[i];
+			pipe = amdgpu_queue_to_bit(adev,
+						   iring->me,
+						   iring->pipe,
+						   0);
+			reserve = test_bit(pipe, adev->gfx.pipe_reserve_bitmap);
+			gfx_v8_0_ring_set_pipe_percent(iring, reserve);
+		}
+	}
+
+	spin_unlock(&adev->gfx.pipe_reserve_lock);
+}
+
+static void gfx_v8_0_hqd_set_priority(struct amdgpu_device *adev,
+				      struct amdgpu_ring *ring,
+				      enum amd_sched_priority priority)
+{
+	spin_lock(&adev->gfx.kiq.srbm_lock);
+	vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0, true);
+
+	switch (priority) {
+	case AMD_SCHED_PRIORITY_NORMAL:
+		WREG32_KIQ_ASYNC(mmCP_HQD_PIPE_PRIORITY, 0x0);
+		WREG32_KIQ_ASYNC(mmCP_HQD_QUEUE_PRIORITY, 0x0);
+		break;
+	case AMD_SCHED_PRIORITY_HIGH:
+		WREG32_KIQ_ASYNC(mmCP_HQD_PIPE_PRIORITY, 0x2);
+		WREG32_KIQ_ASYNC(mmCP_HQD_QUEUE_PRIORITY, 0xf);
+		break;
+	default:
+		WARN(1, "Attempt to set invalid SPI priority:%d for ring:%d\n",
+				priority, ring->idx);
+		break;
+	}
+
+	vi_srbm_select(adev, 0, 0, 0, 0, true);
+	spin_unlock(&adev->gfx.kiq.srbm_lock);
+}
+static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
+					       enum amd_sched_priority priority)
+{
+	struct amdgpu_device *adev = ring->adev;
+
+	if (ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE)
+		return;
+
+	gfx_v8_0_hqd_set_priority(adev, ring, priority);
+	gfx_v8_0_pipe_reserve_resources(adev, ring, priority >= AMD_SCHED_PRIORITY_HIGH);
+}
+
 static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
 					     u64 addr, u64 seq,
 					     unsigned flags)
@@ -7075,6 +7191,7 @@ static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_compute = {
 	.test_ib = gfx_v8_0_ring_test_ib,
 	.insert_nop = amdgpu_ring_insert_nop,
 	.pad_ib = amdgpu_ring_generic_pad_ib,
+	.set_priority = gfx_v8_0_ring_set_priority_compute,
 };
 
 static const struct amdgpu_ring_funcs gfx_v8_0_ring_funcs_kiq = {
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC] Problems with SRBM select on KIQ

[Alex Deucher]

On Fri, May 5, 2017 at 1:10 PM, Andres Rodriguez <andresx7@gmail.com> wrote:
> Hey Everyone,
>
> On one of the previous comments for this series I received feedback that
> the register programming should be done through the KIQ. This series has
> the relevant changes.
>
> However, register writes that require an srbm_select are not working
> correctly. The effect is as if the write never happened.
>
> Maybe I'm just doing something silly or dumb in here, but I can't really
> see it. Possibly the way I try to perform the srbm_select for KIQ?
>
> Any feedback on this issue is appreciated.

I think to make this work, you'd probably need to do is unmap the
queue, update the priority in the mqd, and then map it again.  It's
probably more efficient to use runlists, but that is a lot of work.

Alex
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC] Problems with SRBM select on KIQ

[Felix Kuehling]

We ran into a similar problem when we played with priorities on KFD
queues. You can't change an MQD of a currently mapped queue. To change a
queue priority we need to unmap it, update the MQD, and then map it again.

I wonder if a queue can change its own priority by using register write
commands in the queue itself.

Regards,
  Felix


On 17-05-05 06:40 PM, Alex Deucher wrote:
> On Fri, May 5, 2017 at 1:10 PM, Andres Rodriguez <andresx7@gmail.com> wrote:
>> Hey Everyone,
>>
>> On one of the previous comments for this series I received feedback that
>> the register programming should be done through the KIQ. This series has
>> the relevant changes.
>>
>> However, register writes that require an srbm_select are not working
>> correctly. The effect is as if the write never happened.
>>
>> Maybe I'm just doing something silly or dumb in here, but I can't really
>> see it. Possibly the way I try to perform the srbm_select for KIQ?
>>
>> Any feedback on this issue is appreciated.
> I think to make this work, you'd probably need to do is unmap the
> queue, update the priority in the mqd, and then map it again.  It's
> probably more efficient to use runlists, but that is a lot of work.
>
> Alex
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC] Problems with SRBM select on KIQ

[Andres Rodriguez]

This works perfectly fine if I use MMIO writes. But KIQ breaks when done 
over KIQ, which is why I find it weird.

Regards,
Andres

On 2017-05-05 06:57 PM, Felix Kuehling wrote:
> We ran into a similar problem when we played with priorities on KFD
> queues. You can't change an MQD of a currently mapped queue. To change a
> queue priority we need to unmap it, update the MQD, and then map it again.
>
> I wonder if a queue can change its own priority by using register write
> commands in the queue itself.
>
> Regards,
>    Felix
>
>
> On 17-05-05 06:40 PM, Alex Deucher wrote:
>> On Fri, May 5, 2017 at 1:10 PM, Andres Rodriguez <andresx7@gmail.com> wrote:
>>> Hey Everyone,
>>>
>>> On one of the previous comments for this series I received feedback that
>>> the register programming should be done through the KIQ. This series has
>>> the relevant changes.
>>>
>>> However, register writes that require an srbm_select are not working
>>> correctly. The effect is as if the write never happened.
>>>
>>> Maybe I'm just doing something silly or dumb in here, but I can't really
>>> see it. Possibly the way I try to perform the srbm_select for KIQ?
>>>
>>> Any feedback on this issue is appreciated.
>> I think to make this work, you'd probably need to do is unmap the
>> queue, update the priority in the mqd, and then map it again.  It's
>> probably more efficient to use runlists, but that is a lot of work.
>>
>> Alex
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [RFC] Problems with SRBM select on KIQ

[zhoucm1]

On 2017年05月06日 06:57, Felix Kuehling wrote:
> We ran into a similar problem when we played with priorities on KFD
> queues. You can't change an MQD of a currently mapped queue. To change a
> queue priority we need to unmap it, update the MQD, and then map it again.
I wonder if there is similar requirement when engine soft reset happens.
We encounter the issue that KIQ doesn't work any more after gfx/compute 
engine soft reset.

Regards,
David Zhou
>
> I wonder if a queue can change its own priority by using register write
> commands in the queue itself.
>
> Regards,
>    Felix
>
>
> On 17-05-05 06:40 PM, Alex Deucher wrote:
>> On Fri, May 5, 2017 at 1:10 PM, Andres Rodriguez <andresx7@gmail.com> wrote:
>>> Hey Everyone,
>>>
>>> On one of the previous comments for this series I received feedback that
>>> the register programming should be done through the KIQ. This series has
>>> the relevant changes.
>>>
>>> However, register writes that require an srbm_select are not working
>>> correctly. The effect is as if the write never happened.
>>>
>>> Maybe I'm just doing something silly or dumb in here, but I can't really
>>> see it. Possibly the way I try to perform the srbm_select for KIQ?
>>>
>>> Any feedback on this issue is appreciated.
>> I think to make this work, you'd probably need to do is unmap the
>> queue, update the priority in the mqd, and then map it again.  It's
>> probably more efficient to use runlists, but that is a lot of work.
>>
>> Alex
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx