* [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
@ 2020-03-03 12:50 Nirmoy Das
2020-03-03 12:50 ` [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list Nirmoy Das
` (5 more replies)
0 siblings, 6 replies; 26+ messages in thread
From: Nirmoy Das @ 2020-03-03 12:50 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
We were changing compute ring priority while rings were being used
before every job submission which is not recommended. This patch
sets compute queue priority at mqd initialization for gfx8, gfx9 and
gfx10.
Policy: make queue 0 of each pipe as high priority compute queue
High/normal priority compute sched lists are generated from set of high/normal
priority compute queues. At context creation, entity of compute queue
get a sched list from high or normal priority depending on ctx->priority
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 --
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 60 +++++++++++++++++++++---
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 8 ++++
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 15 +++++-
drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 ---
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 +
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 19 ++++++++
drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 23 +++++++--
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 20 ++++++++
9 files changed, 135 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index f397ff97b4e4..8304d0c87899 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1205,7 +1205,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
struct drm_sched_entity *entity = p->entity;
enum drm_sched_priority priority;
- struct amdgpu_ring *ring;
struct amdgpu_bo_list_entry *e;
struct amdgpu_job *job;
uint64_t seq;
@@ -1258,9 +1257,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
priority = job->base.s_priority;
drm_sched_entity_push_job(&job->base, entity);
- ring = to_amdgpu_ring(entity->rq->sched);
- amdgpu_ring_priority_get(ring, priority);
-
amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);
ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 94a6c42f29ea..4ad944f85672 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -61,12 +61,30 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
return -EACCES;
}
+static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_compute_prio(enum drm_sched_priority prio)
+{
+ switch(prio) {
+ case DRM_SCHED_PRIORITY_MIN:
+ case DRM_SCHED_PRIORITY_NORMAL:
+ case DRM_SCHED_PRIORITY_HIGH_SW:
+ return AMDGPU_GFX_PIPE_PRIO_NORMAL;
+ case DRM_SCHED_PRIORITY_HIGH_HW:
+ case DRM_SCHED_PRIORITY_KERNEL:
+ return AMDGPU_GFX_PIPE_PRIO_HIGH;
+ default:
+ return AMDGPU_GFX_PIPE_PRIO_NORMAL;
+ }
+
+ return AMDGPU_GFX_PIPE_PRIO_NORMAL;
+}
+
static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const u32 ring)
{
struct amdgpu_device *adev = ctx->adev;
struct amdgpu_ctx_entity *entity;
struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
unsigned num_scheds = 0;
+ enum gfx_pipe_priority hw_prio;
enum drm_sched_priority priority;
int r;
@@ -85,8 +103,9 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const
num_scheds = 1;
break;
case AMDGPU_HW_IP_COMPUTE:
- scheds = adev->gfx.compute_sched;
- num_scheds = adev->gfx.num_compute_sched;
+ hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
+ scheds = adev->gfx.compute_prio_sched[hw_prio];
+ num_scheds = adev->gfx.num_compute_sched[hw_prio];
break;
case AMDGPU_HW_IP_DMA:
scheds = adev->sdma.sdma_sched;
@@ -628,20 +647,47 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
mutex_destroy(&mgr->lock);
}
+
+static void amdgpu_ctx_init_compute_sched(struct amdgpu_device *adev)
+{
+ int num_compute_sched_normal = 0;
+ int num_compute_sched_high = AMDGPU_MAX_COMPUTE_RINGS - 1;
+ int i;
+
+ /* fill compute_sched array as: start from 0th index for normal priority scheds and
+ * start from (last_index - num_compute_sched_normal) for high priority
+ * scheds */
+ for (i = 0; i < adev->gfx.num_compute_rings; i++) {
+ if (!adev->gfx.compute_ring[i].has_high_prio)
+ adev->gfx.compute_sched[num_compute_sched_normal++] =
+ &adev->gfx.compute_ring[i].sched;
+ else
+ adev->gfx.compute_sched[num_compute_sched_high--] =
+ &adev->gfx.compute_ring[i].sched;
+ }
+
+ /* compute ring only has two priority for now */
+ i = AMDGPU_GFX_PIPE_PRIO_NORMAL;
+ adev->gfx.compute_prio_sched[i] = &adev->gfx.compute_sched[0];
+ adev->gfx.num_compute_sched[i] = num_compute_sched_normal;
+
+ i = AMDGPU_GFX_PIPE_PRIO_HIGH;
+ adev->gfx.compute_prio_sched[i] =
+ &adev->gfx.compute_sched[num_compute_sched_high - 1];
+ adev->gfx.num_compute_sched[i] =
+ adev->gfx.num_compute_rings - num_compute_sched_normal;
+}
+
void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
{
int i, j;
+ amdgpu_ctx_init_compute_sched(adev);
for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
adev->gfx.gfx_sched[i] = &adev->gfx.gfx_ring[i].sched;
adev->gfx.num_gfx_sched++;
}
- for (i = 0; i < adev->gfx.num_compute_rings; i++) {
- adev->gfx.compute_sched[i] = &adev->gfx.compute_ring[i].sched;
- adev->gfx.num_compute_sched++;
- }
-
for (i = 0; i < adev->sdma.num_instances; i++) {
adev->sdma.sdma_sched[i] = &adev->sdma.instance[i].ring.sched;
adev->sdma.num_sdma_sched++;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 7403588684b3..952725e7243c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -192,6 +192,14 @@ static bool amdgpu_gfx_is_multipipe_capable(struct amdgpu_device *adev)
return adev->gfx.mec.num_mec > 1;
}
+bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
+ int queue)
+{
+ /* Policy: make queue 0 of each pipe as high priority compute queue */
+ return (queue == 0);
+
+}
+
void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev)
{
int i, queue, pipe, mec;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index 37ba05b63b2a..47a5cdae28c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -41,6 +41,16 @@
#define AMDGPU_MAX_GFX_QUEUES KGD_MAX_QUEUES
#define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
+enum gfx_pipe_priority {
+ AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
+ AMDGPU_GFX_PIPE_PRIO_HIGH,
+ AMDGPU_GFX_PIPE_PRIO_MAX
+
+};
+
+#define AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM 0
+#define AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM 15
+
struct amdgpu_mec {
struct amdgpu_bo *hpd_eop_obj;
u64 hpd_eop_gpu_addr;
@@ -280,8 +290,9 @@ struct amdgpu_gfx {
uint32_t num_gfx_sched;
unsigned num_gfx_rings;
struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
+ struct drm_gpu_scheduler **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
struct drm_gpu_scheduler *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
- uint32_t num_compute_sched;
+ uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
unsigned num_compute_rings;
struct amdgpu_irq_src eop_irq;
struct amdgpu_irq_src priv_reg_irq;
@@ -363,6 +374,8 @@ void amdgpu_gfx_bit_to_mec_queue(struct amdgpu_device *adev, int bit,
int *mec, int *pipe, int *queue);
bool amdgpu_gfx_is_mec_queue_enabled(struct amdgpu_device *adev, int mec,
int pipe, int queue);
+bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
+ int queue);
int amdgpu_gfx_me_queue_to_bit(struct amdgpu_device *adev, int me,
int pipe, int queue);
void amdgpu_gfx_bit_to_me_queue(struct amdgpu_device *adev, int bit,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index d42be880a236..4981e443a884 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -117,12 +117,10 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
{
- struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
struct amdgpu_job *job = to_amdgpu_job(s_job);
drm_sched_job_cleanup(s_job);
- amdgpu_ring_priority_put(ring, s_job->s_priority);
dma_fence_put(job->fence);
amdgpu_sync_free(&job->sync);
amdgpu_sync_free(&job->sched_sync);
@@ -143,7 +141,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
void *owner, struct dma_fence **f)
{
enum drm_sched_priority priority;
- struct amdgpu_ring *ring;
int r;
if (!f)
@@ -158,9 +155,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
priority = job->base.s_priority;
drm_sched_entity_push_job(&job->base, entity);
- ring = to_amdgpu_ring(entity->rq->sched);
- amdgpu_ring_priority_get(ring, priority);
-
return 0;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 24caff085d00..dcea1ef92883 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -222,6 +222,7 @@ struct amdgpu_ring {
struct mutex priority_mutex;
/* protected by priority_mutex */
int priority;
+ bool has_high_prio;
#if defined(CONFIG_DEBUG_FS)
struct dentry *ent;
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index 96cf617e41d1..c1da41e35323 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -3211,6 +3211,22 @@ static int gfx_v10_0_cp_async_gfx_ring_resume(struct amdgpu_device *adev)
return r;
}
+static void gfx_v10_0_compute_mqd_set_priority(struct amdgpu_ring *ring, struct v10_compute_mqd *mqd)
+{
+ struct amdgpu_device *adev = ring->adev;
+
+ if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
+ if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
+ mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
+ ring->has_high_prio = true;
+ mqd->cp_hqd_queue_priority =
+ AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
+ } else {
+ ring->has_high_prio = false;
+ }
+ }
+}
+
static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -3336,6 +3352,9 @@ static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
mqd->cp_hqd_ib_control = tmp;
+ /* set static priority for a compute queue/ring */
+ gfx_v10_0_compute_mqd_set_priority(ring, mqd);
+
/* map_queues packet doesn't need activate the queue,
* so only kiq need set this field.
*/
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 393a1324daa9..668c8eb2b2cc 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -4430,6 +4430,22 @@ static int gfx_v8_0_deactivate_hqd(struct amdgpu_device *adev, u32 req)
return r;
}
+static void gfx_v8_0_mqd_set_priority(struct amdgpu_ring *ring, struct vi_mqd *mqd)
+{
+ struct amdgpu_device *adev = ring->adev;
+
+ if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
+ if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
+ mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
+ ring->has_high_prio = true;
+ mqd->cp_hqd_queue_priority =
+ AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
+ } else {
+ ring->has_high_prio = false;
+ }
+ }
+}
+
static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -4553,9 +4569,6 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
/* defaults */
mqd->cp_hqd_eop_rptr = RREG32(mmCP_HQD_EOP_RPTR);
mqd->cp_hqd_eop_wptr = RREG32(mmCP_HQD_EOP_WPTR);
- mqd->cp_hqd_pipe_priority = RREG32(mmCP_HQD_PIPE_PRIORITY);
- mqd->cp_hqd_queue_priority = RREG32(mmCP_HQD_QUEUE_PRIORITY);
- mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
mqd->cp_hqd_ctx_save_base_addr_lo = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_LO);
mqd->cp_hqd_ctx_save_base_addr_hi = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_HI);
mqd->cp_hqd_cntl_stack_offset = RREG32(mmCP_HQD_CNTL_STACK_OFFSET);
@@ -4567,6 +4580,10 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
mqd->cp_hqd_eop_wptr_mem = RREG32(mmCP_HQD_EOP_WPTR_MEM);
mqd->cp_hqd_eop_dones = RREG32(mmCP_HQD_EOP_DONES);
+ /* set static priority for a queue/ring */
+ gfx_v8_0_mqd_set_priority(ring, mqd);
+ mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
+
/* map_queues packet doesn't need activate the queue,
* so only kiq need set this field.
*/
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index 015647959d69..726d1ac41637 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -3314,6 +3314,22 @@ static void gfx_v9_0_kiq_setting(struct amdgpu_ring *ring)
WREG32_SOC15_RLC(GC, 0, mmRLC_CP_SCHEDULERS, tmp);
}
+static void gfx_v9_0_mqd_set_priority(struct amdgpu_ring *ring, struct v9_mqd *mqd)
+{
+ struct amdgpu_device *adev = ring->adev;
+
+ if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
+ if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
+ mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
+ ring->has_high_prio = true;
+ mqd->cp_hqd_queue_priority =
+ AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
+ } else {
+ ring->has_high_prio = false;
+ }
+ }
+}
+
static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -3450,6 +3466,10 @@ static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
mqd->cp_hqd_ib_control = tmp;
+ /* set static priority for a queue/ring */
+ gfx_v9_0_mqd_set_priority(ring, mqd);
+ mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
+
/* map_queues packet doesn't need activate the queue,
* so only kiq need set this field.
*/
--
2.25.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list
2020-03-03 12:50 [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Nirmoy Das
@ 2020-03-03 12:50 ` Nirmoy Das
2020-03-04 22:00 ` Luben Tuikov
2020-03-03 12:50 ` [PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override Nirmoy Das
` (4 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Nirmoy Das @ 2020-03-03 12:50 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
implement drm_sched_entity_modify_sched() which can modify existing
sched_list with a different one. This is going to be helpful when
userspace changes priority of a ctx/entity then driver can switch to
corresponding hw shced list for that priority
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/scheduler/sched_entity.c | 19 +++++++++++++++++++
include/drm/gpu_scheduler.h | 4 ++++
2 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 63bccd201b97..b94312154e56 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -83,6 +83,25 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
}
EXPORT_SYMBOL(drm_sched_entity_init);
+/**
+ * drm_sched_entity_modify_sched - Modify sched of an entity
+ *
+ * @entity: scheduler entity to init
+ * @sched_list: the list of new drm scheds which will replace
+ * existing entity->sched_list
+ * @num_sched_list: number of drm sched in sched_list
+ */
+void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
+ struct drm_gpu_scheduler **sched_list,
+ unsigned int num_sched_list)
+{
+ WARN_ON(!num_sched_list || !sched_list);
+
+ entity->sched_list = sched_list;
+ entity->num_sched_list = num_sched_list;
+}
+EXPORT_SYMBOL(drm_sched_entity_modify_sched);
+
/**
* drm_sched_entity_is_idle - Check if entity is idle
*
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 589be851f8a1..f70a84aaaf7a 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -297,6 +297,10 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched);
int drm_sched_job_init(struct drm_sched_job *job,
struct drm_sched_entity *entity,
void *owner);
+void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
+ struct drm_gpu_scheduler **sched_list,
+ unsigned int num_sched_list);
+
void drm_sched_job_cleanup(struct drm_sched_job *job);
void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
--
2.25.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override
2020-03-03 12:50 [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Nirmoy Das
2020-03-03 12:50 ` [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list Nirmoy Das
@ 2020-03-03 12:50 ` Nirmoy Das
2020-03-04 22:25 ` Luben Tuikov
2020-03-03 12:50 ` [PATCH 4/4] drm/amdgpu: remove unused functions Nirmoy Das
` (3 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Nirmoy Das @ 2020-03-03 12:50 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
Switch to appropriate sched list for an entity on priority override.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 32 +++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 8c52152e3a6e..a0bf14ab9d33 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -522,6 +522,32 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
return fence;
}
+static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
+ struct amdgpu_ctx_entity *aentity,
+ int hw_ip, enum drm_sched_priority priority)
+{
+ struct amdgpu_device *adev = ctx->adev;
+ enum gfx_pipe_priority hw_prio;
+ struct drm_gpu_scheduler **scheds = NULL;
+ unsigned num_scheds;
+
+ /* set sw priority */
+ drm_sched_entity_set_priority(&aentity->entity, priority);
+
+ /* set hw priority */
+ switch (hw_ip) {
+ case AMDGPU_HW_IP_COMPUTE:
+ hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
+ scheds = adev->gfx.compute_prio_sched[hw_prio];
+ num_scheds = adev->gfx.num_compute_sched[hw_prio];
+ break;
+ default:
+ return;
+ }
+
+ drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
+}
+
void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
enum drm_sched_priority priority)
{
@@ -534,13 +560,11 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
ctx->init_priority : ctx->override_priority;
for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
- struct drm_sched_entity *entity;
-
if (!ctx->entities[i][j])
continue;
- entity = &ctx->entities[i][j]->entity;
- drm_sched_entity_set_priority(entity, ctx_prio);
+ amdgpu_ctx_set_entity_priority(ctx, ctx->entities[i][j],
+ i, ctx_prio);
}
}
}
--
2.25.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/4] drm/amdgpu: remove unused functions
2020-03-03 12:50 [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Nirmoy Das
2020-03-03 12:50 ` [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list Nirmoy Das
2020-03-03 12:50 ` [PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override Nirmoy Das
@ 2020-03-03 12:50 ` Nirmoy Das
2020-03-04 22:38 ` Luben Tuikov
2020-03-03 14:03 ` [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Christian König
` (2 subsequent siblings)
5 siblings, 1 reply; 26+ messages in thread
From: Nirmoy Das @ 2020-03-03 12:50 UTC (permalink / raw)
To: amd-gfx; +Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
amdgpu statically set priority for compute queues
at initialization so remove all the functions
responsible changing compute queue priority dynamically
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 70 ----------------
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 7 --
drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 99 ----------------------
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 100 -----------------------
4 files changed, 276 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index ca6b52054b4b..a7e1d0425ed0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -150,76 +150,6 @@ void amdgpu_ring_undo(struct amdgpu_ring *ring)
ring->funcs->end_use(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 drm_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 == DRM_SCHED_PRIORITY_NORMAL)
- return;
-
- mutex_lock(&ring->priority_mutex);
- /* 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 >= DRM_SCHED_PRIORITY_MIN; i--) {
- if (i == DRM_SCHED_PRIORITY_NORMAL
- || atomic_read(&ring->num_jobs[i])) {
- ring->priority = i;
- ring->funcs->set_priority(ring, i);
- break;
- }
- }
-
-out_unlock:
- mutex_unlock(&ring->priority_mutex);
-}
-
-/**
- * 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).
- */
-void amdgpu_ring_priority_get(struct amdgpu_ring *ring,
- enum drm_sched_priority priority)
-{
- if (!ring->funcs->set_priority)
- return;
-
- if (atomic_inc_return(&ring->num_jobs[priority]) <= 0)
- return;
-
- mutex_lock(&ring->priority_mutex);
- if (priority <= ring->priority)
- goto out_unlock;
-
- ring->priority = priority;
- ring->funcs->set_priority(ring, priority);
-
-out_unlock:
- mutex_unlock(&ring->priority_mutex);
-}
-
/**
* amdgpu_ring_init - init driver ring struct.
*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index 201c6ac7bf9d..a75e2418a20e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -167,9 +167,6 @@ struct amdgpu_ring_funcs {
uint32_t reg0, uint32_t reg1,
uint32_t ref, uint32_t mask);
void (*emit_tmz)(struct amdgpu_ring *ring, bool start);
- /* priority functions */
- void (*set_priority) (struct amdgpu_ring *ring,
- enum drm_sched_priority priority);
/* Try to soft recover the ring to make the fence signal */
void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
int (*preempt_ib)(struct amdgpu_ring *ring);
@@ -259,10 +256,6 @@ 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);
-void amdgpu_ring_priority_get(struct amdgpu_ring *ring,
- enum drm_sched_priority priority);
-void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
- enum drm_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/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 05b6f01e1228..f5029eb9ac12 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -6275,104 +6275,6 @@ 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;
-
- 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(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;
-
- mutex_lock(&adev->gfx.pipe_reserve_mutex);
- pipe = amdgpu_gfx_mec_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_gfx_mec_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_gfx_mec_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);
- }
- }
-
- mutex_unlock(&adev->gfx.pipe_reserve_mutex);
-}
-
-static void gfx_v8_0_hqd_set_priority(struct amdgpu_device *adev,
- struct amdgpu_ring *ring,
- bool acquire)
-{
- uint32_t pipe_priority = acquire ? 0x2 : 0x0;
- uint32_t queue_priority = acquire ? 0xf : 0x0;
-
- mutex_lock(&adev->srbm_mutex);
- vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
-
- WREG32(mmCP_HQD_PIPE_PRIORITY, pipe_priority);
- WREG32(mmCP_HQD_QUEUE_PRIORITY, queue_priority);
-
- vi_srbm_select(adev, 0, 0, 0, 0);
- mutex_unlock(&adev->srbm_mutex);
-}
-static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
- enum drm_sched_priority priority)
-{
- struct amdgpu_device *adev = ring->adev;
- bool acquire = priority == DRM_SCHED_PRIORITY_HIGH_HW;
-
- if (ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE)
- return;
-
- gfx_v8_0_hqd_set_priority(adev, ring, acquire);
- gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
-}
-
static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
u64 addr, u64 seq,
unsigned flags)
@@ -7005,7 +6907,6 @@ 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,
.emit_wreg = gfx_v8_0_ring_emit_wreg,
};
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
index ef263d075e06..a27b18d58aca 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
@@ -5130,105 +5130,6 @@ static u64 gfx_v9_0_ring_get_wptr_compute(struct amdgpu_ring *ring)
return wptr;
}
-static void gfx_v9_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;
-
- reg = SOC15_REG_OFFSET(GC, 0, mmSPI_WCL_PIPE_PERCENT_GFX) + pipe_num;
- tmp = RREG32(reg);
- tmp = REG_SET_FIELD(tmp, SPI_WCL_PIPE_PERCENT_GFX, VALUE, pipe_percent);
- WREG32(reg, tmp);
-}
-
-static void gfx_v9_0_pipe_reserve_resources(struct amdgpu_device *adev,
- struct amdgpu_ring *ring,
- bool acquire)
-{
- int i, pipe;
- bool reserve;
- struct amdgpu_ring *iring;
-
- mutex_lock(&adev->gfx.pipe_reserve_mutex);
- pipe = amdgpu_gfx_mec_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_v9_0_ring_set_pipe_percent(&adev->gfx.gfx_ring[i],
- true);
-
- for (i = 0; i < adev->gfx.num_compute_rings; ++i)
- gfx_v9_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_gfx_mec_queue_to_bit(adev,
- iring->me,
- iring->pipe,
- 0);
- reserve = test_bit(pipe, adev->gfx.pipe_reserve_bitmap);
- gfx_v9_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_gfx_mec_queue_to_bit(adev,
- iring->me,
- iring->pipe,
- 0);
- reserve = test_bit(pipe, adev->gfx.pipe_reserve_bitmap);
- gfx_v9_0_ring_set_pipe_percent(iring, reserve);
- }
- }
-
- mutex_unlock(&adev->gfx.pipe_reserve_mutex);
-}
-
-static void gfx_v9_0_hqd_set_priority(struct amdgpu_device *adev,
- struct amdgpu_ring *ring,
- bool acquire)
-{
- uint32_t pipe_priority = acquire ? 0x2 : 0x0;
- uint32_t queue_priority = acquire ? 0xf : 0x0;
-
- mutex_lock(&adev->srbm_mutex);
- soc15_grbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
-
- WREG32_SOC15_RLC(GC, 0, mmCP_HQD_PIPE_PRIORITY, pipe_priority);
- WREG32_SOC15_RLC(GC, 0, mmCP_HQD_QUEUE_PRIORITY, queue_priority);
-
- soc15_grbm_select(adev, 0, 0, 0, 0);
- mutex_unlock(&adev->srbm_mutex);
-}
-
-static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
- enum drm_sched_priority priority)
-{
- struct amdgpu_device *adev = ring->adev;
- bool acquire = priority == DRM_SCHED_PRIORITY_HIGH_HW;
-
- if (ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE)
- return;
-
- gfx_v9_0_hqd_set_priority(adev, ring, acquire);
- gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
-}
-
static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
{
struct amdgpu_device *adev = ring->adev;
@@ -6599,7 +6500,6 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
.test_ib = gfx_v9_0_ring_test_ib,
.insert_nop = amdgpu_ring_insert_nop,
.pad_ib = amdgpu_ring_generic_pad_ib,
- .set_priority = gfx_v9_0_ring_set_priority_compute,
.emit_wreg = gfx_v9_0_ring_emit_wreg,
.emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
.emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
--
2.25.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-03 12:50 [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Nirmoy Das
` (2 preceding siblings ...)
2020-03-03 12:50 ` [PATCH 4/4] drm/amdgpu: remove unused functions Nirmoy Das
@ 2020-03-03 14:03 ` Christian König
2020-03-03 14:29 ` Nirmoy
2020-03-04 20:43 ` Alex Deucher
2020-03-04 21:41 ` Luben Tuikov
5 siblings, 1 reply; 26+ messages in thread
From: Christian König @ 2020-03-03 14:03 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: alexander.deucher, Ray.Huang, nirmoy.das
Am 03.03.20 um 13:50 schrieb Nirmoy Das:
> [SNIP]
> struct amdgpu_mec {
> struct amdgpu_bo *hpd_eop_obj;
> u64 hpd_eop_gpu_addr;
> @@ -280,8 +290,9 @@ struct amdgpu_gfx {
> uint32_t num_gfx_sched;
> unsigned num_gfx_rings;
> struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
> + struct drm_gpu_scheduler **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
> struct drm_gpu_scheduler *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
> - uint32_t num_compute_sched;
> + uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
> unsigned num_compute_rings;
> struct amdgpu_irq_src eop_irq;
> struct amdgpu_irq_src priv_reg_irq;
Well my question is why we we need compute_prio_sched here?
Can't we just design that as:
struct drm_gpu_scheduler
*compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_HI_COMPUTE_RINGS];
uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
I mean the drm_gpu_scheduler * array doesn't needs to be constructed by
the context code in the first place.
Or am I missing something?
Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-03 14:03 ` [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Christian König
@ 2020-03-03 14:29 ` Nirmoy
2020-03-03 15:14 ` Christian König
0 siblings, 1 reply; 26+ messages in thread
From: Nirmoy @ 2020-03-03 14:29 UTC (permalink / raw)
To: Christian König, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das
On 3/3/20 3:03 PM, Christian König wrote:
> Am 03.03.20 um 13:50 schrieb Nirmoy Das:
>> [SNIP]
>> struct amdgpu_mec {
>> struct amdgpu_bo *hpd_eop_obj;
>> u64 hpd_eop_gpu_addr;
>> @@ -280,8 +290,9 @@ struct amdgpu_gfx {
>> uint32_t num_gfx_sched;
>> unsigned num_gfx_rings;
>> struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
>> + struct drm_gpu_scheduler
>> **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
>> struct drm_gpu_scheduler *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
>> - uint32_t num_compute_sched;
>> + uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
>> unsigned num_compute_rings;
>> struct amdgpu_irq_src eop_irq;
>> struct amdgpu_irq_src priv_reg_irq;
>
> Well my question is why we we need compute_prio_sched here?
This one is so that I can leverage single sched array
compute_sched[AMDGPU_MAX_COMPUTE_RINGS]
to store both priority sched list instead of
struct drm_gpu_scheduler
*compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_COMPUTE_RINGS];
I guess this make ctx code unnecessarily complex.
>
> Can't we just design that as:
> struct drm_gpu_scheduler
> *compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_HI_COMPUTE_RINGS];
> uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
What should be the value of AMDGPU_MAX_HI_COMPUTE_RINGS ?
>
> I mean the drm_gpu_scheduler * array doesn't needs to be constructed
> by the context code in the first place.
Do you mean amdgpu_ctx_init_sched() should belong to somewhere else may
be in amdgpu_ring.c ?
>
> Or am I missing something?
>
> Regards,
> Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-03 14:29 ` Nirmoy
@ 2020-03-03 15:14 ` Christian König
2020-03-03 15:22 ` Nirmoy
0 siblings, 1 reply; 26+ messages in thread
From: Christian König @ 2020-03-03 15:14 UTC (permalink / raw)
To: Nirmoy, Christian König, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das
Am 03.03.20 um 15:29 schrieb Nirmoy:
>
> On 3/3/20 3:03 PM, Christian König wrote:
>> Am 03.03.20 um 13:50 schrieb Nirmoy Das:
>>> [SNIP]
>>> struct amdgpu_mec {
>>> struct amdgpu_bo *hpd_eop_obj;
>>> u64 hpd_eop_gpu_addr;
>>> @@ -280,8 +290,9 @@ struct amdgpu_gfx {
>>> uint32_t num_gfx_sched;
>>> unsigned num_gfx_rings;
>>> struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
>>> + struct drm_gpu_scheduler
>>> **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
>>> struct drm_gpu_scheduler
>>> *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
>>> - uint32_t num_compute_sched;
>>> + uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
>>> unsigned num_compute_rings;
>>> struct amdgpu_irq_src eop_irq;
>>> struct amdgpu_irq_src priv_reg_irq;
>>
>> Well my question is why we we need compute_prio_sched here?
>
> This one is so that I can leverage single sched array
> compute_sched[AMDGPU_MAX_COMPUTE_RINGS]
>
> to store both priority sched list instead of
>
> struct drm_gpu_scheduler
> *compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_COMPUTE_RINGS];
>
> I guess this make ctx code unnecessarily complex.
Well, it would be good if the ctx code didn't need to fill in
compute_sched in the first place.
E.g. that the hardware backends provide to the ctx handling which
schedulers to use for a specific use case.
>> Can't we just design that as:
>> struct drm_gpu_scheduler
>> *compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX][AMDGPU_MAX_HI_COMPUTE_RINGS];
>> uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
> What should be the value of AMDGPU_MAX_HI_COMPUTE_RINGS ?
>>
>> I mean the drm_gpu_scheduler * array doesn't needs to be constructed
>> by the context code in the first place.
> Do you mean amdgpu_ctx_init_sched() should belong to somewhere else
> may be in amdgpu_ring.c ?
That's one possibility, yes. On the other hand feel free to go ahead
with the boolean for now.
This seems to be a to complex and to wide cleanup that we should do it
as part of this patch set.
Regards,
Christian.
>>
>> Or am I missing something?
>>
>> Regards,
>> Christian.
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-03 15:14 ` Christian König
@ 2020-03-03 15:22 ` Nirmoy
2020-03-03 15:30 ` Christian König
0 siblings, 1 reply; 26+ messages in thread
From: Nirmoy @ 2020-03-03 15:22 UTC (permalink / raw)
To: christian.koenig, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das
Hi Christian,
On 3/3/20 4:14 PM, Christian König wrote:
>
>>> I mean the drm_gpu_scheduler * array doesn't needs to be constructed
>>> by the context code in the first place.
>> Do you mean amdgpu_ctx_init_sched() should belong to somewhere else
>> may be in amdgpu_ring.c ?
>
> That's one possibility, yes. On the other hand feel free to go ahead
> with the boolean for now. \
Are you fine with struct drm_gpu_scheduler
**compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX]; for now as well ?
Regards,
Nirmoy
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-03 15:22 ` Nirmoy
@ 2020-03-03 15:30 ` Christian König
2020-03-04 9:28 ` Nirmoy
0 siblings, 1 reply; 26+ messages in thread
From: Christian König @ 2020-03-03 15:30 UTC (permalink / raw)
To: Nirmoy, Nirmoy Das, amd-gfx; +Cc: alexander.deucher, Ray.Huang, nirmoy.das
Am 03.03.20 um 16:22 schrieb Nirmoy:
> Hi Christian,
>
> On 3/3/20 4:14 PM, Christian König wrote:
>>
>>>> I mean the drm_gpu_scheduler * array doesn't needs to be
>>>> constructed by the context code in the first place.
>>> Do you mean amdgpu_ctx_init_sched() should belong to somewhere else
>>> may be in amdgpu_ring.c ?
>>
>> That's one possibility, yes. On the other hand feel free to go ahead
>> with the boolean for now. \
>
> Are you fine with struct drm_gpu_scheduler
> **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX]; for now as well ?
Yeah, that should work anyway. My only concern was adding the boolean to
the ring structure.
regards,
Christian.
>
>
> Regards,
>
> Nirmoy
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-03 15:30 ` Christian König
@ 2020-03-04 9:28 ` Nirmoy
0 siblings, 0 replies; 26+ messages in thread
From: Nirmoy @ 2020-03-04 9:28 UTC (permalink / raw)
To: Christian König, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das
On 3/3/20 4:30 PM, Christian König wrote:
> Am 03.03.20 um 16:22 schrieb Nirmoy:
>> Hi Christian,
>>
>> On 3/3/20 4:14 PM, Christian König wrote:
>>>
>>>>> I mean the drm_gpu_scheduler * array doesn't needs to be
>>>>> constructed by the context code in the first place.
>>>> Do you mean amdgpu_ctx_init_sched() should belong to somewhere else
>>>> may be in amdgpu_ring.c ?
>>>
>>> That's one possibility, yes. On the other hand feel free to go ahead
>>> with the boolean for now. \
>>
>> Are you fine with struct drm_gpu_scheduler
>> **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX]; for now as well ?
>
> Yeah, that should work anyway. My only concern was adding the boolean
> to the ring structure.
Thanks.
I will later work on the ctx code cleanup to move drm_gpu_scheduler
array construction to a proper place.
Regards,
Nirmoy
>
> regards,
> Christian.
>
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-03 12:50 [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Nirmoy Das
` (3 preceding siblings ...)
2020-03-03 14:03 ` [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Christian König
@ 2020-03-04 20:43 ` Alex Deucher
2020-03-04 21:41 ` Luben Tuikov
5 siblings, 0 replies; 26+ messages in thread
From: Alex Deucher @ 2020-03-04 20:43 UTC (permalink / raw)
To: Nirmoy Das
Cc: Deucher, Alexander, Huang Rui, Nirmoy Das, Christian Koenig,
amd-gfx list
On Tue, Mar 3, 2020 at 7:47 AM Nirmoy Das <nirmoy.aiemd@gmail.com> wrote:
>
> We were changing compute ring priority while rings were being used
> before every job submission which is not recommended. This patch
> sets compute queue priority at mqd initialization for gfx8, gfx9 and
> gfx10.
>
> Policy: make queue 0 of each pipe as high priority compute queue
>
> High/normal priority compute sched lists are generated from set of high/normal
> priority compute queues. At context creation, entity of compute queue
> get a sched list from high or normal priority depending on ctx->priority
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
I haven't had a chance to follow all of the scheduler discussions, but
the MQD setup looks good to me, so:
Acked-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 --
> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 60 +++++++++++++++++++++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 8 ++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 15 +++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 +
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 19 ++++++++
> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 23 +++++++--
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 20 ++++++++
> 9 files changed, 135 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index f397ff97b4e4..8304d0c87899 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1205,7 +1205,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
> struct drm_sched_entity *entity = p->entity;
> enum drm_sched_priority priority;
> - struct amdgpu_ring *ring;
> struct amdgpu_bo_list_entry *e;
> struct amdgpu_job *job;
> uint64_t seq;
> @@ -1258,9 +1257,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> priority = job->base.s_priority;
> drm_sched_entity_push_job(&job->base, entity);
>
> - ring = to_amdgpu_ring(entity->rq->sched);
> - amdgpu_ring_priority_get(ring, priority);
> -
> amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);
>
> ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index 94a6c42f29ea..4ad944f85672 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -61,12 +61,30 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
> return -EACCES;
> }
>
> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_compute_prio(enum drm_sched_priority prio)
> +{
> + switch(prio) {
> + case DRM_SCHED_PRIORITY_MIN:
> + case DRM_SCHED_PRIORITY_NORMAL:
> + case DRM_SCHED_PRIORITY_HIGH_SW:
> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
> + case DRM_SCHED_PRIORITY_HIGH_HW:
> + case DRM_SCHED_PRIORITY_KERNEL:
> + return AMDGPU_GFX_PIPE_PRIO_HIGH;
> + default:
> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
> + }
> +
> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
> +}
> +
> static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const u32 ring)
> {
> struct amdgpu_device *adev = ctx->adev;
> struct amdgpu_ctx_entity *entity;
> struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
> unsigned num_scheds = 0;
> + enum gfx_pipe_priority hw_prio;
> enum drm_sched_priority priority;
> int r;
>
> @@ -85,8 +103,9 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const
> num_scheds = 1;
> break;
> case AMDGPU_HW_IP_COMPUTE:
> - scheds = adev->gfx.compute_sched;
> - num_scheds = adev->gfx.num_compute_sched;
> + hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
> + scheds = adev->gfx.compute_prio_sched[hw_prio];
> + num_scheds = adev->gfx.num_compute_sched[hw_prio];
> break;
> case AMDGPU_HW_IP_DMA:
> scheds = adev->sdma.sdma_sched;
> @@ -628,20 +647,47 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
> mutex_destroy(&mgr->lock);
> }
>
> +
> +static void amdgpu_ctx_init_compute_sched(struct amdgpu_device *adev)
> +{
> + int num_compute_sched_normal = 0;
> + int num_compute_sched_high = AMDGPU_MAX_COMPUTE_RINGS - 1;
> + int i;
> +
> + /* fill compute_sched array as: start from 0th index for normal priority scheds and
> + * start from (last_index - num_compute_sched_normal) for high priority
> + * scheds */
> + for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> + if (!adev->gfx.compute_ring[i].has_high_prio)
> + adev->gfx.compute_sched[num_compute_sched_normal++] =
> + &adev->gfx.compute_ring[i].sched;
> + else
> + adev->gfx.compute_sched[num_compute_sched_high--] =
> + &adev->gfx.compute_ring[i].sched;
> + }
> +
> + /* compute ring only has two priority for now */
> + i = AMDGPU_GFX_PIPE_PRIO_NORMAL;
> + adev->gfx.compute_prio_sched[i] = &adev->gfx.compute_sched[0];
> + adev->gfx.num_compute_sched[i] = num_compute_sched_normal;
> +
> + i = AMDGPU_GFX_PIPE_PRIO_HIGH;
> + adev->gfx.compute_prio_sched[i] =
> + &adev->gfx.compute_sched[num_compute_sched_high - 1];
> + adev->gfx.num_compute_sched[i] =
> + adev->gfx.num_compute_rings - num_compute_sched_normal;
> +}
> +
> void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
> {
> int i, j;
>
> + amdgpu_ctx_init_compute_sched(adev);
> for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
> adev->gfx.gfx_sched[i] = &adev->gfx.gfx_ring[i].sched;
> adev->gfx.num_gfx_sched++;
> }
>
> - for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> - adev->gfx.compute_sched[i] = &adev->gfx.compute_ring[i].sched;
> - adev->gfx.num_compute_sched++;
> - }
> -
> for (i = 0; i < adev->sdma.num_instances; i++) {
> adev->sdma.sdma_sched[i] = &adev->sdma.instance[i].ring.sched;
> adev->sdma.num_sdma_sched++;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 7403588684b3..952725e7243c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -192,6 +192,14 @@ static bool amdgpu_gfx_is_multipipe_capable(struct amdgpu_device *adev)
> return adev->gfx.mec.num_mec > 1;
> }
>
> +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
> + int queue)
> +{
> + /* Policy: make queue 0 of each pipe as high priority compute queue */
> + return (queue == 0);
> +
> +}
> +
> void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev)
> {
> int i, queue, pipe, mec;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index 37ba05b63b2a..47a5cdae28c5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -41,6 +41,16 @@
> #define AMDGPU_MAX_GFX_QUEUES KGD_MAX_QUEUES
> #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
>
> +enum gfx_pipe_priority {
> + AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
> + AMDGPU_GFX_PIPE_PRIO_HIGH,
> + AMDGPU_GFX_PIPE_PRIO_MAX
> +
> +};
> +
> +#define AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM 0
> +#define AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM 15
> +
> struct amdgpu_mec {
> struct amdgpu_bo *hpd_eop_obj;
> u64 hpd_eop_gpu_addr;
> @@ -280,8 +290,9 @@ struct amdgpu_gfx {
> uint32_t num_gfx_sched;
> unsigned num_gfx_rings;
> struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
> + struct drm_gpu_scheduler **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
> struct drm_gpu_scheduler *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
> - uint32_t num_compute_sched;
> + uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
> unsigned num_compute_rings;
> struct amdgpu_irq_src eop_irq;
> struct amdgpu_irq_src priv_reg_irq;
> @@ -363,6 +374,8 @@ void amdgpu_gfx_bit_to_mec_queue(struct amdgpu_device *adev, int bit,
> int *mec, int *pipe, int *queue);
> bool amdgpu_gfx_is_mec_queue_enabled(struct amdgpu_device *adev, int mec,
> int pipe, int queue);
> +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
> + int queue);
> int amdgpu_gfx_me_queue_to_bit(struct amdgpu_device *adev, int me,
> int pipe, int queue);
> void amdgpu_gfx_bit_to_me_queue(struct amdgpu_device *adev, int bit,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index d42be880a236..4981e443a884 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -117,12 +117,10 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
>
> static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
> {
> - struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
> struct amdgpu_job *job = to_amdgpu_job(s_job);
>
> drm_sched_job_cleanup(s_job);
>
> - amdgpu_ring_priority_put(ring, s_job->s_priority);
> dma_fence_put(job->fence);
> amdgpu_sync_free(&job->sync);
> amdgpu_sync_free(&job->sched_sync);
> @@ -143,7 +141,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
> void *owner, struct dma_fence **f)
> {
> enum drm_sched_priority priority;
> - struct amdgpu_ring *ring;
> int r;
>
> if (!f)
> @@ -158,9 +155,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
> priority = job->base.s_priority;
> drm_sched_entity_push_job(&job->base, entity);
>
> - ring = to_amdgpu_ring(entity->rq->sched);
> - amdgpu_ring_priority_get(ring, priority);
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 24caff085d00..dcea1ef92883 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -222,6 +222,7 @@ struct amdgpu_ring {
> struct mutex priority_mutex;
> /* protected by priority_mutex */
> int priority;
> + bool has_high_prio;
>
> #if defined(CONFIG_DEBUG_FS)
> struct dentry *ent;
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 96cf617e41d1..c1da41e35323 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -3211,6 +3211,22 @@ static int gfx_v10_0_cp_async_gfx_ring_resume(struct amdgpu_device *adev)
> return r;
> }
>
> +static void gfx_v10_0_compute_mqd_set_priority(struct amdgpu_ring *ring, struct v10_compute_mqd *mqd)
> +{
> + struct amdgpu_device *adev = ring->adev;
> +
> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
> + ring->has_high_prio = true;
> + mqd->cp_hqd_queue_priority =
> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
> + } else {
> + ring->has_high_prio = false;
> + }
> + }
> +}
> +
> static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
> {
> struct amdgpu_device *adev = ring->adev;
> @@ -3336,6 +3352,9 @@ static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
> tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
> mqd->cp_hqd_ib_control = tmp;
>
> + /* set static priority for a compute queue/ring */
> + gfx_v10_0_compute_mqd_set_priority(ring, mqd);
> +
> /* map_queues packet doesn't need activate the queue,
> * so only kiq need set this field.
> */
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 393a1324daa9..668c8eb2b2cc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -4430,6 +4430,22 @@ static int gfx_v8_0_deactivate_hqd(struct amdgpu_device *adev, u32 req)
> return r;
> }
>
> +static void gfx_v8_0_mqd_set_priority(struct amdgpu_ring *ring, struct vi_mqd *mqd)
> +{
> + struct amdgpu_device *adev = ring->adev;
> +
> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
> + ring->has_high_prio = true;
> + mqd->cp_hqd_queue_priority =
> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
> + } else {
> + ring->has_high_prio = false;
> + }
> + }
> +}
> +
> static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> {
> struct amdgpu_device *adev = ring->adev;
> @@ -4553,9 +4569,6 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> /* defaults */
> mqd->cp_hqd_eop_rptr = RREG32(mmCP_HQD_EOP_RPTR);
> mqd->cp_hqd_eop_wptr = RREG32(mmCP_HQD_EOP_WPTR);
> - mqd->cp_hqd_pipe_priority = RREG32(mmCP_HQD_PIPE_PRIORITY);
> - mqd->cp_hqd_queue_priority = RREG32(mmCP_HQD_QUEUE_PRIORITY);
> - mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
> mqd->cp_hqd_ctx_save_base_addr_lo = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_LO);
> mqd->cp_hqd_ctx_save_base_addr_hi = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_HI);
> mqd->cp_hqd_cntl_stack_offset = RREG32(mmCP_HQD_CNTL_STACK_OFFSET);
> @@ -4567,6 +4580,10 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> mqd->cp_hqd_eop_wptr_mem = RREG32(mmCP_HQD_EOP_WPTR_MEM);
> mqd->cp_hqd_eop_dones = RREG32(mmCP_HQD_EOP_DONES);
>
> + /* set static priority for a queue/ring */
> + gfx_v8_0_mqd_set_priority(ring, mqd);
> + mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
> +
> /* map_queues packet doesn't need activate the queue,
> * so only kiq need set this field.
> */
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 015647959d69..726d1ac41637 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -3314,6 +3314,22 @@ static void gfx_v9_0_kiq_setting(struct amdgpu_ring *ring)
> WREG32_SOC15_RLC(GC, 0, mmRLC_CP_SCHEDULERS, tmp);
> }
>
> +static void gfx_v9_0_mqd_set_priority(struct amdgpu_ring *ring, struct v9_mqd *mqd)
> +{
> + struct amdgpu_device *adev = ring->adev;
> +
> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
> + ring->has_high_prio = true;
> + mqd->cp_hqd_queue_priority =
> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
> + } else {
> + ring->has_high_prio = false;
> + }
> + }
> +}
> +
> static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
> {
> struct amdgpu_device *adev = ring->adev;
> @@ -3450,6 +3466,10 @@ static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
> tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
> mqd->cp_hqd_ib_control = tmp;
>
> + /* set static priority for a queue/ring */
> + gfx_v9_0_mqd_set_priority(ring, mqd);
> + mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
> +
> /* map_queues packet doesn't need activate the queue,
> * so only kiq need set this field.
> */
> --
> 2.25.0
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-03 12:50 [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Nirmoy Das
` (4 preceding siblings ...)
2020-03-04 20:43 ` Alex Deucher
@ 2020-03-04 21:41 ` Luben Tuikov
2020-03-04 22:32 ` Luben Tuikov
2020-03-05 6:21 ` Nirmoy
5 siblings, 2 replies; 26+ messages in thread
From: Luben Tuikov @ 2020-03-04 21:41 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
> We were changing compute ring priority while rings were being used
> before every job submission which is not recommended. This patch
> sets compute queue priority at mqd initialization for gfx8, gfx9 and
> gfx10.
>
> Policy: make queue 0 of each pipe as high priority compute queue
>
> High/normal priority compute sched lists are generated from set of high/normal
> priority compute queues. At context creation, entity of compute queue
> get a sched list from high or normal priority depending on ctx->priority
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 --
> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 60 +++++++++++++++++++++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 8 ++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 15 +++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 +
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 19 ++++++++
> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 23 +++++++--
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 20 ++++++++
> 9 files changed, 135 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index f397ff97b4e4..8304d0c87899 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1205,7 +1205,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
> struct drm_sched_entity *entity = p->entity;
> enum drm_sched_priority priority;
> - struct amdgpu_ring *ring;
> struct amdgpu_bo_list_entry *e;
> struct amdgpu_job *job;
> uint64_t seq;
> @@ -1258,9 +1257,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
> priority = job->base.s_priority;
> drm_sched_entity_push_job(&job->base, entity);
>
> - ring = to_amdgpu_ring(entity->rq->sched);
> - amdgpu_ring_priority_get(ring, priority);
> -
> amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);
>
> ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index 94a6c42f29ea..4ad944f85672 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -61,12 +61,30 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
> return -EACCES;
> }
>
> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_compute_prio(enum drm_sched_priority prio)
> +{
> + switch(prio) {
LKCS wants a space after a keyword ("switch") and before parenthesis "(".
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
Please read the LKCS in you local Linux source code:
Documentation/process/coding-style.rst
so we don't have to point that out anymore.
> + case DRM_SCHED_PRIORITY_MIN:
> + case DRM_SCHED_PRIORITY_NORMAL:
> + case DRM_SCHED_PRIORITY_HIGH_SW:
> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
This is taken care of by the "default:" label and
unnecessary here (can be removed completely).
> + case DRM_SCHED_PRIORITY_HIGH_HW:
> + case DRM_SCHED_PRIORITY_KERNEL:
> + return AMDGPU_GFX_PIPE_PRIO_HIGH;
> + default:
> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
> + }
This can be a map. We're mapping from one integer
space to another. There is no reason for a jump switch.
For instance,
/* Map of the DRM scheduling priority to pipe
* priority.
*/
const enum gfx_pipe_priority s2p_prio_map[] = {
[0] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
[1] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
[2] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
[3] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
[4] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
[5] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
[6] = AMDGPU_GFX_PIPE_PRIO_HIGH,
[7] = AMDGPU_GFX_PIPE_PRIO_HIGH,
[8] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
};
/* Map it!
*/
pipe_prio = s2p_prio_map[sched_prio + 2]; ## You can view this as y = f(x + 2).
Note that if you fix enum drm_sched_priority as I described
in an earlier review, you'd not need the additive factor of 2
in the above linear transformation.
> +
> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
You don't need a return here at all, when you have a "default:" label
up there.
> +}
> +
> static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const u32 ring)
> {
> struct amdgpu_device *adev = ctx->adev;
> struct amdgpu_ctx_entity *entity;
> struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
> unsigned num_scheds = 0;
> + enum gfx_pipe_priority hw_prio;
> enum drm_sched_priority priority;
> int r;
>
> @@ -85,8 +103,9 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const
> num_scheds = 1;
> break;
> case AMDGPU_HW_IP_COMPUTE:
> - scheds = adev->gfx.compute_sched;
> - num_scheds = adev->gfx.num_compute_sched;
> + hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
> + scheds = adev->gfx.compute_prio_sched[hw_prio];
> + num_scheds = adev->gfx.num_compute_sched[hw_prio];
> break;
> case AMDGPU_HW_IP_DMA:
> scheds = adev->sdma.sdma_sched;
> @@ -628,20 +647,47 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
> mutex_destroy(&mgr->lock);
> }
>
> +
> +static void amdgpu_ctx_init_compute_sched(struct amdgpu_device *adev)
> +{
> + int num_compute_sched_normal = 0;
> + int num_compute_sched_high = AMDGPU_MAX_COMPUTE_RINGS - 1;
> + int i;
> +
> + /* fill compute_sched array as: start from 0th index for normal priority scheds and
> + * start from (last_index - num_compute_sched_normal) for high priority
> + * scheds */
See the LKCS guide here on comments.
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
To quote from Documentation/process/coding-style.rst:
"NEVER
try to explain HOW your code works in a comment: it's much better to
write the code so that the **working** is obvious, and it's a waste of
time to explain badly written code.
Generally, you want your comments to tell WHAT your code does, not HOW."
Please comment on why and what you're doing as opposed to how.
Or if you don't want to do this, simply remove the comment.
Remember, you want to make it clear why and what the code does here,
as opposed to how it's doing it.
Use this style of commenting, like Christian does:
previous code ends here;
/* Comment line 1.
* Comment line 2.
*/
code starts here--no extra empty lines...
> + for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> + if (!adev->gfx.compute_ring[i].has_high_prio)
> + adev->gfx.compute_sched[num_compute_sched_normal++] =
> + &adev->gfx.compute_ring[i].sched;
> + else
> + adev->gfx.compute_sched[num_compute_sched_high--] =
> + &adev->gfx.compute_ring[i].sched;
> + }
> +
> + /* compute ring only has two priority for now */
> + i = AMDGPU_GFX_PIPE_PRIO_NORMAL;
> + adev->gfx.compute_prio_sched[i] = &adev->gfx.compute_sched[0];
> + adev->gfx.num_compute_sched[i] = num_compute_sched_normal;
> +
> + i = AMDGPU_GFX_PIPE_PRIO_HIGH;
> + adev->gfx.compute_prio_sched[i] =
> + &adev->gfx.compute_sched[num_compute_sched_high - 1];
> + adev->gfx.num_compute_sched[i] =
> + adev->gfx.num_compute_rings - num_compute_sched_normal;
> +}
> +
> void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
> {
> int i, j;
>
> + amdgpu_ctx_init_compute_sched(adev);
> for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
> adev->gfx.gfx_sched[i] = &adev->gfx.gfx_ring[i].sched;
> adev->gfx.num_gfx_sched++;
> }
>
> - for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> - adev->gfx.compute_sched[i] = &adev->gfx.compute_ring[i].sched;
> - adev->gfx.num_compute_sched++;
> - }
> -
> for (i = 0; i < adev->sdma.num_instances; i++) {
> adev->sdma.sdma_sched[i] = &adev->sdma.instance[i].ring.sched;
> adev->sdma.num_sdma_sched++;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 7403588684b3..952725e7243c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -192,6 +192,14 @@ static bool amdgpu_gfx_is_multipipe_capable(struct amdgpu_device *adev)
> return adev->gfx.mec.num_mec > 1;
> }
>
> +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
> + int queue)
> +{
> + /* Policy: make queue 0 of each pipe as high priority compute queue */
> + return (queue == 0);
> +
> +}
> +
> void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev)
> {
> int i, queue, pipe, mec;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> index 37ba05b63b2a..47a5cdae28c5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
> @@ -41,6 +41,16 @@
> #define AMDGPU_MAX_GFX_QUEUES KGD_MAX_QUEUES
> #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
>
> +enum gfx_pipe_priority {
> + AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
> + AMDGPU_GFX_PIPE_PRIO_HIGH,
> + AMDGPU_GFX_PIPE_PRIO_MAX
Please don't call it "MAX". "Maximum value" means
that that value is in the domain of values
the priory can take, and it is not. That is,
priority could never be AMDGPU_GFX_PIPE_PRIO_MAX.
If you want to know the size of the domain of values,
just call it that:
AMDGPU_GFX_PIPE_PRIO_NUM/_SIZE.
enum gfx_pipe_priority {
AMDGPU_GFX_PIPE_PRIO_UNDEFINED,
AMDGPU_GFX_PIPE_PRIO_NORMAL,
AMDGPU_GFX_PIPE_PRIO_HIGH,
AMDGPU_GFX_PIPE_PRIO_SIZE,
};
Naturally 0 would be undefined, yet named.
Note the empty line before the size.
Regards,
Luben
> +
> +};
> +
> +#define AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM 0
> +#define AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM 15
> +
> struct amdgpu_mec {
> struct amdgpu_bo *hpd_eop_obj;
> u64 hpd_eop_gpu_addr;
> @@ -280,8 +290,9 @@ struct amdgpu_gfx {
> uint32_t num_gfx_sched;
> unsigned num_gfx_rings;
> struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
> + struct drm_gpu_scheduler **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
> struct drm_gpu_scheduler *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
> - uint32_t num_compute_sched;
> + uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
> unsigned num_compute_rings;
> struct amdgpu_irq_src eop_irq;
> struct amdgpu_irq_src priv_reg_irq;
> @@ -363,6 +374,8 @@ void amdgpu_gfx_bit_to_mec_queue(struct amdgpu_device *adev, int bit,
> int *mec, int *pipe, int *queue);
> bool amdgpu_gfx_is_mec_queue_enabled(struct amdgpu_device *adev, int mec,
> int pipe, int queue);
> +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
> + int queue);
> int amdgpu_gfx_me_queue_to_bit(struct amdgpu_device *adev, int me,
> int pipe, int queue);
> void amdgpu_gfx_bit_to_me_queue(struct amdgpu_device *adev, int bit,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index d42be880a236..4981e443a884 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -117,12 +117,10 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
>
> static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
> {
> - struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
> struct amdgpu_job *job = to_amdgpu_job(s_job);
>
> drm_sched_job_cleanup(s_job);
>
> - amdgpu_ring_priority_put(ring, s_job->s_priority);
> dma_fence_put(job->fence);
> amdgpu_sync_free(&job->sync);
> amdgpu_sync_free(&job->sched_sync);
> @@ -143,7 +141,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
> void *owner, struct dma_fence **f)
> {
> enum drm_sched_priority priority;
> - struct amdgpu_ring *ring;
> int r;
>
> if (!f)
> @@ -158,9 +155,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
> priority = job->base.s_priority;
> drm_sched_entity_push_job(&job->base, entity);
>
> - ring = to_amdgpu_ring(entity->rq->sched);
> - amdgpu_ring_priority_get(ring, priority);
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 24caff085d00..dcea1ef92883 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -222,6 +222,7 @@ struct amdgpu_ring {
> struct mutex priority_mutex;
> /* protected by priority_mutex */
> int priority;
> + bool has_high_prio;
>
> #if defined(CONFIG_DEBUG_FS)
> struct dentry *ent;
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 96cf617e41d1..c1da41e35323 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -3211,6 +3211,22 @@ static int gfx_v10_0_cp_async_gfx_ring_resume(struct amdgpu_device *adev)
> return r;
> }
>
> +static void gfx_v10_0_compute_mqd_set_priority(struct amdgpu_ring *ring, struct v10_compute_mqd *mqd)
> +{
> + struct amdgpu_device *adev = ring->adev;
> +
> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
> + ring->has_high_prio = true;
> + mqd->cp_hqd_queue_priority =
> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
> + } else {
> + ring->has_high_prio = false;
> + }
> + }
> +}
> +
> static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
> {
> struct amdgpu_device *adev = ring->adev;
> @@ -3336,6 +3352,9 @@ static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
> tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
> mqd->cp_hqd_ib_control = tmp;
>
> + /* set static priority for a compute queue/ring */
> + gfx_v10_0_compute_mqd_set_priority(ring, mqd);
> +
> /* map_queues packet doesn't need activate the queue,
> * so only kiq need set this field.
> */
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 393a1324daa9..668c8eb2b2cc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -4430,6 +4430,22 @@ static int gfx_v8_0_deactivate_hqd(struct amdgpu_device *adev, u32 req)
> return r;
> }
>
> +static void gfx_v8_0_mqd_set_priority(struct amdgpu_ring *ring, struct vi_mqd *mqd)
> +{
> + struct amdgpu_device *adev = ring->adev;
> +
> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
> + ring->has_high_prio = true;
> + mqd->cp_hqd_queue_priority =
> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
> + } else {
> + ring->has_high_prio = false;
> + }
> + }
> +}
> +
> static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> {
> struct amdgpu_device *adev = ring->adev;
> @@ -4553,9 +4569,6 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> /* defaults */
> mqd->cp_hqd_eop_rptr = RREG32(mmCP_HQD_EOP_RPTR);
> mqd->cp_hqd_eop_wptr = RREG32(mmCP_HQD_EOP_WPTR);
> - mqd->cp_hqd_pipe_priority = RREG32(mmCP_HQD_PIPE_PRIORITY);
> - mqd->cp_hqd_queue_priority = RREG32(mmCP_HQD_QUEUE_PRIORITY);
> - mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
> mqd->cp_hqd_ctx_save_base_addr_lo = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_LO);
> mqd->cp_hqd_ctx_save_base_addr_hi = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_HI);
> mqd->cp_hqd_cntl_stack_offset = RREG32(mmCP_HQD_CNTL_STACK_OFFSET);
> @@ -4567,6 +4580,10 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
> mqd->cp_hqd_eop_wptr_mem = RREG32(mmCP_HQD_EOP_WPTR_MEM);
> mqd->cp_hqd_eop_dones = RREG32(mmCP_HQD_EOP_DONES);
>
> + /* set static priority for a queue/ring */
> + gfx_v8_0_mqd_set_priority(ring, mqd);
> + mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
> +
> /* map_queues packet doesn't need activate the queue,
> * so only kiq need set this field.
> */
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 015647959d69..726d1ac41637 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -3314,6 +3314,22 @@ static void gfx_v9_0_kiq_setting(struct amdgpu_ring *ring)
> WREG32_SOC15_RLC(GC, 0, mmRLC_CP_SCHEDULERS, tmp);
> }
>
> +static void gfx_v9_0_mqd_set_priority(struct amdgpu_ring *ring, struct v9_mqd *mqd)
> +{
> + struct amdgpu_device *adev = ring->adev;
> +
> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
> + ring->has_high_prio = true;
> + mqd->cp_hqd_queue_priority =
> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
> + } else {
> + ring->has_high_prio = false;
> + }
> + }
> +}
> +
> static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
> {
> struct amdgpu_device *adev = ring->adev;
> @@ -3450,6 +3466,10 @@ static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
> tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
> mqd->cp_hqd_ib_control = tmp;
>
> + /* set static priority for a queue/ring */
> + gfx_v9_0_mqd_set_priority(ring, mqd);
> + mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
> +
> /* map_queues packet doesn't need activate the queue,
> * so only kiq need set this field.
> */
> --
> 2.25.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cfa70ffa2d9874bbe1a5908d7bf71077e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188364502419515&sdata=vIM2clFPPCfIDK24UxAiJDZ5JH%2F83K2JESZtZKTGQUc%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list
2020-03-03 12:50 ` [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list Nirmoy Das
@ 2020-03-04 22:00 ` Luben Tuikov
2020-03-05 6:23 ` Nirmoy
2020-03-05 9:10 ` Nirmoy
0 siblings, 2 replies; 26+ messages in thread
From: Luben Tuikov @ 2020-03-04 22:00 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
> implement drm_sched_entity_modify_sched() which can modify existing
> sched_list with a different one. This is going to be helpful when
> userspace changes priority of a ctx/entity then driver can switch to
> corresponding hw shced list for that priority
>
This commit message should start with a capitalized sentence:
"implement" --> "Implement". "can modify" --> "modifies"
Also a spell check should be done on it before committing:
"shced" --> "sched".
"then the driver", "to the corresponding", "HW scheduler".
And the commit message paragraph should end with a period.
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/scheduler/sched_entity.c | 19 +++++++++++++++++++
> include/drm/gpu_scheduler.h | 4 ++++
> 2 files changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 63bccd201b97..b94312154e56 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -83,6 +83,25 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
> }
> EXPORT_SYMBOL(drm_sched_entity_init);
>
> +/**
> + * drm_sched_entity_modify_sched - Modify sched of an entity
> + *
This empty line is unncessary.
> + * @entity: scheduler entity to init
> + * @sched_list: the list of new drm scheds which will replace
> + * existing entity->sched_list
> + * @num_sched_list: number of drm sched in sched_list
> + */
> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> + struct drm_gpu_scheduler **sched_list,
> + unsigned int num_sched_list)
> +{
> + WARN_ON(!num_sched_list || !sched_list);
> +
> + entity->sched_list = sched_list;
> + entity->num_sched_list = num_sched_list;
> +}
> +EXPORT_SYMBOL(drm_sched_entity_modify_sched);
> +
> /**
> * drm_sched_entity_is_idle - Check if entity is idle
> *
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 589be851f8a1..f70a84aaaf7a 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -297,6 +297,10 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched);
> int drm_sched_job_init(struct drm_sched_job *job,
> struct drm_sched_entity *entity,
> void *owner);
> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> + struct drm_gpu_scheduler **sched_list,
> + unsigned int num_sched_list);
> +
Again, the argument list here is unaligned. Please align it
correctly. The correct indentation would add the maximum number of
leading TAB chars followed by the 0 to 7 spaces, to align
the argument list to the first argument in the opening parenthesis.
The LKCS describes this and also has the settings for Emacs.
In Emacs, pressing the TAB key aligns, after which pressing it more
does nothing (on an aligned line).
Regards,
Luben
> void drm_sched_job_cleanup(struct drm_sched_job *job);
> void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
> void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
> --
> 2.25.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cbe8445945c194bd8b3cc08d7bf7109c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188364557354217&sdata=UAxV2NtkNcj6VXAPhDbk4GliUOnPEfuLOH4BVuOrqdw%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override
2020-03-03 12:50 ` [PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override Nirmoy Das
@ 2020-03-04 22:25 ` Luben Tuikov
2020-03-05 6:28 ` Nirmoy
0 siblings, 1 reply; 26+ messages in thread
From: Luben Tuikov @ 2020-03-04 22:25 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
> Switch to appropriate sched list for an entity on priority override.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 32 +++++++++++++++++++++----
> 1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index 8c52152e3a6e..a0bf14ab9d33 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -522,6 +522,32 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
> return fence;
> }
>
> +static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
> + struct amdgpu_ctx_entity *aentity,
> + int hw_ip, enum drm_sched_priority priority)
> +{
> + struct amdgpu_device *adev = ctx->adev;
> + enum gfx_pipe_priority hw_prio;
> + struct drm_gpu_scheduler **scheds = NULL;
> + unsigned num_scheds;
> +
> + /* set sw priority */
> + drm_sched_entity_set_priority(&aentity->entity, priority);
> +
> + /* set hw priority */
> + switch (hw_ip) {
> + case AMDGPU_HW_IP_COMPUTE:
> + hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
> + scheds = adev->gfx.compute_prio_sched[hw_prio];
> + num_scheds = adev->gfx.num_compute_sched[hw_prio];
> + break;
> + default:
> + return;
> + }
> +
> + drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
> +}
I'd rather this not be over-engineered (in expectations of more case labels,
and a simple if-else to do it. Over-engineering it "just in case" creates
difficult to maintain code. I believe there is a document about this somewhere
in Documentation/.
You don't need a break only to execute one statement, which you can pull
into the case: label. If you did this you'll see that you just want to do:
static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
struct amdgpu_ctx_entity *aentity,
int hw_ip, enum drm_sched_priority priority)
{
...
/* Set software priority.
*/
drm_sched_entity_set_priority(&aentity->entity, priority);
/* Set hardware priority.
*/
if (hw_ip == AMDGPU_HW_IP_COMPUTE) {
hw_prio = s2p_prio_map(priority - 2); ## or perhaps from a static inline from a header file, so we wouldn't care for the - 2 here
scheds = adev->gfx.compute_prio_sched[hw_prio];
num_scheds = adev->gfx.num_compute_sched[hw_prio];
drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
}
}
Regards,
Luben
> +
> void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
> enum drm_sched_priority priority)
> {
> @@ -534,13 +560,11 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
> ctx->init_priority : ctx->override_priority;
> for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
> for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
> - struct drm_sched_entity *entity;
> -
> if (!ctx->entities[i][j])
> continue;
>
> - entity = &ctx->entities[i][j]->entity;
> - drm_sched_entity_set_priority(entity, ctx_prio);
> + amdgpu_ctx_set_entity_priority(ctx, ctx->entities[i][j],
> + i, ctx_prio);
> }
> }
> }
> --
> 2.25.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cf436ca206d94469f4ed908d7bf710856%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188364522588061&sdata=kPMrGVCt00XTJVIG5lDFugkv5CxZne8W1Hqqc2baZZg%3D&reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-04 21:41 ` Luben Tuikov
@ 2020-03-04 22:32 ` Luben Tuikov
2020-03-05 6:21 ` Nirmoy
1 sibling, 0 replies; 26+ messages in thread
From: Luben Tuikov @ 2020-03-04 22:32 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-04 4:41 p.m., Luben Tuikov wrote:
> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
[snip]
>> + case DRM_SCHED_PRIORITY_HIGH_HW:
>> + case DRM_SCHED_PRIORITY_KERNEL:
>> + return AMDGPU_GFX_PIPE_PRIO_HIGH;
>> + default:
>> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
>> + }
>
> This can be a map. We're mapping from one integer
> space to another. There is no reason for a jump switch.
>
> For instance,
>
> /* Map of the DRM scheduling priority to pipe
> * priority.
> */
> const enum gfx_pipe_priority s2p_prio_map[] = {
> [0] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [1] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [2] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [3] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [4] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [5] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [6] = AMDGPU_GFX_PIPE_PRIO_HIGH,
> [7] = AMDGPU_GFX_PIPE_PRIO_HIGH,
> [8] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> };
>
> /* Map it!
> */
> pipe_prio = s2p_prio_map[sched_prio + 2]; ## You can view this as y = f(x + 2).
Note that you can make this into a
static inline enum gfx_pipe_priority s2p_prio_map(enum drm_sched_priority sp)
{
return _s2p_prio_map[sched_prio + 2];
}
Regards,
Luben
>
> Note that if you fix enum drm_sched_priority as I described
> in an earlier review, you'd not need the additive factor of 2
> in the above linear transformation.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/4] drm/amdgpu: remove unused functions
2020-03-03 12:50 ` [PATCH 4/4] drm/amdgpu: remove unused functions Nirmoy Das
@ 2020-03-04 22:38 ` Luben Tuikov
0 siblings, 0 replies; 26+ messages in thread
From: Luben Tuikov @ 2020-03-04 22:38 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
> amdgpu statically set priority for compute queues
> at initialization so remove all the functions
> responsible changing compute queue priority dynamically
If this is just a blob of words and not a proper
sentence or paragraph, how can we trust the changes
of this patch series?
"amdgpu" is "AMDGPU", "sets", end with a period.
Regards,
Luben
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> Reviewed-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 70 ----------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 7 --
> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 99 ----------------------
> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 100 -----------------------
> 4 files changed, 276 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index ca6b52054b4b..a7e1d0425ed0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -150,76 +150,6 @@ void amdgpu_ring_undo(struct amdgpu_ring *ring)
> ring->funcs->end_use(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 drm_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 == DRM_SCHED_PRIORITY_NORMAL)
> - return;
> -
> - mutex_lock(&ring->priority_mutex);
> - /* 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 >= DRM_SCHED_PRIORITY_MIN; i--) {
> - if (i == DRM_SCHED_PRIORITY_NORMAL
> - || atomic_read(&ring->num_jobs[i])) {
> - ring->priority = i;
> - ring->funcs->set_priority(ring, i);
> - break;
> - }
> - }
> -
> -out_unlock:
> - mutex_unlock(&ring->priority_mutex);
> -}
> -
> -/**
> - * 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).
> - */
> -void amdgpu_ring_priority_get(struct amdgpu_ring *ring,
> - enum drm_sched_priority priority)
> -{
> - if (!ring->funcs->set_priority)
> - return;
> -
> - if (atomic_inc_return(&ring->num_jobs[priority]) <= 0)
> - return;
> -
> - mutex_lock(&ring->priority_mutex);
> - if (priority <= ring->priority)
> - goto out_unlock;
> -
> - ring->priority = priority;
> - ring->funcs->set_priority(ring, priority);
> -
> -out_unlock:
> - mutex_unlock(&ring->priority_mutex);
> -}
> -
> /**
> * amdgpu_ring_init - init driver ring struct.
> *
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 201c6ac7bf9d..a75e2418a20e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -167,9 +167,6 @@ struct amdgpu_ring_funcs {
> uint32_t reg0, uint32_t reg1,
> uint32_t ref, uint32_t mask);
> void (*emit_tmz)(struct amdgpu_ring *ring, bool start);
> - /* priority functions */
> - void (*set_priority) (struct amdgpu_ring *ring,
> - enum drm_sched_priority priority);
> /* Try to soft recover the ring to make the fence signal */
> void (*soft_recovery)(struct amdgpu_ring *ring, unsigned vmid);
> int (*preempt_ib)(struct amdgpu_ring *ring);
> @@ -259,10 +256,6 @@ 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);
> -void amdgpu_ring_priority_get(struct amdgpu_ring *ring,
> - enum drm_sched_priority priority);
> -void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
> - enum drm_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/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 05b6f01e1228..f5029eb9ac12 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -6275,104 +6275,6 @@ 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;
> -
> - 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(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;
> -
> - mutex_lock(&adev->gfx.pipe_reserve_mutex);
> - pipe = amdgpu_gfx_mec_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_gfx_mec_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_gfx_mec_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);
> - }
> - }
> -
> - mutex_unlock(&adev->gfx.pipe_reserve_mutex);
> -}
> -
> -static void gfx_v8_0_hqd_set_priority(struct amdgpu_device *adev,
> - struct amdgpu_ring *ring,
> - bool acquire)
> -{
> - uint32_t pipe_priority = acquire ? 0x2 : 0x0;
> - uint32_t queue_priority = acquire ? 0xf : 0x0;
> -
> - mutex_lock(&adev->srbm_mutex);
> - vi_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
> -
> - WREG32(mmCP_HQD_PIPE_PRIORITY, pipe_priority);
> - WREG32(mmCP_HQD_QUEUE_PRIORITY, queue_priority);
> -
> - vi_srbm_select(adev, 0, 0, 0, 0);
> - mutex_unlock(&adev->srbm_mutex);
> -}
> -static void gfx_v8_0_ring_set_priority_compute(struct amdgpu_ring *ring,
> - enum drm_sched_priority priority)
> -{
> - struct amdgpu_device *adev = ring->adev;
> - bool acquire = priority == DRM_SCHED_PRIORITY_HIGH_HW;
> -
> - if (ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE)
> - return;
> -
> - gfx_v8_0_hqd_set_priority(adev, ring, acquire);
> - gfx_v8_0_pipe_reserve_resources(adev, ring, acquire);
> -}
> -
> static void gfx_v8_0_ring_emit_fence_compute(struct amdgpu_ring *ring,
> u64 addr, u64 seq,
> unsigned flags)
> @@ -7005,7 +6907,6 @@ 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,
> .emit_wreg = gfx_v8_0_ring_emit_wreg,
> };
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index ef263d075e06..a27b18d58aca 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -5130,105 +5130,6 @@ static u64 gfx_v9_0_ring_get_wptr_compute(struct amdgpu_ring *ring)
> return wptr;
> }
>
> -static void gfx_v9_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;
> -
> - reg = SOC15_REG_OFFSET(GC, 0, mmSPI_WCL_PIPE_PERCENT_GFX) + pipe_num;
> - tmp = RREG32(reg);
> - tmp = REG_SET_FIELD(tmp, SPI_WCL_PIPE_PERCENT_GFX, VALUE, pipe_percent);
> - WREG32(reg, tmp);
> -}
> -
> -static void gfx_v9_0_pipe_reserve_resources(struct amdgpu_device *adev,
> - struct amdgpu_ring *ring,
> - bool acquire)
> -{
> - int i, pipe;
> - bool reserve;
> - struct amdgpu_ring *iring;
> -
> - mutex_lock(&adev->gfx.pipe_reserve_mutex);
> - pipe = amdgpu_gfx_mec_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_v9_0_ring_set_pipe_percent(&adev->gfx.gfx_ring[i],
> - true);
> -
> - for (i = 0; i < adev->gfx.num_compute_rings; ++i)
> - gfx_v9_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_gfx_mec_queue_to_bit(adev,
> - iring->me,
> - iring->pipe,
> - 0);
> - reserve = test_bit(pipe, adev->gfx.pipe_reserve_bitmap);
> - gfx_v9_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_gfx_mec_queue_to_bit(adev,
> - iring->me,
> - iring->pipe,
> - 0);
> - reserve = test_bit(pipe, adev->gfx.pipe_reserve_bitmap);
> - gfx_v9_0_ring_set_pipe_percent(iring, reserve);
> - }
> - }
> -
> - mutex_unlock(&adev->gfx.pipe_reserve_mutex);
> -}
> -
> -static void gfx_v9_0_hqd_set_priority(struct amdgpu_device *adev,
> - struct amdgpu_ring *ring,
> - bool acquire)
> -{
> - uint32_t pipe_priority = acquire ? 0x2 : 0x0;
> - uint32_t queue_priority = acquire ? 0xf : 0x0;
> -
> - mutex_lock(&adev->srbm_mutex);
> - soc15_grbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
> -
> - WREG32_SOC15_RLC(GC, 0, mmCP_HQD_PIPE_PRIORITY, pipe_priority);
> - WREG32_SOC15_RLC(GC, 0, mmCP_HQD_QUEUE_PRIORITY, queue_priority);
> -
> - soc15_grbm_select(adev, 0, 0, 0, 0);
> - mutex_unlock(&adev->srbm_mutex);
> -}
> -
> -static void gfx_v9_0_ring_set_priority_compute(struct amdgpu_ring *ring,
> - enum drm_sched_priority priority)
> -{
> - struct amdgpu_device *adev = ring->adev;
> - bool acquire = priority == DRM_SCHED_PRIORITY_HIGH_HW;
> -
> - if (ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE)
> - return;
> -
> - gfx_v9_0_hqd_set_priority(adev, ring, acquire);
> - gfx_v9_0_pipe_reserve_resources(adev, ring, acquire);
> -}
> -
> static void gfx_v9_0_ring_set_wptr_compute(struct amdgpu_ring *ring)
> {
> struct amdgpu_device *adev = ring->adev;
> @@ -6599,7 +6500,6 @@ static const struct amdgpu_ring_funcs gfx_v9_0_ring_funcs_compute = {
> .test_ib = gfx_v9_0_ring_test_ib,
> .insert_nop = amdgpu_ring_insert_nop,
> .pad_ib = amdgpu_ring_generic_pad_ib,
> - .set_priority = gfx_v9_0_ring_set_priority_compute,
> .emit_wreg = gfx_v9_0_ring_emit_wreg,
> .emit_reg_wait = gfx_v9_0_ring_emit_reg_wait,
> .emit_reg_write_reg_wait = gfx_v9_0_ring_emit_reg_write_reg_wait,
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-04 21:41 ` Luben Tuikov
2020-03-04 22:32 ` Luben Tuikov
@ 2020-03-05 6:21 ` Nirmoy
2020-03-05 18:42 ` Luben Tuikov
1 sibling, 1 reply; 26+ messages in thread
From: Nirmoy @ 2020-03-05 6:21 UTC (permalink / raw)
To: Luben Tuikov, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 3/4/20 10:41 PM, Luben Tuikov wrote:
> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
>> We were changing compute ring priority while rings were being used
>> before every job submission which is not recommended. This patch
>> sets compute queue priority at mqd initialization for gfx8, gfx9 and
>> gfx10.
>>
>> Policy: make queue 0 of each pipe as high priority compute queue
>>
>> High/normal priority compute sched lists are generated from set of high/normal
>> priority compute queues. At context creation, entity of compute queue
>> get a sched list from high or normal priority depending on ctx->priority
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 --
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 60 +++++++++++++++++++++---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 8 ++++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 15 +++++-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 +
>> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 19 ++++++++
>> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 23 +++++++--
>> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 20 ++++++++
>> 9 files changed, 135 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index f397ff97b4e4..8304d0c87899 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1205,7 +1205,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>> struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
>> struct drm_sched_entity *entity = p->entity;
>> enum drm_sched_priority priority;
>> - struct amdgpu_ring *ring;
>> struct amdgpu_bo_list_entry *e;
>> struct amdgpu_job *job;
>> uint64_t seq;
>> @@ -1258,9 +1257,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>> priority = job->base.s_priority;
>> drm_sched_entity_push_job(&job->base, entity);
>>
>> - ring = to_amdgpu_ring(entity->rq->sched);
>> - amdgpu_ring_priority_get(ring, priority);
>> -
>> amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);
>>
>> ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index 94a6c42f29ea..4ad944f85672 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -61,12 +61,30 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
>> return -EACCES;
>> }
>>
>> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_compute_prio(enum drm_sched_priority prio)
>> +{
>> + switch(prio) {
> LKCS wants a space after a keyword ("switch") and before parenthesis "(".
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
>
> Please read the LKCS in you local Linux source code:
> Documentation/process/coding-style.rst
> so we don't have to point that out anymore.
Yes this is happening more than often with me. I will be careful.
>
>> + case DRM_SCHED_PRIORITY_MIN:
>> + case DRM_SCHED_PRIORITY_NORMAL:
>> + case DRM_SCHED_PRIORITY_HIGH_SW:
>> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
> This is taken care of by the "default:" label and
> unnecessary here (can be removed completely).
Thanks!
>
>> + case DRM_SCHED_PRIORITY_HIGH_HW:
>> + case DRM_SCHED_PRIORITY_KERNEL:
>> + return AMDGPU_GFX_PIPE_PRIO_HIGH;
>> + default:
>> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
>> + }
> This can be a map. We're mapping from one integer
> space to another. There is no reason for a jump switch.
>
> For instance,
>
> /* Map of the DRM scheduling priority to pipe
> * priority.
> */
> const enum gfx_pipe_priority s2p_prio_map[] = {
> [0] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [1] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [2] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [3] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [4] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [5] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> [6] = AMDGPU_GFX_PIPE_PRIO_HIGH,
> [7] = AMDGPU_GFX_PIPE_PRIO_HIGH,
> [8] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
> };
>
> /* Map it!
> */
> pipe_prio = s2p_prio_map[sched_prio + 2]; ## You can view this as y = f(x + 2).
I think that would be over-engineering for not so much of extra benefit.
A switch statement here is more expressive and
bit more immune to changes that might happen to DRM_SCHED_PRIORITY_*. I
would rather let compiler does its
job.
>
> Note that if you fix enum drm_sched_priority as I described
> in an earlier review, you'd not need the additive factor of 2
> in the above linear transformation.
A quick search leads me amdgpu_sched_ioctl() which is using
DRM_SCHED_PRIORITY_INVALID
to indicate a invalid value from userspace. I don't know much about drm
api to suggest any useful
changes regarding this. But again this isn't in the scope of this patch
series.
>
>> +
>> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
> You don't need a return here at all, when you have a "default:" label
> up there.
I was doing to to make gcc happy but it seems gcc is happy even without
that.
>> +}
>> +
>> static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const u32 ring)
>> {
>> struct amdgpu_device *adev = ctx->adev;
>> struct amdgpu_ctx_entity *entity;
>> struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
>> unsigned num_scheds = 0;
>> + enum gfx_pipe_priority hw_prio;
>> enum drm_sched_priority priority;
>> int r;
>>
>> @@ -85,8 +103,9 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const
>> num_scheds = 1;
>> break;
>> case AMDGPU_HW_IP_COMPUTE:
>> - scheds = adev->gfx.compute_sched;
>> - num_scheds = adev->gfx.num_compute_sched;
>> + hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
>> + scheds = adev->gfx.compute_prio_sched[hw_prio];
>> + num_scheds = adev->gfx.num_compute_sched[hw_prio];
>> break;
>> case AMDGPU_HW_IP_DMA:
>> scheds = adev->sdma.sdma_sched;
>> @@ -628,20 +647,47 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
>> mutex_destroy(&mgr->lock);
>> }
>>
>> +
>> +static void amdgpu_ctx_init_compute_sched(struct amdgpu_device *adev)
>> +{
>> + int num_compute_sched_normal = 0;
>> + int num_compute_sched_high = AMDGPU_MAX_COMPUTE_RINGS - 1;
>> + int i;
>> +
>> + /* fill compute_sched array as: start from 0th index for normal priority scheds and
>> + * start from (last_index - num_compute_sched_normal) for high priority
>> + * scheds */
> See the LKCS guide here on comments.
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
>
> To quote from Documentation/process/coding-style.rst:
>
> "NEVER
> try to explain HOW your code works in a comment: it's much better to
> write the code so that the **working** is obvious, and it's a waste of
> time to explain badly written code.
>
> Generally, you want your comments to tell WHAT your code does, not HOW."
>
> Please comment on why and what you're doing as opposed to how.
> Or if you don't want to do this, simply remove the comment.
>
> Remember, you want to make it clear why and what the code does here,
> as opposed to how it's doing it.
noted.
>
> Use this style of commenting, like Christian does:
>
> previous code ends here;
>
> /* Comment line 1.
> * Comment line 2.
> */
> code starts here--no extra empty lines...
>
>> + for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>> + if (!adev->gfx.compute_ring[i].has_high_prio)
>> + adev->gfx.compute_sched[num_compute_sched_normal++] =
>> + &adev->gfx.compute_ring[i].sched;
>> + else
>> + adev->gfx.compute_sched[num_compute_sched_high--] =
>> + &adev->gfx.compute_ring[i].sched;
>> + }
>> +
>> + /* compute ring only has two priority for now */
>> + i = AMDGPU_GFX_PIPE_PRIO_NORMAL;
>> + adev->gfx.compute_prio_sched[i] = &adev->gfx.compute_sched[0];
>> + adev->gfx.num_compute_sched[i] = num_compute_sched_normal;
>> +
>> + i = AMDGPU_GFX_PIPE_PRIO_HIGH;
>> + adev->gfx.compute_prio_sched[i] =
>> + &adev->gfx.compute_sched[num_compute_sched_high - 1];
>> + adev->gfx.num_compute_sched[i] =
>> + adev->gfx.num_compute_rings - num_compute_sched_normal;
>> +}
>> +
>> void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
>> {
>> int i, j;
>>
>> + amdgpu_ctx_init_compute_sched(adev);
>> for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
>> adev->gfx.gfx_sched[i] = &adev->gfx.gfx_ring[i].sched;
>> adev->gfx.num_gfx_sched++;
>> }
>>
>> - for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>> - adev->gfx.compute_sched[i] = &adev->gfx.compute_ring[i].sched;
>> - adev->gfx.num_compute_sched++;
>> - }
>> -
>> for (i = 0; i < adev->sdma.num_instances; i++) {
>> adev->sdma.sdma_sched[i] = &adev->sdma.instance[i].ring.sched;
>> adev->sdma.num_sdma_sched++;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> index 7403588684b3..952725e7243c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> @@ -192,6 +192,14 @@ static bool amdgpu_gfx_is_multipipe_capable(struct amdgpu_device *adev)
>> return adev->gfx.mec.num_mec > 1;
>> }
>>
>> +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
>> + int queue)
>> +{
>> + /* Policy: make queue 0 of each pipe as high priority compute queue */
>> + return (queue == 0);
>> +
>> +}
>> +
>> void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev)
>> {
>> int i, queue, pipe, mec;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> index 37ba05b63b2a..47a5cdae28c5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>> @@ -41,6 +41,16 @@
>> #define AMDGPU_MAX_GFX_QUEUES KGD_MAX_QUEUES
>> #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
>>
>> +enum gfx_pipe_priority {
>> + AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
>> + AMDGPU_GFX_PIPE_PRIO_HIGH,
>> + AMDGPU_GFX_PIPE_PRIO_MAX
> Please don't call it "MAX". "Maximum value" means
> that that value is in the domain of values
> the priory can take, and it is not. That is,
> priority could never be AMDGPU_GFX_PIPE_PRIO_MAX.
>
> If you want to know the size of the domain of values,
> just call it that:
> AMDGPU_GFX_PIPE_PRIO_NUM/_SIZE.
>
> enum gfx_pipe_priority {
> AMDGPU_GFX_PIPE_PRIO_UNDEFINED,
> AMDGPU_GFX_PIPE_PRIO_NORMAL,
> AMDGPU_GFX_PIPE_PRIO_HIGH,
>
> AMDGPU_GFX_PIPE_PRIO_SIZE,
> };
>
> Naturally 0 would be undefined, yet named.
> Note the empty line before the size.
>
> Regards,
> Luben
>
>> +
>> +};
>> +
>> +#define AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM 0
>> +#define AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM 15
>> +
>> struct amdgpu_mec {
>> struct amdgpu_bo *hpd_eop_obj;
>> u64 hpd_eop_gpu_addr;
>> @@ -280,8 +290,9 @@ struct amdgpu_gfx {
>> uint32_t num_gfx_sched;
>> unsigned num_gfx_rings;
>> struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
>> + struct drm_gpu_scheduler **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
>> struct drm_gpu_scheduler *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
>> - uint32_t num_compute_sched;
>> + uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
>> unsigned num_compute_rings;
>> struct amdgpu_irq_src eop_irq;
>> struct amdgpu_irq_src priv_reg_irq;
>> @@ -363,6 +374,8 @@ void amdgpu_gfx_bit_to_mec_queue(struct amdgpu_device *adev, int bit,
>> int *mec, int *pipe, int *queue);
>> bool amdgpu_gfx_is_mec_queue_enabled(struct amdgpu_device *adev, int mec,
>> int pipe, int queue);
>> +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
>> + int queue);
>> int amdgpu_gfx_me_queue_to_bit(struct amdgpu_device *adev, int me,
>> int pipe, int queue);
>> void amdgpu_gfx_bit_to_me_queue(struct amdgpu_device *adev, int bit,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> index d42be880a236..4981e443a884 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>> @@ -117,12 +117,10 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
>>
>> static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
>> {
>> - struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
>> struct amdgpu_job *job = to_amdgpu_job(s_job);
>>
>> drm_sched_job_cleanup(s_job);
>>
>> - amdgpu_ring_priority_put(ring, s_job->s_priority);
>> dma_fence_put(job->fence);
>> amdgpu_sync_free(&job->sync);
>> amdgpu_sync_free(&job->sched_sync);
>> @@ -143,7 +141,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
>> void *owner, struct dma_fence **f)
>> {
>> enum drm_sched_priority priority;
>> - struct amdgpu_ring *ring;
>> int r;
>>
>> if (!f)
>> @@ -158,9 +155,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
>> priority = job->base.s_priority;
>> drm_sched_entity_push_job(&job->base, entity);
>>
>> - ring = to_amdgpu_ring(entity->rq->sched);
>> - amdgpu_ring_priority_get(ring, priority);
>> -
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> index 24caff085d00..dcea1ef92883 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>> @@ -222,6 +222,7 @@ struct amdgpu_ring {
>> struct mutex priority_mutex;
>> /* protected by priority_mutex */
>> int priority;
>> + bool has_high_prio;
>>
>> #if defined(CONFIG_DEBUG_FS)
>> struct dentry *ent;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> index 96cf617e41d1..c1da41e35323 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>> @@ -3211,6 +3211,22 @@ static int gfx_v10_0_cp_async_gfx_ring_resume(struct amdgpu_device *adev)
>> return r;
>> }
>>
>> +static void gfx_v10_0_compute_mqd_set_priority(struct amdgpu_ring *ring, struct v10_compute_mqd *mqd)
>> +{
>> + struct amdgpu_device *adev = ring->adev;
>> +
>> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
>> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
>> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
>> + ring->has_high_prio = true;
>> + mqd->cp_hqd_queue_priority =
>> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
>> + } else {
>> + ring->has_high_prio = false;
>> + }
>> + }
>> +}
>> +
>> static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
>> {
>> struct amdgpu_device *adev = ring->adev;
>> @@ -3336,6 +3352,9 @@ static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
>> tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
>> mqd->cp_hqd_ib_control = tmp;
>>
>> + /* set static priority for a compute queue/ring */
>> + gfx_v10_0_compute_mqd_set_priority(ring, mqd);
>> +
>> /* map_queues packet doesn't need activate the queue,
>> * so only kiq need set this field.
>> */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> index 393a1324daa9..668c8eb2b2cc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>> @@ -4430,6 +4430,22 @@ static int gfx_v8_0_deactivate_hqd(struct amdgpu_device *adev, u32 req)
>> return r;
>> }
>>
>> +static void gfx_v8_0_mqd_set_priority(struct amdgpu_ring *ring, struct vi_mqd *mqd)
>> +{
>> + struct amdgpu_device *adev = ring->adev;
>> +
>> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
>> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
>> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
>> + ring->has_high_prio = true;
>> + mqd->cp_hqd_queue_priority =
>> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
>> + } else {
>> + ring->has_high_prio = false;
>> + }
>> + }
>> +}
>> +
>> static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
>> {
>> struct amdgpu_device *adev = ring->adev;
>> @@ -4553,9 +4569,6 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
>> /* defaults */
>> mqd->cp_hqd_eop_rptr = RREG32(mmCP_HQD_EOP_RPTR);
>> mqd->cp_hqd_eop_wptr = RREG32(mmCP_HQD_EOP_WPTR);
>> - mqd->cp_hqd_pipe_priority = RREG32(mmCP_HQD_PIPE_PRIORITY);
>> - mqd->cp_hqd_queue_priority = RREG32(mmCP_HQD_QUEUE_PRIORITY);
>> - mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
>> mqd->cp_hqd_ctx_save_base_addr_lo = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_LO);
>> mqd->cp_hqd_ctx_save_base_addr_hi = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_HI);
>> mqd->cp_hqd_cntl_stack_offset = RREG32(mmCP_HQD_CNTL_STACK_OFFSET);
>> @@ -4567,6 +4580,10 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
>> mqd->cp_hqd_eop_wptr_mem = RREG32(mmCP_HQD_EOP_WPTR_MEM);
>> mqd->cp_hqd_eop_dones = RREG32(mmCP_HQD_EOP_DONES);
>>
>> + /* set static priority for a queue/ring */
>> + gfx_v8_0_mqd_set_priority(ring, mqd);
>> + mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
>> +
>> /* map_queues packet doesn't need activate the queue,
>> * so only kiq need set this field.
>> */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> index 015647959d69..726d1ac41637 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>> @@ -3314,6 +3314,22 @@ static void gfx_v9_0_kiq_setting(struct amdgpu_ring *ring)
>> WREG32_SOC15_RLC(GC, 0, mmRLC_CP_SCHEDULERS, tmp);
>> }
>>
>> +static void gfx_v9_0_mqd_set_priority(struct amdgpu_ring *ring, struct v9_mqd *mqd)
>> +{
>> + struct amdgpu_device *adev = ring->adev;
>> +
>> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
>> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
>> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
>> + ring->has_high_prio = true;
>> + mqd->cp_hqd_queue_priority =
>> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
>> + } else {
>> + ring->has_high_prio = false;
>> + }
>> + }
>> +}
>> +
>> static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
>> {
>> struct amdgpu_device *adev = ring->adev;
>> @@ -3450,6 +3466,10 @@ static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
>> tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
>> mqd->cp_hqd_ib_control = tmp;
>>
>> + /* set static priority for a queue/ring */
>> + gfx_v9_0_mqd_set_priority(ring, mqd);
>> + mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
>> +
>> /* map_queues packet doesn't need activate the queue,
>> * so only kiq need set this field.
>> */
>> --
>> 2.25.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cfa70ffa2d9874bbe1a5908d7bf71077e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188364502419515&sdata=vIM2clFPPCfIDK24UxAiJDZ5JH%2F83K2JESZtZKTGQUc%3D&reserved=0
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list
2020-03-04 22:00 ` Luben Tuikov
@ 2020-03-05 6:23 ` Nirmoy
2020-03-05 18:46 ` Luben Tuikov
2020-03-05 9:10 ` Nirmoy
1 sibling, 1 reply; 26+ messages in thread
From: Nirmoy @ 2020-03-05 6:23 UTC (permalink / raw)
To: Luben Tuikov, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 3/4/20 11:00 PM, Luben Tuikov wrote:
> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
>> implement drm_sched_entity_modify_sched() which can modify existing
>> sched_list with a different one. This is going to be helpful when
>> userspace changes priority of a ctx/entity then driver can switch to
>> corresponding hw shced list for that priority
>>
> This commit message should start with a capitalized sentence:
> "implement" --> "Implement". "can modify" --> "modifies"
>
> Also a spell check should be done on it before committing:
> "shced" --> "sched".
>
> "then the driver", "to the corresponding", "HW scheduler".
>
> And the commit message paragraph should end with a period.
Thanks!
>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> ---
>> drivers/gpu/drm/scheduler/sched_entity.c | 19 +++++++++++++++++++
>> include/drm/gpu_scheduler.h | 4 ++++
>> 2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 63bccd201b97..b94312154e56 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -83,6 +83,25 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>> }
>> EXPORT_SYMBOL(drm_sched_entity_init);
>>
>> +/**
>> + * drm_sched_entity_modify_sched - Modify sched of an entity
>> + *
> This empty line is unncessary.
>
>> + * @entity: scheduler entity to init
>> + * @sched_list: the list of new drm scheds which will replace
>> + * existing entity->sched_list
>> + * @num_sched_list: number of drm sched in sched_list
>> + */
>> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>> + struct drm_gpu_scheduler **sched_list,
>> + unsigned int num_sched_list)
>> +{
>> + WARN_ON(!num_sched_list || !sched_list);
>> +
>> + entity->sched_list = sched_list;
>> + entity->num_sched_list = num_sched_list;
>> +}
>> +EXPORT_SYMBOL(drm_sched_entity_modify_sched);
>> +
>> /**
>> * drm_sched_entity_is_idle - Check if entity is idle
>> *
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 589be851f8a1..f70a84aaaf7a 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -297,6 +297,10 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched);
>> int drm_sched_job_init(struct drm_sched_job *job,
>> struct drm_sched_entity *entity,
>> void *owner);
>> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>> + struct drm_gpu_scheduler **sched_list,
>> + unsigned int num_sched_list);
>> +
This generally doesn't happen with my current vi config. I will be extra
careful.
> Again, the argument list here is unaligned. Please align it
> correctly. The correct indentation would add the maximum number of
> leading TAB chars followed by the 0 to 7 spaces, to align
> the argument list to the first argument in the opening parenthesis.
> The LKCS describes this and also has the settings for Emacs.
> In Emacs, pressing the TAB key aligns, after which pressing it more
> does nothing (on an aligned line).
>
> Regards,
> Luben
>
>
>> void drm_sched_job_cleanup(struct drm_sched_job *job);
>> void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>> void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>> --
>> 2.25.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cbe8445945c194bd8b3cc08d7bf7109c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188364557354217&sdata=UAxV2NtkNcj6VXAPhDbk4GliUOnPEfuLOH4BVuOrqdw%3D&reserved=0
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override
2020-03-04 22:25 ` Luben Tuikov
@ 2020-03-05 6:28 ` Nirmoy
2020-03-05 18:51 ` Luben Tuikov
0 siblings, 1 reply; 26+ messages in thread
From: Nirmoy @ 2020-03-05 6:28 UTC (permalink / raw)
To: Luben Tuikov, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 3/4/20 11:25 PM, Luben Tuikov wrote:
> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
>> Switch to appropriate sched list for an entity on priority override.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 32 +++++++++++++++++++++----
>> 1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> index 8c52152e3a6e..a0bf14ab9d33 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> @@ -522,6 +522,32 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
>> return fence;
>> }
>>
>> +static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
>> + struct amdgpu_ctx_entity *aentity,
>> + int hw_ip, enum drm_sched_priority priority)
>> +{
>> + struct amdgpu_device *adev = ctx->adev;
>> + enum gfx_pipe_priority hw_prio;
>> + struct drm_gpu_scheduler **scheds = NULL;
>> + unsigned num_scheds;
>> +
>> + /* set sw priority */
>> + drm_sched_entity_set_priority(&aentity->entity, priority);
>> +
>> + /* set hw priority */
>> + switch (hw_ip) {
>> + case AMDGPU_HW_IP_COMPUTE:
>> + hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
>> + scheds = adev->gfx.compute_prio_sched[hw_prio];
>> + num_scheds = adev->gfx.num_compute_sched[hw_prio];
>> + break;
>> + default:
>> + return;
>> + }
>> +
>> + drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
>> +}
> I'd rather this not be over-engineered (in expectations of more case labels,
> and a simple if-else to do it. Over-engineering it "just in case" creates
> difficult to maintain code. I believe there is a document about this somewhere
> in Documentation/.
This switch is in expectation of extending it for VCN and GFX but I
guess that can wait for now,.
thanks,
Nirmoy
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list
2020-03-04 22:00 ` Luben Tuikov
2020-03-05 6:23 ` Nirmoy
@ 2020-03-05 9:10 ` Nirmoy
2020-03-05 18:56 ` Luben Tuikov
1 sibling, 1 reply; 26+ messages in thread
From: Nirmoy @ 2020-03-05 9:10 UTC (permalink / raw)
To: Luben Tuikov, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 3/4/20 11:00 PM, Luben Tuikov wrote:
> struct drm_sched_entity *entity,
>> void *owner);
>> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>> + struct drm_gpu_scheduler **sched_list,
>> + unsigned int num_sched_list);
>> +
> Again, the argument list here is unaligned. Please align it
looks good with cat and vim. Not sure why git format-patch is acting
strange in this case
Nirmoy
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-05 6:21 ` Nirmoy
@ 2020-03-05 18:42 ` Luben Tuikov
2020-03-05 21:13 ` Nirmoy
0 siblings, 1 reply; 26+ messages in thread
From: Luben Tuikov @ 2020-03-05 18:42 UTC (permalink / raw)
To: Nirmoy, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
It is very difficult to find your comment replies when
you do not add an empty line around them.
Do you not see how everyone responds and adds
an empty line around them?
Why don't you?
cont'd below
On 2020-03-05 01:21, Nirmoy wrote:
>
> On 3/4/20 10:41 PM, Luben Tuikov wrote:
>> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
>>> We were changing compute ring priority while rings were being used
>>> before every job submission which is not recommended. This patch
>>> sets compute queue priority at mqd initialization for gfx8, gfx9 and
>>> gfx10.
>>>
>>> Policy: make queue 0 of each pipe as high priority compute queue
>>>
>>> High/normal priority compute sched lists are generated from set of high/normal
>>> priority compute queues. At context creation, entity of compute queue
>>> get a sched list from high or normal priority depending on ctx->priority
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 --
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 60 +++++++++++++++++++++---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 8 ++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 15 +++++-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 +
>>> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 19 ++++++++
>>> drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 23 +++++++--
>>> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 20 ++++++++
>>> 9 files changed, 135 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index f397ff97b4e4..8304d0c87899 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -1205,7 +1205,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>> struct amdgpu_fpriv *fpriv = p->filp->driver_priv;
>>> struct drm_sched_entity *entity = p->entity;
>>> enum drm_sched_priority priority;
>>> - struct amdgpu_ring *ring;
>>> struct amdgpu_bo_list_entry *e;
>>> struct amdgpu_job *job;
>>> uint64_t seq;
>>> @@ -1258,9 +1257,6 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>> priority = job->base.s_priority;
>>> drm_sched_entity_push_job(&job->base, entity);
>>>
>>> - ring = to_amdgpu_ring(entity->rq->sched);
>>> - amdgpu_ring_priority_get(ring, priority);
>>> -
>>> amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);
>>>
>>> ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> index 94a6c42f29ea..4ad944f85672 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> @@ -61,12 +61,30 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
>>> return -EACCES;
>>> }
>>>
>>> +static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_compute_prio(enum drm_sched_priority prio)
>>> +{
>>> + switch(prio) {
>> LKCS wants a space after a keyword ("switch") and before parenthesis "(".
>> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
>>
>> Please read the LKCS in you local Linux source code:
>> Documentation/process/coding-style.rst
>> so we don't have to point that out anymore.
> Yes this is happening more than often with me. I will be careful.
>>
See above? No empty lines around your reply.
Just read the LKCS document a few times.
>>> + case DRM_SCHED_PRIORITY_MIN:
>>> + case DRM_SCHED_PRIORITY_NORMAL:
>>> + case DRM_SCHED_PRIORITY_HIGH_SW:
>>> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
>> This is taken care of by the "default:" label and
>> unnecessary here (can be removed completely).
> Thanks!
>>
>>> + case DRM_SCHED_PRIORITY_HIGH_HW:
>>> + case DRM_SCHED_PRIORITY_KERNEL:
>>> + return AMDGPU_GFX_PIPE_PRIO_HIGH;
>>> + default:
>>> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
>>> + }
>> This can be a map. We're mapping from one integer
>> space to another. There is no reason for a jump switch.
>>
>> For instance,
>>
>> /* Map of the DRM scheduling priority to pipe
>> * priority.
>> */
>> const enum gfx_pipe_priority s2p_prio_map[] = {
>> [0] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
>> [1] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
>> [2] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
>> [3] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
>> [4] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
>> [5] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
>> [6] = AMDGPU_GFX_PIPE_PRIO_HIGH,
>> [7] = AMDGPU_GFX_PIPE_PRIO_HIGH,
>> [8] = AMDGPU_GFX_PIPE_PRIO_NORMAL,
>> };
>>
>> /* Map it!
>> */
>> pipe_prio = s2p_prio_map[sched_prio + 2]; ## You can view this as y = f(x + 2).
>
> I think that would be over-engineering for not so much of extra benefit.
> A switch statement here is more expressive and
>
> bit more immune to changes that might happen to DRM_SCHED_PRIORITY_*. I
> would rather let compiler does its
>
> job.
Nirmoy, that's nonsense.
You're using a dynamic method (code execution) for something which could
be a static lookup.
A static map with an inline gives a much better code footprint, much more
direct and instant code execution than a switch-case statement.
Imagine this as an inverview question.
This is a NAK from me on this patch.
>
>>
>> Note that if you fix enum drm_sched_priority as I described
>> in an earlier review, you'd not need the additive factor of 2
>> in the above linear transformation.
>
> A quick search leads me amdgpu_sched_ioctl() which is using
> DRM_SCHED_PRIORITY_INVALID
>
> to indicate a invalid value from userspace. I don't know much about drm
> api to suggest any useful
>
> changes regarding this. But again this isn't in the scope of this patch
> series.
I'm not asking you to eliminate "DRM_SCHED_PRIORITY_INVALID".
Oh wait! You forgot what I suggested in a previous review on
how to fix enum drm_sched_priority, did you? :-D Search
for that email.
>
>>
>>> +
>>> + return AMDGPU_GFX_PIPE_PRIO_NORMAL;
>> You don't need a return here at all, when you have a "default:" label
>> up there.
> I was doing to to make gcc happy but it seems gcc is happy even without
> that.
>>> +}
Your comment above--very difficult to see. Why do you not make the effort,
like everyone does here, to add an empty line above and below
your comment?
Yes, GCC is happy without the return as you've written it--as I told you.
GCC can prove the function complete using different methods--it's not
explicitly searching for a "return X;" statement.
>>> +
>>> static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const u32 ring)
>>> {
>>> struct amdgpu_device *adev = ctx->adev;
>>> struct amdgpu_ctx_entity *entity;
>>> struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
>>> unsigned num_scheds = 0;
>>> + enum gfx_pipe_priority hw_prio;
>>> enum drm_sched_priority priority;
>>> int r;
>>>
>>> @@ -85,8 +103,9 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, const u32 hw_ip, const
>>> num_scheds = 1;
>>> break;
>>> case AMDGPU_HW_IP_COMPUTE:
>>> - scheds = adev->gfx.compute_sched;
>>> - num_scheds = adev->gfx.num_compute_sched;
>>> + hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
>>> + scheds = adev->gfx.compute_prio_sched[hw_prio];
>>> + num_scheds = adev->gfx.num_compute_sched[hw_prio];
>>> break;
>>> case AMDGPU_HW_IP_DMA:
>>> scheds = adev->sdma.sdma_sched;
>>> @@ -628,20 +647,47 @@ void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr)
>>> mutex_destroy(&mgr->lock);
>>> }
>>>
>>> +
>>> +static void amdgpu_ctx_init_compute_sched(struct amdgpu_device *adev)
>>> +{
>>> + int num_compute_sched_normal = 0;
>>> + int num_compute_sched_high = AMDGPU_MAX_COMPUTE_RINGS - 1;
>>> + int i;
>>> +
>>> + /* fill compute_sched array as: start from 0th index for normal priority scheds and
>>> + * start from (last_index - num_compute_sched_normal) for high priority
>>> + * scheds */
>> See the LKCS guide here on comments.
>> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting
>>
>> To quote from Documentation/process/coding-style.rst:
>>
>> "NEVER
>> try to explain HOW your code works in a comment: it's much better to
>> write the code so that the **working** is obvious, and it's a waste of
>> time to explain badly written code.
>>
>> Generally, you want your comments to tell WHAT your code does, not HOW."
>>
>> Please comment on why and what you're doing as opposed to how.
>> Or if you don't want to do this, simply remove the comment.
>>
>> Remember, you want to make it clear why and what the code does here,
>> as opposed to how it's doing it.
> noted.
>>
Another one of your comments, which is very difficult to see. Why
not include an empty line above and below your replied comments,
like everyone does?
Regards,
Luben
>> Use this style of commenting, like Christian does:
>>
>> previous code ends here;
>>
>> /* Comment line 1.
>> * Comment line 2.
>> */
>> code starts here--no extra empty lines...
>>
>>> + for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>>> + if (!adev->gfx.compute_ring[i].has_high_prio)
>>> + adev->gfx.compute_sched[num_compute_sched_normal++] =
>>> + &adev->gfx.compute_ring[i].sched;
>>> + else
>>> + adev->gfx.compute_sched[num_compute_sched_high--] =
>>> + &adev->gfx.compute_ring[i].sched;
>>> + }
>>> +
>>> + /* compute ring only has two priority for now */
>>> + i = AMDGPU_GFX_PIPE_PRIO_NORMAL;
>>> + adev->gfx.compute_prio_sched[i] = &adev->gfx.compute_sched[0];
>>> + adev->gfx.num_compute_sched[i] = num_compute_sched_normal;
>>> +
>>> + i = AMDGPU_GFX_PIPE_PRIO_HIGH;
>>> + adev->gfx.compute_prio_sched[i] =
>>> + &adev->gfx.compute_sched[num_compute_sched_high - 1];
>>> + adev->gfx.num_compute_sched[i] =
>>> + adev->gfx.num_compute_rings - num_compute_sched_normal;
>>> +}
>>> +
>>> void amdgpu_ctx_init_sched(struct amdgpu_device *adev)
>>> {
>>> int i, j;
>>>
>>> + amdgpu_ctx_init_compute_sched(adev);
>>> for (i = 0; i < adev->gfx.num_gfx_rings; i++) {
>>> adev->gfx.gfx_sched[i] = &adev->gfx.gfx_ring[i].sched;
>>> adev->gfx.num_gfx_sched++;
>>> }
>>>
>>> - for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>>> - adev->gfx.compute_sched[i] = &adev->gfx.compute_ring[i].sched;
>>> - adev->gfx.num_compute_sched++;
>>> - }
>>> -
>>> for (i = 0; i < adev->sdma.num_instances; i++) {
>>> adev->sdma.sdma_sched[i] = &adev->sdma.instance[i].ring.sched;
>>> adev->sdma.num_sdma_sched++;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> index 7403588684b3..952725e7243c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> @@ -192,6 +192,14 @@ static bool amdgpu_gfx_is_multipipe_capable(struct amdgpu_device *adev)
>>> return adev->gfx.mec.num_mec > 1;
>>> }
>>>
>>> +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
>>> + int queue)
>>> +{
>>> + /* Policy: make queue 0 of each pipe as high priority compute queue */
>>> + return (queue == 0);
>>> +
>>> +}
>>> +
>>> void amdgpu_gfx_compute_queue_acquire(struct amdgpu_device *adev)
>>> {
>>> int i, queue, pipe, mec;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>>> index 37ba05b63b2a..47a5cdae28c5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
>>> @@ -41,6 +41,16 @@
>>> #define AMDGPU_MAX_GFX_QUEUES KGD_MAX_QUEUES
>>> #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES
>>>
>>> +enum gfx_pipe_priority {
>>> + AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
>>> + AMDGPU_GFX_PIPE_PRIO_HIGH,
>>> + AMDGPU_GFX_PIPE_PRIO_MAX
>> Please don't call it "MAX". "Maximum value" means
>> that that value is in the domain of values
>> the priory can take, and it is not. That is,
>> priority could never be AMDGPU_GFX_PIPE_PRIO_MAX.
>>
>> If you want to know the size of the domain of values,
>> just call it that:
>> AMDGPU_GFX_PIPE_PRIO_NUM/_SIZE.
>>
>> enum gfx_pipe_priority {
>> AMDGPU_GFX_PIPE_PRIO_UNDEFINED,
>> AMDGPU_GFX_PIPE_PRIO_NORMAL,
>> AMDGPU_GFX_PIPE_PRIO_HIGH,
>>
>> AMDGPU_GFX_PIPE_PRIO_SIZE,
>> };
>>
>> Naturally 0 would be undefined, yet named.
>> Note the empty line before the size.
>>
>> Regards,
>> Luben
>>
>>> +
>>> +};
>>> +
>>> +#define AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM 0
>>> +#define AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM 15
>>> +
>>> struct amdgpu_mec {
>>> struct amdgpu_bo *hpd_eop_obj;
>>> u64 hpd_eop_gpu_addr;
>>> @@ -280,8 +290,9 @@ struct amdgpu_gfx {
>>> uint32_t num_gfx_sched;
>>> unsigned num_gfx_rings;
>>> struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
>>> + struct drm_gpu_scheduler **compute_prio_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
>>> struct drm_gpu_scheduler *compute_sched[AMDGPU_MAX_COMPUTE_RINGS];
>>> - uint32_t num_compute_sched;
>>> + uint32_t num_compute_sched[AMDGPU_GFX_PIPE_PRIO_MAX];
>>> unsigned num_compute_rings;
>>> struct amdgpu_irq_src eop_irq;
>>> struct amdgpu_irq_src priv_reg_irq;
>>> @@ -363,6 +374,8 @@ void amdgpu_gfx_bit_to_mec_queue(struct amdgpu_device *adev, int bit,
>>> int *mec, int *pipe, int *queue);
>>> bool amdgpu_gfx_is_mec_queue_enabled(struct amdgpu_device *adev, int mec,
>>> int pipe, int queue);
>>> +bool amdgpu_gfx_is_high_priority_compute_queue(struct amdgpu_device *adev,
>>> + int queue);
>>> int amdgpu_gfx_me_queue_to_bit(struct amdgpu_device *adev, int me,
>>> int pipe, int queue);
>>> void amdgpu_gfx_bit_to_me_queue(struct amdgpu_device *adev, int bit,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> index d42be880a236..4981e443a884 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> @@ -117,12 +117,10 @@ void amdgpu_job_free_resources(struct amdgpu_job *job)
>>>
>>> static void amdgpu_job_free_cb(struct drm_sched_job *s_job)
>>> {
>>> - struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched);
>>> struct amdgpu_job *job = to_amdgpu_job(s_job);
>>>
>>> drm_sched_job_cleanup(s_job);
>>>
>>> - amdgpu_ring_priority_put(ring, s_job->s_priority);
>>> dma_fence_put(job->fence);
>>> amdgpu_sync_free(&job->sync);
>>> amdgpu_sync_free(&job->sched_sync);
>>> @@ -143,7 +141,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
>>> void *owner, struct dma_fence **f)
>>> {
>>> enum drm_sched_priority priority;
>>> - struct amdgpu_ring *ring;
>>> int r;
>>>
>>> if (!f)
>>> @@ -158,9 +155,6 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
>>> priority = job->base.s_priority;
>>> drm_sched_entity_push_job(&job->base, entity);
>>>
>>> - ring = to_amdgpu_ring(entity->rq->sched);
>>> - amdgpu_ring_priority_get(ring, priority);
>>> -
>>> return 0;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> index 24caff085d00..dcea1ef92883 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> @@ -222,6 +222,7 @@ struct amdgpu_ring {
>>> struct mutex priority_mutex;
>>> /* protected by priority_mutex */
>>> int priority;
>>> + bool has_high_prio;
>>>
>>> #if defined(CONFIG_DEBUG_FS)
>>> struct dentry *ent;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>>> index 96cf617e41d1..c1da41e35323 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
>>> @@ -3211,6 +3211,22 @@ static int gfx_v10_0_cp_async_gfx_ring_resume(struct amdgpu_device *adev)
>>> return r;
>>> }
>>>
>>> +static void gfx_v10_0_compute_mqd_set_priority(struct amdgpu_ring *ring, struct v10_compute_mqd *mqd)
>>> +{
>>> + struct amdgpu_device *adev = ring->adev;
>>> +
>>> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
>>> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
>>> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
>>> + ring->has_high_prio = true;
>>> + mqd->cp_hqd_queue_priority =
>>> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
>>> + } else {
>>> + ring->has_high_prio = false;
>>> + }
>>> + }
>>> +}
>>> +
>>> static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
>>> {
>>> struct amdgpu_device *adev = ring->adev;
>>> @@ -3336,6 +3352,9 @@ static int gfx_v10_0_compute_mqd_init(struct amdgpu_ring *ring)
>>> tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
>>> mqd->cp_hqd_ib_control = tmp;
>>>
>>> + /* set static priority for a compute queue/ring */
>>> + gfx_v10_0_compute_mqd_set_priority(ring, mqd);
>>> +
>>> /* map_queues packet doesn't need activate the queue,
>>> * so only kiq need set this field.
>>> */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> index 393a1324daa9..668c8eb2b2cc 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> @@ -4430,6 +4430,22 @@ static int gfx_v8_0_deactivate_hqd(struct amdgpu_device *adev, u32 req)
>>> return r;
>>> }
>>>
>>> +static void gfx_v8_0_mqd_set_priority(struct amdgpu_ring *ring, struct vi_mqd *mqd)
>>> +{
>>> + struct amdgpu_device *adev = ring->adev;
>>> +
>>> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
>>> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
>>> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
>>> + ring->has_high_prio = true;
>>> + mqd->cp_hqd_queue_priority =
>>> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
>>> + } else {
>>> + ring->has_high_prio = false;
>>> + }
>>> + }
>>> +}
>>> +
>>> static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
>>> {
>>> struct amdgpu_device *adev = ring->adev;
>>> @@ -4553,9 +4569,6 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
>>> /* defaults */
>>> mqd->cp_hqd_eop_rptr = RREG32(mmCP_HQD_EOP_RPTR);
>>> mqd->cp_hqd_eop_wptr = RREG32(mmCP_HQD_EOP_WPTR);
>>> - mqd->cp_hqd_pipe_priority = RREG32(mmCP_HQD_PIPE_PRIORITY);
>>> - mqd->cp_hqd_queue_priority = RREG32(mmCP_HQD_QUEUE_PRIORITY);
>>> - mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
>>> mqd->cp_hqd_ctx_save_base_addr_lo = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_LO);
>>> mqd->cp_hqd_ctx_save_base_addr_hi = RREG32(mmCP_HQD_CTX_SAVE_BASE_ADDR_HI);
>>> mqd->cp_hqd_cntl_stack_offset = RREG32(mmCP_HQD_CNTL_STACK_OFFSET);
>>> @@ -4567,6 +4580,10 @@ static int gfx_v8_0_mqd_init(struct amdgpu_ring *ring)
>>> mqd->cp_hqd_eop_wptr_mem = RREG32(mmCP_HQD_EOP_WPTR_MEM);
>>> mqd->cp_hqd_eop_dones = RREG32(mmCP_HQD_EOP_DONES);
>>>
>>> + /* set static priority for a queue/ring */
>>> + gfx_v8_0_mqd_set_priority(ring, mqd);
>>> + mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
>>> +
>>> /* map_queues packet doesn't need activate the queue,
>>> * so only kiq need set this field.
>>> */
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> index 015647959d69..726d1ac41637 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>> @@ -3314,6 +3314,22 @@ static void gfx_v9_0_kiq_setting(struct amdgpu_ring *ring)
>>> WREG32_SOC15_RLC(GC, 0, mmRLC_CP_SCHEDULERS, tmp);
>>> }
>>>
>>> +static void gfx_v9_0_mqd_set_priority(struct amdgpu_ring *ring, struct v9_mqd *mqd)
>>> +{
>>> + struct amdgpu_device *adev = ring->adev;
>>> +
>>> + if (ring->funcs->type == AMDGPU_RING_TYPE_COMPUTE) {
>>> + if (amdgpu_gfx_is_high_priority_compute_queue(adev, ring->queue)) {
>>> + mqd->cp_hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_HIGH;
>>> + ring->has_high_prio = true;
>>> + mqd->cp_hqd_queue_priority =
>>> + AMDGPU_GFX_QUEUE_PRIORITY_MAXIMUM;
>>> + } else {
>>> + ring->has_high_prio = false;
>>> + }
>>> + }
>>> +}
>>> +
>>> static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
>>> {
>>> struct amdgpu_device *adev = ring->adev;
>>> @@ -3450,6 +3466,10 @@ static int gfx_v9_0_mqd_init(struct amdgpu_ring *ring)
>>> tmp = REG_SET_FIELD(tmp, CP_HQD_IB_CONTROL, MIN_IB_AVAIL_SIZE, 3);
>>> mqd->cp_hqd_ib_control = tmp;
>>>
>>> + /* set static priority for a queue/ring */
>>> + gfx_v9_0_mqd_set_priority(ring, mqd);
>>> + mqd->cp_hqd_quantum = RREG32(mmCP_HQD_QUANTUM);
>>> +
>>> /* map_queues packet doesn't need activate the queue,
>>> * so only kiq need set this field.
>>> */
>>> --
>>> 2.25.0
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cfa70ffa2d9874bbe1a5908d7bf71077e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188364502419515&sdata=vIM2clFPPCfIDK24UxAiJDZ5JH%2F83K2JESZtZKTGQUc%3D&reserved=0
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list
2020-03-05 6:23 ` Nirmoy
@ 2020-03-05 18:46 ` Luben Tuikov
0 siblings, 0 replies; 26+ messages in thread
From: Luben Tuikov @ 2020-03-05 18:46 UTC (permalink / raw)
To: Nirmoy, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-05 01:23, Nirmoy wrote:
>
> On 3/4/20 11:00 PM, Luben Tuikov wrote:
>> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
>>> implement drm_sched_entity_modify_sched() which can modify existing
>>> sched_list with a different one. This is going to be helpful when
>>> userspace changes priority of a ctx/entity then driver can switch to
>>> corresponding hw shced list for that priority
>>>
>> This commit message should start with a capitalized sentence:
>> "implement" --> "Implement". "can modify" --> "modifies"
>>
>> Also a spell check should be done on it before committing:
>> "shced" --> "sched".
>>
>> "then the driver", "to the corresponding", "HW scheduler".
>>
>> And the commit message paragraph should end with a period.
> Thanks!
>>
A difficult to see comment of yours above?
Why not add an empty line before and above your comments?
Are you trying to make them hard to find?
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>> ---
>>> drivers/gpu/drm/scheduler/sched_entity.c | 19 +++++++++++++++++++
>>> include/drm/gpu_scheduler.h | 4 ++++
>>> 2 files changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index 63bccd201b97..b94312154e56 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -83,6 +83,25 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>>> }
>>> EXPORT_SYMBOL(drm_sched_entity_init);
>>>
>>> +/**
>>> + * drm_sched_entity_modify_sched - Modify sched of an entity
>>> + *
>> This empty line is unncessary.
>>
>>> + * @entity: scheduler entity to init
>>> + * @sched_list: the list of new drm scheds which will replace
>>> + * existing entity->sched_list
>>> + * @num_sched_list: number of drm sched in sched_list
>>> + */
>>> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>>> + struct drm_gpu_scheduler **sched_list,
>>> + unsigned int num_sched_list)
>>> +{
>>> + WARN_ON(!num_sched_list || !sched_list);
>>> +
>>> + entity->sched_list = sched_list;
>>> + entity->num_sched_list = num_sched_list;
>>> +}
>>> +EXPORT_SYMBOL(drm_sched_entity_modify_sched);
>>> +
>>> /**
>>> * drm_sched_entity_is_idle - Check if entity is idle
>>> *
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index 589be851f8a1..f70a84aaaf7a 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -297,6 +297,10 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched);
>>> int drm_sched_job_init(struct drm_sched_job *job,
>>> struct drm_sched_entity *entity,
>>> void *owner);
>>> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>>> + struct drm_gpu_scheduler **sched_list,
>>> + unsigned int num_sched_list);
>>> +
> This generally doesn't happen with my current vi config. I will be extra
> careful.
>> Again, the argument list here is unaligned. Please align it
Another hard to see and find comment from you. Why? Why not
take the extra care and diligence to add an empty line before
and after your comment, so that it is easy to see? Why do you do this?
I think it is "generally" unaligned coming from you as you can
see in the reviews in this list. Configure "vi" or use something
else, "Emacs", etc.
Regards,
Luben
>> correctly. The correct indentation would add the maximum number of
>> leading TAB chars followed by the 0 to 7 spaces, to align
>> the argument list to the first argument in the opening parenthesis.
>> The LKCS describes this and also has the settings for Emacs.
>> In Emacs, pressing the TAB key aligns, after which pressing it more
>> does nothing (on an aligned line).
>>
>> Regards,
>> Luben
>>
>>
>>> void drm_sched_job_cleanup(struct drm_sched_job *job);
>>> void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>>> void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>>> --
>>> 2.25.0
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=02%7C01%7Cluben.tuikov%40amd.com%7Cbe8445945c194bd8b3cc08d7bf7109c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637188364557354217&sdata=UAxV2NtkNcj6VXAPhDbk4GliUOnPEfuLOH4BVuOrqdw%3D&reserved=0
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override
2020-03-05 6:28 ` Nirmoy
@ 2020-03-05 18:51 ` Luben Tuikov
0 siblings, 0 replies; 26+ messages in thread
From: Luben Tuikov @ 2020-03-05 18:51 UTC (permalink / raw)
To: Nirmoy, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-05 01:28, Nirmoy wrote:
>
> On 3/4/20 11:25 PM, Luben Tuikov wrote:
>> On 2020-03-03 7:50 a.m., Nirmoy Das wrote:
>>> Switch to appropriate sched list for an entity on priority override.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 32 +++++++++++++++++++++----
>>> 1 file changed, 28 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> index 8c52152e3a6e..a0bf14ab9d33 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>>> @@ -522,6 +522,32 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
>>> return fence;
>>> }
>>>
>>> +static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
>>> + struct amdgpu_ctx_entity *aentity,
>>> + int hw_ip, enum drm_sched_priority priority)
>>> +{
>>> + struct amdgpu_device *adev = ctx->adev;
>>> + enum gfx_pipe_priority hw_prio;
>>> + struct drm_gpu_scheduler **scheds = NULL;
>>> + unsigned num_scheds;
>>> +
>>> + /* set sw priority */
>>> + drm_sched_entity_set_priority(&aentity->entity, priority);
>>> +
>>> + /* set hw priority */
>>> + switch (hw_ip) {
>>> + case AMDGPU_HW_IP_COMPUTE:
>>> + hw_prio = amdgpu_ctx_sched_prio_to_compute_prio(priority);
>>> + scheds = adev->gfx.compute_prio_sched[hw_prio];
>>> + num_scheds = adev->gfx.num_compute_sched[hw_prio];
>>> + break;
>>> + default:
>>> + return;
>>> + }
>>> +
>>> + drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
>>> +}
>> I'd rather this not be over-engineered (in expectations of more case labels,
>> and a simple if-else to do it. Over-engineering it "just in case" creates
>> difficult to maintain code. I believe there is a document about this somewhere
>> in Documentation/.
>
> This switch is in expectation of extending it for VCN and GFX but I
> guess that can wait for now,.
No, please, do not over-engineer. Don't make decisions on what the next
programmer and implementation would be.
If you search the LKML you'll see plenty of emails from Linus and
other Linux leaders about the drawbacks of over-engineering.
Just have an if statement as I wrote which you remove without
adding "[snip]". Please add "[snip]" if you're removing content
when you reply. Everyone does this. It's only common courtesy.
Here it is again, because you removed it, conveniently:
You don't need a break only to execute one statement, which you can pull
into the case: label. If you did this you'll see that you just want to do:
static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
struct amdgpu_ctx_entity *aentity,
int hw_ip, enum drm_sched_priority priority)
{
...
/* Set software priority.
*/
drm_sched_entity_set_priority(&aentity->entity, priority);
/* Set hardware priority.
*/
if (hw_ip == AMDGPU_HW_IP_COMPUTE) {
hw_prio = s2p_prio_map(priority - 2); ## or perhaps from a static inline from a header file, so we wouldn't care for the - 2 here
scheds = adev->gfx.compute_prio_sched[hw_prio];
num_scheds = adev->gfx.num_compute_sched[hw_prio];
drm_sched_entity_modify_sched(&aentity->entity, scheds, num_scheds);
}
}
Regards,
Luben
>
>
> thanks,
>
> Nirmoy
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list
2020-03-05 9:10 ` Nirmoy
@ 2020-03-05 18:56 ` Luben Tuikov
0 siblings, 0 replies; 26+ messages in thread
From: Luben Tuikov @ 2020-03-05 18:56 UTC (permalink / raw)
To: Nirmoy, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-05 04:10, Nirmoy wrote:
>
> On 3/4/20 11:00 PM, Luben Tuikov wrote:
>> struct drm_sched_entity *entity,
>>> void *owner);
>>> +void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>>> + struct drm_gpu_scheduler **sched_list,
>>> + unsigned int num_sched_list);
>>> +
>> Again, the argument list here is unaligned. Please align it
> looks good with cat and vim. Not sure why git format-patch is acting
> strange in this case
You mean, "cat" and "vim" are acting strange. Then don't use them.
Use Emacs. It's easy to use and you don't need to switch modes to
edit.
Regards,
Luben
P.S. Please surround your replied comments with empty lines.
>
>
> Nirmoy
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-05 18:42 ` Luben Tuikov
@ 2020-03-05 21:13 ` Nirmoy
2020-03-05 21:47 ` Luben Tuikov
0 siblings, 1 reply; 26+ messages in thread
From: Nirmoy @ 2020-03-05 21:13 UTC (permalink / raw)
To: Luben Tuikov, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
[-- Attachment #1.1: Type: text/plain, Size: 1551 bytes --]
On 3/5/20 7:42 PM, Luben Tuikov wrote:
>
>> A quick search leads me amdgpu_sched_ioctl() which is using
>> DRM_SCHED_PRIORITY_INVALID
>>
>> to indicate a invalid value from userspace. I don't know much about drm
>> api to suggest any useful
>>
>> changes regarding this. But again this isn't in the scope of this patch
>> series.
> I'm not asking you to eliminate "DRM_SCHED_PRIORITY_INVALID".
> Oh wait! You forgot what I suggested in a previous review on
> how to fix enum drm_sched_priority, did you? :-D Search
> for that email.
Let me quote[1]:
"
Also consider changing to this:
enum drm_sched_priority {
DRM_SCHED_PRIORITY_UNSET,
--------DRM_SCHED_PRIORITY_INVALID,--------<--- remove
DRM_SCHED_PRIORITY_MIN,
DRM_SCHED_PRIORITY_LOW = DRM_SCHED_PRIORITY_MIN,
DRM_SCHED_PRIORITY_NORMAL,
DRM_SCHED_PRIORITY_HIGH_SW,
DRM_SCHED_PRIORITY_HIGH_HW,
DRM_SCHED_PRIORITY_KERNEL,
DRM_SCHED_PRIORITY_MAX,
};
We should never have an "invalid priority", just ignored priority. :-)
Second, having 0 as UNSET gives you easy priority when you set
map[0] = normal_priority, as memory usually comes memset to 0.
IOW, you'd not need to check against UNSET, and simply use
the default [0] which you'd set to normal priority.
"
I guess understood it wrong.
[1]https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg45621.html
<https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg45621.html>
Regards,
Nirmoy
[-- Attachment #1.2: Type: text/html, Size: 2528 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init
2020-03-05 21:13 ` Nirmoy
@ 2020-03-05 21:47 ` Luben Tuikov
0 siblings, 0 replies; 26+ messages in thread
From: Luben Tuikov @ 2020-03-05 21:47 UTC (permalink / raw)
To: Nirmoy, Nirmoy Das, amd-gfx
Cc: alexander.deucher, Ray.Huang, nirmoy.das, christian.koenig
On 2020-03-05 16:13, Nirmoy wrote:
>
> On 3/5/20 7:42 PM, Luben Tuikov wrote:
>>
>>> A quick search leads me amdgpu_sched_ioctl() which is using
>>> DRM_SCHED_PRIORITY_INVALID
>>>
>>> to indicate a invalid value from userspace. I don't know much about drm
>>> api to suggest any useful
>>>
>>> changes regarding this. But again this isn't in the scope of this patch
>>> series.
>> I'm not asking you to eliminate "DRM_SCHED_PRIORITY_INVALID".
>> Oh wait! You forgot what I suggested in a previous review on
>> how to fix enum drm_sched_priority, did you? :-D Search
>> for that email.
>
> Let me quote[1]:
>
> "
>
> Also consider changing to this:
>
>
> enum drm_sched_priority {
> DRM_SCHED_PRIORITY_UNSET,
> --------DRM_SCHED_PRIORITY_INVALID,--------<--- remove
> DRM_SCHED_PRIORITY_MIN,
> DRM_SCHED_PRIORITY_LOW = DRM_SCHED_PRIORITY_MIN,
> DRM_SCHED_PRIORITY_NORMAL,
> DRM_SCHED_PRIORITY_HIGH_SW,
> DRM_SCHED_PRIORITY_HIGH_HW,
> DRM_SCHED_PRIORITY_KERNEL,
> DRM_SCHED_PRIORITY_MAX,
> };
>
> We should never have an "invalid priority", just ignored priority. :-)
> Second, having 0 as UNSET gives you easy priority when you set
> map[0] = normal_priority, as memory usually comes memset to 0.
> IOW, you'd not need to check against UNSET, and simply use
> the default [0] which you'd set to normal priority.
>
> "
>
Excellent! You found the email.
Yes, there is no reason to represent and carry around
an "invalid" priority--it simply means that the *input*
was invalid, but the priority would always be set to
something valid and meaningful: the input was invalid,
not the priority.
If you encounter an invalid input, simply set the priority
to unset, or even better, minimum. From then on,
any further map look-ups, would be a trivial linear
transformation.
It is generally a good idea to represent 0, default software
memory initialization, to "*_NONE", in enums. Then start the
meaningful labels at 1, so one can distinguish between
"this data has never been touched by software" and "this was
set to something by someone after memory initialization".
So you should rename "_UNSET" to "_NONE". It *was* set (by memset()),
just not meaningfully so.
To look like this:
enum drm_sched_priority {
DRM_SCHED_PRIORITY_NONE,
DRM_SCHED_PRIORITY_LOW,
DRM_SCHED_PRIORITY_NORMAL,
DRM_SCHED_PRIORITY_HIGH,
DRM_SCHED_PRIORITY_HIGHER,
DRM_SCHED_PRIORITY_KERNEL,
DRM_SCHED_PRIORITY_SIZE /* intentionally no comma */
};
Valid values are "*_NONE" to "*_KERNEL".
Meaningful values are "*_LOW" to "*_KERNEL". (i.e. intentional)
"*_SIZE" you use to allocate storage (array, for instance)
of size which can index all valid priorities.
Then you can do:
enum drm_sched_priority some_map[XYZ_DOMAIN_PRIO_SIZE] = {
[0 ... XYZ_DOMAIN_PRIO_SIZE - 1] = XYZ_DOMAIN_PRIO_DEFAULT,
[A] = XYZ_DOMAIN_PRIO_NORMAL,
[B] = XYZ_DOMAIN_PRIO_HIGH,
};
Where 0 <= A, B < XYZ_DOMAIN_PRIO_SIZE.
This also allows you to chain those transformations, similarly.
Regards,
Luben
> I guess understood it wrong.
>
>
> [1]https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg45621.html <https://www.mail-archive.com/amd-gfx@lists.freedesktop.org/msg45621.html>
>
>
> Regards,
>
> Nirmoy
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-03-05 21:47 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03 12:50 [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Nirmoy Das
2020-03-03 12:50 ` [PATCH v2 2/4] drm/scheduler: implement a function to modify sched list Nirmoy Das
2020-03-04 22:00 ` Luben Tuikov
2020-03-05 6:23 ` Nirmoy
2020-03-05 18:46 ` Luben Tuikov
2020-03-05 9:10 ` Nirmoy
2020-03-05 18:56 ` Luben Tuikov
2020-03-03 12:50 ` [PATCH v4 3/4] drm/amdgpu: change hw sched list on ctx priority override Nirmoy Das
2020-03-04 22:25 ` Luben Tuikov
2020-03-05 6:28 ` Nirmoy
2020-03-05 18:51 ` Luben Tuikov
2020-03-03 12:50 ` [PATCH 4/4] drm/amdgpu: remove unused functions Nirmoy Das
2020-03-04 22:38 ` Luben Tuikov
2020-03-03 14:03 ` [PATCH v6 1/1] drm/amdgpu: set compute queue priority at mqd_init Christian König
2020-03-03 14:29 ` Nirmoy
2020-03-03 15:14 ` Christian König
2020-03-03 15:22 ` Nirmoy
2020-03-03 15:30 ` Christian König
2020-03-04 9:28 ` Nirmoy
2020-03-04 20:43 ` Alex Deucher
2020-03-04 21:41 ` Luben Tuikov
2020-03-04 22:32 ` Luben Tuikov
2020-03-05 6:21 ` Nirmoy
2020-03-05 18:42 ` Luben Tuikov
2020-03-05 21:13 ` Nirmoy
2020-03-05 21:47 ` Luben Tuikov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.