On 10/29/2024 12:18 PM, SRINIVASAN SHANMUGAM wrote: > > On 10/29/2024 12:07 PM, SRINIVASAN SHANMUGAM wrote: >> >> >> On 10/29/2024 10:57 AM, Lijo Lazar wrote: >>> Make amdgpu_gfx_sysfs_init/fini functions as common entry points for all >>> gfx related sysfs nodes. >>> >>> Signed-off-by: Lijo Lazar <lijo.lazar@xxxxxxx> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 37 ++++++++++++++++++++++--- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h | 2 -- >>> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 5 ++-- >>> drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 4 +-- >>> drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c | 4 +-- >>> drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 4 +-- >>> drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 5 ---- >>> 7 files changed, 42 insertions(+), 19 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >>> index e96984c53e72..86a6fd3015c2 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c >>> @@ -1602,7 +1602,7 @@ static DEVICE_ATTR(current_compute_partition, 0644, >>> static DEVICE_ATTR(available_compute_partition, 0444, >>> amdgpu_gfx_get_available_compute_partition, NULL); >>> >>> -int amdgpu_gfx_sysfs_init(struct amdgpu_device *adev) >>> +static int amdgpu_gfx_sysfs_xcp_init(struct amdgpu_device *adev) >>> { >>> struct amdgpu_xcp_mgr *xcp_mgr = adev->xcp_mgr; >>> bool xcp_switch_supported; >>> @@ -1629,7 +1629,7 @@ int amdgpu_gfx_sysfs_init(struct amdgpu_device *adev) >>> return r; >>> } >>> >>> -void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev) >>> +static void amdgpu_gfx_sysfs_xcp_fini(struct amdgpu_device *adev) >>> { >>> struct amdgpu_xcp_mgr *xcp_mgr = adev->xcp_mgr; >>> bool xcp_switch_supported; >>> @@ -1646,10 +1646,13 @@ void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev) >>> &dev_attr_available_compute_partition); >>> } >>> >>> -int amdgpu_gfx_sysfs_isolation_shader_init(struct amdgpu_device *adev) >>> +static int amdgpu_gfx_sysfs_isolation_shader_init(struct amdgpu_device *adev) >>> { >>> int r; >>> >>> + if (!adev->gfx.enable_cleaner_shader) >>> + return 0; >>> + >> >> This check seems to be incorrect here, because enforce_isolation and >> cleaner shader are two different entities, with this check >> enforce_isolation node won't be created for some of the ASIC's where >> we don't load cleaner shader explictly. >> > Correction, this check " > > !adev->gfx.enable_cleaner_shader" handles for ASIC's where we don't load cleaner shader explictly, but > having it here I'm not certain cz I think we expect enforce_isolation > node to be created independent of run_cleaner_shader, would request > Alex/Christian, to comment onto it further. -Srini My understanding is that isolation will work only if cleaner shader can be run. If that's true, it doesn't make sense to provide a sysfs interface when cleaner shader runs are not supported. > >> And moreover this grouping, its better to be done for all sysfs >> entires in amdgpu ie., not only gfx, for other modules like even pm >> etc., so that we can have one common sysfs amdgpu framework, improving >> code consistency and maintainability We still want pm/individual IP modules to decide which sysfs files need to be created. Centralizing is not good. Thanks, Lijo >> >> I had initiated this https://patchwork.freedesktop.org/patch/595215/ >> <https://patchwork.freedesktop.org/patch/595215/> , but I couldn't >> finish it because of other work commitments. >> >>> r = device_create_file(adev->dev, &dev_attr_enforce_isolation); >>> if (r) >>> return r; >>> @@ -1661,12 +1664,38 @@ int amdgpu_gfx_sysfs_isolation_shader_init(struct amdgpu_device *adev) >>> return 0; >>> } >>> >>> -void amdgpu_gfx_sysfs_isolation_shader_fini(struct amdgpu_device *adev) >>> +static void amdgpu_gfx_sysfs_isolation_shader_fini(struct amdgpu_device *adev) >>> { >>> + if (!adev->gfx.enable_cleaner_shader) >>> + return; >>> + >> >> Same here >> >> -Srini >> >>> device_remove_file(adev->dev, &dev_attr_enforce_isolation); >>> device_remove_file(adev->dev, &dev_attr_run_cleaner_shader); >>> } >>> >>> +int amdgpu_gfx_sysfs_init(struct amdgpu_device *adev) >>> +{ >>> + int r; >>> + >>> + r = amdgpu_gfx_sysfs_xcp_init(adev); >>> + if (r) { >>> + dev_err(adev->dev, "failed to create xcp sysfs files"); >>> + return r; >>> + } >>> + >>> + r = amdgpu_gfx_sysfs_isolation_shader_init(adev); >>> + if (r) >>> + dev_err(adev->dev, "failed to create isolation sysfs files"); >>> + >>> + return r; >>> +} >>> + >>> +void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev) >>> +{ >>> + amdgpu_gfx_sysfs_xcp_fini(adev); >>> + amdgpu_gfx_sysfs_isolation_shader_fini(adev); >>> +} >>> + >>> int amdgpu_gfx_cleaner_shader_sw_init(struct amdgpu_device *adev, >>> unsigned int cleaner_shader_size) >>> { >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h >>> index f710178a21bc..b8a2f60688dc 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h >>> @@ -577,8 +577,6 @@ void amdgpu_gfx_cleaner_shader_sw_fini(struct amdgpu_device *adev); >>> void amdgpu_gfx_cleaner_shader_init(struct amdgpu_device *adev, >>> unsigned int cleaner_shader_size, >>> const void *cleaner_shader_ptr); >>> -int amdgpu_gfx_sysfs_isolation_shader_init(struct amdgpu_device *adev); >>> -void amdgpu_gfx_sysfs_isolation_shader_fini(struct amdgpu_device *adev); >>> void amdgpu_gfx_enforce_isolation_handler(struct work_struct *work); >>> void amdgpu_gfx_enforce_isolation_ring_begin_use(struct amdgpu_ring *ring); >>> void amdgpu_gfx_enforce_isolation_ring_end_use(struct amdgpu_ring *ring); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c >>> index 9da95b25e158..d1a18ca584dd 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c >>> @@ -4853,9 +4853,10 @@ static int gfx_v10_0_sw_init(struct amdgpu_ip_block *ip_block) >>> >>> gfx_v10_0_alloc_ip_dump(adev); >>> >>> - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); >>> + r = amdgpu_gfx_sysfs_init(adev); >>> if (r) >>> return r; >>> + >>> return 0; >>> } >>> >>> @@ -4907,7 +4908,7 @@ static int gfx_v10_0_sw_fini(struct amdgpu_ip_block *ip_block) >>> gfx_v10_0_rlc_backdoor_autoload_buffer_fini(adev); >>> >>> gfx_v10_0_free_microcode(adev); >>> - amdgpu_gfx_sysfs_isolation_shader_fini(adev); >>> + amdgpu_gfx_sysfs_fini(adev); >>> >>> kfree(adev->gfx.ip_dump_core); >>> kfree(adev->gfx.ip_dump_compute_queues); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c >>> index 5aff8f72de9c..22811b624532 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c >>> @@ -1717,7 +1717,7 @@ static int gfx_v11_0_sw_init(struct amdgpu_ip_block *ip_block) >>> >>> gfx_v11_0_alloc_ip_dump(adev); >>> >>> - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); >>> + r = amdgpu_gfx_sysfs_init(adev); >>> if (r) >>> return r; >>> >>> @@ -1782,7 +1782,7 @@ static int gfx_v11_0_sw_fini(struct amdgpu_ip_block *ip_block) >>> >>> gfx_v11_0_free_microcode(adev); >>> >>> - amdgpu_gfx_sysfs_isolation_shader_fini(adev); >>> + amdgpu_gfx_sysfs_fini(adev); >>> >>> kfree(adev->gfx.ip_dump_core); >>> kfree(adev->gfx.ip_dump_compute_queues); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c >>> index 9fec28d8a5fc..1b99f90cd193 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v12_0.c >>> @@ -1466,7 +1466,7 @@ static int gfx_v12_0_sw_init(struct amdgpu_ip_block *ip_block) >>> >>> gfx_v12_0_alloc_ip_dump(adev); >>> >>> - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); >>> + r = amdgpu_gfx_sysfs_init(adev); >>> if (r) >>> return r; >>> >>> @@ -1529,7 +1529,7 @@ static int gfx_v12_0_sw_fini(struct amdgpu_ip_block *ip_block) >>> >>> gfx_v12_0_free_microcode(adev); >>> >>> - amdgpu_gfx_sysfs_isolation_shader_fini(adev); >>> + amdgpu_gfx_sysfs_fini(adev); >>> >>> kfree(adev->gfx.ip_dump_core); >>> kfree(adev->gfx.ip_dump_compute_queues); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c >>> index 66947850d7e4..987e52c47635 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c >>> @@ -2402,7 +2402,7 @@ static int gfx_v9_0_sw_init(struct amdgpu_ip_block *ip_block) >>> >>> gfx_v9_0_alloc_ip_dump(adev); >>> >>> - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); >>> + r = amdgpu_gfx_sysfs_init(adev); >>> if (r) >>> return r; >>> >>> @@ -2443,7 +2443,7 @@ static int gfx_v9_0_sw_fini(struct amdgpu_ip_block *ip_block) >>> } >>> gfx_v9_0_free_microcode(adev); >>> >>> - amdgpu_gfx_sysfs_isolation_shader_fini(adev); >>> + amdgpu_gfx_sysfs_fini(adev); >>> >>> kfree(adev->gfx.ip_dump_core); >>> kfree(adev->gfx.ip_dump_compute_queues); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c >>> index 016290f00592..983088805c3a 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c >>> @@ -1171,10 +1171,6 @@ static int gfx_v9_4_3_sw_init(struct amdgpu_ip_block *ip_block) >>> >>> gfx_v9_4_3_alloc_ip_dump(adev); >>> >>> - r = amdgpu_gfx_sysfs_isolation_shader_init(adev); >>> - if (r) >>> - return r; >>> - >>> return 0; >>> } >>> >>> @@ -1199,7 +1195,6 @@ static int gfx_v9_4_3_sw_fini(struct amdgpu_ip_block *ip_block) >>> amdgpu_bo_unref(&adev->gfx.rlc.clear_state_obj); >>> gfx_v9_4_3_free_microcode(adev); >>> amdgpu_gfx_sysfs_fini(adev); >>> - amdgpu_gfx_sysfs_isolation_shader_fini(adev); >>> >>> kfree(adev->gfx.ip_dump_core); >>> kfree(adev->gfx.ip_dump_compute_queues);