| Message ID | 20240913160559.49054-7-tursulin@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | DRM scheduler fixes and improvements | expand |
On Fri, 2024-09-13 at 17:05 +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > > Current kerneldoc for struct drm_sched_rq incompletely documents what > fields are protected by the lock. > > This is not good because it is misleading. > > Lets fix it by listing all the elements which are protected by the > lock. > > While at it, lets also re-order the members so all protected by the > lock > are in a single group. > > v2: > * Refer variables by kerneldoc syntax, more verbose commit text. > (Philipp) > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Luben Tuikov <ltuikov89@gmail.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: Philipp Stanner <pstanner@redhat.com> > Reviewed-by: Christian König <christian.koenig@amd.com> Looks good, thx Reviewed-by: Philipp Stanner <pstanner@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 38465b78c7d5..2f58af00f792 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 @entities, @rb_tree_root and @current_entity. nit: in case you'll provide a new version anyways you could consider sorting these three to be congruent with the lines below. Thank you! 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; > }; >
On 16/09/2024 09:16, Philipp Stanner wrote: > On Fri, 2024-09-13 at 17:05 +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> >> Current kerneldoc for struct drm_sched_rq incompletely documents what >> fields are protected by the lock. >> >> This is not good because it is misleading. >> >> Lets fix it by listing all the elements which are protected by the >> lock. >> >> While at it, lets also re-order the members so all protected by the >> lock >> are in a single group. >> >> v2: >> * Refer variables by kerneldoc syntax, more verbose commit text. >> (Philipp) >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> Cc: Christian König <christian.koenig@amd.com> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Cc: Luben Tuikov <ltuikov89@gmail.com> >> Cc: Matthew Brost <matthew.brost@intel.com> >> Cc: Philipp Stanner <pstanner@redhat.com> >> Reviewed-by: Christian König <christian.koenig@amd.com> > > Looks good, thx > > Reviewed-by: Philipp Stanner <pstanner@redhat.com> Thanks! 4/8 and 8/8 are now the only two left with no r-b. >> --- >> 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 38465b78c7d5..2f58af00f792 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 @entities, @rb_tree_root and @current_entity. > > nit: in case you'll provide a new version anyways you could consider > sorting these three to be congruent with the lines below. To me it looks the order of kerneldoc vs members is aligned. Unless I missed what you mean here? Regards, Tvrtko >> * @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; >> }; >> >
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 38465b78c7d5..2f58af00f792 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 @entities, @rb_tree_root and @current_entity. * @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; };