On 10/09/2024 11:05, Philipp Stanner wrote:
On Mon, 2024-09-09 at 18:19 +0100, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
Lets re-order the members to make it clear which are protected by the
lock
and at the same time document it via kerneldoc.
I'd prefer if commit messages follow the idiomatic kernel style of that
order:
1. Describe the current situation
2. State why it's bad or undesirable
3. (describe the solution)
4. Conclude commit message through sentences in imperative stating
what the commit does.
In this case I would go for:
"struct drm_sched_rq contains a spinlock that protects several struct
members. The current documentation incorrectly states that this lock
only guards the entities list. In truth, it guards that list, the
rb_tree and the current entity.
Document what the lock actually guards. Rearrange struct members so
that this becomes even more visible."
IMO a bit much to ask for a text book format, for a trivial patch, when
all points are already implicitly obvious. That is "lets make it clear"
= current situation is not clear -> obviously bad with no need to
explain; "and the same time document" = means it is currently not
documented -> again obviously not desirable.
But okay, since I agree with the point below (*), I can explode the text
for maximum redundancy.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
Cc: Christian König <christian.koenig@xxxxxxx>
Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Cc: Luben Tuikov <ltuikov89@xxxxxxxxx>
Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
Cc: Philipp Stanner <pstanner@xxxxxxxxxx>
---
include/drm/gpu_scheduler.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/drm/gpu_scheduler.h
b/include/drm/gpu_scheduler.h
index a06753987d93..d4a3ba333568 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -243,10 +243,10 @@ struct drm_sched_entity {
/**
* struct drm_sched_rq - queue of entities to be scheduled.
*
- * @lock: to modify the entities list.
* @sched: the scheduler to which this rq belongs to.
- * @entities: list of the entities to be scheduled.
+ * @lock: protects the list, tree and current entity.
Would be more consistent with the below comment if you'd address them
with their full name, aka "protects @entities, @rb_tree_root and
@current_entity".
*) this one I agree with.
Regards,
Tvrtko
Thanks,
P.
* @current_entity: the entity which is to be scheduled.
+ * @entities: list of the entities to be scheduled.
* @rb_tree_root: root of time based priory queue of entities for
FIFO scheduling
*
* Run queue is a set of entities scheduling command submissions for
@@ -254,10 +254,12 @@ struct drm_sched_entity {
* the next entity to emit commands from.
*/
struct drm_sched_rq {
- spinlock_t lock;
struct drm_gpu_scheduler *sched;
- struct list_head entities;
+
+ spinlock_t lock;
+ /* Following members are protected by the @lock: */
struct drm_sched_entity *current_entity;
+ struct list_head entities;
struct rb_root_cached rb_tree_root;
};
