[PATCH] Revert "drm/sched: Qualify drm_sched_wakeup() by drm_sched_entity_is_ready()"

[PATCH] Revert "drm/sched: Qualify drm_sched_wakeup() by drm_sched_entity_is_ready()"

[Bert Karwatzki]

Commit f3123c25 (in combination with the use of work queues by the gpu
scheduler) leads to random lock ups of the GUI [1,2].

This is not a complete revert of commit f3123c25 as drm_sched_wakeup
still needs its entity argument to pass it to drm_sched_can_queue.

[1] https://gitlab.freedesktop.org/drm/amd/-/issues/2994
[2] https://lists.freedesktop.org/archives/dri-devel/2023-November/431606.html

This reverts commit f3123c2590005c5ff631653d31428e40cd10c618.
---
 drivers/gpu/drm/scheduler/sched_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 682aebe96db7..550492a7a031 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1029,9 +1029,8 @@ EXPORT_SYMBOL(drm_sched_job_cleanup);
 void drm_sched_wakeup(struct drm_gpu_scheduler *sched,
 		      struct drm_sched_entity *entity)
 {
-	if (drm_sched_entity_is_ready(entity))
-		if (drm_sched_can_queue(sched, entity))
-			drm_sched_run_job_queue(sched);
+	if (drm_sched_can_queue(sched, entity))
+		drm_sched_run_job_queue(sched);
 }

 /**
--
2.43.0

Re: [PATCH] drm/sched: Don't disturb the entity when in RR-mode scheduling

[Luben Tuikov]

[-- Attachment #1.1.1: Type: text/plain, Size: 5011 bytes --]

On 2023-11-09 18:41, Danilo Krummrich wrote:
> On 11/9/23 20:24, Danilo Krummrich wrote:
>> On 11/9/23 07:52, Luben Tuikov wrote:
>>> Hi,
>>>
>>> On 2023-11-07 19:41, Danilo Krummrich wrote:
>>>> On 11/7/23 05:10, Luben Tuikov wrote:
>>>>> Don't call drm_sched_select_entity() in drm_sched_run_job_queue().  In fact,
>>>>> rename __drm_sched_run_job_queue() to just drm_sched_run_job_queue(), and let
>>>>> it do just that, schedule the work item for execution.
>>>>>
>>>>> The problem is that drm_sched_run_job_queue() calls drm_sched_select_entity()
>>>>> to determine if the scheduler has an entity ready in one of its run-queues,
>>>>> and in the case of the Round-Robin (RR) scheduling, the function
>>>>> drm_sched_rq_select_entity_rr() does just that, selects the _next_ entity
>>>>> which is ready, sets up the run-queue and completion and returns that
>>>>> entity. The FIFO scheduling algorithm is unaffected.
>>>>>
>>>>> Now, since drm_sched_run_job_work() also calls drm_sched_select_entity(), then
>>>>> in the case of RR scheduling, that would result in drm_sched_select_entity()
>>>>> having been called twice, which may result in skipping a ready entity if more
>>>>> than one entity is ready. This commit fixes this by eliminating the call to
>>>>> drm_sched_select_entity() from drm_sched_run_job_queue(), and leaves it only
>>>>> in drm_sched_run_job_work().
>>>>>
>>>>> v2: Rebased on top of Tvrtko's renames series of patches. (Luben)
>>>>>       Add fixes-tag. (Tvrtko)
>>>>>
>>>>> Signed-off-by: Luben Tuikov <ltuikov89@gmail.com>
>>>>> Fixes: f7fe64ad0f22ff ("drm/sched: Split free_job into own work item")
>>>>> ---
>>>>>    drivers/gpu/drm/scheduler/sched_main.c | 16 +++-------------
>>>>>    1 file changed, 3 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> index 27843e37d9b769..cd0dc3f81d05f0 100644
>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>> @@ -256,10 +256,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>>>>    }
>>>>>    /**
>>>>> - * __drm_sched_run_job_queue - enqueue run-job work
>>>>> + * drm_sched_run_job_queue - enqueue run-job work
>>>>>     * @sched: scheduler instance
>>>>>     */
>>>>> -static void __drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
>>>>> +static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
>>>>>    {
>>>>>        if (!READ_ONCE(sched->pause_submit))
>>>>>            queue_work(sched->submit_wq, &sched->work_run_job);
>>>>> @@ -928,7 +928,7 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched)
>>>>>    void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
>>>>>    {
>>>>>        if (drm_sched_can_queue(sched))
>>>>> -        __drm_sched_run_job_queue(sched);
>>>>> +        drm_sched_run_job_queue(sched);
>>>>>    }
>>>>>    /**
>>>>> @@ -1040,16 +1040,6 @@ drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>>>>>    }
>>>>>    EXPORT_SYMBOL(drm_sched_pick_best);
>>>>> -/**
>>>>> - * drm_sched_run_job_queue - enqueue run-job work if there are ready entities
>>>>> - * @sched: scheduler instance
>>>>> - */
>>>>> -static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
>>>>> -{
>>>>> -    if (drm_sched_select_entity(sched))
>>>>
>>>> Hm, now that I rebase my patch to implement dynamic job-flow control I recognize that
>>>> we probably need the peek semantics here. If we do not select an entity here, we also
>>>> do not check whether the corresponding job fits on the ring.
>>>>
>>>> Alternatively, we simply can't do this check in drm_sched_wakeup(). The consequence would
>>>> be that we don't detect that we need to wait for credits to free up before the run work is
>>>> already executing and the run work selects an entity.
>>>
>>> So I rebased v5 on top of the latest drm-misc-next, and looked around and found out that
>>> drm_sched_wakeup() is missing drm_sched_entity_is_ready(). It should look like the following,
>>
>> Yeah, but that's just the consequence of re-basing it onto Tvrtko's patch.
>>
>> My point is that by removing drm_sched_select_entity() from drm_sched_run_job_queue() we do not
>> only loose the check whether the selected entity is ready, but also whether we have enough
>> credits to actually run a new job. This can lead to queuing up work that does nothing but calling
>> drm_sched_select_entity() and return.
> 
> Ok, I see it now.  We don't need to peek, we know the entity at drm_sched_wakeup().
> 
> However, the missing drm_sched_entity_is_ready() check should have been added already when
> drm_sched_select_entity() was removed. Gonna send a fix for that as well.

Let me do that, since I added it to your patch.
Then you can rebase your credits patch onto mine.

-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[PATCH] Revert "drm/sched: Qualify drm_sched_wakeup() by drm_sched_entity_is_ready()"

[Bert Karwatzki]

Commit f3123c25 (in combination with the use of work queues by the gpu
scheduler) leads to random lock ups of the GUI [1,2].

This is not a complete revert of commit f3123c25 as drm_sched_wakeup
still needs its entity argument to pass it to drm_sched_can_queue.

[1] https://gitlab.freedesktop.org/drm/amd/-/issues/2994
[2] https://lists.freedesktop.org/archives/dri-devel/2023-November/431606.html

This reverts commit f3123c2590005c5ff631653d31428e40cd10c618.
---
 drivers/gpu/drm/scheduler/sched_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 682aebe96db7..550492a7a031 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1029,9 +1029,8 @@ EXPORT_SYMBOL(drm_sched_job_cleanup);
 void drm_sched_wakeup(struct drm_gpu_scheduler *sched,
 		      struct drm_sched_entity *entity)
 {
-	if (drm_sched_entity_is_ready(entity))
-		if (drm_sched_can_queue(sched, entity))
-			drm_sched_run_job_queue(sched);
+	if (drm_sched_can_queue(sched, entity))
+		drm_sched_run_job_queue(sched);
 }

 /**
--
2.43.0

Re: [PATCH] Revert "drm/sched: Qualify drm_sched_wakeup() by drm_sched_entity_is_ready()"

[Luben Tuikov]

[-- Attachment #1.1.1: Type: text/plain, Size: 1945 bytes --]

Hi Bert,

# The title of the patch should be:

drm/sched: Partial revert of "Qualify drm_sched_wakeup() by drm_sched_entity_is_ready()"

On 2023-11-27 08:30, Bert Karwatzki wrote:
> Commit f3123c25 (in combination with the use of work queues by the gpu

Commit f3123c2590005c, in combination with the use of work queues by the GPU
scheduler, leads to random lock-ups of the GUI.

> scheduler) leads to random lock ups of the GUI [1,2].
> 
> This is not a complete revert of commit f3123c25 as drm_sched_wakeup

This is a partial revert of of commit f3123c2590005c since drm_sched_wakeup()

> still needs its entity argument to pass it to drm_sched_can_queue.

... drm_sched_can_queue().

# Don't forget a SoB line!

Signed-off-by: Bert ...

>> [1] https://gitlab.freedesktop.org/drm/amd/-/issues/2994

# Use a Link: tag instead, like this:
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2994

> [2] https://lists.freedesktop.org/archives/dri-devel/2023-November/431606.html

# Use a Link: tag instead, like this:
Link: https://lists.freedesktop.org/archives/dri-devel/2023-November/431606.html

> 
> This reverts commit f3123c2590005c5ff631653d31428e40cd10c618.

# The line above is *not* necessary, since this is a partial commit. Instead we need
# a Fixes: line, like this:

Fixes: f3123c2590005c ("drm/sched: Qualify drm_sched_wakeup() by drm_sched_entity_is_ready()")

#######---

Then after you do "git format-patch", post it like this:

git send-email \
    --in-reply-to=c5292d06-2e37-4715-96dc-699f369111fa@gmail.com \
    --to=ltuikov89@gmail.com \
    --cc=christian.koenig@amd.com \
    --cc=dakr@redhat.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=matthew.brost@intel.com \
    --cc=spasswolf@web.de \
    --cc=tvrtko.ursulin@intel.com \
    /path/to/PATCH

This follows your thread where all the information is stored.

Thanks!
-- 
Regards,
Luben

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 677 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]