On 10/10/2024 1:42 PM, Lazar, Lijo wrote:
On 10/10/2024 1:13 PM, Christian König wrote:
Am 09.10.24 um 16:24 schrieb Sunil Khatri:
Before making a function call to suspend, validate
the function pointer like we do in sw_init.
Signed-off-by: Sunil Khatri <sunil.khatri@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/aldebaran.c | 15 ++++++------
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 26 ++++++++++++---------
drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 12 ++++++----
drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c | 15 ++++++------
drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c | 15 ++++++------
5 files changed, 46 insertions(+), 37 deletions(-)
Original patch series is somehow missing in my inbox, hence posting it
here.
Below ones are SOC specific files where we know those blocks implement
the suspend sequence. If they are taken out, then that needs to generate
attention. But this check will cause a silent skip.
Hello Lijo,
I have not idea why the series has not reached you in first time.
Second i did not get your point clearly, I am just cleaning up code
which just return 0 and doing nothing else. I guess those suspend/resume
functions were expected to be implemented but right
now its empty and just an additional call in stack doing nothing and
hence needed cleanup.
Regards
Sunil Khatri
aldebaran.c
sienna_cichlid.c
smu_v13_0_10.c
Thanks,
Lijo
diff --git a/drivers/gpu/drm/amd/amdgpu/aldebaran.c
b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
index c1ff24335a0c..e55d680d95ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/aldebaran.c
+++ b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
@@ -85,13 +85,14 @@ static int aldebaran_mode2_suspend_ip(struct
amdgpu_device *adev)
AMD_IP_BLOCK_TYPE_SDMA))
continue;
- r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
-
- if (r) {
- dev_err(adev->dev,
- "suspend of IP block <%s> failed %d\n",
- adev->ip_blocks[i].version->funcs->name, r);
- return r;
+ if (adev->ip_blocks[i].version->funcs->suspend) {
+ r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
+ if (r) {
+ dev_err(adev->dev,
+ "suspend of IP block <%s> failed %d\n",
+ adev->ip_blocks[i].version->funcs->name, r);
+ return r;
+ }
I think we should probably create a function for that code and error
message repeated a number of times. Same for the resume function.
Apart from that the whole set looks good to me.
Regards,
Christian.
}
adev->ip_blocks[i].status.hw = false;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 340141a74c12..51607ac8adb9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3471,12 +3471,14 @@ static int
amdgpu_device_ip_suspend_phase1(struct amdgpu_device *adev)
continue;
/* XXX handle errors */
- r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
- /* XXX handle errors */
- if (r) {
- DRM_ERROR("suspend of IP block <%s> failed %d\n",
- adev->ip_blocks[i].version->funcs->name, r);
- return r;
+ if (adev->ip_blocks[i].version->funcs->suspend) {
+ r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
+ /* XXX handle errors */
+ if (r) {
+ DRM_ERROR("suspend of IP block <%s> failed %d\n",
+ adev->ip_blocks[i].version->funcs->name, r);
+ return r;
+ }
}
adev->ip_blocks[i].status.hw = false;
@@ -3553,11 +3555,13 @@ static int
amdgpu_device_ip_suspend_phase2(struct amdgpu_device *adev)
continue;
/* XXX handle errors */
- r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
- /* XXX handle errors */
- if (r) {
- DRM_ERROR("suspend of IP block <%s> failed %d\n",
- adev->ip_blocks[i].version->funcs->name, r);
+ if (adev->ip_blocks[i].version->funcs->suspend) {
+ r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
+ /* XXX handle errors */
+ if (r) {
+ DRM_ERROR("suspend of IP block <%s> failed %d\n",
+ adev->ip_blocks[i].version->funcs->name, r);
+ }
}
adev->ip_blocks[i].status.hw = false;
/* handle putting the SMC in the appropriate state */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
index 3e2724590dbf..6bc75aa1c3b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c
@@ -40,11 +40,13 @@ static int
amdgpu_reset_xgmi_reset_on_init_suspend(struct amdgpu_device *adev)
continue;
/* XXX handle errors */
- r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
- /* XXX handle errors */
- if (r) {
- dev_err(adev->dev, "suspend of IP block <%s> failed %d",
- adev->ip_blocks[i].version->funcs->name, r);
+ if (adev->ip_blocks[i].version->funcs->suspend) {
+ r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
+ /* XXX handle errors */
+ if (r) {
+ dev_err(adev->dev, "suspend of IP block <%s> failed %d",
+ adev->ip_blocks[i].version->funcs->name, r);
+ }
}
adev->ip_blocks[i].status.hw = false;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
b/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
index 475b7df3a908..10dece12509f 100644
--- a/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
+++ b/drivers/gpu/drm/amd/amdgpu/sienna_cichlid.c
@@ -81,13 +81,14 @@ static int sienna_cichlid_mode2_suspend_ip(struct
amdgpu_device *adev)
AMD_IP_BLOCK_TYPE_SDMA))
continue;
- r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
-
- if (r) {
- dev_err(adev->dev,
- "suspend of IP block <%s> failed %d\n",
- adev->ip_blocks[i].version->funcs->name, r);
- return r;
+ if (adev->ip_blocks[i].version->funcs->suspend) {
+ r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
+ if (r) {
+ dev_err(adev->dev,
+ "suspend of IP block <%s> failed %d\n",
+ adev->ip_blocks[i].version->funcs->name, r);
+ return r;
+ }
}
adev->ip_blocks[i].status.hw = false;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
b/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
index 5ea9090b5040..ab049f0b4d39 100644
--- a/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
+++ b/drivers/gpu/drm/amd/amdgpu/smu_v13_0_10.c
@@ -80,13 +80,14 @@ static int smu_v13_0_10_mode2_suspend_ip(struct
amdgpu_device *adev)
AMD_IP_BLOCK_TYPE_MES))
continue;
- r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
-
- if (r) {
- dev_err(adev->dev,
- "suspend of IP block <%s> failed %d\n",
- adev->ip_blocks[i].version->funcs->name, r);
- return r;
+ if (adev->ip_blocks[i].version->funcs->suspend) {
+ r =
adev->ip_blocks[i].version->funcs->suspend(&adev->ip_blocks[i]);
+ if (r) {
+ dev_err(adev->dev,
+ "suspend of IP block <%s> failed %d\n",
+ adev->ip_blocks[i].version->funcs->name, r);
+ return r;
+ }
}
adev->ip_blocks[i].status.hw = false;
}
