Re: [PATCH] drm/amdgpu: Group gfx sysfs functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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);



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux