Messages in this thread | | | Date | Thu, 17 Aug 2023 10:45:30 -0300 | Subject | Re: [PATCH v4 2/4] drm/amdgpu: Rework coredump to use memory dynamically | From | André Almeida <> |
| |
Hi Shashank,
Em 17/08/2023 03:41, Shashank Sharma escreveu: > Hello Andre, > > On 15/08/2023 21:50, André Almeida wrote: >> 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> >> --- >> v4: change kmalloc to kzalloc >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 14 +++-- >> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 63 ++++++++++++++-------- >> 2 files changed, 49 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> index 9c6a332261ab..0d560b713948 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> @@ -1088,11 +1088,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; >> @@ -1105,6 +1100,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; >> +}; > > The patch looks good to me in general, but I would recommend slightly > different arrangement and segregation of GPU reset information. > > Please consider a higher level structure adev->gpu_reset_info, and move > everything related to reset dump info into that, including this new > coredump_info structure, something like this: > > struct amdgpu_reset_info { > > uint32_t *reset_dump_reg_list; > > uint32_t *reset_dump_reg_value; > > int num_regs; >
Right, I can encapsulate there reset_dump members,
> #ifdef CONFIG_DEV_COREDUMP > > struct amdgpu_coredump_info *coredump_info;/* keep this dynamic > allocation */
but we don't need a pointer for amdgpu_coredump_info inside amdgpu_device or inside of amdgpu_device->gpu_reset_info, right?
> > #endif > > } > > > This will make sure that all the relevant information is at the same place. > > - Shashank > amdgpu_inc_vram_lost(tmp_adev);
| |