Am 28.02.20 um 06:08 schrieb Luben Tuikov:
On 2020-02-27 4:40 p.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
Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
---
drivers/gpu/drm/scheduler/sched_entity.c | 24 ++++++++++++++++++++++++
include/drm/gpu_scheduler.h | 4 ++++
2 files changed, 28 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 63bccd201b97..711e9d504bcb 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -83,6 +83,30 @@ 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
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
+ struct drm_gpu_scheduler **sched_list,
+ unsigned int num_sched_list)
+{
+ if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
+ return -EINVAL;
This seems unmaintainable. I'd write it in its natural form:
This is probably just copy & pasted from the init function and complete
overkill here.
int drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
struct drm_gpu_scheduler **sched_list,
unsigned int num_sched_list)
{
if (entity && sched_list && (num_sched_list == 0 || sched_list[0] != NULL)) {
entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
entity->num_sched_list = num_sched_list;
return 0;
} else {
return -EINVAL;
}
}
Actually that's a rather bad idea. Error handling should always be in
the form of:
if (check_error || missing_prerequisite)
return_or_goto_cleanup;
That's too heavy. Can we improve the architecture
so we don't have to check for this in leaf functions like this one?
We can just return a parameterization.
Why would this be called with entity being NULL?
Or with sched_list being NULL? Or num_sched_list being not zero
yet sched_list[0] being NULL? Why not make sure that sched_list[0] is
never NULL and that num_sched_list is greater than 0 always?
Does this make it to user space?
Why would the state of execution be one such that this is true/false
for the code to return -EINVAL?
From patch 3/4 it seems that an error is printed inside amdgpu_ctx_priority_override()
and the result is not propagated up the stack.
I think we should improve the code where here this condition above
is never true. Then we can use parameterization for those two
statements below:
+
+ entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
So if we're here, we know from the top conditional that
either num_sched_list is 0 or that sched_list[0] not NULL
or both.
So if num_sched_list is 0 or 1 we return NULL?
And if num_sched_list is 2 or greater we return sched_list
of which sched_list[0] could be NULL?
This is also copy&pasted from the init code and completely wrong here.
What we should do instead is just: WARN_ON(!num_sched_list || !sched_list);
And to the checking for not keeping around the scheduler list in the
init function.
Why not fix the architecture so that this is simply copied?
We had that and moved away from it because the scheduler list is
actually const and shouldn't be allocated with each entity (which we can
easily have thousands of).
Regards,
Christian.
(in which case
we can simply index-parameterize it and simply copy it.
Why are there so many checks everywhere?
+ entity->num_sched_list = num_sched_list;
+
I mean, all we're doing in this function is initializing
entity->sched_list and entity->num_sched_list. Why does this
function have to be so complex and do so many checks?
Can we improve/fix the architecture instead?
Regards,
Luben
+ return 0;
+}
+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..0c164a96d51b 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);
+int 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);
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
