[PATCH 6/8] drm/sched: Re-order struct drm_sched_rq members for clarity

[Philipp Stanner]

On Tue, 2024-09-10 at 11:42 +0100, Tvrtko Ursulin wrote:
> 
> 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.

I agree that for very short / trivial changes one can keep it short.
But the line separating what is obvious for oneself and for others is
often thin.

P.

> 
> > > 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;
> > >   };
> > >   
> > 
>