[PATCH 6/8] drm/sched: Re-order struct drm_sched_rq members for clarity
Tvrtko Ursulin
tvrtko.ursulin at igalia.com
Tue Sep 10 10:42:20 UTC 2024
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 at igalia.com>
>>
>> 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 at igalia.com>
>> Cc: Christian König <christian.koenig at amd.com>
>> Cc: Alex Deucher <alexander.deucher at amd.com>
>> Cc: Luben Tuikov <ltuikov89 at gmail.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: Philipp Stanner <pstanner at redhat.com>
>> ---
>> 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;
>> };
>>
>
More information about the dri-devel
mailing list