Re: drm_sched with panfrost crash on T820

[Steven Price]

On 04/10/2019 17:33, Koenig, Christian wrote:
> 
> 
> Am 04.10.2019 18:02 schrieb Steven Price <steven.price@xxxxxxx>:
> On 04/10/2019 16:34, Koenig, Christian wrote:
>> Am 04.10.19 um 17:27 schrieb Steven Price:
>>> On 04/10/2019 16:03, Neil Armstrong wrote:
>>>> On 04/10/2019 16:53, Grodzovsky, Andrey wrote:
>>>>> On 10/3/19 4:34 AM, Neil Armstrong wrote:
>>>>>> Hi Andrey,
>>>>>>
>>>>>> Le 02/10/2019 Ã 16:40, Grodzovsky, Andrey a Ãcrit :
>>>>>>> On 9/30/19 10:52 AM, Hillf Danton wrote:
>>>>>>>> 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
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>>>> @@ -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) >
>>>>>>>> @@ -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.
>>>>>>
>>>>>> 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 ?
>>>> 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.
>>
>> 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