[PATCH 1/3] drm/sched: add drm_sched_prealloc_dependency_slots v3
Tvrtko Ursulin
tursulin at ursulin.net
Mon May 19 08:51:30 UTC 2025
On 16/05/2025 18:16, Philipp Stanner wrote:
> On Fri, 2025-05-16 at 15:30 +0100, Tvrtko Ursulin wrote:
>>
>> On 16/05/2025 14:38, Philipp Stanner wrote:
>>> On Fri, 2025-05-16 at 13:10 +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 16/05/2025 12:53, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 16/05/2025 08:28, Philipp Stanner wrote:
>>>>>> On Thu, 2025-05-15 at 17:17 +0100, Tvrtko Ursulin wrote:
>>>>>>>
>>>>>>> On 15/05/2025 16:00, Christian König wrote:
>>>>>>>> Sometimes drivers need to be able to submit multiple jobs
>>>>>>>> which
>>>>>>>> depend on
>>>>>>>> each other to different schedulers at the same time, but
>>>>>>>> using
>>>>>>>> drm_sched_job_add_dependency() can't fail any more after
>>>>>>>> the
>>>>>>>> first
>>>>>>>> job is
>>>>>>>> initialized.
>>>>>>>>
>>>>>>>> This function preallocate memory for dependency slots so
>>>>>>>> that
>>>>>>>> no
>>>>>>>> ENOMEM
>>>>>>>> can come later while adding dependencies.
>>>>>>>>
>>>>>>>> v2: rework implementation an documentation
>>>>>>>> v3: rework from scratch, use separate function to add
>>>>>>>> preallocated
>>>>>>>> deps
>>>>>>
>>>>>> I think we agreed to not put change logs into commit messages
>>>>>> anymore
>>>>>> :)
>>>>>>
>>>>>> They aren't useful for any reader. Who needs the changelog
>>>>>> afterwards
>>>>>> can retreive it through the mail thread link that we add.
>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/scheduler/sched_main.c | 45
>>>>>>>> ++++++++++++++++++++++++++
>>>>>>>> include/drm/gpu_scheduler.h | 4 +++
>>>>>>>> 2 files changed, 49 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> index f7118497e47a..b95e7089aa70 100644
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> @@ -858,6 +858,51 @@ void drm_sched_job_arm(struct
>>>>>>>> drm_sched_job
>>>>>>>> *job)
>>>>>>>> }
>>>>>>>> EXPORT_SYMBOL(drm_sched_job_arm);
>>>>>>>> +/**
>>>>>>>> + * drm_sched_job_prealloc_dependency_slot - avoid ENOMEM
>>>>>>>> on
>>>>>>>> adding
>>>>>>>> dependencies
>>>>>>>> + * @job: scheduler job where dependencies will be added
>>>>>>>> + * @id: id for the allocated slot
>>>>>>>> + *
>>>>>>>> + * Sometimes drivers need to be able to submit multiple
>>>>>>>> jobs
>>>>>>>> which
>>>>>>>> depend on
>>>>>>>> + * each other to different schedulers at the same time,
>>>>>>>> but
>>>>>>>> using
>>>>>>>> + * drm_sched_job_add_dependency() can't fail any more
>>>>>>>> after
>>>>>>>> the
>>>>>>>> first job is
>>>>>>>> + * initialized.
>>>>>>>> + *
>>>>>>>> + * This function preallocate memory for a dependency
>>>>>>>> slot so
>>>>>>>> that
>>>>>>>> no ENOMEM can
>>>>>>>> + * come later while adding dependencies. The index of
>>>>>>>> the
>>>>>>>> preallocated slot is
>>>>>>>> + * returned in @id.
>>>>>>>> + *
>>>>>>>> + * Return:
>>>>>>>> + * 0 on success, or an error on failing to expand the
>>>>>>>> array.
>>>>>>>> + */
>>>>>>>> +int drm_sched_job_prealloc_dependency_slot(struct
>>>>>>>> drm_sched_job
>>>>>>>> *job,
>>>>>>>> + u32 *id)
>>>>>>>> +{
>>>>>>>> + return xa_alloc(&job->dependencies, id, NULL,
>>>>>>>> xa_limit_32b, GFP_KERNEL);
>>>>>>>> +}
>>>>>>>> +EXPORT_SYMBOL(drm_sched_job_prealloc_dependency_slot);
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * drm_sched_job_add_prealloc_dep - add dependency to
>>>>>>>> preallocated
>>>>>>>> slot
>>>>>>>> + * @job: scheduler job where dependencies will be added
>>>>>>>> + * @id: the preallocated slot index
>>>>>>>> + * @fence: the dependency to add
>>>>>>>> + *
>>>>>>>> + * Consumes @fence and adds it to the preallocated slot
>>>>>>>> dependency.
>>>>>>>> + */
>>>>>>>> +void drm_sched_job_add_prealloc_dep(struct drm_sched_job
>>>>>>>> *job, u32
>>>>>>>> id,
>>>>>>>> + struct dma_fence *fence)
>>>>>>>> +{
>>>>>>>> + fence = xa_store(&job->dependencies, id, fence,
>>>>>>>> GFP_ATOMIC);
>>>>>>>
>>>>>>> Add assert that the passed id exists (was preallocated) and
>>>>>>> is
>>>>>>> NULL?
>>>>>>
>>>>>> You
>>>>>
>>>>> Hm?
>>>>>
>>>>>>>
>>>>>>> Also, if someone preallocates and does not consume the slot
>>>>>>> will that
>>>>>>> confuse the iteration in drm_sched_job_dependency()?
>>>>>>
>>>>>> drm_sched_job_add_dependency() you mean.
>>>>>
>>>>> I was actually thinking of drm_sched_job_dependency() because
>>>>> that
>>>>> looked it would skip dependencies upon encountering an
>>>>> unconsumed
>>>>> preallocated slot, but yes, drm_sched_job_add_dependency()
>>>>> could
>>>>> explode
>>>>> even earlier if adding a normal dependency after preallocating
>>>>> a
>>>>> slot.
>>>>>
>>>>>> Yes, it would. All operations simply give you NULL for those
>>>>>> slots. So
>>>>>> seems to me you have to check for NULL wherever a
>>>>>> preallocated
>>>>>> slot
>>>>>> might drop out. That would then be a bug.
>>>>>>
>>>>>> It's kind of tricky, all that. It's a pity that Wilcox didn't
>>>>>> answer
>>>>>> our questions about the idiomatic way to do it.
>>>>>>
>>>>>> Maybe reserving slots with already signaled fences wasn't
>>>>>> such a
>>>>>> bad
>>>>>> idea after all?
>>>>>>
>>>>>> If we go for the NULL approach, it's probably the only sane
>>>>>> way
>>>>>> to then
>>>>>> check for NULL wherever dependencies are accessed :(
>>>>>>
>>>>>> Opinions?
>>>>>
>>>>> Well if the xarray API returns the NULL consistently the
>>>>> approach
>>>>> from
>>>>> this patch is fine I think.
>>>>>
>>>>> We just need to add two more checks to the above mentioned
>>>>> functions,
>>>>
>>>> I need to correct myself, drm_sched_job_dependency() wouldn't be
>>>> able
>>>> to
>>>> just skip NULLs since it relies on NULL for "no more
>>>> dependencies".
>>>> We
>>>> would need to track something like job->max_dependency and
>>>> terminate
>>>> on
>>>> job->last_dependency > job->max_dependency or so.
>>>
>>> Agreed, that would have to be fixed.
>>>
>>> I believe we should reconsider Christian's first idea [1].
>>>
>>> Thinking about it some more:
>>> * With the NULL version, suddenly the xarray containing only
>>> valid
>>> dependencies can sometimes contain NULL entries.
>>> * If we could create our own tag, entries could be returned that
>>> were
>>> neither NULL nor valid fences, also requiring checks
>>> 'everywhere'.
>>> * Only the "signaled fence as prealloc reservation" approach is
>>> fully
>>> backwards compatible and will never cause anyone to block after
>>> later reworks.
>>>
>>> So maybe it's actually the best idea?
>>>
>>> Sorry for the zigg-zagg. No hard requirements intended from my
>>> side,
>>> I'm willing to go with what you guys think.
>>>
>>> Just saying, at least now I think that the already-signaled fence
>>> seems
>>> the most elegant solution. And since there's a function
>>> (dma_fence_get_stub()) for that, it seems to be in alignment with
>>> official dma_fence rules.
>>
>> Potential problem there was dma_fence_is_signaled() and fence
>> signaling
>> annotations. In case some driver is holding a lock over the arm+push
>> pair. I wish we had a non-signaling is_signaled helper..
>>
>
> Yes! +1!
>
> But Christian doesn't like that direction:
>
> https://lore.kernel.org/all/20250409120640.106408-2-phasta@kernel.org/
Thanks, I read this but ended up uncertain on the conclusion.
For instance Christian at the end comments like this:
"""
You can test the flag if you know what the fence means to you, that is
not a problem at all.
"""
That was in the context of testing the signaled bit without
opportunistic signaling.
For me, from the scheduler dependencies side, that should exactly apply.
Scheduler knows it does not need to add a signaled fence to the dep
array so AFAICS it is fine to skip it. And it may easily be
opportunistic signaling ends up a problem for the scheduler.
So maybe such helper would be okay after all.
Or if the concern is helper might encourage some potentially unsafe
usage, in that case it should come with kerneldoc describing. It is not
like review is guaranteed to catch someone using test_bit directly
anyway so for me, on balance, helper is always better.
Regards,
Tvrtko
>
> P.
>
>>
>>
>
>
>>
>>
>> Anyway, I think both options are passable. I even like the NULL entry
>> slightly more since it is simpler in a way and I don't mind some
>> extra
>> checks completely hidden in scheduler internals.
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>>
>>> Philipp
>>>
>>>
>>> [1]
>>> https://lore.kernel.org/all/20250318120313.19099-2-christian.koenig@amd.com
>>> /
>>>
>>>
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> some more unit tests probably to make sure, and that should be
>>>>> fine
>>>>> for
>>>>> now.
>>>>>
>>>>> On the bikeshedding front I would perhaps suggest:
>>>>>
>>>>> - drm_sched_job_preallocate_dependency()
>>>>> - drm_sched_job_replace_dependency()
>>>>>
>>>>> Reads a little bit more aligned with the rest of the API and a
>>>>> bit
>>>>> easier on the eyes, to my eyes at least.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>>
>>>>
>>>
>>
>
More information about the dri-devel
mailing list