[PATCH 1/6] drm/sched: Add internal job peek/pop API
Tvrtko Ursulin
tvrtko.ursulin at igalia.com
Tue Feb 18 08:35:21 UTC 2025
On 18/02/2025 08:12, Philipp Stanner wrote:
> On Thu, 2025-02-13 at 14:05 -0800, Matthew Brost wrote:
>> On Wed, Feb 12, 2025 at 01:36:58PM +0100, Philipp Stanner wrote:
>>> On Wed, 2025-02-12 at 12:30 +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 12/02/2025 10:40, Philipp Stanner wrote:
>>>>> On Wed, 2025-02-12 at 09:32 +0000, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 12/02/2025 09:02, Philipp Stanner wrote:
>>>>>>> On Fri, 2025-02-07 at 14:50 +0000, Tvrtko Ursulin wrote:
>>>>>>>> Idea is to add helpers for peeking and popping jobs from
>>>>>>>> entities
>>>>>>>> with
>>>>>>>> the goal of decoupling the hidden assumption in the code
>>>>>>>> that
>>>>>>>> queue_node
>>>>>>>> is the first element in struct drm_sched_job.
>>>>>>>>
>>>>>>>> That assumption usually comes in the form of:
>>>>>>>>
>>>>>>>> while ((job =
>>>>>>>> to_drm_sched_job(spsc_queue_pop(&entity-
>>>>>>>>> job_queue))))
>>>>>>>>
>>>>>>>> Which breaks if the queue_node is re-positioned due
>>>>>>>> to_drm_sched_job
>>>>>>>> being implemented with a container_of.
>>>>>>>>
>>>>>>>> This also allows us to remove duplicate definitions of
>>>>>>>> to_drm_sched_job.
>>>>>>>>
>>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
>>>>>>>> Cc: Christian König <christian.koenig at amd.com>
>>>>>>>> Cc: Danilo Krummrich <dakr at kernel.org>
>>>>>>>> Cc: Matthew Brost <matthew.brost at intel.com>
>>>>>>>> Cc: Philipp Stanner <phasta at kernel.org>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/scheduler/sched_entity.c | 11 +++---
>>>>>>>> drivers/gpu/drm/scheduler/sched_internal.h | 46
>>>>>>>> ++++++++++++++++++++++
>>>>>>>> drivers/gpu/drm/scheduler/sched_main.c | 7 ++--
>>>>>>>> 3 files changed, 54 insertions(+), 10 deletions(-)
>>>>>>>> create mode 100644
>>>>>>>> drivers/gpu/drm/scheduler/sched_internal.h
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> index 69bcf0e99d57..a171f05ad761 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>>>> @@ -28,11 +28,10 @@
>>>>>>>> #include <drm/drm_print.h>
>>>>>>>> #include <drm/gpu_scheduler.h>
>>>>>>>>
>>>>>>>> +#include "sched_internal.h"
>>>>>>>> +
>>>>>>>> #include "gpu_scheduler_trace.h"
>>>>>>>>
>>>>>>>> -#define to_drm_sched_job(sched_job) \
>>>>>>>> - container_of((sched_job), struct
>>>>>>>> drm_sched_job,
>>>>>>>> queue_node)
>>>>>>>> -
>>>>>>>> /**
>>>>>>>> * drm_sched_entity_init - Init a context entity used
>>>>>>>> by
>>>>>>>> scheduler
>>>>>>>> when
>>>>>>>> * submit to HW ring.
>>>>>>>> @@ -255,7 +254,7 @@ static void
>>>>>>>> drm_sched_entity_kill(struct
>>>>>>>> drm_sched_entity *entity)
>>>>>>>> /* The entity is guaranteed to not be used by
>>>>>>>> the
>>>>>>>> scheduler
>>>>>>>> */
>>>>>>>> prev = rcu_dereference_check(entity-
>>>>>>>>> last_scheduled,
>>>>>>>> true);
>>>>>>>> dma_fence_get(prev);
>>>>>>>> - while ((job =
>>>>>>>> to_drm_sched_job(spsc_queue_pop(&entity-
>>>>>>>>> job_queue)))) {
>>>>>>>> + while ((job =
>>>>>>>> drm_sched_entity_queue_pop(entity))) {
>>>>>>>> struct drm_sched_fence *s_fence = job-
>>>>>>>>> s_fence;
>>>>>>>>
>>>>>>>> dma_fence_get(&s_fence->finished);
>>>>>>>> @@ -477,7 +476,7 @@ struct drm_sched_job
>>>>>>>> *drm_sched_entity_pop_job(struct drm_sched_entity
>>>>>>>> *entity)
>>>>>>>> {
>>>>>>>> struct drm_sched_job *sched_job;
>>>>>>>>
>>>>>>>> - sched_job =
>>>>>>>> to_drm_sched_job(spsc_queue_peek(&entity-
>>>>>>>>> job_queue));
>>>>>>>> + sched_job = drm_sched_entity_queue_peek(entity);
>>>>>>>> if (!sched_job)
>>>>>>>> return NULL;
>>>>>>>>
>>>>>>>> @@ -513,7 +512,7 @@ struct drm_sched_job
>>>>>>>> *drm_sched_entity_pop_job(struct drm_sched_entity
>>>>>>>> *entity)
>>>>>>>> if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) {
>>>>>>>> struct drm_sched_job *next;
>>>>>>>>
>>>>>>>> - next =
>>>>>>>> to_drm_sched_job(spsc_queue_peek(&entity-
>>>>>>>>> job_queue));
>>>>>>>> + next =
>>>>>>>> drm_sched_entity_queue_peek(entity);
>>>>>>>> if (next) {
>>>>>>>> struct drm_sched_rq *rq;
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_internal.h
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_internal.h
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..25ac62ac2bf3
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_internal.h
>>>>>>>> @@ -0,0 +1,46 @@
>>>>>>>> +#ifndef _DRM_GPU_SCHEDULER_INTERNAL_H_
>>>>>>>> +#define _DRM_GPU_SCHEDULER_INTERNAL_H_
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_sched_entity_queue_pop - Low level helper for
>>>>>>>> popping
>>>>>>>> queued
>>>>>>>> jobs
>>>>>>>> + *
>>>>>>>> + * @entity: scheduler entity
>>>>>>>> + *
>>>>>>>> + * Low level helper for popping queued jobs.
>>>>>>>> + *
>>>>>>>> + * Returns the job dequeued or NULL.
>>>>>>>> + */
>>>>>>>> +static inline struct drm_sched_job *
>>>>>>>> +drm_sched_entity_queue_pop(struct drm_sched_entity
>>>>>>>> *entity)
>>>>>>>> +{
>>>>>>>> + struct spsc_node *node;
>>>>>>>> +
>>>>>>>> + node = spsc_queue_pop(&entity->job_queue);
>>>>>>>> + if (!node)
>>>>>>>> + return NULL;
>>>>>>>> +
>>>>>>>> + return container_of(node, struct drm_sched_job,
>>>>>>>> queue_node);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_sched_entity_queue_peek - Low level helper for
>>>>>>>> peeking at
>>>>>>>> the
>>>>>>>> job queue
>>>>>>>> + *
>>>>>>>> + * @entity: scheduler entity
>>>>>>>> + *
>>>>>>>> + * Low level helper for peeking at the job queue
>>>>>>>> + *
>>>>>>>> + * Returns the job at the head of the queue or NULL.
>>>>>>>
>>>>>>> I would like to (slowly) work towards a unified style
>>>>>>> regarding
>>>>>>> the
>>>>>>> docstrings. They're currently relatively inconsistent in
>>>>>>> drm/sched.
>>>>>>>
>>>>>>> I think we should do it that way:
>>>>>>>
>>>>>>> ""
>>>>>>> @entity: scheduler entity
>>>>>>>
>>>>>>> Returns: the job at the head of the queue or NULL.
>>>>>>>
>>>>>>> Low level helper for peeking at the the job queue.
>>>>>>> ""
>>>>>>
>>>>>> Returns before the description would be yet another new
>>>>>> style,
>>>>>> no?
>>>>>> I's
>>>>>> say that if we are churning lets follow
>>>>>> Documentation/doc-guide/kernel-doc.rst.
>>>>>
>>>>> Oh yes, you are right – official guideline demands "Return:" at
>>>>> the
>>>>> end. So let's go for that for contributions.
>>>>
>>>> So you want me to respin or are you okay with doing all scheduler
>>>> kernel
>>>> doc in one patch afterwards?
>>>
>>> Both's OK I guess. It's not a big deal, it's just one letter being
>>> replaced. If you find some other nits you'd like to address you
>>> could
>>> give a v5 with that change?
>>>
>>> Rest of the series looks good to me. Having an ACK by AMD pro forma
>>> for
>>> merging everything together would be nice, though
>>>
>>
>> +1. Series looks good to me and all for moving some public DRM
>> scheduler
>> functions to internal headers. Good from Xe's end too.
>>
>> I would send this to the Xe list to get a CI run though ahead of
>> merging.
>
> With "I would" you mean me and/or Tvrtko? :)
>
> @Tvrtko, if you provide a v5, can you +Cc Xe?
It's there since four days ago:
https://lore.kernel.org/intel-xe/20250214101944.19390-1-tvrtko.ursulin@igalia.com/T/#t
BAT was a pass and full run looks good to me too, although some failures
were logged they do not seem logged or related to me.
Regards,
Tvrtko
>>>>>> Or even consider sending a patch
>>>>>> which churns everything at once.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Tvrtko
>>>>>>
>>>>>>>> + */
>>>>>>>> +static inline struct drm_sched_job *
>>>>>>>> +drm_sched_entity_queue_peek(struct drm_sched_entity
>>>>>>>> *entity)
>>>>>>>> +{
>>>>>>>> + struct spsc_node *node;
>>>>>>>> +
>>>>>>>> + node = spsc_queue_peek(&entity->job_queue);
>>>>>>>> + if (!node)
>>>>>>>> + return NULL;
>>>>>>>> +
>>>>>>>> + return container_of(node, struct drm_sched_job,
>>>>>>>> queue_node);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +#endif
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> index a48be16ab84f..9f614a775c49 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> @@ -78,6 +78,8 @@
>>>>>>>> #include <drm/gpu_scheduler.h>
>>>>>>>> #include <drm/spsc_queue.h>
>>>>>>>>
>>>>>>>> +#include "sched_internal.h"
>>>>>>>> +
>>>>>>>> #define CREATE_TRACE_POINTS
>>>>>>>> #include "gpu_scheduler_trace.h"
>>>>>>>>
>>>>>>>> @@ -87,9 +89,6 @@ static struct lockdep_map
>>>>>>>> drm_sched_lockdep_map
>>>>>>>> = {
>>>>>>>> };
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> -#define to_drm_sched_job(sched_job) \
>>>>>>>> - container_of((sched_job), struct
>>>>>>>> drm_sched_job,
>>>>>>>> queue_node)
>>>>>>>> -
>>>>>>>> int drm_sched_policy = DRM_SCHED_POLICY_FIFO;
>>>>>>>>
>>>>>>>> /**
>>>>>>>> @@ -123,7 +122,7 @@ static bool
>>>>>>>> drm_sched_can_queue(struct
>>>>>>>> drm_gpu_scheduler *sched,
>>>>>>>> {
>>>>>>>> struct drm_sched_job *s_job;
>>>>>>>>
>>>>>>>> - s_job =
>>>>>>>> to_drm_sched_job(spsc_queue_peek(&entity-
>>>>>>>>> job_queue));
>>>>>>>> + s_job = drm_sched_entity_queue_peek(entity);
>>>>>>>> if (!s_job)
>>>>>>>> return false;
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
More information about the amd-gfx
mailing list