drm_sched with panfrost crash on T820

DISCUSSION Koenig, Christian 2 participants 2 messages Oct 4, 2019 – Oct 7, 2019 Arm/Amlogic Meson SoC support

lore

Share & tools

lore

Koenig, Christian

author

2019-10-04 16:34 UTC #0 lore raw

Am 04.10.2019 18:02 schrieb Steven Price <[email protected]>:

Quoted 65 lines · Christian

Am 04.10.19 um 17:27 schrieb Steven Price:
Quoted 64 lines · Neil Armstrong
Quoted 49 lines · Andrey
Quoted 43 lines · Neil Armstrong
Quoted 42 lines
Hi Andrey,
Quoted 29 lines · Andrey
Quoted 28 lines · Hillf Danton
Quoted 7 lines · On Mon, 30 Sep 2019 11:17:45 +0200 Neil Armstrong wrote:
Did a new run from 5.3:
[   35.971972] Call trace:
[   35.974391]  drm_sched_increase_karma+0x5c/0xf0
                        ffff000010667f38        FFFF000010667F94
                        drivers/gpu/drm/scheduler/sched_main.c:335
Quoted 5 lines
The crashing line is :
                                   if (bad->s_fence->scheduled.context ==
                                       entity->fence_context) {
Doesn't seem related to guilty job.
Bail out if s_fence is no longer fresh.
drivers/gpu/drm/scheduler/sched_main.c +4 -0
@@ -333,6 +333,10 @@ void drm_sched_increase_karma(struct drm
 
                         spin_lock(&rq->lock);
                         list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
+                               if (!smp_load_acquire(&bad->s_fence)) {
+                                       spin_unlock(&rq->lock);
+                                       return;
+                               }
                                 if (bad->s_fence->scheduled.context ==
                                     entity->fence_context) {
                                         if (atomic_read(&bad->karma) >
Quoted 8 lines
drivers/gpu/drm/scheduler/sched_main.c EXPORT_SYMBOL(drm_sched_job_init); · line 552
Quoted 8 lines
drivers/gpu/drm/scheduler/sched_main.c +1 -1
@@ -543,7 +547,7 @@ EXPORT_SYMBOL(drm_sched_job_init);
    void drm_sched_job_cleanup(struct drm_sched_job *job)
    {
         dma_fence_put(&job->s_fence->finished);
-       job->s_fence = NULL;
+       smp_store_release(&job->s_fence, 0);
    }
    EXPORT_SYMBOL(drm_sched_job_cleanup);
This fixed the problem on the 10 CI runs.
Quoted 5 lines
Quoted 1 line
Neil
These are good news but I still fail to see how this fixes the problem - Hillf, do you mind explaining how you came up with this particular fix - what was the bug you saw ?
Quoted 8 lines
As Steven explained, seems the same job was submitted on both HW slots, and then when timeout occurs each thread calls panfrost_job_timedout which leads to drm_sched_stop() on the first call and on the second call the job was already freed.
Steven proposed a working fix, and this one does the same but on the drm_sched side. This one looks cleaner, but panfrost should not call drm_sched_stop() twice for the same job.
I'm not sure that Hillf's fix is sufficient. In particular in drm_sched_increase_karma() I don't understand how the smp_load_acquire() call prevents bad->s_fence becoming NULL immediately afterwards (but admittedly the window is much smaller). But really this is just a Panfrost bug (calling drm_sched_stop() twice on the same job).

Quoted 7 lines

Quoted 5 lines
The part of my change that I'd welcome feedback on is changing cancel_delayed_work() to cancel_delayed_work_sync() in drm_sched_stop() when called on different scheduler to the bad job. It's not clear to me exactly what the semantics of the function should be, and I haven't tested the effect of the change on drivers other than Panfrost.
Yeah, at least of hand that change doesn't seem to make sense to me.

We need to ensure that any other timeouts that might have started processing are complete before actually resetting the hardware. Otherwise after the reset another thread could come along and attempt to reset the hardware again (and cause a double free of a job).

This is intentional behaviour. If you don't want the double reset in Panfrost you should probably call the cancel_sync yourself.

Regards,
Christian.

My change
to use the _sync() variant prevents this happening.

Quoted 7 lines

Multiple timeout workers can perfectly run in parallel, Panfrost needs to make sure that they don't affect each other.
The simplest way of doing this is to have a mutex you trylock when starting the reset.
The first one grabbing it wins, all other just silently return.

Ah that would simplify my change removing the need for the new variable. I hadn't thought to use trylock. I'll give that a spin and post a new simpler version.

Thanks,

Steve

Quoted 29 lines

Regards,
Christian.
Quoted 25 lines
Steve
Quoted 22 lines
Neil
Quoted 15 lines
Andrey
Quoted 13 lines
Quoted 13 lines
Does this change help the problem ? Note that drm_sched_job_cleanup is called from scheduler thread which is stopped at all times when work_tdr thread is running and anyway the 'bad' job is still in the ring_mirror_list while it's being accessed from drm_sched_increase_karma so I don't think drm_sched_job_cleanup can be called for it BEFORE or while drm_sched_increase_karma is executed.
Andrey
Quoted 3 lines
drivers/gpu/drm/scheduler/sched_main.c +0 -1
--
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 5 lines

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Steven Price

2019-10-07 12:47 UTC #1 lore raw

Quoted 75 lines · Christian

Am 04.10.2019 18:02 schrieb Steven Price <[email protected]>:
Quoted 71 lines · Christian
Am 04.10.19 um 17:27 schrieb Steven Price:
Quoted 69 lines · Neil Armstrong
Quoted 49 lines · Andrey
Quoted 43 lines · Neil Armstrong
Quoted 42 lines
Hi Andrey,
Quoted 29 lines · Andrey
Quoted 28 lines · Hillf Danton
Quoted 7 lines · On Mon, 30 Sep 2019 11:17:45 +0200 Neil Armstrong wrote:
Did a new run from 5.3:
[   35.971972] Call trace:
[   35.974391]  drm_sched_increase_karma+0x5c/0xf0
                        ffff000010667f38        FFFF000010667F94
                        drivers/gpu/drm/scheduler/sched_main.c:335
Quoted 5 lines
The crashing line is :
                                   if (bad->s_fence->scheduled.context ==
                                       entity->fence_context) {
Doesn't seem related to guilty job.
Bail out if s_fence is no longer fresh.
drivers/gpu/drm/scheduler/sched_main.c +4 -0
@@ -333,6 +333,10 @@ void drm_sched_increase_karma(struct drm
 
                         spin_lock(&rq->lock);
                         list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
+                               if (!smp_load_acquire(&bad->s_fence)) {
+                                       spin_unlock(&rq->lock);
+                                       return;
+                               }
                                 if (bad->s_fence->scheduled.context ==
                                     entity->fence_context) {
                                         if (atomic_read(&bad->karma) >
Quoted 8 lines
drivers/gpu/drm/scheduler/sched_main.c EXPORT_SYMBOL(drm_sched_job_init); · line 552
Quoted 8 lines
drivers/gpu/drm/scheduler/sched_main.c +1 -1
@@ -543,7 +547,7 @@ EXPORT_SYMBOL(drm_sched_job_init);
    void drm_sched_job_cleanup(struct drm_sched_job *job)
    {
         dma_fence_put(&job->s_fence->finished);
-       job->s_fence = NULL;
+       smp_store_release(&job->s_fence, 0);
    }
    EXPORT_SYMBOL(drm_sched_job_cleanup);
This fixed the problem on the 10 CI runs.
Quoted 5 lines
Quoted 1 line
Neil
These are good news but I still fail to see how this fixes the problem - Hillf, do you mind explaining how you came up with this particular fix - what was the bug you saw ?
Quoted 8 lines
As Steven explained, seems the same job was submitted on both HW slots, and then when timeout occurs each thread calls panfrost_job_timedout which leads to drm_sched_stop() on the first call and on the second call the job was already freed.
Steven proposed a working fix, and this one does the same but on the drm_sched side. This one looks cleaner, but panfrost should not call drm_sched_stop() twice for the same job.
I'm not sure that Hillf's fix is sufficient. In particular in drm_sched_increase_karma() I don't understand how the smp_load_acquire() call prevents bad->s_fence becoming NULL immediately afterwards (but admittedly the window is much smaller). But really this is just a Panfrost bug (calling drm_sched_stop() twice on the same job).
The part of my change that I'd welcome feedback on is changing cancel_delayed_work() to cancel_delayed_work_sync() in drm_sched_stop() when called on different scheduler to the bad job. It's not clear to me exactly what the semantics of the function should be, and I haven't tested the effect of the change on drivers other than Panfrost.

Quoted 8 lines

Quoted 1 line
Yeah, at least of hand that change doesn't seem to make sense to me.
We need to ensure that any other timeouts that might have started processing are complete before actually resetting the hardware. Otherwise after the reset another thread could come along and attempt to reset the hardware again (and cause a double free of a job).
This is intentional behaviour. If you don't want the double reset in Panfrost you should probably call the cancel_sync yourself.

It's less the double reset that is the problem, more that the job gets cleaned up twice: drm_sched_stop() will either free the job or mark it to be freed later. By having two threads both drm_sched_stop()ing all slots you end up with the guilty job(s) potentially being double freed.

I've move the call to cancel_delayed_work_sync() into Panfrost since I'm not sure whether this is generically useful to other drivers.

Steve