Hi Philipp,
On 23/01/25 05:10, Philipp Stanner wrote:
On Wed, 2025-01-22 at 19:07 -0300, Maíra Canal wrote:
Hi Philipp,
On 22/01/25 11:08, Philipp Stanner wrote:
drm_sched_init() has a great many parameters and upcoming new
functionality for the scheduler might add even more. Generally, the
great number of parameters reduces readability and has already
caused
one missnaming in:
commit 6f1cacf4eba7 ("drm/nouveau: Improve variable name in
nouveau_sched_init()").
Introduce a new struct for the scheduler init parameters and port
all
users.
Signed-off-by: Philipp Stanner <phasta@xxxxxxxxxx>
[...]
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c
b/drivers/gpu/drm/v3d/v3d_sched.c
index 99ac4995b5a1..716e6d074d87 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -814,67 +814,124 @@ static const struct drm_sched_backend_ops
v3d_cpu_sched_ops = {
.free_job = v3d_cpu_job_free
};
+/*
+ * v3d's scheduler instances are all identical, except for ops and
name.
+ */
+static void
+v3d_common_sched_init(struct drm_sched_init_params *params, struct
device *dev)
+{
+ memset(params, 0, sizeof(struct drm_sched_init_params));
+
+ params->submit_wq = NULL; /* Use the system_wq. */
+ params->num_rqs = DRM_SCHED_PRIORITY_COUNT;
+ params->credit_limit = 1;
+ params->hang_limit = 0;
+ params->timeout = msecs_to_jiffies(500);
+ params->timeout_wq = NULL; /* Use the system_wq. */
+ params->score = NULL;
+ params->dev = dev;
+}
Could we use only one function that takes struct v3d_dev *v3d, enum
v3d_queue, and sched_ops as arguments (instead of one function per
queue)? You can get the name of the scheduler by concatenating "v3d_"
to
the return of v3d_queue_to_string().
I believe it would make the code much simpler.
Hello,
so just to get that right:
You'd like to have one universal function that switch-cases over an
enum, sets the ops and creates the name with string concatenation?
I'm not convinced that this is simpler than a few small functions, but
it's not my component, so…
Whatever we'll do will be simpler than the existing code, though. Right
now no reader can see at first glance whether all those schedulers are
identically parametrized or not.
This is my proposal (just a quick draft, please check if it compiles):
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c
b/drivers/gpu/drm/v3d/v3d_sched.c
index 961465128d80..7cc45a0c6ca0 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -820,67 +820,62 @@ static const struct drm_sched_backend_ops
v3d_cpu_sched_ops = {
.free_job = v3d_cpu_job_free
};
+static int
+v3d_sched_queue_init(struct v3d_dev *v3d, enum v3d_queue queue,
+ const struct drm_sched_backend_ops *ops, const char
*name)
+{
+ struct drm_sched_init_params params = {
+ .submit_wq = NULL,
+ .num_rqs = DRM_SCHED_PRIORITY_COUNT,
+ .credit_limit = 1,
+ .hang_limit = 0,
+ .timeout = msecs_to_jiffies(500),
+ .timeout_wq = NULL,
+ .score = NULL,
+ .dev = v3d->drm.dev,
+ };
+
+ params.ops = ops;
+ params.name = name;
+
+ return drm_sched_init(&v3d->queue[queue].sched, ¶ms);
+}
+
int
v3d_sched_init(struct v3d_dev *v3d)
{
- int hw_jobs_limit = 1;
- int job_hang_limit = 0;
- int hang_limit_ms = 500;
int ret;
- ret = drm_sched_init(&v3d->queue[V3D_BIN].sched,
- &v3d_bin_sched_ops, NULL,
- DRM_SCHED_PRIORITY_COUNT,
- hw_jobs_limit, job_hang_limit,
- msecs_to_jiffies(hang_limit_ms), NULL,
- NULL, "v3d_bin", v3d->drm.dev);
+ ret = v3d_sched_queue_init(v3d, V3D_BIN, &v3d_bin_sched_ops,
+ "v3d_bin");
if (ret)
return ret;
- ret = drm_sched_init(&v3d->queue[V3D_RENDER].sched,
- &v3d_render_sched_ops, NULL,
- DRM_SCHED_PRIORITY_COUNT,
- hw_jobs_limit, job_hang_limit,
- msecs_to_jiffies(hang_limit_ms), NULL,
- NULL, "v3d_render", v3d->drm.dev);
+ ret = v3d_sched_queue_init(v3d, V3D_RENDER, &v3d_render_sched_ops,
+ "v3d_render");
if (ret)
goto fail;
[...]
At least for me, this looks much simpler than one function for each
V3D queue.
Best Regards,
- Maíra
P.
Best Regards,
- Maíra
