On 10/29/2024 6:59 PM, Alex Deucher wrote:
> On Tue, Oct 29, 2024 at 2:58 AM SRINIVASAN SHANMUGAM
> <[email protected]> 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 <[email protected]>
>> ---
>> 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.
>>
>
> We want sysfs enforce_isolation whether or not there is a cleaner
> shader for a chip yet or not. It's useful for serializing access to
> gfx. Before we added the cleaner shader stuff it was still a useful
> option for certain use cases.
>
Ok, in that case will make the check specific for run_cleaner_shader
attribute.
Thanks,
Lijo
> Alex
>
>> -Srini
>>
>> 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
>>
>> I had initiated this 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);
