[PATCH 4/4] drm/scheduler: do not keep a copy of sched list
entity should not keep copy and maintain sched list for
itself.
Signed-off-by: Nirmoy Das
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_entity.c | 19 ---
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index f9b6ce29c58f..2e3a058fc239 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -56,8 +56,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
unsigned int num_sched_list,
atomic_t *guilty)
{
- int i;
-
if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
return -EINVAL;
@@ -67,22 +65,14 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->guilty = guilty;
entity->num_sched_list = num_sched_list;
entity->priority = priority;
- entity->sched_list = kcalloc(num_sched_list,
- sizeof(struct drm_gpu_scheduler *),
GFP_KERNEL);
+ entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+ entity->last_scheduled = NULL;
- if(!entity->sched_list)
- return -ENOMEM;
+ if(num_sched_list)
+ entity->rq = _list[0]->sched_rq[entity->priority];
init_completion(>entity_idle);
- for (i = 0; i < num_sched_list; i++)
- entity->sched_list[i] = sched_list[i];
-
- if (num_sched_list)
- entity->rq = >sched_list[0]->sched_rq[entity->priority];
-
- entity->last_scheduled = NULL;
-
spin_lock_init(>rq_lock);
spsc_queue_init(>job_queue);
@@ -312,7 +302,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
dma_fence_put(entity->last_scheduled);
entity->last_scheduled = NULL;
- kfree(entity->sched_list);
}
EXPORT_SYMBOL(drm_sched_entity_fini);
--
2.24.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/4] drm/scheduler: do not keep a copy of sched list
entity should not keep copy and maintain sched list for
itself.
Signed-off-by: Nirmoy Das
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_entity.c | 19 ---
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index f9b6ce29c58f..2e3a058fc239 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -56,8 +56,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
unsigned int num_sched_list,
atomic_t *guilty)
{
- int i;
-
if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
return -EINVAL;
@@ -67,22 +65,14 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->guilty = guilty;
entity->num_sched_list = num_sched_list;
entity->priority = priority;
- entity->sched_list = kcalloc(num_sched_list,
- sizeof(struct drm_gpu_scheduler *),
GFP_KERNEL);
+ entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+ entity->last_scheduled = NULL;
- if(!entity->sched_list)
- return -ENOMEM;
+ if(num_sched_list)
+ entity->rq = _list[0]->sched_rq[entity->priority];
init_completion(>entity_idle);
- for (i = 0; i < num_sched_list; i++)
- entity->sched_list[i] = sched_list[i];
-
- if (num_sched_list)
- entity->rq = >sched_list[0]->sched_rq[entity->priority];
-
- entity->last_scheduled = NULL;
-
spin_lock_init(>rq_lock);
spsc_queue_init(>job_queue);
@@ -312,7 +302,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
dma_fence_put(entity->last_scheduled);
entity->last_scheduled = NULL;
- kfree(entity->sched_list);
}
EXPORT_SYMBOL(drm_sched_entity_fini);
--
2.24.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
Am 10.12.19 um 19:17 schrieb Nirmoy Das:
entity should not keep copy and maintain sched list for
itself.
Signed-off-by: Nirmoy Das
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_entity.c | 19 ---
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index f9b6ce29c58f..2e3a058fc239 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -56,8 +56,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
unsigned int num_sched_list,
atomic_t *guilty)
{
- int i;
-
if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
return -EINVAL;
@@ -67,22 +65,14 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->guilty = guilty;
entity->num_sched_list = num_sched_list;
entity->priority = priority;
- entity->sched_list = kcalloc(num_sched_list,
- sizeof(struct drm_gpu_scheduler *),
GFP_KERNEL);
+ entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+ entity->last_scheduled = NULL;
- if(!entity->sched_list)
- return -ENOMEM;
+ if(num_sched_list)
+ entity->rq = _list[0]->sched_rq[entity->priority];
init_completion(>entity_idle);
- for (i = 0; i < num_sched_list; i++)
- entity->sched_list[i] = sched_list[i];
-
- if (num_sched_list)
- entity->rq = >sched_list[0]->sched_rq[entity->priority];
-
- entity->last_scheduled = NULL;
-
spin_lock_init(>rq_lock);
spsc_queue_init(>job_queue);
@@ -312,7 +302,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
dma_fence_put(entity->last_scheduled);
entity->last_scheduled = NULL;
- kfree(entity->sched_list);
}
EXPORT_SYMBOL(drm_sched_entity_fini);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
On 12/10/19 6:32 PM, Christian König wrote: Maybe make this "num_sched_list > 1 ? sched_list : NULL" to avoid accidentally dereferencing a stale pointer to the stack. Do you mean "num_sched_list >= 1 ? sched_list : NULL" No, the entity->sched_list field should be NULL when num_sched_list==1. When num_sched_list==1 the entity->sched_list shouldn't be used and we can use a dummy list on the stack as parameter. But we should set the pointer to NULL in this case just to make sure that nobody is dereferencing it. Okay I understand now. - if(!entity->sched_list) - return -ENOMEM; init_completion(>entity_idle); - - for (i = 0; i < num_sched_list; i++) - entity->sched_list[i] = sched_list[i]; - if (num_sched_list) That check can actually be dropped as well. We return -EINVAL when the num_sched_list is zero. This one took me some time to understand. So we don't really return -EINVAL if num_sched_list == 0 if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0]))) return -EINVAL; This is coming from below patch. Are we suppose to tolerate IPs with uninitialized sched so that ctx creation dosn't return error ? Yes, exactly. That's intentionally this way. GPU reset sometimes resulted in schedulers being disabled because we couldn't re-init them. In this case we had entities with an empty scheduler list. That can't happen any more after you make the scheduler arrays constant, but I would stick with that behavior. So I will keep the check. Regards, Christian. Regards, Nirmoy ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/4] drm/scheduler: do not keep a copy of sched list
entity should not keep copy and maintain sched list for
itself.
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/scheduler/sched_entity.c | 19 ---
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index f9b6ce29c58f..2e3a058fc239 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -56,8 +56,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
unsigned int num_sched_list,
atomic_t *guilty)
{
- int i;
-
if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
return -EINVAL;
@@ -67,22 +65,14 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->guilty = guilty;
entity->num_sched_list = num_sched_list;
entity->priority = priority;
- entity->sched_list = kcalloc(num_sched_list,
- sizeof(struct drm_gpu_scheduler *),
GFP_KERNEL);
+ entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+ entity->last_scheduled = NULL;
- if(!entity->sched_list)
- return -ENOMEM;
+ if(num_sched_list)
+ entity->rq = _list[0]->sched_rq[entity->priority];
init_completion(>entity_idle);
- for (i = 0; i < num_sched_list; i++)
- entity->sched_list[i] = sched_list[i];
-
- if (num_sched_list)
- entity->rq = >sched_list[0]->sched_rq[entity->priority];
-
- entity->last_scheduled = NULL;
-
spin_lock_init(>rq_lock);
spsc_queue_init(>job_queue);
@@ -312,7 +302,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
dma_fence_put(entity->last_scheduled);
entity->last_scheduled = NULL;
- kfree(entity->sched_list);
}
EXPORT_SYMBOL(drm_sched_entity_fini);
--
2.24.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
Am 10.12.19 um 16:08 schrieb Nirmoy: I think amdgpu_ctx_init() should check for num_scheds and not call drm_sched_entity_init() if its zero. Ah, that's where that came from. No that is intentionally this way, but see below. On 12/10/19 3:47 PM, Nirmoy wrote: On 12/10/19 2:00 PM, Christian König wrote: Am 10.12.19 um 13:53 schrieb Nirmoy Das: entity should not keep copy and maintain sched list for itself. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/scheduler/sched_entity.c | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f9b6ce29c58f..a5f729f421f8 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -67,17 +67,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, entity->guilty = guilty; entity->num_sched_list = num_sched_list; entity->priority = priority; - entity->sched_list = kcalloc(num_sched_list, - sizeof(struct drm_gpu_scheduler *), GFP_KERNEL); + entity->sched_list = sched_list; Maybe make this "num_sched_list > 1 ? sched_list : NULL" to avoid accidentally dereferencing a stale pointer to the stack. Do you mean "num_sched_list >= 1 ? sched_list : NULL" No, the entity->sched_list field should be NULL when num_sched_list==1. When num_sched_list==1 the entity->sched_list shouldn't be used and we can use a dummy list on the stack as parameter. But we should set the pointer to NULL in this case just to make sure that nobody is dereferencing it. - if(!entity->sched_list) - return -ENOMEM; init_completion(>entity_idle); - - for (i = 0; i < num_sched_list; i++) - entity->sched_list[i] = sched_list[i]; - if (num_sched_list) That check can actually be dropped as well. We return -EINVAL when the num_sched_list is zero. This one took me some time to understand. So we don't really return -EINVAL if num_sched_list == 0 if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0]))) return -EINVAL; This is coming from below patch. Are we suppose to tolerate IPs with uninitialized sched so that ctx creation dosn't return error ? Yes, exactly. That's intentionally this way. GPU reset sometimes resulted in schedulers being disabled because we couldn't re-init them. In this case we had entities with an empty scheduler list. That can't happen any more after you make the scheduler arrays constant, but I would stick with that behavior. Regards, Christian. commit 1decbf6bb0b4dc56c9da6c5e57b994ebfc2be3aa Author: Bas Nieuwenhuizen Date: Wed Jan 30 02:53:19 2019 +0100 drm/sched: Fix entities with 0 rqs. Some blocks in amdgpu can have 0 rqs. Job creation already fails with -ENOENT when entity->rq is NULL, so jobs cannot be pushed. Without a rq there is no scheduler to pop jobs, and rq selection already does the right thing with a list of length 0. So the operations we need to fix are: - Creation, do not set rq to rq_list[0] if the list can have length 0. - Do not flush any jobs when there is no rq. - On entity destruction handle the rq = NULL case. - on set_priority, do not try to change the rq if it is NULL. Regards, Christian. entity->rq = >sched_list[0]->sched_rq[entity->priority]; @@ -312,7 +305,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) dma_fence_put(entity->last_scheduled); entity->last_scheduled = NULL; - kfree(entity->sched_list); } EXPORT_SYMBOL(drm_sched_entity_fini); ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/4] drm/scheduler: do not keep a copy of sched list
entity should not keep copy and maintain sched list for itself. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/scheduler/sched_entity.c | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f9b6ce29c58f..a5f729f421f8 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -67,17 +67,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, entity->guilty = guilty; entity->num_sched_list = num_sched_list; entity->priority = priority; - entity->sched_list = kcalloc(num_sched_list, - sizeof(struct drm_gpu_scheduler *), GFP_KERNEL); + entity->sched_list = sched_list; - if(!entity->sched_list) - return -ENOMEM; init_completion(>entity_idle); - - for (i = 0; i < num_sched_list; i++) - entity->sched_list[i] = sched_list[i]; - if (num_sched_list) entity->rq = >sched_list[0]->sched_rq[entity->priority]; @@ -312,7 +305,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) dma_fence_put(entity->last_scheduled); entity->last_scheduled = NULL; - kfree(entity->sched_list); } EXPORT_SYMBOL(drm_sched_entity_fini); -- 2.24.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
I think amdgpu_ctx_init() should check for num_scheds and not call drm_sched_entity_init() if its zero. On 12/10/19 3:47 PM, Nirmoy wrote: On 12/10/19 2:00 PM, Christian König wrote: Am 10.12.19 um 13:53 schrieb Nirmoy Das: entity should not keep copy and maintain sched list for itself. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/scheduler/sched_entity.c | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f9b6ce29c58f..a5f729f421f8 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -67,17 +67,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, entity->guilty = guilty; entity->num_sched_list = num_sched_list; entity->priority = priority; - entity->sched_list = kcalloc(num_sched_list, - sizeof(struct drm_gpu_scheduler *), GFP_KERNEL); + entity->sched_list = sched_list; Maybe make this "num_sched_list > 1 ? sched_list : NULL" to avoid accidentally dereferencing a stale pointer to the stack. Do you mean "num_sched_list >= 1 ? sched_list : NULL" - if(!entity->sched_list) - return -ENOMEM; init_completion(>entity_idle); - - for (i = 0; i < num_sched_list; i++) - entity->sched_list[i] = sched_list[i]; - if (num_sched_list) That check can actually be dropped as well. We return -EINVAL when the num_sched_list is zero. This one took me some time to understand. So we don't really return -EINVAL if num_sched_list == 0 if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0]))) return -EINVAL; This is coming from below patch. Are we suppose to tolerate IPs with uninitialized sched so that ctx creation dosn't return error ? commit 1decbf6bb0b4dc56c9da6c5e57b994ebfc2be3aa Author: Bas Nieuwenhuizen Date: Wed Jan 30 02:53:19 2019 +0100 drm/sched: Fix entities with 0 rqs. Some blocks in amdgpu can have 0 rqs. Job creation already fails with -ENOENT when entity->rq is NULL, so jobs cannot be pushed. Without a rq there is no scheduler to pop jobs, and rq selection already does the right thing with a list of length 0. So the operations we need to fix are: - Creation, do not set rq to rq_list[0] if the list can have length 0. - Do not flush any jobs when there is no rq. - On entity destruction handle the rq = NULL case. - on set_priority, do not try to change the rq if it is NULL. Regards, Christian. entity->rq = >sched_list[0]->sched_rq[entity->priority]; @@ -312,7 +305,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) dma_fence_put(entity->last_scheduled); entity->last_scheduled = NULL; - kfree(entity->sched_list); } EXPORT_SYMBOL(drm_sched_entity_fini); ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
On 12/10/19 2:00 PM, Christian König wrote: Am 10.12.19 um 13:53 schrieb Nirmoy Das: entity should not keep copy and maintain sched list for itself. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/scheduler/sched_entity.c | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f9b6ce29c58f..a5f729f421f8 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -67,17 +67,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, entity->guilty = guilty; entity->num_sched_list = num_sched_list; entity->priority = priority; - entity->sched_list = kcalloc(num_sched_list, - sizeof(struct drm_gpu_scheduler *), GFP_KERNEL); + entity->sched_list = sched_list; Maybe make this "num_sched_list > 1 ? sched_list : NULL" to avoid accidentally dereferencing a stale pointer to the stack. Do you mean "num_sched_list >= 1 ? sched_list : NULL" - if(!entity->sched_list) - return -ENOMEM; init_completion(>entity_idle); - - for (i = 0; i < num_sched_list; i++) - entity->sched_list[i] = sched_list[i]; - if (num_sched_list) That check can actually be dropped as well. We return -EINVAL when the num_sched_list is zero. This one took me some time to understand. So we don't really return -EINVAL if num_sched_list == 0 if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0]))) return -EINVAL; This is coming from below patch. Are we suppose to tolerate IPs with uninitialized sched so that ctx creation dosn't return error ? commit 1decbf6bb0b4dc56c9da6c5e57b994ebfc2be3aa Author: Bas Nieuwenhuizen Date: Wed Jan 30 02:53:19 2019 +0100 drm/sched: Fix entities with 0 rqs. Some blocks in amdgpu can have 0 rqs. Job creation already fails with -ENOENT when entity->rq is NULL, so jobs cannot be pushed. Without a rq there is no scheduler to pop jobs, and rq selection already does the right thing with a list of length 0. So the operations we need to fix are: - Creation, do not set rq to rq_list[0] if the list can have length 0. - Do not flush any jobs when there is no rq. - On entity destruction handle the rq = NULL case. - on set_priority, do not try to change the rq if it is NULL. Regards, Christian. entity->rq = >sched_list[0]->sched_rq[entity->priority]; @@ -312,7 +305,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) dma_fence_put(entity->last_scheduled); entity->last_scheduled = NULL; - kfree(entity->sched_list); } EXPORT_SYMBOL(drm_sched_entity_fini); ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
Am 10.12.19 um 14:00 schrieb Christian König: Am 10.12.19 um 13:53 schrieb Nirmoy Das: entity should not keep copy and maintain sched list for itself. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/scheduler/sched_entity.c | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f9b6ce29c58f..a5f729f421f8 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -67,17 +67,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, entity->guilty = guilty; entity->num_sched_list = num_sched_list; entity->priority = priority; - entity->sched_list = kcalloc(num_sched_list, - sizeof(struct drm_gpu_scheduler *), GFP_KERNEL); + entity->sched_list = sched_list; Maybe make this "num_sched_list > 1 ? sched_list : NULL" to avoid accidentally dereferencing a stale pointer to the stack. - if(!entity->sched_list) - return -ENOMEM; init_completion(>entity_idle); - - for (i = 0; i < num_sched_list; i++) - entity->sched_list[i] = sched_list[i]; - if (num_sched_list) That check can actually be dropped as well. We return -EINVAL when the num_sched_list is zero. Regards, Christian. entity->rq = >sched_list[0]->sched_rq[entity->priority]; Forgot to note that this should then probably use "sched_list[0]->sched_rq[entity->priority]" directly when we change the assignment above. Christian. @@ -312,7 +305,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) dma_fence_put(entity->last_scheduled); entity->last_scheduled = NULL; - kfree(entity->sched_list); } EXPORT_SYMBOL(drm_sched_entity_fini); ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
Am 10.12.19 um 13:53 schrieb Nirmoy Das: entity should not keep copy and maintain sched list for itself. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/scheduler/sched_entity.c | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f9b6ce29c58f..a5f729f421f8 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -67,17 +67,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, entity->guilty = guilty; entity->num_sched_list = num_sched_list; entity->priority = priority; - entity->sched_list = kcalloc(num_sched_list, - sizeof(struct drm_gpu_scheduler *), GFP_KERNEL); + entity->sched_list = sched_list; Maybe make this "num_sched_list > 1 ? sched_list : NULL" to avoid accidentally dereferencing a stale pointer to the stack. - if(!entity->sched_list) - return -ENOMEM; init_completion(>entity_idle); - - for (i = 0; i < num_sched_list; i++) - entity->sched_list[i] = sched_list[i]; - if (num_sched_list) That check can actually be dropped as well. We return -EINVAL when the num_sched_list is zero. Regards, Christian. entity->rq = >sched_list[0]->sched_rq[entity->priority]; @@ -312,7 +305,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) dma_fence_put(entity->last_scheduled); entity->last_scheduled = NULL; - kfree(entity->sched_list); } EXPORT_SYMBOL(drm_sched_entity_fini); ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/4] drm/scheduler: do not keep a copy of sched list
entity should not keep copy and maintain sched list for itself. Signed-off-by: Nirmoy Das --- drivers/gpu/drm/scheduler/sched_entity.c | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f9b6ce29c58f..a5f729f421f8 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -67,17 +67,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, entity->guilty = guilty; entity->num_sched_list = num_sched_list; entity->priority = priority; - entity->sched_list = kcalloc(num_sched_list, - sizeof(struct drm_gpu_scheduler *), GFP_KERNEL); + entity->sched_list = sched_list; - if(!entity->sched_list) - return -ENOMEM; init_completion(>entity_idle); - - for (i = 0; i < num_sched_list; i++) - entity->sched_list[i] = sched_list[i]; - if (num_sched_list) entity->rq = >sched_list[0]->sched_rq[entity->priority]; @@ -312,7 +305,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity) dma_fence_put(entity->last_scheduled); entity->last_scheduled = NULL; - kfree(entity->sched_list); } EXPORT_SYMBOL(drm_sched_entity_fini); -- 2.24.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
Yeah, that won't work very well.
During bring-up we also often have the case that we can't correctly
initialize all engines, e.g. only the first SDMA comes up etc.
So better stick with the initial approach of constructing the scheduler
array for each engine type which needs it.
Regards,
Christian.
Am 09.12.19 um 15:09 schrieb Nirmoy:
I can see one issue with this. I am ignoring/removing changes from
commit 2a84e48e9712ea8591a10dd59d59ccab3d54efd6 drm/amdgpu: Only add
rqs for initialized rings.
I wonder if we can handle that differently.
Regards,
Nirmoy
On 12/9/19 2:56 PM, Nirmoy wrote:
Hi Christian,
I got a different idea, a bit more simple let me know what do you
think about it:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 50bab33cba39..8de4de4f7a43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -870,6 +870,7 @@ struct amdgpu_device {
u64 fence_context;
unsigned num_rings;
struct amdgpu_ring *rings[AMDGPU_MAX_RINGS];
+ struct drm_gpu_scheduler *rings_sched_list[AMDGPU_MAX_RINGS];
bool ib_pool_ready;
struct amdgpu_sa_manager ring_tmp_bo;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 1d6850af9908..52b3a5d85a1d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -122,9 +122,8 @@ static int amdgpu_ctx_init(struct amdgpu_device
*adev,
for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
struct amdgpu_ring *rings[AMDGPU_MAX_RINGS];
- struct drm_gpu_scheduler *sched_list[AMDGPU_MAX_RINGS]
+ struct drm_gpu_scheduler **sched_list;
unsigned num_rings = 0;
- unsigned num_rqs = 0;
switch (i) {
case AMDGPU_HW_IP_GFX:
@@ -177,17 +176,11 @@ static int amdgpu_ctx_init(struct amdgpu_device
*adev,
break;
}
- for (j = 0; j < num_rings; ++j) {
- if (!rings[j]->adev)
- continue;
-
- sched_list[num_rqs++] = [j]->sched;
- }
-
+ sched_list= adev->rings_sched_list+rings[0]->idx;
for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j)
r =
drm_sched_entity_init(>entities[i][j].entity,
priority, sched_list,
- num_rqs,
>guilty);
+ num_rings,
>guilty);
if (r)
goto error_cleanup_entities;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 377fe20bce23..e8cfa357e445 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -480,6 +480,8 @@ int amdgpu_fence_driver_init_ring(struct
amdgpu_ring *ring,
ring->name);
return r;
}
+
+ adev->rings_sched_list[ring->idx] = >sched;
}
return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index bd9ed33bab43..bfe36199ffed 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -1744,8 +1744,11 @@ static int sdma_v4_0_sw_init(void *handle)
AMDGPU_SDMA_IRQ_INSTANCE0 + i);
if (r)
return r;
+ }
+
+ if (adev->sdma.has_page_queue) {
+ for (i = 0; i < adev->sdma.num_instances; i++) {
- if (adev->sdma.has_page_queue) {
ring = >sdma.instance[i].page;
ring->ring_obj = NULL;
ring->use_doorbell = true;
It relies on contiguous ring initialization that's why I had to
change sdma_v4_0.c so that we do ring_init(sdma0, sdma1, page0, page1}
instead of ring_init{sdma0, page0, sdma1, page1}
Regards,
Nirmoy
On 12/9/19 1:20 PM, Christian König wrote:
Yes, you need to do this for the SDMA as well but in general that
looks like the idea I had in mind as well.
I would do it like this:
1. Change the special case when you only get one scheduler for an
entity to drop the pointer to the scheduler list.
This way we always use the same scheduler for the entity and can
pass in the array on the stack.
2. Change all callers which use more than one scheduler in the list
to pass in pointers which are not allocated on the stack.
This obviously also means that we build the list of schedulers
for each type only once during device init and not for each context
init.
3. Make the scheduler
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
I can see one issue with this. I am ignoring/removing changes from
commit 2a84e48e9712ea8591a10dd59d59ccab3d54efd6 drm/amdgpu: Only add rqs
for initialized rings.
I wonder if we can handle that differently.
Regards,
Nirmoy
On 12/9/19 2:56 PM, Nirmoy wrote:
Hi Christian,
I got a different idea, a bit more simple let me know what do you
think about it:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 50bab33cba39..8de4de4f7a43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -870,6 +870,7 @@ struct amdgpu_device {
u64 fence_context;
unsigned num_rings;
struct amdgpu_ring *rings[AMDGPU_MAX_RINGS];
+ struct drm_gpu_scheduler *rings_sched_list[AMDGPU_MAX_RINGS];
bool ib_pool_ready;
struct amdgpu_sa_manager ring_tmp_bo;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 1d6850af9908..52b3a5d85a1d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -122,9 +122,8 @@ static int amdgpu_ctx_init(struct amdgpu_device
*adev,
for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
struct amdgpu_ring *rings[AMDGPU_MAX_RINGS];
- struct drm_gpu_scheduler *sched_list[AMDGPU_MAX_RINGS]
+ struct drm_gpu_scheduler **sched_list;
unsigned num_rings = 0;
- unsigned num_rqs = 0;
switch (i) {
case AMDGPU_HW_IP_GFX:
@@ -177,17 +176,11 @@ static int amdgpu_ctx_init(struct amdgpu_device
*adev,
break;
}
- for (j = 0; j < num_rings; ++j) {
- if (!rings[j]->adev)
- continue;
-
- sched_list[num_rqs++] = [j]->sched;
- }
-
+ sched_list= adev->rings_sched_list+rings[0]->idx;
for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j)
r =
drm_sched_entity_init(>entities[i][j].entity,
priority, sched_list,
- num_rqs, >guilty);
+ num_rings,
>guilty);
if (r)
goto error_cleanup_entities;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 377fe20bce23..e8cfa357e445 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -480,6 +480,8 @@ int amdgpu_fence_driver_init_ring(struct
amdgpu_ring *ring,
ring->name);
return r;
}
+
+ adev->rings_sched_list[ring->idx] = >sched;
}
return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index bd9ed33bab43..bfe36199ffed 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -1744,8 +1744,11 @@ static int sdma_v4_0_sw_init(void *handle)
AMDGPU_SDMA_IRQ_INSTANCE0 + i);
if (r)
return r;
+ }
+
+ if (adev->sdma.has_page_queue) {
+ for (i = 0; i < adev->sdma.num_instances; i++) {
- if (adev->sdma.has_page_queue) {
ring = >sdma.instance[i].page;
ring->ring_obj = NULL;
ring->use_doorbell = true;
It relies on contiguous ring initialization that's why I had to
change sdma_v4_0.c so that we do ring_init(sdma0, sdma1, page0, page1}
instead of ring_init{sdma0, page0, sdma1, page1}
Regards,
Nirmoy
On 12/9/19 1:20 PM, Christian König wrote:
Yes, you need to do this for the SDMA as well but in general that
looks like the idea I had in mind as well.
I would do it like this:
1. Change the special case when you only get one scheduler for an
entity to drop the pointer to the scheduler list.
This way we always use the same scheduler for the entity and can
pass in the array on the stack.
2. Change all callers which use more than one scheduler in the list
to pass in pointers which are not allocated on the stack.
This obviously also means that we build the list of schedulers
for each type only once during device init and not for each context
init.
3. Make the scheduler list const and drop the kcalloc()/kfree() from
the entity code.
Regards,
Christian.
Am 08.12.19 um 20:57 schrieb Nirmoy:
On 12/6/19 8:41 PM, Christian König wrote:
Am 06.12.19 um 18:33 schrieb Nirmoy Das:
entity should not keep copy and maintain sched list for
itself.
That is a good step, but we need to take this
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
Hi Christian,
I got a different idea, a bit more simple let me know what do you think
about it:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 50bab33cba39..8de4de4f7a43 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -870,6 +870,7 @@ struct amdgpu_device {
u64 fence_context;
unsigned num_rings;
struct amdgpu_ring *rings[AMDGPU_MAX_RINGS];
+ struct drm_gpu_scheduler *rings_sched_list[AMDGPU_MAX_RINGS];
bool ib_pool_ready;
struct amdgpu_sa_manager ring_tmp_bo;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index 1d6850af9908..52b3a5d85a1d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -122,9 +122,8 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev,
for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
struct amdgpu_ring *rings[AMDGPU_MAX_RINGS];
- struct drm_gpu_scheduler *sched_list[AMDGPU_MAX_RINGS]
+ struct drm_gpu_scheduler **sched_list;
unsigned num_rings = 0;
- unsigned num_rqs = 0;
switch (i) {
case AMDGPU_HW_IP_GFX:
@@ -177,17 +176,11 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev,
break;
}
- for (j = 0; j < num_rings; ++j) {
- if (!rings[j]->adev)
- continue;
-
- sched_list[num_rqs++] = [j]->sched;
- }
-
+ sched_list= adev->rings_sched_list+rings[0]->idx;
for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j)
r =
drm_sched_entity_init(>entities[i][j].entity,
priority, sched_list,
- num_rqs, >guilty);
+ num_rings, >guilty);
if (r)
goto error_cleanup_entities;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
index 377fe20bce23..e8cfa357e445 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
@@ -480,6 +480,8 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring
*ring,
ring->name);
return r;
}
+
+ adev->rings_sched_list[ring->idx] = >sched;
}
return 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index bd9ed33bab43..bfe36199ffed 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -1744,8 +1744,11 @@ static int sdma_v4_0_sw_init(void *handle)
AMDGPU_SDMA_IRQ_INSTANCE0 + i);
if (r)
return r;
+ }
+
+ if (adev->sdma.has_page_queue) {
+ for (i = 0; i < adev->sdma.num_instances; i++) {
- if (adev->sdma.has_page_queue) {
ring = >sdma.instance[i].page;
ring->ring_obj = NULL;
ring->use_doorbell = true;
It relies on contiguous ring initialization that's why I had to change
sdma_v4_0.c so that we do ring_init(sdma0, sdma1, page0, page1}
instead of ring_init{sdma0, page0, sdma1, page1}
Regards,
Nirmoy
On 12/9/19 1:20 PM, Christian König wrote:
Yes, you need to do this for the SDMA as well but in general that
looks like the idea I had in mind as well.
I would do it like this:
1. Change the special case when you only get one scheduler for an
entity to drop the pointer to the scheduler list.
This way we always use the same scheduler for the entity and can
pass in the array on the stack.
2. Change all callers which use more than one scheduler in the list to
pass in pointers which are not allocated on the stack.
This obviously also means that we build the list of schedulers for
each type only once during device init and not for each context init.
3. Make the scheduler list const and drop the kcalloc()/kfree() from
the entity code.
Regards,
Christian.
Am 08.12.19 um 20:57 schrieb Nirmoy:
On 12/6/19 8:41 PM, Christian König wrote:
Am 06.12.19 um 18:33 schrieb Nirmoy Das:
entity should not keep copy and maintain sched list for
itself.
That is a good step, but we need to take this further.
How about something like ?
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index 0ae0a2715b0d..a71ee084b47a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -269,8
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
Yes, you need to do this for the SDMA as well but in general that looks
like the idea I had in mind as well.
I would do it like this:
1. Change the special case when you only get one scheduler for an entity
to drop the pointer to the scheduler list.
This way we always use the same scheduler for the entity and can
pass in the array on the stack.
2. Change all callers which use more than one scheduler in the list to
pass in pointers which are not allocated on the stack.
This obviously also means that we build the list of schedulers for
each type only once during device init and not for each context init.
3. Make the scheduler list const and drop the kcalloc()/kfree() from the
entity code.
Regards,
Christian.
Am 08.12.19 um 20:57 schrieb Nirmoy:
On 12/6/19 8:41 PM, Christian König wrote:
Am 06.12.19 um 18:33 schrieb Nirmoy Das:
entity should not keep copy and maintain sched list for
itself.
That is a good step, but we need to take this further.
How about something like ?
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index 0ae0a2715b0d..a71ee084b47a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -269,8 +269,10 @@ struct amdgpu_gfx {
bool me_fw_write_wait;
bool cp_fw_write_wait;
struct amdgpu_ring gfx_ring[AMDGPU_MAX_GFX_RINGS];
+ struct drm_gpu_scheduler *gfx_sched_list[AMDGPU_MAX_GFX_RINGS];
unsigned num_gfx_rings;
struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
+ struct drm_gpu_scheduler
*compute_sched_list[AMDGPU_MAX_COMPUTE_RINGS];
unsigned num_compute_rings;
struct amdgpu_irq_src eop_irq;
struct amdgpu_irq_src priv_reg_irq;
Regards,
Nirmoy
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
On 12/6/19 8:41 PM, Christian König wrote:
Am 06.12.19 um 18:33 schrieb Nirmoy Das:
entity should not keep copy and maintain sched list for
itself.
That is a good step, but we need to take this further.
How about something like ?
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index 0ae0a2715b0d..a71ee084b47a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -269,8 +269,10 @@ struct amdgpu_gfx {
bool me_fw_write_wait;
bool cp_fw_write_wait;
struct amdgpu_ring gfx_ring[AMDGPU_MAX_GFX_RINGS];
+ struct drm_gpu_scheduler *gfx_sched_list[AMDGPU_MAX_GFX_RINGS];
unsigned num_gfx_rings;
struct amdgpu_ring compute_ring[AMDGPU_MAX_COMPUTE_RINGS];
+ struct drm_gpu_scheduler
*compute_sched_list[AMDGPU_MAX_COMPUTE_RINGS];
unsigned num_compute_rings;
struct amdgpu_irq_src eop_irq;
struct amdgpu_irq_src priv_reg_irq;
Regards,
Nirmoy
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/4] drm/scheduler: do not keep a copy of sched list
Am 06.12.19 um 18:33 schrieb Nirmoy Das:
entity should not keep copy and maintain sched list for
itself.
That is a good step, but we need to take this further.
The sched_list is static for the whole device and we shouldn't allocate
it inside the context at all.
Christian.
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 11 ---
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h | 1 +
drivers/gpu/drm/scheduler/sched_entity.c | 12 +---
3 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index c7643af8827f..1939b414d23b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -71,7 +71,6 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip)
{
struct amdgpu_ring *rings[AMDGPU_MAX_RINGS];
- struct drm_gpu_scheduler *sched_list[AMDGPU_MAX_RINGS];
struct amdgpu_device *adev = ctx->adev;
unsigned num_rings = 0;
unsigned num_scheds = 0;
@@ -129,16 +128,21 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx,
u32 hw_ip)
break;
}
+ ctx->sched_list = kcalloc(num_rings,
+ sizeof(struct drm_gpu_scheduler *),
GFP_KERNEL);
+ if (ctx->sched_list == NULL )
+ return -ENOMEM;
+
for (i = 0; i < num_rings; ++i) {
if (!rings[i]->adev)
continue;
- sched_list[num_scheds++] = [i]->sched;
+ ctx->sched_list[num_scheds++] = [i]->sched;
}
for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i)
r = drm_sched_entity_init(>entities[hw_ip][i].entity,
- ctx->init_priority, sched_list, num_scheds,
>guilty);
+ ctx->init_priority, ctx->sched_list, num_scheds,
>guilty);
if (r)
goto error_cleanup_entities;
@@ -228,6 +232,7 @@ static void amdgpu_ctx_fini(struct kref *ref)
for (j = 0; j < amdgpu_sched_jobs; ++j)
dma_fence_put(ctx->entities[0][i].fences[j]);
kfree(ctx->fences);
+ kfree(ctx->sched_list);
kfree(ctx->entities[0]);
mutex_destroy(>lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index da808633732b..9fd02cc47352 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -44,6 +44,7 @@ struct amdgpu_ctx {
spinlock_t ring_lock;
struct dma_fence**fences;
struct amdgpu_ctx_entity*entities[AMDGPU_HW_IP_NUM];
+ struct drm_gpu_scheduler**sched_list;
boolpreamble_presented;
enum drm_sched_priority init_priority;
enum drm_sched_priority override_priority;
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index f9b6ce29c58f..c84a9a66f7f0 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -56,8 +56,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
unsigned int num_sched_list,
atomic_t *guilty)
{
- int i;
-
if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
return -EINVAL;
@@ -67,17 +65,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->guilty = guilty;
entity->num_sched_list = num_sched_list;
entity->priority = priority;
- entity->sched_list = kcalloc(num_sched_list,
- sizeof(struct drm_gpu_scheduler *),
GFP_KERNEL);
-
- if(!entity->sched_list)
- return -ENOMEM;
+ entity->sched_list = sched_list;
init_completion(>entity_idle);
- for (i = 0; i < num_sched_list; i++)
- entity->sched_list[i] = sched_list[i];
-
if (num_sched_list)
entity->rq = >sched_list[0]->sched_rq[entity->priority];
@@ -312,7 +303,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
dma_fence_put(entity->last_scheduled);
entity->last_scheduled = NULL;
- kfree(entity->sched_list);
}
EXPORT_SYMBOL(drm_sched_entity_fini);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 4/4] drm/scheduler: do not keep a copy of sched list
entity should not keep copy and maintain sched list for
itself.
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 11 ---
drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h | 1 +
drivers/gpu/drm/scheduler/sched_entity.c | 12 +---
3 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index c7643af8827f..1939b414d23b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -71,7 +71,6 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip)
{
struct amdgpu_ring *rings[AMDGPU_MAX_RINGS];
- struct drm_gpu_scheduler *sched_list[AMDGPU_MAX_RINGS];
struct amdgpu_device *adev = ctx->adev;
unsigned num_rings = 0;
unsigned num_scheds = 0;
@@ -129,16 +128,21 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx,
u32 hw_ip)
break;
}
+ ctx->sched_list = kcalloc(num_rings,
+ sizeof(struct drm_gpu_scheduler *),
GFP_KERNEL);
+ if (ctx->sched_list == NULL )
+ return -ENOMEM;
+
for (i = 0; i < num_rings; ++i) {
if (!rings[i]->adev)
continue;
- sched_list[num_scheds++] = [i]->sched;
+ ctx->sched_list[num_scheds++] = [i]->sched;
}
for (i = 0; i < amdgpu_ctx_num_entities[hw_ip]; ++i)
r = drm_sched_entity_init(>entities[hw_ip][i].entity,
- ctx->init_priority, sched_list, num_scheds,
>guilty);
+ ctx->init_priority, ctx->sched_list,
num_scheds, >guilty);
if (r)
goto error_cleanup_entities;
@@ -228,6 +232,7 @@ static void amdgpu_ctx_fini(struct kref *ref)
for (j = 0; j < amdgpu_sched_jobs; ++j)
dma_fence_put(ctx->entities[0][i].fences[j]);
kfree(ctx->fences);
+ kfree(ctx->sched_list);
kfree(ctx->entities[0]);
mutex_destroy(>lock);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index da808633732b..9fd02cc47352 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -44,6 +44,7 @@ struct amdgpu_ctx {
spinlock_t ring_lock;
struct dma_fence**fences;
struct amdgpu_ctx_entity*entities[AMDGPU_HW_IP_NUM];
+ struct drm_gpu_scheduler**sched_list;
boolpreamble_presented;
enum drm_sched_priority init_priority;
enum drm_sched_priority override_priority;
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index f9b6ce29c58f..c84a9a66f7f0 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -56,8 +56,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
unsigned int num_sched_list,
atomic_t *guilty)
{
- int i;
-
if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
return -EINVAL;
@@ -67,17 +65,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
entity->guilty = guilty;
entity->num_sched_list = num_sched_list;
entity->priority = priority;
- entity->sched_list = kcalloc(num_sched_list,
- sizeof(struct drm_gpu_scheduler *),
GFP_KERNEL);
-
- if(!entity->sched_list)
- return -ENOMEM;
+ entity->sched_list = sched_list;
init_completion(>entity_idle);
- for (i = 0; i < num_sched_list; i++)
- entity->sched_list[i] = sched_list[i];
-
if (num_sched_list)
entity->rq = >sched_list[0]->sched_rq[entity->priority];
@@ -312,7 +303,6 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
dma_fence_put(entity->last_scheduled);
entity->last_scheduled = NULL;
- kfree(entity->sched_list);
}
EXPORT_SYMBOL(drm_sched_entity_fini);
--
2.24.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
