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.ursu...@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.ursu...@igalia.com>
> > > Cc: Christian König <christian.koe...@amd.com>
> > > Cc: Alex Deucher <alexander.deuc...@amd.com>
> > > Cc: Luben Tuikov <ltuiko...@gmail.com>
> > > Cc: Matthew Brost <matthew.br...@intel.com>
> > > Cc: Philipp Stanner <pstan...@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;
> > > };
> > >
> >
>
