From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5FBCFC47404 for ; Mon, 7 Oct 2019 12:47:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3C33320867 for ; Mon, 7 Oct 2019 12:47:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727830AbfJGMrf (ORCPT ); Mon, 7 Oct 2019 08:47:35 -0400 Received: from foss.arm.com ([217.140.110.172]:33704 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727536AbfJGMrf (ORCPT ); Mon, 7 Oct 2019 08:47:35 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 268591570; Mon, 7 Oct 2019 05:47:34 -0700 (PDT) Received: from [10.1.196.133] (e112269-lin.cambridge.arm.com [10.1.196.133]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 051253F706; Mon, 7 Oct 2019 05:47:32 -0700 (PDT) Subject: Re: drm_sched with panfrost crash on T820 To: "Koenig, Christian" Cc: Hillf Danton , Tomeu Vizoso , Neil Armstrong , "airlied@linux.ie" , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "open list:ARM/Amlogic Meson..." , Erico Nunes References: <8e003dfd-2761-4941-8b5b-ecc186222229@email.android.com> From: Steven Price Message-ID: Date: Mon, 7 Oct 2019 13:47:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <8e003dfd-2761-4941-8b5b-ecc186222229@email.android.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/10/2019 17:33, Koenig, Christian wrote: > > > Am 04.10.2019 18:02 schrieb Steven Price : > 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