<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body bgcolor="white" lang="EN-US" link="blue" vlink="purple"> <div class="WordSection1"> [AMD Official Use Only - Internal Distribution Only]<o:p></o:p> <o:p> </o:p> Hi, Jiange,<o:p></o:p> How to handle the case that multi-apps do the auto dump? This patch seems not multi-process safety. <o:p></o:p> <o:p> </o:p> Best Regards<o:p></o:p> Dennis Li<o:p></o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König Sent: Thursday, May 14, 2020 4:29 PM To: Zhao, Jiange <Jiange.Zhao@amd.com>; amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Pelloux-prayer, Pierre-eric <Pierre-eric.Pelloux-prayer@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com> Subject: Re: [PATCH] drm/amdgpu: Add autodump debugfs node for gpu reset v4<o:p></o:p> </div> </div> <o:p> </o:p> <div> Hi Jiange, it probably won't hurt, but I would just drop that. You need roughly 4 billion GPU resets until the UINT_MAX-1 becomes zero again. Christian. Am 14.05.20 um 09:14 schrieb Zhao, Jiange:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> [AMD Official Use Only - Internal Distribution Only]<o:p></o:p> <o:p> </o:p> <div> <div> Hi Christian,<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> wait_for_completion_interruptible_timeout() would decrease autodump.dumping.done to UINT_MAX-1.<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> complete_all() here would restore autodump.dumping to the state as in amdgpu_debugfs_autodump_init().<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> I want to make sure every open() deals with the same situation.<o:p></o:p> </div> <div> <o:p> </o:p> </div> <div> Jiange<o:p></o:p> </div> <div class="MsoNormal" align="center" style="text-align:center"> <hr size="2" width="98%" align="center"> </div> <div id="divRplyFwdMsg"> From: Christian König <a href="mailto:ckoenig.leichtzumerken@gmail.com"> <ckoenig.leichtzumerken@gmail.com></a> Sent: Thursday, May 14, 2020 3:01 PM To: Zhao, Jiange <a href="mailto:Jiange.Zhao@amd.com"><Jiange.Zhao@amd.com></a>; <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> <a href="mailto:amd-gfx@lists.freedesktop.org"> <amd-gfx@lists.freedesktop.org></a> Cc: Pelloux-prayer, Pierre-eric <a href="mailto:Pierre-eric.Pelloux-prayer@amd.com"> <Pierre-eric.Pelloux-prayer@amd.com></a>; Zhao, Jiange <a href="mailto:Jiange.Zhao@amd.com"> <Jiange.Zhao@amd.com></a>; Kuehling, Felix <a href="mailto:Felix.Kuehling@amd.com"> <Felix.Kuehling@amd.com></a>; Deucher, Alexander <a href="mailto:Alexander.Deucher@amd.com"> <Alexander.Deucher@amd.com></a>; Koenig, Christian <a href="mailto:Christian.Koenig@amd.com"> <Christian.Koenig@amd.com></a>; Liu, Monk <a href="mailto:Monk.Liu@amd.com"><Monk.Liu@amd.com></a>; Zhang, Hawking <a href="mailto:Hawking.Zhang@amd.com"><Hawking.Zhang@amd.com></a> Subject: Re: [PATCH] drm/amdgpu: Add autodump debugfs node for gpu reset v4 <o:p></o:p> <div> <o:p></o:p> </div> </div> <div> <div> Am 14.05.20 um 07:29 schrieb <a href="mailto:jianzh@amd.com"> jianzh@amd.com</a>: > From: Jiange Zhao <a href="mailto:Jiange.Zhao@amd.com"><Jiange.Zhao@amd.com></a> > > When GPU got timeout, it would notify an interested part > of an opportunity to dump info before actual GPU reset. > > A usermode app would open 'autodump' node under debugfs system > and poll() for readable/writable. When a GPU reset is due, > amdgpu would notify usermode app through wait_queue_head and give > it 10 minutes to dump info. > > After usermode app has done its work, this 'autodump' node is closed. > On node closure, amdgpu gets to know the dump is done through > the completion that is triggered in release(). > > There is no write or read callback because necessary info can be > obtained through dmesg and umr. Messages back and forth between > usermode app and amdgpu are unnecessary. > > v2: (1) changed 'registered' to 'app_listening' > (2) add a mutex in open() to prevent race condition > > v3 (chk): grab the reset lock to avoid race in autodump_open, > rename debugfs file to amdgpu_autodump, > provide autodump_read as well, > style and code cleanups > > v4: add 'bool app_listening' to differentiate situations, so that > the node can be reopened; also, there is no need to wait for > completion when no app is waiting for a dump. > > v5: change 'bool app_listening' to 'enum amdgpu_autodump_state' > add 'app_state_mutex' for race conditions: > (1)Only 1 user can open this file node > (2)wait_dump() can only take effect after poll() executed. > (3)eliminated the race condition between release() and > wait_dump() > > v6: removed 'enum amdgpu_autodump_state' and 'app_state_mutex' > removed state checking in amdgpu_debugfs_wait_dump > Improve on top of version 3 so that the node can be reopened. > > v7: move reinit_completion into open() so that only one user > can open it. > > Signed-off-by: Jiange Zhao <a href="mailto:Jiange.Zhao@amd.com"><Jiange.Zhao@amd.com></a> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 + > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 79 ++++++++++++++++++++- > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h | 6 ++ > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 + > 4 files changed, 88 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index 2a806cb55b78..9e8eeddfe7ce 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -992,6 +992,8 @@ struct amdgpu_device { > char product_number[16]; > char product_name[32]; > char serial[16]; > + > + struct amdgpu_autodump autodump; > }; > > static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_bo_device *bdev) > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > index 1a4894fa3693..efee3f1adecf 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > @@ -27,7 +27,7 @@ > #include <linux/pci.h> > #include <linux/uaccess.h> > #include <linux/pm_runtime.h> > - > +#include <linux/poll.h> > #include <drm/drm_debugfs.h> > > #include "amdgpu.h" > @@ -74,8 +74,83 @@ int amdgpu_debugfs_add_files(struct amdgpu_device *adev, > return 0; > } > > +int amdgpu_debugfs_wait_dump(struct amdgpu_device *adev) > +{ > +#if defined(CONFIG_DEBUG_FS) > + unsigned long timeout = 600 * HZ; > + int ret; > + > + wake_up_interruptible(&adev->autodump.gpu_hang); > + > + ret = wait_for_completion_interruptible_timeout(&adev->autodump.dumping, timeout); > + complete_all(&adev->autodump.dumping); Sorry that I'm mentioning this only now. But what is this complete_all() here good for? I mean we already waited for completion, didn't we? Christian. > + if (ret == 0) { > + pr_err("autodump: timeout, move on to gpu recovery\n"); > + return -ETIMEDOUT; > + } > +#endif > + return 0; > +} > + > #if defined(CONFIG_DEBUG_FS) > > +static int amdgpu_debugfs_autodump_open(struct inode *inode, struct file *file) > +{ > + struct amdgpu_device *adev = inode->i_private; > + int ret; > + > + file->private_data = adev; > + > + mutex_lock(&adev->lock_reset); > + if (adev->autodump.dumping.done) { > + reinit_completion(&adev->autodump.dumping); > + ret = 0; > + } else { > + ret = -EBUSY; > + } > + mutex_unlock(&adev->lock_reset); > + > + return ret; > +} > + > +static int amdgpu_debugfs_autodump_release(struct inode *inode, struct file *file) > +{ > + struct amdgpu_device *adev = file->private_data; > + > + complete_all(&adev->autodump.dumping); > + return 0; > +} > + > +static unsigned int amdgpu_debugfs_autodump_poll(struct file *file, struct poll_table_struct *poll_table) > +{ > + struct amdgpu_device *adev = file->private_data; > + > + poll_wait(file, &adev->autodump.gpu_hang, poll_table); > + > + if (adev->in_gpu_reset) > + return POLLIN | POLLRDNORM | POLLWRNORM; > + > + return 0; > +} > + > +static const struct file_operations autodump_debug_fops = { > + .owner = THIS_MODULE, > + .open = amdgpu_debugfs_autodump_open, > + .poll = amdgpu_debugfs_autodump_poll, > + .release = amdgpu_debugfs_autodump_release, > +}; > + > +static void amdgpu_debugfs_autodump_init(struct amdgpu_device *adev) > +{ > + init_completion(&adev->autodump.dumping); > + complete_all(&adev->autodump.dumping); > + init_waitqueue_head(&adev->autodump.gpu_hang); > + > + debugfs_create_file("amdgpu_autodump", 0600, > + adev->ddev->primary->debugfs_root, > + adev, &autodump_debug_fops); > +} > + > /** > * amdgpu_debugfs_process_reg_op - Handle MMIO register reads/writes > * > @@ -1434,6 +1509,8 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev) > > amdgpu_ras_debugfs_create_all(adev); > > + amdgpu_debugfs_autodump_init(adev); > + > return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_list, > ARRAY_SIZE(amdgpu_debugfs_list)); > } > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h > index de12d1101526..2803884d338d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h > @@ -31,6 +31,11 @@ struct amdgpu_debugfs { > unsigned num_files; > }; > > +struct amdgpu_autodump { > + struct completion dumping; > + struct wait_queue_head gpu_hang; > +}; > + > int amdgpu_debugfs_regs_init(struct amdgpu_device *adev); > int amdgpu_debugfs_init(struct amdgpu_device *adev); > void amdgpu_debugfs_fini(struct amdgpu_device *adev); > @@ -40,3 +45,4 @@ int amdgpu_debugfs_add_files(struct amdgpu_device *adev, > int amdgpu_debugfs_fence_init(struct amdgpu_device *adev); > int amdgpu_debugfs_firmware_init(struct amdgpu_device *adev); > int amdgpu_debugfs_gem_init(struct amdgpu_device *adev); > +int amdgpu_debugfs_wait_dump(struct amdgpu_device *adev); > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index cc41e8f5ad14..545beebcf43e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -3927,6 +3927,8 @@ static int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev, > int i, r = 0; > bool need_full_reset = *need_full_reset_arg; > > + amdgpu_debugfs_wait_dump(adev); > + > /* block all schedulers and reset given job's ring */ > for (i = 0; i < AMDGPU_MAX_RINGS; ++i) { > struct amdgpu_ring *ring = adev->rings[i];<o:p></o:p> </div> </div> </div> </blockquote> <o:p> </o:p> </div> </body> </html>