| Message ID | 20230713213242.680944-6-andrealmeid@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu: Add new reset option and rework coredump | expand |
Am 13.07.23 um 23:32 schrieb André Almeida: > Log the IB addresses used by the hung job along with the stuck ring > name. Note that due to nested IBs, the one that caused the reset itself > may be in not listed address. > > Signed-off-by: André Almeida <andrealmeid@igalia.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 +++ > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 31 +++++++++++++++++++++- > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index e1cc83a89d46..cfeaf93934fd 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -1086,6 +1086,9 @@ struct amdgpu_coredump_info { > struct amdgpu_task_info reset_task_info; > struct timespec64 reset_time; > bool reset_vram_lost; > + u64 *ibs; > + u32 num_ibs; > + char ring_name[16]; > }; > #endif > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 07546781b8b8..431ccc3d7857 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -5008,12 +5008,24 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, > coredump->adev->reset_dump_reg_value[i]); > } > > + if (coredump->num_ibs) { > + drm_printf(&p, "IBs:\n"); > + for (i = 0; i < coredump->num_ibs; i++) > + drm_printf(&p, "\t[%d] 0x%llx\n", i, coredump->ibs[i]); > + } > + > + if (coredump->ring_name[0] != '\0') > + drm_printf(&p, "ring name: %s\n", coredump->ring_name); > + > return count - iter.remain; > } > > static void amdgpu_devcoredump_free(void *data) > { > - kfree(data); > + struct amdgpu_coredump_info *coredump = data; > + > + kfree(coredump->ibs); > + kfree(coredump); > } > > static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > @@ -5021,6 +5033,8 @@ static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > { > struct amdgpu_coredump_info *coredump; > struct drm_device *dev = adev_to_drm(adev); > + struct amdgpu_job *job = reset_context->job; > + int i; > > coredump = kmalloc(sizeof(*coredump), GFP_NOWAIT); > > @@ -5038,6 +5052,21 @@ static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, > > coredump->adev = adev; > > + if (job && job->num_ibs) { I really really really don't want any dependency of the core dump feature towards the job. What we could do is to record the first executed IB VAs in the hw fence, but I'm not sure how useful this is in the first place. We have some internal feature in progress to query the VA of the draw command which cause the waves currently executing in the SQ to be retrieved. > + struct amdgpu_ring *ring = to_amdgpu_ring(job->base.sched); > + u32 num_ibs = job->num_ibs; > + > + coredump->ibs = kmalloc_array(num_ibs, sizeof(coredump->ibs), GFP_NOWAIT); This can fail pretty easily. Christian. > + if (coredump->ibs) > + coredump->num_ibs = num_ibs; > + > + for (i = 0; i < coredump->num_ibs; i++) > + coredump->ibs[i] = job->ibs[i].gpu_addr; > + > + if (ring) > + strncpy(coredump->ring_name, ring->name, 16); > + } > + > ktime_get_ts64(&coredump->reset_time); > > dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_NOWAIT,
Em 14/07/2023 04:57, Christian König escreveu: > Am 13.07.23 um 23:32 schrieb André Almeida: >> Log the IB addresses used by the hung job along with the stuck ring >> name. Note that due to nested IBs, the one that caused the reset itself >> may be in not listed address. >> >> Signed-off-by: André Almeida <andrealmeid@igalia.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 +++ >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 31 +++++++++++++++++++++- >> 2 files changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> index e1cc83a89d46..cfeaf93934fd 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> @@ -1086,6 +1086,9 @@ struct amdgpu_coredump_info { >> struct amdgpu_task_info reset_task_info; >> struct timespec64 reset_time; >> bool reset_vram_lost; >> + u64 *ibs; >> + u32 num_ibs; >> + char ring_name[16]; >> }; >> #endif >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index 07546781b8b8..431ccc3d7857 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -5008,12 +5008,24 @@ static ssize_t amdgpu_devcoredump_read(char >> *buffer, loff_t offset, >> coredump->adev->reset_dump_reg_value[i]); >> } >> + if (coredump->num_ibs) { >> + drm_printf(&p, "IBs:\n"); >> + for (i = 0; i < coredump->num_ibs; i++) >> + drm_printf(&p, "\t[%d] 0x%llx\n", i, coredump->ibs[i]); >> + } >> + >> + if (coredump->ring_name[0] != '\0') >> + drm_printf(&p, "ring name: %s\n", coredump->ring_name); >> + >> return count - iter.remain; >> } >> static void amdgpu_devcoredump_free(void *data) >> { >> - kfree(data); >> + struct amdgpu_coredump_info *coredump = data; >> + >> + kfree(coredump->ibs); >> + kfree(coredump); >> } >> static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, >> @@ -5021,6 +5033,8 @@ static void amdgpu_coredump(struct amdgpu_device >> *adev, bool vram_lost, >> { >> struct amdgpu_coredump_info *coredump; >> struct drm_device *dev = adev_to_drm(adev); >> + struct amdgpu_job *job = reset_context->job; >> + int i; >> coredump = kmalloc(sizeof(*coredump), GFP_NOWAIT); >> @@ -5038,6 +5052,21 @@ static void amdgpu_coredump(struct >> amdgpu_device *adev, bool vram_lost, >> coredump->adev = adev; >> + if (job && job->num_ibs) { > > I really really really don't want any dependency of the core dump > feature towards the job. > Because of the lifetime of job? Do you think implementing amdgpu_job_get()/put() would help here? > What we could do is to record the first executed IB VAs in the hw fence, > but I'm not sure how useful this is in the first place. > I see, any hint here of the timedout job would be helpful AFAIK. > We have some internal feature in progress to query the VA of the draw > command which cause the waves currently executing in the SQ to be > retrieved. > >> + struct amdgpu_ring *ring = to_amdgpu_ring(job->base.sched); >> + u32 num_ibs = job->num_ibs; >> + >> + coredump->ibs = kmalloc_array(num_ibs, sizeof(coredump->ibs), >> GFP_NOWAIT); > > This can fail pretty easily. Because of its size? > > Christian. > >> + if (coredump->ibs) >> + coredump->num_ibs = num_ibs; >> + >> + for (i = 0; i < coredump->num_ibs; i++) >> + coredump->ibs[i] = job->ibs[i].gpu_addr; >> + >> + if (ring) >> + strncpy(coredump->ring_name, ring->name, 16); >> + } >> + >> ktime_get_ts64(&coredump->reset_time); >> dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_NOWAIT, >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index e1cc83a89d46..cfeaf93934fd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1086,6 +1086,9 @@ struct amdgpu_coredump_info { struct amdgpu_task_info reset_task_info; struct timespec64 reset_time; bool reset_vram_lost; + u64 *ibs; + u32 num_ibs; + char ring_name[16]; }; #endif diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 07546781b8b8..431ccc3d7857 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -5008,12 +5008,24 @@ static ssize_t amdgpu_devcoredump_read(char *buffer, loff_t offset, coredump->adev->reset_dump_reg_value[i]); } + if (coredump->num_ibs) { + drm_printf(&p, "IBs:\n"); + for (i = 0; i < coredump->num_ibs; i++) + drm_printf(&p, "\t[%d] 0x%llx\n", i, coredump->ibs[i]); + } + + if (coredump->ring_name[0] != '\0') + drm_printf(&p, "ring name: %s\n", coredump->ring_name); + return count - iter.remain; } static void amdgpu_devcoredump_free(void *data) { - kfree(data); + struct amdgpu_coredump_info *coredump = data; + + kfree(coredump->ibs); + kfree(coredump); } static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, @@ -5021,6 +5033,8 @@ static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, { struct amdgpu_coredump_info *coredump; struct drm_device *dev = adev_to_drm(adev); + struct amdgpu_job *job = reset_context->job; + int i; coredump = kmalloc(sizeof(*coredump), GFP_NOWAIT); @@ -5038,6 +5052,21 @@ static void amdgpu_coredump(struct amdgpu_device *adev, bool vram_lost, coredump->adev = adev; + if (job && job->num_ibs) { + struct amdgpu_ring *ring = to_amdgpu_ring(job->base.sched); + u32 num_ibs = job->num_ibs; + + coredump->ibs = kmalloc_array(num_ibs, sizeof(coredump->ibs), GFP_NOWAIT); + if (coredump->ibs) + coredump->num_ibs = num_ibs; + + for (i = 0; i < coredump->num_ibs; i++) + coredump->ibs[i] = job->ibs[i].gpu_addr; + + if (ring) + strncpy(coredump->ring_name, ring->name, 16); + } + ktime_get_ts64(&coredump->reset_time); dev_coredumpm(dev->dev, THIS_MODULE, coredump, 0, GFP_NOWAIT,
Log the IB addresses used by the hung job along with the stuck ring name. Note that due to nested IBs, the one that caused the reset itself may be in not listed address. Signed-off-by: André Almeida <andrealmeid@igalia.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 31 +++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-)