Re: [PATCH] drm/amdgpu: Move SR-IOV check into amdgpu_gfx_sysfs_compute_init

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

 




On 6/6/2024 10:58 PM, Lazar, Lijo wrote:

On 6/6/2024 5:35 PM, Srinivasan Shanmugam wrote:
Previously, this check was performed in the gfx_v9_4_3_sw_init function,
and the amdgpu_gfx_sysfs_compute_init function was only called if the
GPU was not a VF in SR-IOV mode. This is because the sysfs entries
created by amdgpu_gfx_sysfs_compute_init are specific to compute
partitions, which are only applicable on GFX9 and not on a VF in SR-IOV
mode.

By moving the check into amdgpu_gfx_sysfs_compute_init, we make this
function responsible for deciding whether or not to create the compute
partition sysfs entries.

This change improves the code organization and maintainability. If in
the future the  conditions for creating the compute partition sysfs
entries change, we  would only need to update the
amdgpu_gfx_sysfs_compute_init function.
This is not correct. If this has to be true, this will reside somewhere
in amdgpu_gfx and you would also need IP version inside this one. If for
a new IP version say gfx v9.4.5 this needs to be created for VF also,

In this case, how about below?

int amdgpu_gfx_sysfs_compute_init(struct amdgpu_device *adev, bool check_sriov)  
{  
    int r; 
 
    if (!check_sriov || !amdgpu_sriov_vf(adev)) { 
        r = device_create_file(adev->dev, &dev_attr_current_compute_partition); 
        if (r) 
            return r; 
 
        r = device_create_file(adev->dev, &dev_attr_available_compute_partition); 
        if (r) 
            return r; 
    } 
 
    return 0; 

In gfx_v9_4_3_sw_init you would call amdgpu_gfx_sysfs_compute_init(adev, true),

to perform the check, and in gfx_v9_4_5_sw_init you would call amdgpu_gfx_sysfs_compute_init(adev, false) to skip the check.

This way, we can control the behavior of the function without needing to put condition in IP code version.?

But would like have Christian's view also, onto this "a new IP version say gfx v9.4.5 this needs to be created for VF also,

"

then this check here won't work. This is the specific reason why we put
the conditions inside IP code.

Thanks,
Lijo

Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Cc: Christian König <christian.koenig@xxxxxxx>
Suggested-by: Christian König <christian.koenig@xxxxxxx>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 24 +++++++++++++++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h |  4 ++--
 drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c | 11 +++++------
 3 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 19b1817b55d7..72477a5aedca 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -1376,21 +1376,27 @@ 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)
+int amdgpu_gfx_sysfs_compute_init(struct amdgpu_device *adev)
 {
 	int r;
 
-	r = device_create_file(adev->dev, &dev_attr_current_compute_partition);
-	if (r)
-		return r;
+	if (!amdgpu_sriov_vf(adev)) {
+		r = device_create_file(adev->dev, &dev_attr_current_compute_partition);
+		if (r)
+			return r;
 
-	r = device_create_file(adev->dev, &dev_attr_available_compute_partition);
+		r = device_create_file(adev->dev, &dev_attr_available_compute_partition);
+		if (r)
+			return r;
+	}
 
-	return r;
+	return 0;
 }
 
-void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev)
+void amdgpu_gfx_sysfs_compute_fini(struct amdgpu_device *adev)
 {
-	device_remove_file(adev->dev, &dev_attr_current_compute_partition);
-	device_remove_file(adev->dev, &dev_attr_available_compute_partition);
+	if (!amdgpu_sriov_vf(adev)) {
+		device_remove_file(adev->dev, &dev_attr_current_compute_partition);
+		device_remove_file(adev->dev, &dev_attr_available_compute_partition);
+	}
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index 6b0416777c5b..b65c459b3aa9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -533,8 +533,8 @@ int amdgpu_gfx_poison_consumption_handler(struct amdgpu_device *adev,
 						struct amdgpu_iv_entry *entry);
 
 bool amdgpu_gfx_is_master_xcc(struct amdgpu_device *adev, int xcc_id);
-int amdgpu_gfx_sysfs_init(struct amdgpu_device *adev);
-void amdgpu_gfx_sysfs_fini(struct amdgpu_device *adev);
+int amdgpu_gfx_sysfs_compute_init(struct amdgpu_device *adev);
+void amdgpu_gfx_sysfs_compute_fini(struct amdgpu_device *adev);
 void amdgpu_gfx_ras_error_func(struct amdgpu_device *adev,
 		void *ras_error_status,
 		void (*func)(struct amdgpu_device *adev, void *ras_error_status,
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 aecc2bcea145..07ce614ef282 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_4_3.c
@@ -939,11 +939,11 @@ static int gfx_v9_4_3_sw_init(void *handle)
 	if (r)
 		return r;
 
+	r = amdgpu_gfx_sysfs_compute_init(adev);
+	if (r)
+		return r;
 
-	if (!amdgpu_sriov_vf(adev))
-		r = amdgpu_gfx_sysfs_init(adev);
-
-	return r;
+	return 0;
 }
 
 static int gfx_v9_4_3_sw_fini(void *handle)
@@ -964,8 +964,7 @@ static int gfx_v9_4_3_sw_fini(void *handle)
 	gfx_v9_4_3_mec_fini(adev);
 	amdgpu_bo_unref(&adev->gfx.rlc.clear_state_obj);
 	gfx_v9_4_3_free_microcode(adev);
-	if (!amdgpu_sriov_vf(adev))
-		amdgpu_gfx_sysfs_fini(adev);
+	amdgpu_gfx_sysfs_compute_fini(adev);
 
 	return 0;
 }

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

  Powered by Linux