Message ID | 20230711213501.526237-4-andrealmeid@igalia.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a6b2:0:b0:3e4:2afc:c1 with SMTP id c18csp763305vqm; Tue, 11 Jul 2023 14:59:33 -0700 (PDT) X-Google-Smtp-Source: APBJJlG36YhjGoQE1TnScMhNL9KFHZsQ6ScSsnp7c2qVkhWaKR0kkrENuaDyHnczdnW3w/IjXxwF X-Received: by 2002:a17:902:ecce:b0:1b9:ebe9:5f01 with SMTP id a14-20020a170902ecce00b001b9ebe95f01mr5413622plh.19.1689112772800; Tue, 11 Jul 2023 14:59:32 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689112772; cv=none; d=google.com; s=arc-20160816; b=I85kWTsQOdSp0fKa7mUrXaglQScSe/7oG8Qa+qvmFG01ZzEaL89yG3o4a6R+/4DheC ZK/XdAXupj7jg6Xm8v7QA5qv3/5tjFeEH+G/ZaR1qPWHvdg9USv3Hyu2w0WHaK+f7InX hlUBKULddA34eSNE/whGQCUFaFLM1+kcxSshbsfEKH/tisgf1809qBTlqMvx9eDgFxyB N+8MvPjC3xnrD3BnE6QiFvHNKA18kVx+0/nUHnS0J/utvFOMlRTBFs23t7NU5FlijtwO I9jW/JCDQQXXAPqUYRPfTl/JZVWXmFPQE3pZr4ZuAAfQW4fpXWcD+lKzAlgIYfBwPOCh /+Lg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=KTRbZwUZgZnlGc0WMOlsXvvWVPDAjN3SirsI8t8QzuM=; fh=aYwOFvh45AiUk04AC35/CxFbCKpYy0XHNT+Ky7LGVhk=; b=V0PjeEv9cYZaap4v5p0lIxN468NL1CWBSKf+QmCiVsJqKgB8TY0bDrsW1DVChW7k5i xR6PKuQUQI+8nP98Z2n2wPDopD3ZGaZ2DC9uiWr+RJiN+SibxewDBNB4pl3G2pAwlJPl q6cVgOkeMN5vmXINtvmUO6nX6PKdhNjbgo6Nu12+nVGHSz13Vrnxpkk6QZ+ODy4nbeBg Gx3Y9XPbfg7NMhEStdHWKrJiM2lML/G1/7XSxFYgwOm2sZ4BLTA5UxxKLdd19CVxvZjL eHk8KrfTnjGustk6djPQ/m0PjVqv9tk2SHYZZfbi8Ej36zX3wqe9FHHijPpAU39QVab0 hAVg== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@igalia.com header.s=20170329 header.b=LFtKd7KQ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id kv4-20020a17090328c400b001b891de90dbsi2102562plb.72.2023.07.11.14.59.19; Tue, 11 Jul 2023 14:59:32 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@igalia.com header.s=20170329 header.b=LFtKd7KQ; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230446AbjGKVf6 (ORCPT <rfc822;gnulinuxfreebsd@gmail.com> + 99 others); Tue, 11 Jul 2023 17:35:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38838 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231533AbjGKVfx (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 11 Jul 2023 17:35:53 -0400 Received: from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DBDF5E69 for <linux-kernel@vger.kernel.org>; Tue, 11 Jul 2023 14:35:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:MIME-Version:References: In-Reply-To:Message-ID:Date:Subject:Cc:To:From:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=KTRbZwUZgZnlGc0WMOlsXvvWVPDAjN3SirsI8t8QzuM=; b=LFtKd7KQNb4Fif9xGzTjkrwSAi lfL8gI2EaPuNVIs6NFCmsMRqH0V/NuNdlvlUF4hdZfU7CuC6zIvOKXh3M5DQwyHgKiBTsijeK0nUb yzpxrSB7wWWOtsB1O0lu2mkIZyX6Oy5LbBXb5soTTi6MOshDZrv9QJPiahvi03mThXHRF998PwxFc Sy0ES6jvMweiKXBx2ALG9adsHdvTH1KoJ9DDIzKxSDZa/MUBWtWo+YTz+GLOJxqXnIZB6W9D0lvjs mDKvG1Eat2DrOcZwBFH+rOLKIHMXsD4JWBDCMF2gqeOIC3SY/PKO5g0UAE783NOssKaJAXpoWawTQ OJZsyAdA==; Received: from [187.74.70.209] (helo=steammachine.lan) by fanzine2.igalia.com with esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim) id 1qJL1R-00Cl0M-IK; Tue, 11 Jul 2023 23:35:49 +0200 From: =?utf-8?q?Andr=C3=A9_Almeida?= <andrealmeid@igalia.com> To: dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: kernel-dev@igalia.com, alexander.deucher@amd.com, christian.koenig@amd.com, pierre-eric.pelloux-prayer@amd.com, =?utf-8?b?J01hcmVrIE9sxaHDoWsn?= <maraeo@gmail.com>, Samuel Pitoiset <samuel.pitoiset@gmail.com>, Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>, =?utf-8?q?Timur_Krist=C3=B3f?= <timur.kristof@gmail.com>, michel.daenzer@mailbox.org, =?utf-8?q?Andr=C3=A9_Almeida?= <andrealmeid@igalia.com> Subject: [PATCH 3/6] drm/amdgpu: Rework coredump to use memory dynamically Date: Tue, 11 Jul 2023 18:34:58 -0300 Message-ID: <20230711213501.526237-4-andrealmeid@igalia.com> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20230711213501.526237-1-andrealmeid@igalia.com> References: <20230711213501.526237-1-andrealmeid@igalia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771163115085129723 X-GMAIL-MSGID: 1771163115085129723 |
Series |
drm/amdgpu: Add new reset option and rework coredump
|
|
Commit Message
André Almeida
July 11, 2023, 9:34 p.m. UTC
Instead of storing coredump information inside amdgpu_device struct,
move if to a proper separated struct and allocate it dynamically. This
will make it easier to further expand the logged information.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 14 +++--
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 ++++++++++++++--------
2 files changed, 51 insertions(+), 28 deletions(-)
Comments
Am 11.07.23 um 23:34 schrieb André Almeida: > Instead of storing coredump information inside amdgpu_device struct, > move if to a proper separated struct and allocate it dynamically. This > will make it easier to further expand the logged information. Verry big NAK to this. The problem is that memory allocation isn't allowed during a GPU reset. What you could do is to allocate the memory with GFP_ATOMIC or similar, but for a large structure that might not be possible. Regards, Christian. > > Signed-off-by: André Almeida <andrealmeid@igalia.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 14 +++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 ++++++++++++++-------- > 2 files changed, 51 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index dbe062a087c5..e1cc83a89d46 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -1068,11 +1068,6 @@ struct amdgpu_device { > uint32_t *reset_dump_reg_list; > uint32_t *reset_dump_reg_value; > int num_regs; > -#ifdef CONFIG_DEV_COREDUMP > - struct amdgpu_task_info reset_task_info; > - bool reset_vram_lost; > - struct timespec64 reset_time; > -#endif > > bool scpm_enabled; > uint32_t scpm_status; > @@ -1085,6 +1080,15 @@ struct amdgpu_device { > uint32_t aid_mask; > }; > > +#ifdef CONFIG_DEV_COREDUMP > +struct amdgpu_coredump_info { > + struct amdgpu_device *adev; > + struct amdgpu_task_info reset_task_info; > + struct timespec64 reset_time; > + bool reset_vram_lost; > +}; > +#endif > + > static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) > { > return container_of(ddev, struct amdgpu_device, ddev); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index e25f085ee886..23b9784e9787 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -4963,12 +4963,17 @@ static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev) > return 0; > } > > -#ifdef CONFIG_DEV_COREDUMP > +#ifndef CONFIG_DEV_COREDUMP > +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > + struct amdgpu_reset_context *reset_context) > +{ > +} > +#else > static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > size_t count, void *data, size_t datalen) > { > struct drm_printer p; > - struct amdgpu_device *adev = data; > + struct amdgpu_coredump_info *coredump = data; > struct drm_print_iterator iter; > int i; > > @@ -4982,21 +4987,21 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > drm_printf(&p, "**** AMDGPU Device Coredump ****\n"); > drm_printf(&p, "kernel: " UTS_RELEASE "\n"); > drm_printf(&p, "module: " KBUILD_MODNAME "\n"); > - drm_printf(&p, "time: %lld.%09ld\n", adev->reset_time.tv_sec, adev->reset_time.tv_nsec); > - if (adev->reset_task_info.pid) > + drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec, coredump->reset_time.tv_nsec); > + if (coredump->reset_task_info.pid) > drm_printf(&p, "process_name: %s PID: %d\n", > - adev->reset_task_info.process_name, > - adev->reset_task_info.pid); > + coredump->reset_task_info.process_name, > + coredump->reset_task_info.pid); > > - if (adev->reset_vram_lost) > + if (coredump->reset_vram_lost) > drm_printf(&p, "VRAM is lost due to GPU reset!\n"); > - if (adev->num_regs) { > + if (coredump->adev->num_regs) { > drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n"); > > - for (i = 0; i < adev->num_regs; i++) > + for (i = 0; i < coredump->adev->num_regs; i++) > drm_printf(&p, "0x%08x: 0x%08x\n", > - adev->reset_dump_reg_list[i], > - adev->reset_dump_reg_value[i]); > + coredump->adev->reset_dump_reg_list[i], > + coredump->adev->reset_dump_reg_value[i]); > } > > return count - iter.remain; > @@ -5004,14 +5009,34 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > static void amdgpu_devcoredump_free(void *data) > { > + kfree(data); > } > > -static void amdgpu_reset_capture_coredumpm(struct amdgpu_device *adev) > +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > + struct amdgpu_reset_context *reset_context) > { > + struct amdgpu_coredump_info *coredump; > struct drm_device *dev = adev_to_drm(adev); > > - ktime_get_ts64(&adev->reset_time); > - dev_coredumpm(dev->dev, THIS_MODULE, adev, 0, GFP_KERNEL, > + coredump = kmalloc(sizeof(*coredump), GFP_KERNEL); > + > + if (!coredump) { > + DRM_ERROR("%s: failed to allocate memory for coredump\n", __func__); > + return; > + } > + > + memset(coredump, 0, sizeof(*coredump)); > + > + coredump->reset_vram_lost = vram_lost; > + > + if (reset_context->job && reset_context->job->vm) > + coredump->reset_task_info = reset_context->job->vm->task_info; > + > + coredump->adev = adev; > + > + ktime_get_ts64(&coredump->reset_time); > + > + dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_KERNEL, > amdgpu_devcoredump_read, amdgpu_devcoredump_free); > } > #endif > @@ -5119,15 +5144,9 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, > goto out; > > vram_lost = amdgpu_device_check_vram_lost(tmp_adev); > -#ifdef CONFIG_DEV_COREDUMP > - tmp_adev->reset_vram_lost = vram_lost; > - memset(&tmp_adev->reset_task_info, 0, > - sizeof(tmp_adev->reset_task_info)); > - if (reset_context->job && reset_context->job->vm) > - tmp_adev->reset_task_info = > - reset_context->job->vm->task_info; > - amdgpu_reset_capture_coredumpm(tmp_adev); > -#endif > + > + amdgpu_coredump(tmp_adev, vram_lost, reset_context); > + > if (vram_lost) { > DRM_INFO("VRAM is lost due to GPU reset!\n"); > amdgpu_inc_vram_lost(tmp_adev);
Am Mittwoch, dem 12.07.2023 um 10:37 +0200 schrieb Christian König: > Am 11.07.23 um 23:34 schrieb André Almeida: > > Instead of storing coredump information inside amdgpu_device struct, > > move if to a proper separated struct and allocate it dynamically. This > > will make it easier to further expand the logged information. > > Verry big NAK to this. The problem is that memory allocation isn't > allowed during a GPU reset. > > What you could do is to allocate the memory with GFP_ATOMIC or similar, > but for a large structure that might not be possible. > I'm still not fully clear on what the rules are here. In etnaviv we do devcoredump allocation in the GPU reset path with __GFP_NOWARN | __GFP_NORETRY, which means the allocation will kick memory reclaim if necessary, but will just give up if no memory could be made available easily. This satisfies the forward progress guarantee in the absence of successful memory allocation, which is the most important property in this path, I think. However, I'm not sure if the reclaim could lead to locking issues or something like that with the more complex use-cases with MMU notifiers and stuff like that. Christian, do you have any experience or information that would be good to share in this regard? Regards, Lucas > Regards, > Christian. > > > > > Signed-off-by: André Almeida <andrealmeid@igalia.com> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 14 +++-- > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 ++++++++++++++-------- > > 2 files changed, 51 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > index dbe062a087c5..e1cc83a89d46 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > @@ -1068,11 +1068,6 @@ struct amdgpu_device { > > uint32_t *reset_dump_reg_list; > > uint32_t *reset_dump_reg_value; > > int num_regs; > > -#ifdef CONFIG_DEV_COREDUMP > > - struct amdgpu_task_info reset_task_info; > > - bool reset_vram_lost; > > - struct timespec64 reset_time; > > -#endif > > > > bool scpm_enabled; > > uint32_t scpm_status; > > @@ -1085,6 +1080,15 @@ struct amdgpu_device { > > uint32_t aid_mask; > > }; > > > > +#ifdef CONFIG_DEV_COREDUMP > > +struct amdgpu_coredump_info { > > + struct amdgpu_device *adev; > > + struct amdgpu_task_info reset_task_info; > > + struct timespec64 reset_time; > > + bool reset_vram_lost; > > +}; > > +#endif > > + > > static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) > > { > > return container_of(ddev, struct amdgpu_device, ddev); > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > index e25f085ee886..23b9784e9787 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > @@ -4963,12 +4963,17 @@ static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev) > > return 0; > > } > > > > -#ifdef CONFIG_DEV_COREDUMP > > +#ifndef CONFIG_DEV_COREDUMP > > +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > > + struct amdgpu_reset_context *reset_context) > > +{ > > +} > > +#else > > static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > size_t count, void *data, size_t datalen) > > { > > struct drm_printer p; > > - struct amdgpu_device *adev = data; > > + struct amdgpu_coredump_info *coredump = data; > > struct drm_print_iterator iter; > > int i; > > > > @@ -4982,21 +4987,21 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > drm_printf(&p, "**** AMDGPU Device Coredump ****\n"); > > drm_printf(&p, "kernel: " UTS_RELEASE "\n"); > > drm_printf(&p, "module: " KBUILD_MODNAME "\n"); > > - drm_printf(&p, "time: %lld.%09ld\n", adev->reset_time.tv_sec, adev->reset_time.tv_nsec); > > - if (adev->reset_task_info.pid) > > + drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec, coredump->reset_time.tv_nsec); > > + if (coredump->reset_task_info.pid) > > drm_printf(&p, "process_name: %s PID: %d\n", > > - adev->reset_task_info.process_name, > > - adev->reset_task_info.pid); > > + coredump->reset_task_info.process_name, > > + coredump->reset_task_info.pid); > > > > - if (adev->reset_vram_lost) > > + if (coredump->reset_vram_lost) > > drm_printf(&p, "VRAM is lost due to GPU reset!\n"); > > - if (adev->num_regs) { > > + if (coredump->adev->num_regs) { > > drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n"); > > > > - for (i = 0; i < adev->num_regs; i++) > > + for (i = 0; i < coredump->adev->num_regs; i++) > > drm_printf(&p, "0x%08x: 0x%08x\n", > > - adev->reset_dump_reg_list[i], > > - adev->reset_dump_reg_value[i]); > > + coredump->adev->reset_dump_reg_list[i], > > + coredump->adev->reset_dump_reg_value[i]); > > } > > > > return count - iter.remain; > > @@ -5004,14 +5009,34 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > > > static void amdgpu_devcoredump_free(void *data) > > { > > + kfree(data); > > } > > > > -static void amdgpu_reset_capture_coredumpm(struct amdgpu_device *adev) > > +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > > + struct amdgpu_reset_context *reset_context) > > { > > + struct amdgpu_coredump_info *coredump; > > struct drm_device *dev = adev_to_drm(adev); > > > > - ktime_get_ts64(&adev->reset_time); > > - dev_coredumpm(dev->dev, THIS_MODULE, adev, 0, GFP_KERNEL, > > + coredump = kmalloc(sizeof(*coredump), GFP_KERNEL); > > + > > + if (!coredump) { > > + DRM_ERROR("%s: failed to allocate memory for coredump\n", __func__); > > + return; > > + } > > + > > + memset(coredump, 0, sizeof(*coredump)); > > + > > + coredump->reset_vram_lost = vram_lost; > > + > > + if (reset_context->job && reset_context->job->vm) > > + coredump->reset_task_info = reset_context->job->vm->task_info; > > + > > + coredump->adev = adev; > > + > > + ktime_get_ts64(&coredump->reset_time); > > + > > + dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_KERNEL, > > amdgpu_devcoredump_read, amdgpu_devcoredump_free); > > } > > #endif > > @@ -5119,15 +5144,9 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, > > goto out; > > > > vram_lost = amdgpu_device_check_vram_lost(tmp_adev); > > -#ifdef CONFIG_DEV_COREDUMP > > - tmp_adev->reset_vram_lost = vram_lost; > > - memset(&tmp_adev->reset_task_info, 0, > > - sizeof(tmp_adev->reset_task_info)); > > - if (reset_context->job && reset_context->job->vm) > > - tmp_adev->reset_task_info = > > - reset_context->job->vm->task_info; > > - amdgpu_reset_capture_coredumpm(tmp_adev); > > -#endif > > + > > + amdgpu_coredump(tmp_adev, vram_lost, reset_context); > > + > > if (vram_lost) { > > DRM_INFO("VRAM is lost due to GPU reset!\n"); > > amdgpu_inc_vram_lost(tmp_adev); >
Am 12.07.23 um 10:59 schrieb Lucas Stach: > Am Mittwoch, dem 12.07.2023 um 10:37 +0200 schrieb Christian König: >> Am 11.07.23 um 23:34 schrieb André Almeida: >>> Instead of storing coredump information inside amdgpu_device struct, >>> move if to a proper separated struct and allocate it dynamically. This >>> will make it easier to further expand the logged information. >> Verry big NAK to this. The problem is that memory allocation isn't >> allowed during a GPU reset. >> >> What you could do is to allocate the memory with GFP_ATOMIC or similar, >> but for a large structure that might not be possible. >> > I'm still not fully clear on what the rules are here. In etnaviv we do > devcoredump allocation in the GPU reset path with __GFP_NOWARN | > __GFP_NORETRY, which means the allocation will kick memory reclaim if > necessary, but will just give up if no memory could be made available > easily. This satisfies the forward progress guarantee in the absence of > successful memory allocation, which is the most important property in > this path, I think. > > However, I'm not sure if the reclaim could lead to locking issues or > something like that with the more complex use-cases with MMU notifiers > and stuff like that. Christian, do you have any experience or > information that would be good to share in this regard? Yeah, very good question. __GFP_NORETRY isn't sufficient as far as I know. Reclaim must be completely suppressed to be able to allocate in a GPU reset handler. Daniel added lockdep annotation to some of the dma-fence signaling paths and this yielded quite a bunch of potential deadlocks. It's not even that reclaim itself waits for dma_fences (that can happen, but is quite unlikely), but rather that reclaim needs locks and under those locks we then wait for dma_fences. We should probably add a define somewhere which documents that (GFP_ATOMIC | __NO_WARN) should be used in the GPU reset handlers when allocating memory for coredumps. Regards, Christian. > > Regards, > Lucas > >> Regards, >> Christian. >> >>> Signed-off-by: André Almeida <andrealmeid@igalia.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 14 +++-- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 ++++++++++++++-------- >>> 2 files changed, 51 insertions(+), 28 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>> index dbe062a087c5..e1cc83a89d46 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>> @@ -1068,11 +1068,6 @@ struct amdgpu_device { >>> uint32_t *reset_dump_reg_list; >>> uint32_t *reset_dump_reg_value; >>> int num_regs; >>> -#ifdef CONFIG_DEV_COREDUMP >>> - struct amdgpu_task_info reset_task_info; >>> - bool reset_vram_lost; >>> - struct timespec64 reset_time; >>> -#endif >>> >>> bool scpm_enabled; >>> uint32_t scpm_status; >>> @@ -1085,6 +1080,15 @@ struct amdgpu_device { >>> uint32_t aid_mask; >>> }; >>> >>> +#ifdef CONFIG_DEV_COREDUMP >>> +struct amdgpu_coredump_info { >>> + struct amdgpu_device *adev; >>> + struct amdgpu_task_info reset_task_info; >>> + struct timespec64 reset_time; >>> + bool reset_vram_lost; >>> +}; >>> +#endif >>> + >>> static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) >>> { >>> return container_of(ddev, struct amdgpu_device, ddev); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> index e25f085ee886..23b9784e9787 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>> @@ -4963,12 +4963,17 @@ static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev) >>> return 0; >>> } >>> >>> -#ifdef CONFIG_DEV_COREDUMP >>> +#ifndef CONFIG_DEV_COREDUMP >>> +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, >>> + struct amdgpu_reset_context *reset_context) >>> +{ >>> +} >>> +#else >>> static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, >>> size_t count, void *data, size_t datalen) >>> { >>> struct drm_printer p; >>> - struct amdgpu_device *adev = data; >>> + struct amdgpu_coredump_info *coredump = data; >>> struct drm_print_iterator iter; >>> int i; >>> >>> @@ -4982,21 +4987,21 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, >>> drm_printf(&p, "**** AMDGPU Device Coredump ****\n"); >>> drm_printf(&p, "kernel: " UTS_RELEASE "\n"); >>> drm_printf(&p, "module: " KBUILD_MODNAME "\n"); >>> - drm_printf(&p, "time: %lld.%09ld\n", adev->reset_time.tv_sec, adev->reset_time.tv_nsec); >>> - if (adev->reset_task_info.pid) >>> + drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec, coredump->reset_time.tv_nsec); >>> + if (coredump->reset_task_info.pid) >>> drm_printf(&p, "process_name: %s PID: %d\n", >>> - adev->reset_task_info.process_name, >>> - adev->reset_task_info.pid); >>> + coredump->reset_task_info.process_name, >>> + coredump->reset_task_info.pid); >>> >>> - if (adev->reset_vram_lost) >>> + if (coredump->reset_vram_lost) >>> drm_printf(&p, "VRAM is lost due to GPU reset!\n"); >>> - if (adev->num_regs) { >>> + if (coredump->adev->num_regs) { >>> drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n"); >>> >>> - for (i = 0; i < adev->num_regs; i++) >>> + for (i = 0; i < coredump->adev->num_regs; i++) >>> drm_printf(&p, "0x%08x: 0x%08x\n", >>> - adev->reset_dump_reg_list[i], >>> - adev->reset_dump_reg_value[i]); >>> + coredump->adev->reset_dump_reg_list[i], >>> + coredump->adev->reset_dump_reg_value[i]); >>> } >>> >>> return count - iter.remain; >>> @@ -5004,14 +5009,34 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, >>> >>> static void amdgpu_devcoredump_free(void *data) >>> { >>> + kfree(data); >>> } >>> >>> -static void amdgpu_reset_capture_coredumpm(struct amdgpu_device *adev) >>> +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, >>> + struct amdgpu_reset_context *reset_context) >>> { >>> + struct amdgpu_coredump_info *coredump; >>> struct drm_device *dev = adev_to_drm(adev); >>> >>> - ktime_get_ts64(&adev->reset_time); >>> - dev_coredumpm(dev->dev, THIS_MODULE, adev, 0, GFP_KERNEL, >>> + coredump = kmalloc(sizeof(*coredump), GFP_KERNEL); >>> + >>> + if (!coredump) { >>> + DRM_ERROR("%s: failed to allocate memory for coredump\n", __func__); >>> + return; >>> + } >>> + >>> + memset(coredump, 0, sizeof(*coredump)); >>> + >>> + coredump->reset_vram_lost = vram_lost; >>> + >>> + if (reset_context->job && reset_context->job->vm) >>> + coredump->reset_task_info = reset_context->job->vm->task_info; >>> + >>> + coredump->adev = adev; >>> + >>> + ktime_get_ts64(&coredump->reset_time); >>> + >>> + dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_KERNEL, >>> amdgpu_devcoredump_read, amdgpu_devcoredump_free); >>> } >>> #endif >>> @@ -5119,15 +5144,9 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, >>> goto out; >>> >>> vram_lost = amdgpu_device_check_vram_lost(tmp_adev); >>> -#ifdef CONFIG_DEV_COREDUMP >>> - tmp_adev->reset_vram_lost = vram_lost; >>> - memset(&tmp_adev->reset_task_info, 0, >>> - sizeof(tmp_adev->reset_task_info)); >>> - if (reset_context->job && reset_context->job->vm) >>> - tmp_adev->reset_task_info = >>> - reset_context->job->vm->task_info; >>> - amdgpu_reset_capture_coredumpm(tmp_adev); >>> -#endif >>> + >>> + amdgpu_coredump(tmp_adev, vram_lost, reset_context); >>> + >>> if (vram_lost) { >>> DRM_INFO("VRAM is lost due to GPU reset!\n"); >>> amdgpu_inc_vram_lost(tmp_adev);
Am Mittwoch, dem 12.07.2023 um 12:39 +0200 schrieb Christian König: > Am 12.07.23 um 10:59 schrieb Lucas Stach: > > Am Mittwoch, dem 12.07.2023 um 10:37 +0200 schrieb Christian König: > > > Am 11.07.23 um 23:34 schrieb André Almeida: > > > > Instead of storing coredump information inside amdgpu_device struct, > > > > move if to a proper separated struct and allocate it dynamically. This > > > > will make it easier to further expand the logged information. > > > Verry big NAK to this. The problem is that memory allocation isn't > > > allowed during a GPU reset. > > > > > > What you could do is to allocate the memory with GFP_ATOMIC or similar, > > > but for a large structure that might not be possible. > > > > > I'm still not fully clear on what the rules are here. In etnaviv we do > > devcoredump allocation in the GPU reset path with __GFP_NOWARN | > > __GFP_NORETRY, which means the allocation will kick memory reclaim if > > necessary, but will just give up if no memory could be made available > > easily. This satisfies the forward progress guarantee in the absence of > > successful memory allocation, which is the most important property in > > this path, I think. > > > > However, I'm not sure if the reclaim could lead to locking issues or > > something like that with the more complex use-cases with MMU notifiers > > and stuff like that. Christian, do you have any experience or > > information that would be good to share in this regard? > > Yeah, very good question. > > __GFP_NORETRY isn't sufficient as far as I know. Reclaim must be > completely suppressed to be able to allocate in a GPU reset handler. > > Daniel added lockdep annotation to some of the dma-fence signaling paths > and this yielded quite a bunch of potential deadlocks. > > It's not even that reclaim itself waits for dma_fences (that can happen, > but is quite unlikely), but rather that reclaim needs locks and under > those locks we then wait for dma_fences. > > We should probably add a define somewhere which documents that > (GFP_ATOMIC | __NO_WARN) should be used in the GPU reset handlers when > allocating memory for coredumps. > > Regards, > Christian. > > > > > Regards, > > Lucas > > > > > Regards, > > > Christian. > > > > > > > Signed-off-by: André Almeida <andrealmeid@igalia.com> > > > > --- > > > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 14 +++-- > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 ++++++++++++++-------- > > > > 2 files changed, 51 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > > > index dbe062a087c5..e1cc83a89d46 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > > > @@ -1068,11 +1068,6 @@ struct amdgpu_device { > > > > uint32_t *reset_dump_reg_list; > > > > uint32_t *reset_dump_reg_value; > > > > int num_regs; > > > > -#ifdef CONFIG_DEV_COREDUMP > > > > - struct amdgpu_task_info reset_task_info; > > > > - bool reset_vram_lost; > > > > - struct timespec64 reset_time; > > > > -#endif > > > > > > > > bool scpm_enabled; > > > > uint32_t scpm_status; > > > > @@ -1085,6 +1080,15 @@ struct amdgpu_device { > > > > uint32_t aid_mask; > > > > }; > > > > > > > > +#ifdef CONFIG_DEV_COREDUMP > > > > +struct amdgpu_coredump_info { > > > > + struct amdgpu_device *adev; > > > > + struct amdgpu_task_info reset_task_info; > > > > + struct timespec64 reset_time; > > > > + bool reset_vram_lost; > > > > +}; > > > > +#endif > > > > + > > > > static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) > > > > { > > > > return container_of(ddev, struct amdgpu_device, ddev); > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > index e25f085ee886..23b9784e9787 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > @@ -4963,12 +4963,17 @@ static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev) > > > > return 0; > > > > } > > > > > > > > -#ifdef CONFIG_DEV_COREDUMP > > > > +#ifndef CONFIG_DEV_COREDUMP > > > > +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > > > > + struct amdgpu_reset_context *reset_context) > > > > +{ > > > > +} > > > > +#else > > > > static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > > > size_t count, void *data, size_t datalen) > > > > { > > > > struct drm_printer p; > > > > - struct amdgpu_device *adev = data; > > > > + struct amdgpu_coredump_info *coredump = data; > > > > struct drm_print_iterator iter; > > > > int i; > > > > > > > > @@ -4982,21 +4987,21 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > > > drm_printf(&p, "**** AMDGPU Device Coredump ****\n"); > > > > drm_printf(&p, "kernel: " UTS_RELEASE "\n"); > > > > drm_printf(&p, "module: " KBUILD_MODNAME "\n"); > > > > - drm_printf(&p, "time: %lld.%09ld\n", adev->reset_time.tv_sec, adev->reset_time.tv_nsec); > > > > - if (adev->reset_task_info.pid) > > > > + drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec, coredump->reset_time.tv_nsec); > > > > + if (coredump->reset_task_info.pid) > > > > drm_printf(&p, "process_name: %s PID: %d\n", > > > > - adev->reset_task_info.process_name, > > > > - adev->reset_task_info.pid); > > > > + coredump->reset_task_info.process_name, > > > > + coredump->reset_task_info.pid); > > > > > > > > - if (adev->reset_vram_lost) > > > > + if (coredump->reset_vram_lost) > > > > drm_printf(&p, "VRAM is lost due to GPU reset!\n"); > > > > - if (adev->num_regs) { > > > > + if (coredump->adev->num_regs) { > > > > drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n"); > > > > > > > > - for (i = 0; i < adev->num_regs; i++) > > > > + for (i = 0; i < coredump->adev->num_regs; i++) > > > > drm_printf(&p, "0x%08x: 0x%08x\n", > > > > - adev->reset_dump_reg_list[i], > > > > - adev->reset_dump_reg_value[i]); > > > > + coredump->adev->reset_dump_reg_list[i], > > > > + coredump->adev->reset_dump_reg_value[i]); > > > > } > > > > > > > > return count - iter.remain; > > > > @@ -5004,14 +5009,34 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > > > > > > > static void amdgpu_devcoredump_free(void *data) > > > > { > > > > + kfree(data); > > > > } > > > > > > > > -static void amdgpu_reset_capture_coredumpm(struct amdgpu_device *adev) > > > > +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > > > > + struct amdgpu_reset_context *reset_context) > > > > { > > > > + struct amdgpu_coredump_info *coredump; > > > > struct drm_device *dev = adev_to_drm(adev); > > > > > > > > - ktime_get_ts64(&adev->reset_time); > > > > - dev_coredumpm(dev->dev, THIS_MODULE, adev, 0, GFP_KERNEL, > > > > + coredump = kmalloc(sizeof(*coredump), GFP_KERNEL); > > > > + > > > > + if (!coredump) { > > > > + DRM_ERROR("%s: failed to allocate memory for coredump\n", __func__); > > > > + return; > > > > + } > > > > + > > > > + memset(coredump, 0, sizeof(*coredump)); > > > > + > > > > + coredump->reset_vram_lost = vram_lost; > > > > + > > > > + if (reset_context->job && reset_context->job->vm) > > > > + coredump->reset_task_info = reset_context->job->vm->task_info; > > > > + > > > > + coredump->adev = adev; > > > > + > > > > + ktime_get_ts64(&coredump->reset_time); > > > > + > > > > + dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_KERNEL, > > > > amdgpu_devcoredump_read, amdgpu_devcoredump_free); > > > > } > > > > #endif > > > > @@ -5119,15 +5144,9 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, > > > > goto out; > > > > > > > > vram_lost = amdgpu_device_check_vram_lost(tmp_adev); > > > > -#ifdef CONFIG_DEV_COREDUMP > > > > - tmp_adev->reset_vram_lost = vram_lost; > > > > - memset(&tmp_adev->reset_task_info, 0, > > > > - sizeof(tmp_adev->reset_task_info)); > > > > - if (reset_context->job && reset_context->job->vm) > > > > - tmp_adev->reset_task_info = > > > > - reset_context->job->vm->task_info; > > > > - amdgpu_reset_capture_coredumpm(tmp_adev); > > > > -#endif > > > > + > > > > + amdgpu_coredump(tmp_adev, vram_lost, reset_context); > > > > + > > > > if (vram_lost) { > > > > DRM_INFO("VRAM is lost due to GPU reset!\n"); > > > > amdgpu_inc_vram_lost(tmp_adev); >
Sorry, accidentally hit sent on the previous mail. Am Mittwoch, dem 12.07.2023 um 12:39 +0200 schrieb Christian König: > Am 12.07.23 um 10:59 schrieb Lucas Stach: > > Am Mittwoch, dem 12.07.2023 um 10:37 +0200 schrieb Christian König: > > > Am 11.07.23 um 23:34 schrieb André Almeida: > > > > Instead of storing coredump information inside amdgpu_device struct, > > > > move if to a proper separated struct and allocate it dynamically. This > > > > will make it easier to further expand the logged information. > > > Verry big NAK to this. The problem is that memory allocation isn't > > > allowed during a GPU reset. > > > > > > What you could do is to allocate the memory with GFP_ATOMIC or similar, > > > but for a large structure that might not be possible. > > > > > I'm still not fully clear on what the rules are here. In etnaviv we do > > devcoredump allocation in the GPU reset path with __GFP_NOWARN | > > __GFP_NORETRY, which means the allocation will kick memory reclaim if > > necessary, but will just give up if no memory could be made available > > easily. This satisfies the forward progress guarantee in the absence of > > successful memory allocation, which is the most important property in > > this path, I think. > > > > However, I'm not sure if the reclaim could lead to locking issues or > > something like that with the more complex use-cases with MMU notifiers > > and stuff like that. Christian, do you have any experience or > > information that would be good to share in this regard? > > Yeah, very good question. > > __GFP_NORETRY isn't sufficient as far as I know. Reclaim must be > completely suppressed to be able to allocate in a GPU reset handler. > > Daniel added lockdep annotation to some of the dma-fence signaling paths > and this yielded quite a bunch of potential deadlocks. > > It's not even that reclaim itself waits for dma_fences (that can happen, > but is quite unlikely), but rather that reclaim needs locks and under > those locks we then wait for dma_fences. > > We should probably add a define somewhere which documents that > (GFP_ATOMIC | __NO_WARN) should be used in the GPU reset handlers when > allocating memory for coredumps. > Hm, if the problem is the direct reclaim path where we might recurse on a lock through those indirect dependencies then we should document this somewhere. kswapd reclaim should be fine as far as I can see, as we'll guarantee progress without waiting for the background reclaim. I don't think it's appropriate to dip into the atomic allocation reserves for a best-effort thing like writing the devcoredump, so I think this should be GFP_NOWAIT, which will also avoid the direct reclaim path. Regards, Lucas > Regards, > Christian. > > > > > Regards, > > Lucas > > > > > Regards, > > > Christian. > > > > > > > Signed-off-by: André Almeida <andrealmeid@igalia.com> > > > > --- > > > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 14 +++-- > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 ++++++++++++++-------- > > > > 2 files changed, 51 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > > > index dbe062a087c5..e1cc83a89d46 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > > > @@ -1068,11 +1068,6 @@ struct amdgpu_device { > > > > uint32_t *reset_dump_reg_list; > > > > uint32_t *reset_dump_reg_value; > > > > int num_regs; > > > > -#ifdef CONFIG_DEV_COREDUMP > > > > - struct amdgpu_task_info reset_task_info; > > > > - bool reset_vram_lost; > > > > - struct timespec64 reset_time; > > > > -#endif > > > > > > > > bool scpm_enabled; > > > > uint32_t scpm_status; > > > > @@ -1085,6 +1080,15 @@ struct amdgpu_device { > > > > uint32_t aid_mask; > > > > }; > > > > > > > > +#ifdef CONFIG_DEV_COREDUMP > > > > +struct amdgpu_coredump_info { > > > > + struct amdgpu_device *adev; > > > > + struct amdgpu_task_info reset_task_info; > > > > + struct timespec64 reset_time; > > > > + bool reset_vram_lost; > > > > +}; > > > > +#endif > > > > + > > > > static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) > > > > { > > > > return container_of(ddev, struct amdgpu_device, ddev); > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > index e25f085ee886..23b9784e9787 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > > > @@ -4963,12 +4963,17 @@ static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev) > > > > return 0; > > > > } > > > > > > > > -#ifdef CONFIG_DEV_COREDUMP > > > > +#ifndef CONFIG_DEV_COREDUMP > > > > +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > > > > + struct amdgpu_reset_context *reset_context) > > > > +{ > > > > +} > > > > +#else > > > > static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > > > size_t count, void *data, size_t datalen) > > > > { > > > > struct drm_printer p; > > > > - struct amdgpu_device *adev = data; > > > > + struct amdgpu_coredump_info *coredump = data; > > > > struct drm_print_iterator iter; > > > > int i; > > > > > > > > @@ -4982,21 +4987,21 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > > > drm_printf(&p, "**** AMDGPU Device Coredump ****\n"); > > > > drm_printf(&p, "kernel: " UTS_RELEASE "\n"); > > > > drm_printf(&p, "module: " KBUILD_MODNAME "\n"); > > > > - drm_printf(&p, "time: %lld.%09ld\n", adev->reset_time.tv_sec, adev->reset_time.tv_nsec); > > > > - if (adev->reset_task_info.pid) > > > > + drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec, coredump->reset_time.tv_nsec); > > > > + if (coredump->reset_task_info.pid) > > > > drm_printf(&p, "process_name: %s PID: %d\n", > > > > - adev->reset_task_info.process_name, > > > > - adev->reset_task_info.pid); > > > > + coredump->reset_task_info.process_name, > > > > + coredump->reset_task_info.pid); > > > > > > > > - if (adev->reset_vram_lost) > > > > + if (coredump->reset_vram_lost) > > > > drm_printf(&p, "VRAM is lost due to GPU reset!\n"); > > > > - if (adev->num_regs) { > > > > + if (coredump->adev->num_regs) { > > > > drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n"); > > > > > > > > - for (i = 0; i < adev->num_regs; i++) > > > > + for (i = 0; i < coredump->adev->num_regs; i++) > > > > drm_printf(&p, "0x%08x: 0x%08x\n", > > > > - adev->reset_dump_reg_list[i], > > > > - adev->reset_dump_reg_value[i]); > > > > + coredump->adev->reset_dump_reg_list[i], > > > > + coredump->adev->reset_dump_reg_value[i]); > > > > } > > > > > > > > return count - iter.remain; > > > > @@ -5004,14 +5009,34 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > > > > > > > > static void amdgpu_devcoredump_free(void *data) > > > > { > > > > + kfree(data); > > > > } > > > > > > > > -static void amdgpu_reset_capture_coredumpm(struct amdgpu_device *adev) > > > > +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > > > > + struct amdgpu_reset_context *reset_context) > > > > { > > > > + struct amdgpu_coredump_info *coredump; > > > > struct drm_device *dev = adev_to_drm(adev); > > > > > > > > - ktime_get_ts64(&adev->reset_time); > > > > - dev_coredumpm(dev->dev, THIS_MODULE, adev, 0, GFP_KERNEL, > > > > + coredump = kmalloc(sizeof(*coredump), GFP_KERNEL); > > > > + > > > > + if (!coredump) { > > > > + DRM_ERROR("%s: failed to allocate memory for coredump\n", __func__); > > > > + return; > > > > + } > > > > + > > > > + memset(coredump, 0, sizeof(*coredump)); > > > > + > > > > + coredump->reset_vram_lost = vram_lost; > > > > + > > > > + if (reset_context->job && reset_context->job->vm) > > > > + coredump->reset_task_info = reset_context->job->vm->task_info; > > > > + > > > > + coredump->adev = adev; > > > > + > > > > + ktime_get_ts64(&coredump->reset_time); > > > > + > > > > + dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_KERNEL, > > > > amdgpu_devcoredump_read, amdgpu_devcoredump_free); > > > > } > > > > #endif > > > > @@ -5119,15 +5144,9 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, > > > > goto out; > > > > > > > > vram_lost = amdgpu_device_check_vram_lost(tmp_adev); > > > > -#ifdef CONFIG_DEV_COREDUMP > > > > - tmp_adev->reset_vram_lost = vram_lost; > > > > - memset(&tmp_adev->reset_task_info, 0, > > > > - sizeof(tmp_adev->reset_task_info)); > > > > - if (reset_context->job && reset_context->job->vm) > > > > - tmp_adev->reset_task_info = > > > > - reset_context->job->vm->task_info; > > > > - amdgpu_reset_capture_coredumpm(tmp_adev); > > > > -#endif > > > > + > > > > + amdgpu_coredump(tmp_adev, vram_lost, reset_context); > > > > + > > > > if (vram_lost) { > > > > DRM_INFO("VRAM is lost due to GPU reset!\n"); > > > > amdgpu_inc_vram_lost(tmp_adev); >
Am 12.07.23 um 12:56 schrieb Lucas Stach: > Sorry, accidentally hit sent on the previous mail. > > Am Mittwoch, dem 12.07.2023 um 12:39 +0200 schrieb Christian König: >> Am 12.07.23 um 10:59 schrieb Lucas Stach: >>> Am Mittwoch, dem 12.07.2023 um 10:37 +0200 schrieb Christian König: >>>> Am 11.07.23 um 23:34 schrieb André Almeida: >>>>> Instead of storing coredump information inside amdgpu_device struct, >>>>> move if to a proper separated struct and allocate it dynamically. This >>>>> will make it easier to further expand the logged information. >>>> Verry big NAK to this. The problem is that memory allocation isn't >>>> allowed during a GPU reset. >>>> >>>> What you could do is to allocate the memory with GFP_ATOMIC or similar, >>>> but for a large structure that might not be possible. >>>> >>> I'm still not fully clear on what the rules are here. In etnaviv we do >>> devcoredump allocation in the GPU reset path with __GFP_NOWARN | >>> __GFP_NORETRY, which means the allocation will kick memory reclaim if >>> necessary, but will just give up if no memory could be made available >>> easily. This satisfies the forward progress guarantee in the absence of >>> successful memory allocation, which is the most important property in >>> this path, I think. >>> >>> However, I'm not sure if the reclaim could lead to locking issues or >>> something like that with the more complex use-cases with MMU notifiers >>> and stuff like that. Christian, do you have any experience or >>> information that would be good to share in this regard? >> Yeah, very good question. >> >> __GFP_NORETRY isn't sufficient as far as I know. Reclaim must be >> completely suppressed to be able to allocate in a GPU reset handler. >> >> Daniel added lockdep annotation to some of the dma-fence signaling paths >> and this yielded quite a bunch of potential deadlocks. >> >> It's not even that reclaim itself waits for dma_fences (that can happen, >> but is quite unlikely), but rather that reclaim needs locks and under >> those locks we then wait for dma_fences. >> >> We should probably add a define somewhere which documents that >> (GFP_ATOMIC | __NO_WARN) should be used in the GPU reset handlers when >> allocating memory for coredumps. >> > Hm, if the problem is the direct reclaim path where we might recurse on > a lock through those indirect dependencies then we should document this > somewhere. kswapd reclaim should be fine as far as I can see, as we'll > guarantee progress without waiting for the background reclaim. > > I don't think it's appropriate to dip into the atomic allocation > reserves for a best-effort thing like writing the devcoredump, Yeah, that was also my concern the last time we discussed this. > so I think this should be GFP_NOWAIT, which will also avoid the direct > reclaim path. Yeah, good idea. I wasn't aware that this existed. Regards, Christian. > > Regards, > Lucas > >> Regards, >> Christian. >> >>> Regards, >>> Lucas >>> >>>> Regards, >>>> Christian. >>>> >>>>> Signed-off-by: André Almeida <andrealmeid@igalia.com> >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 14 +++-- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 ++++++++++++++-------- >>>>> 2 files changed, 51 insertions(+), 28 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>> index dbe062a087c5..e1cc83a89d46 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >>>>> @@ -1068,11 +1068,6 @@ struct amdgpu_device { >>>>> uint32_t *reset_dump_reg_list; >>>>> uint32_t *reset_dump_reg_value; >>>>> int num_regs; >>>>> -#ifdef CONFIG_DEV_COREDUMP >>>>> - struct amdgpu_task_info reset_task_info; >>>>> - bool reset_vram_lost; >>>>> - struct timespec64 reset_time; >>>>> -#endif >>>>> >>>>> bool scpm_enabled; >>>>> uint32_t scpm_status; >>>>> @@ -1085,6 +1080,15 @@ struct amdgpu_device { >>>>> uint32_t aid_mask; >>>>> }; >>>>> >>>>> +#ifdef CONFIG_DEV_COREDUMP >>>>> +struct amdgpu_coredump_info { >>>>> + struct amdgpu_device *adev; >>>>> + struct amdgpu_task_info reset_task_info; >>>>> + struct timespec64 reset_time; >>>>> + bool reset_vram_lost; >>>>> +}; >>>>> +#endif >>>>> + >>>>> static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) >>>>> { >>>>> return container_of(ddev, struct amdgpu_device, ddev); >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> index e25f085ee886..23b9784e9787 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >>>>> @@ -4963,12 +4963,17 @@ static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev) >>>>> return 0; >>>>> } >>>>> >>>>> -#ifdef CONFIG_DEV_COREDUMP >>>>> +#ifndef CONFIG_DEV_COREDUMP >>>>> +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, >>>>> + struct amdgpu_reset_context *reset_context) >>>>> +{ >>>>> +} >>>>> +#else >>>>> static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, >>>>> size_t count, void *data, size_t datalen) >>>>> { >>>>> struct drm_printer p; >>>>> - struct amdgpu_device *adev = data; >>>>> + struct amdgpu_coredump_info *coredump = data; >>>>> struct drm_print_iterator iter; >>>>> int i; >>>>> >>>>> @@ -4982,21 +4987,21 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, >>>>> drm_printf(&p, "**** AMDGPU Device Coredump ****\n"); >>>>> drm_printf(&p, "kernel: " UTS_RELEASE "\n"); >>>>> drm_printf(&p, "module: " KBUILD_MODNAME "\n"); >>>>> - drm_printf(&p, "time: %lld.%09ld\n", adev->reset_time.tv_sec, adev->reset_time.tv_nsec); >>>>> - if (adev->reset_task_info.pid) >>>>> + drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec, coredump->reset_time.tv_nsec); >>>>> + if (coredump->reset_task_info.pid) >>>>> drm_printf(&p, "process_name: %s PID: %d\n", >>>>> - adev->reset_task_info.process_name, >>>>> - adev->reset_task_info.pid); >>>>> + coredump->reset_task_info.process_name, >>>>> + coredump->reset_task_info.pid); >>>>> >>>>> - if (adev->reset_vram_lost) >>>>> + if (coredump->reset_vram_lost) >>>>> drm_printf(&p, "VRAM is lost due to GPU reset!\n"); >>>>> - if (adev->num_regs) { >>>>> + if (coredump->adev->num_regs) { >>>>> drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n"); >>>>> >>>>> - for (i = 0; i < adev->num_regs; i++) >>>>> + for (i = 0; i < coredump->adev->num_regs; i++) >>>>> drm_printf(&p, "0x%08x: 0x%08x\n", >>>>> - adev->reset_dump_reg_list[i], >>>>> - adev->reset_dump_reg_value[i]); >>>>> + coredump->adev->reset_dump_reg_list[i], >>>>> + coredump->adev->reset_dump_reg_value[i]); >>>>> } >>>>> >>>>> return count - iter.remain; >>>>> @@ -5004,14 +5009,34 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, >>>>> >>>>> static void amdgpu_devcoredump_free(void *data) >>>>> { >>>>> + kfree(data); >>>>> } >>>>> >>>>> -static void amdgpu_reset_capture_coredumpm(struct amdgpu_device *adev) >>>>> +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, >>>>> + struct amdgpu_reset_context *reset_context) >>>>> { >>>>> + struct amdgpu_coredump_info *coredump; >>>>> struct drm_device *dev = adev_to_drm(adev); >>>>> >>>>> - ktime_get_ts64(&adev->reset_time); >>>>> - dev_coredumpm(dev->dev, THIS_MODULE, adev, 0, GFP_KERNEL, >>>>> + coredump = kmalloc(sizeof(*coredump), GFP_KERNEL); >>>>> + >>>>> + if (!coredump) { >>>>> + DRM_ERROR("%s: failed to allocate memory for coredump\n", __func__); >>>>> + return; >>>>> + } >>>>> + >>>>> + memset(coredump, 0, sizeof(*coredump)); >>>>> + >>>>> + coredump->reset_vram_lost = vram_lost; >>>>> + >>>>> + if (reset_context->job && reset_context->job->vm) >>>>> + coredump->reset_task_info = reset_context->job->vm->task_info; >>>>> + >>>>> + coredump->adev = adev; >>>>> + >>>>> + ktime_get_ts64(&coredump->reset_time); >>>>> + >>>>> + dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_KERNEL, >>>>> amdgpu_devcoredump_read, amdgpu_devcoredump_free); >>>>> } >>>>> #endif >>>>> @@ -5119,15 +5144,9 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, >>>>> goto out; >>>>> >>>>> vram_lost = amdgpu_device_check_vram_lost(tmp_adev); >>>>> -#ifdef CONFIG_DEV_COREDUMP >>>>> - tmp_adev->reset_vram_lost = vram_lost; >>>>> - memset(&tmp_adev->reset_task_info, 0, >>>>> - sizeof(tmp_adev->reset_task_info)); >>>>> - if (reset_context->job && reset_context->job->vm) >>>>> - tmp_adev->reset_task_info = >>>>> - reset_context->job->vm->task_info; >>>>> - amdgpu_reset_capture_coredumpm(tmp_adev); >>>>> -#endif >>>>> + >>>>> + amdgpu_coredump(tmp_adev, vram_lost, reset_context); >>>>> + >>>>> if (vram_lost) { >>>>> DRM_INFO("VRAM is lost due to GPU reset!\n"); >>>>> amdgpu_inc_vram_lost(tmp_adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index dbe062a087c5..e1cc83a89d46 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1068,11 +1068,6 @@ struct amdgpu_device { uint32_t *reset_dump_reg_list; uint32_t *reset_dump_reg_value; int num_regs; -#ifdef CONFIG_DEV_COREDUMP - struct amdgpu_task_info reset_task_info; - bool reset_vram_lost; - struct timespec64 reset_time; -#endif bool scpm_enabled; uint32_t scpm_status; @@ -1085,6 +1080,15 @@ struct amdgpu_device { uint32_t aid_mask; }; +#ifdef CONFIG_DEV_COREDUMP +struct amdgpu_coredump_info { + struct amdgpu_device *adev; + struct amdgpu_task_info reset_task_info; + struct timespec64 reset_time; + bool reset_vram_lost; +}; +#endif + static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev) { return container_of(ddev, struct amdgpu_device, ddev); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index e25f085ee886..23b9784e9787 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4963,12 +4963,17 @@ static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev) return 0; } -#ifdef CONFIG_DEV_COREDUMP +#ifndef CONFIG_DEV_COREDUMP +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, + struct amdgpu_reset_context *reset_context) +{ +} +#else static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, size_t count, void *data, size_t datalen) { struct drm_printer p; - struct amdgpu_device *adev = data; + struct amdgpu_coredump_info *coredump = data; struct drm_print_iterator iter; int i; @@ -4982,21 +4987,21 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, drm_printf(&p, "**** AMDGPU Device Coredump ****\n"); drm_printf(&p, "kernel: " UTS_RELEASE "\n"); drm_printf(&p, "module: " KBUILD_MODNAME "\n"); - drm_printf(&p, "time: %lld.%09ld\n", adev->reset_time.tv_sec, adev->reset_time.tv_nsec); - if (adev->reset_task_info.pid) + drm_printf(&p, "time: %lld.%09ld\n", coredump->reset_time.tv_sec, coredump->reset_time.tv_nsec); + if (coredump->reset_task_info.pid) drm_printf(&p, "process_name: %s PID: %d\n", - adev->reset_task_info.process_name, - adev->reset_task_info.pid); + coredump->reset_task_info.process_name, + coredump->reset_task_info.pid); - if (adev->reset_vram_lost) + if (coredump->reset_vram_lost) drm_printf(&p, "VRAM is lost due to GPU reset!\n"); - if (adev->num_regs) { + if (coredump->adev->num_regs) { drm_printf(&p, "AMDGPU register dumps:\nOffset: Value:\n"); - for (i = 0; i < adev->num_regs; i++) + for (i = 0; i < coredump->adev->num_regs; i++) drm_printf(&p, "0x%08x: 0x%08x\n", - adev->reset_dump_reg_list[i], - adev->reset_dump_reg_value[i]); + coredump->adev->reset_dump_reg_list[i], + coredump->adev->reset_dump_reg_value[i]); } return count - iter.remain; @@ -5004,14 +5009,34 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, static void amdgpu_devcoredump_free(void *data) { + kfree(data); } -static void amdgpu_reset_capture_coredumpm(struct amdgpu_device *adev) +static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, + struct amdgpu_reset_context *reset_context) { + struct amdgpu_coredump_info *coredump; struct drm_device *dev = adev_to_drm(adev); - ktime_get_ts64(&adev->reset_time); - dev_coredumpm(dev->dev, THIS_MODULE, adev, 0, GFP_KERNEL, + coredump = kmalloc(sizeof(*coredump), GFP_KERNEL); + + if (!coredump) { + DRM_ERROR("%s: failed to allocate memory for coredump\n", __func__); + return; + } + + memset(coredump, 0, sizeof(*coredump)); + + coredump->reset_vram_lost = vram_lost; + + if (reset_context->job && reset_context->job->vm) + coredump->reset_task_info = reset_context->job->vm->task_info; + + coredump->adev = adev; + + ktime_get_ts64(&coredump->reset_time); + + dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_KERNEL, amdgpu_devcoredump_read, amdgpu_devcoredump_free); } #endif @@ -5119,15 +5144,9 @@ int amdgpu_do_asic_reset(struct list_head *device_list_handle, goto out; vram_lost = amdgpu_device_check_vram_lost(tmp_adev); -#ifdef CONFIG_DEV_COREDUMP - tmp_adev->reset_vram_lost = vram_lost; - memset(&tmp_adev->reset_task_info, 0, - sizeof(tmp_adev->reset_task_info)); - if (reset_context->job && reset_context->job->vm) - tmp_adev->reset_task_info = - reset_context->job->vm->task_info; - amdgpu_reset_capture_coredumpm(tmp_adev); -#endif + + amdgpu_coredump(tmp_adev, vram_lost, reset_context); + if (vram_lost) { DRM_INFO("VRAM is lost due to GPU reset!\n"); amdgpu_inc_vram_lost(tmp_adev);