Re: [PATCH v5 02/12] drm/amdgpu: add helper function amdgpu_ip_block_suspend

["Khatri, Sunil" <sunil.khatri@xxxxxxx>]

On 10/18/2024 4:40 PM, Christian König wrote:

Am 17.10.24 um 18:25 schrieb Sunil Khatri:

Use the helper function amdgpu_ip_block_suspend where
same checks and calls are repeated.

I strongly suggest to squash this patch and the next one together.

Sure. Noted

Signed-off-by: Sunil Khatri <sunil.khatri@xxxxxxx>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 48c9b9b06905..df57efa019ba 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -364,6 +364,7 @@ int amdgpu_device_ip_wait_for_idle(struct amdgpu_device *adev,
 				   enum amd_ip_block_type block_type);
 bool amdgpu_device_ip_is_valid(struct amdgpu_device *adev,
 			      enum amd_ip_block_type block_type);
+int amdgpu_ip_block_suspend(struct amdgpu_ip_block *ip_block);
 
 #define AMDGPU_MAX_IP_NUM 16
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index b7277bef7463..f69aba68e7b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -272,6 +272,23 @@ void amdgpu_reg_state_sysfs_fini(struct amdgpu_device *adev)
 	sysfs_remove_bin_file(&adev->dev->kobj, &bin_attr_reg_state);
 }
 
+int amdgpu_ip_block_suspend(struct amdgpu_ip_block *ip_block)
+{
+	int r;
+
+	if (ip_block->version->funcs->suspend) {
+		r = ip_block->version->funcs->suspend(ip_block);
+		if (r) {
+			dev_err(ip_block->adev->dev,
+				"suspend of IP block <%s> failed %d\n",
+				ip_block->version->funcs->name, r);
+			return r;
+		}
+	}

Please add "ip_blocks->status.hw = false;" and remove that from the callers as well. I did not add that in the amdgpu_ip_block_suspend as on failure we do return without setting status.hw=false. In case suspend fail and we return then in that case as per the current logic
we dont set that flag. And with the current logic that is maintained.

Apart from that looks good to me, Christian.

+
+	return 0;
+}
+
 /**
  * DOC: board_info
  *