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=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 68AD7C4338F for ; Fri, 13 Aug 2021 16:00:50 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9206060EE0 for ; Fri, 13 Aug 2021 16:00:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 9206060EE0 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=daenzer.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 60C7E6E879; Fri, 13 Aug 2021 16:00:48 +0000 (UTC) Received: from netline-mail3.netline.ch (mail.netline.ch [148.251.143.180]) by gabe.freedesktop.org (Postfix) with ESMTP id 2E03C6E872; Fri, 13 Aug 2021 16:00:47 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by netline-mail3.netline.ch (Postfix) with ESMTP id 604BC20201B; Fri, 13 Aug 2021 18:00:46 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at netline-mail3.netline.ch Received: from netline-mail3.netline.ch ([127.0.0.1]) by localhost (netline-mail3.netline.ch [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 20kUUH7leddE; Fri, 13 Aug 2021 18:00:46 +0200 (CEST) Received: from thor (24.99.2.85.dynamic.wline.res.cust.swisscom.ch [85.2.99.24]) by netline-mail3.netline.ch (Postfix) with ESMTPA id 054A920201A; Fri, 13 Aug 2021 18:00:46 +0200 (CEST) Received: from localhost ([::1]) by thor with esmtp (Exim 4.94.2) (envelope-from ) id 1mEZbx-000E81-46; Fri, 13 Aug 2021 18:00:45 +0200 To: "Lazar, Lijo" , Alex Deucher , =?UTF-8?Q?Christian_K=c3=b6nig?= Cc: Leo Liu , James Zhu , amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org References: <20210811165211.6811-1-michel@daenzer.net> <20210813102920.3458-1-michel@daenzer.net> <9ec17598-0b51-014c-c633-2e4e74c863e9@amd.com> <31c9a190-8329-383e-bbea-3520add4d16a@daenzer.net> <3ab84437-d4f6-c979-ef7f-d29dcf35eedb@amd.com> From: =?UTF-8?Q?Michel_D=c3=a4nzer?= Subject: Re: [PATCH] drm/amdgpu: Cancel delayed work when GFXOFF is disabled Message-ID: Date: Fri, 13 Aug 2021 18:00:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <3ab84437-d4f6-c979-ef7f-d29dcf35eedb@amd.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-CA Content-Transfer-Encoding: 8bit X-BeenThere: amd-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces@lists.freedesktop.org Sender: "amd-gfx" On 2021-08-13 5:07 p.m., Lazar, Lijo wrote: > > > On 8/13/2021 8:10 PM, Michel Dänzer wrote: >> On 2021-08-13 4:14 p.m., Lazar, Lijo wrote: >>> On 8/13/2021 7:04 PM, Michel Dänzer wrote: >>>> On 2021-08-13 1:50 p.m., Lazar, Lijo wrote: >>>>> On 8/13/2021 3:59 PM, Michel Dänzer wrote: >>>>>> From: Michel Dänzer >>>>>> >>>>>> schedule_delayed_work does not push back the work if it was already >>>>>> scheduled before, so amdgpu_device_delay_enable_gfx_off ran ~100 ms >>>>>> after the first time GFXOFF was disabled and re-enabled, even if GFXOFF >>>>>> was disabled and re-enabled again during those 100 ms. >>>>>> >>>>>> This resulted in frame drops / stutter with the upcoming mutter 41 >>>>>> release on Navi 14, due to constantly enabling GFXOFF in the HW and >>>>>> disabling it again (for getting the GPU clock counter). >>>>>> >>>>>> To fix this, call cancel_delayed_work_sync when GFXOFF transitions from >>>>>> enabled to disabled. This makes sure the delayed work will be scheduled >>>>>> as intended in the reverse case. >>>>>> >>>>>> In order to avoid a deadlock, amdgpu_device_delay_enable_gfx_off needs >>>>>> to use mutex_trylock instead of mutex_lock. >>>>>> >>>>>> v2: >>>>>> * Use cancel_delayed_work_sync & mutex_trylock instead of >>>>>>      mod_delayed_work. >>>>>> >>>>>> Signed-off-by: Michel Dänzer >>>>>> --- >>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 11 ++++++++++- >>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c    | 13 +++++++------ >>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h    |  3 +++ >>>>>>     3 files changed, 20 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>>> index f3fd5ec710b6..8b025f70706c 100644 >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>>> @@ -2777,7 +2777,16 @@ static void amdgpu_device_delay_enable_gfx_off(struct work_struct *work) >>>>>>         struct amdgpu_device *adev = >>>>>>             container_of(work, struct amdgpu_device, gfx.gfx_off_delay_work.work); >>>>>>     -    mutex_lock(&adev->gfx.gfx_off_mutex); >>>>>> +    /* mutex_lock could deadlock with cancel_delayed_work_sync in amdgpu_gfx_off_ctrl. */ >>>>>> +    if (!mutex_trylock(&adev->gfx.gfx_off_mutex)) { >>>>>> +        /* If there's a bug which causes amdgpu_gfx_off_ctrl to be called with enable=true >>>>>> +         * when adev->gfx.gfx_off_req_count is already 0, we might race with that. >>>>>> +         * Re-schedule to make sure gfx off will be re-enabled in the HW eventually. >>>>>> +         */ >>>>>> +        schedule_delayed_work(&adev->gfx.gfx_off_delay_work, AMDGPU_GFX_OFF_DELAY_ENABLE); >>>>>> +        return; >>>>> >>>>> This is not needed and is just creating another thread to contend for mutex. >>>> >>>> Still not sure what you mean by that. What other thread? >>> >>> Sorry, I meant it schedules another workitem and delays GFXOFF enablement further. For ex: if it was another function like gfx_off_status holding the lock at the time of check. >>> >>>> >>>>> The checks below take care of enabling gfxoff correctly. If it's already in gfx_off state, it doesn't do anything. So I don't see why this change is needed. >>>> >>>> mutex_trylock is needed to prevent the deadlock discussed before and below. >>>> >>>> schedule_delayed_work is needed due to this scenario hinted at by the comment: >>>> >>>> 1. amdgpu_gfx_off_ctrl locks mutex, calls schedule_delayed_work >>>> 2. amdgpu_device_delay_enable_gfx_off runs, calls mutex_trylock, which fails >>>> >>>> GFXOFF would never get re-enabled in HW in this case (until amdgpu_gfx_off_ctrl calls schedule_delayed_work again). >>>> >>>> (cancel_delayed_work_sync guarantees there's no pending delayed work when it returns, even if amdgpu_device_delay_enable_gfx_off calls schedule_delayed_work) >>>> >>> >>> I think we need to explain based on the original code before. There is an asssumption here that the only other contention of this mutex is with the gfx_off_ctrl function. >> >> Not really. >> >> >>> As far as I understand if the work has already started running when schedule_delayed_work is called, it will insert another in the work queue after delay. Based on that understanding I didn't find a problem with the original code. >> >> Original code as in without this patch or the mod_delayed_work patch? If so, the problem is not when the work has already started running. It's that when it hasn't started running yet, schedule_delayed_work doesn't change the timeout for the already scheduled work, so it ends up enabling GFXOFF earlier than intended (and thus at all in scenarios when it's not supposed to). >> > > I meant the original implementation of amdgpu_device_delay_enable_gfx_off(). > > > If you indeed want to use _sync, there is a small problem with this implementation also which is roughly equivalent to the original problem you faced. > > amdgpu_gfx_off_ctrl(disable) locks mutex > calls cancel_delayed_work_sync > amdgpu_device_delay_enable_gfx_off already started running >     mutex_trylock fails and schedules another one > amdgpu_gfx_off_ctrl(enable) >     schedules_delayed_work() - Delay is not extended, it's the same as when it's rearmed from work item. This cannot happen. When cancel_delayed_work_sync returns, it guarantees that the delayed work is not scheduled , even if amdgpu_device_delay_enable_gfx_off called schedule_delayed_work. In other words, it cancels that as well. > Probably, overthinking about the solution. Looking back, mod_ version is simpler :). May be just delay it further everytime there is a call with enable instead of doing it only for req_cnt==0? That has some issues as well: * Still prone to the "amdgpu_device_delay_enable_gfx_off re-enables GFXOFF immediately after amdgpu_gfx_off_ctrl dropped req_count to 0" race if the former starts running between when the latter locks the mutex and calls mod_delayed_work. * If the work is not scheduled yet, mod_delayed_work would schedule it, even if req_count > 0, in which case it couldn't actually enable GFXOFF. Conceptually, making sure the work is never scheduled while req_count > 0 seems cleaner to me. It's the same principle as in the JPEG/UVD/VCE/VCN ring functions (which are presumably hotter paths than these amdgpu_gfx_off functions) I needlessly modified in patch 2. (It also means amdgpu_device_delay_enable_gfx_off technically no longer needs to test req_count or gfx_off_state; I can spin a v3 for that if desired) -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer