On 10/29/2024 6:59 PM, Alex Deucher wrote:
> On Tue, Oct 29, 2024 at 2:58 AM SRINIVASAN SHANMUGAM
> <srinivasan.shanmugam@xxxxxxx> 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.
>>
>
> 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);
