RE: [PATCH] drm/amdgpu/smu: add an update table lock
Thanks. I went through that bug report. And it seems weird the table lock works
but msg lock does not considering if it was really caused by some race
conditions.
Considering the issue was found on multi monitors setup. Maybe mclk dpm is
related.
Is it possible to try with single monitor only? Or trying disabling mclk dpm?
-Original Message-
From: Alex Deucher
Sent: Tuesday, February 18, 2020 10:35 PM
To: Quan, Evan
Cc: amd-gfx@lists.freedesktop.org; Deucher, Alexander
Subject: Re: [PATCH] drm/amdgpu/smu: add an update table lock
On Mon, Feb 17, 2020 at 10:01 PM Quan, Evan wrote:
>
> Hi Alex,
>
> Did you seen any issue caused by this?
Seems to help on:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2Fissues%2F1047data=02%7C01%7CEvan.Quan%40amd.com%7C1266ea24bc2f4fff2cfb08d7b47fc095%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637176333114972316sdata=Lpts%2FYe%2Bq64ppyuzNIGWFYiGEXqzQVdAO2CiP6mSfFc%3Dreserved=0
I haven't been able to prove to myself that the existing high level locking
covers every case.
Alex
>
> Regards,
> Evan
> -Original Message-
> From: amd-gfx On Behalf Of
> Alex Deucher
> Sent: Tuesday, February 18, 2020 5:38 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander
> Subject: [PATCH] drm/amdgpu/smu: add an update table lock
>
> The driver uses a staging buffer to update tables in the SMU.
> Add a lock to make sure we don't try and do this concurrently by
> accident.
>
> Signed-off-by: Alex Deucher
> ---
> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 7 ++-
> drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 9f2428fd98f6..437a3e7b36b4 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -530,6 +530,7 @@ int smu_update_table(struct smu_context *smu, enum
> smu_table_id table_index, int
>
> table_size = smu_table->tables[table_index].size;
>
> + mutex_lock(>update_table_lock);
> if (drv2smu) {
> memcpy(table->cpu_addr, table_data, table_size);
> /*
> @@ -544,13 +545,16 @@ int smu_update_table(struct smu_context *smu, enum
> smu_table_id table_index, int
> SMU_MSG_TransferTableSmu2Dram,
> table_id | ((argument & 0x) <<
> 16));
> if (ret)
> - return ret;
> + goto unlock;
>
> if (!drv2smu) {
> amdgpu_asic_flush_hdp(adev, NULL);
> memcpy(table_data, table->cpu_addr, table_size);
> }
>
> +unlock:
> + mutex_unlock(>update_table_lock);
> +
> return ret;
> }
>
> @@ -900,6 +904,7 @@ static int smu_sw_init(void *handle)
>
> mutex_init(>sensor_lock);
> mutex_init(>metrics_lock);
> + mutex_init(>update_table_lock);
>
> smu->watermarks_bitmap = 0;
> smu->power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> index 97b6714e83e6..506288072e8e 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> @@ -362,6 +362,7 @@ struct smu_context
> struct mutexmutex;
> struct mutexsensor_lock;
> struct mutexmetrics_lock;
> + struct mutexupdate_table_lock;
> uint64_t pool_size;
>
> struct smu_table_contextsmu_table;
> --
> 2.24.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=02%7C01%7CEv
> an.Quan%40amd.com%7C1266ea24bc2f4fff2cfb08d7b47fc095%7C3dd8961fe4884e6
> 08e11a82d994e183d%7C0%7C0%7C637176333114972316sdata=JiABwHLa0eLLp
> yiwKXU4nSU28OXBuxTnRbisgoC4uK0%3Dreserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/smu: add an update table lock
On Mon, Feb 17, 2020 at 10:01 PM Quan, Evan wrote:
>
> Hi Alex,
>
> Did you seen any issue caused by this?
Seems to help on:
https://gitlab.freedesktop.org/drm/amd/issues/1047
I haven't been able to prove to myself that the existing high level
locking covers every case.
Alex
>
> Regards,
> Evan
> -Original Message-
> From: amd-gfx On Behalf Of Alex
> Deucher
> Sent: Tuesday, February 18, 2020 5:38 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander
> Subject: [PATCH] drm/amdgpu/smu: add an update table lock
>
> The driver uses a staging buffer to update tables in the SMU.
> Add a lock to make sure we don't try and do this concurrently
> by accident.
>
> Signed-off-by: Alex Deucher
> ---
> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 7 ++-
> drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 9f2428fd98f6..437a3e7b36b4 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -530,6 +530,7 @@ int smu_update_table(struct smu_context *smu, enum
> smu_table_id table_index, int
>
> table_size = smu_table->tables[table_index].size;
>
> + mutex_lock(>update_table_lock);
> if (drv2smu) {
> memcpy(table->cpu_addr, table_data, table_size);
> /*
> @@ -544,13 +545,16 @@ int smu_update_table(struct smu_context *smu, enum
> smu_table_id table_index, int
> SMU_MSG_TransferTableSmu2Dram,
> table_id | ((argument & 0x) <<
> 16));
> if (ret)
> - return ret;
> + goto unlock;
>
> if (!drv2smu) {
> amdgpu_asic_flush_hdp(adev, NULL);
> memcpy(table_data, table->cpu_addr, table_size);
> }
>
> +unlock:
> + mutex_unlock(>update_table_lock);
> +
> return ret;
> }
>
> @@ -900,6 +904,7 @@ static int smu_sw_init(void *handle)
>
> mutex_init(>sensor_lock);
> mutex_init(>metrics_lock);
> + mutex_init(>update_table_lock);
>
> smu->watermarks_bitmap = 0;
> smu->power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> index 97b6714e83e6..506288072e8e 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> @@ -362,6 +362,7 @@ struct smu_context
> struct mutexmutex;
> struct mutexsensor_lock;
> struct mutexmetrics_lock;
> + struct mutexupdate_table_lock;
> uint64_t pool_size;
>
> struct smu_table_contextsmu_table;
> --
> 2.24.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=02%7C01%7Cevan.quan%40amd.com%7Cbdb09232f91649c08af408d7b3f1ab89%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175722941118202sdata=2%2FNzcPGthFtelyfXNiiIL3BV3c%2Bvoy%2F2Cq1oFWuZ4%2Bc%3Dreserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu/smu: add an update table lock
Hi Alex,
Did you seen any issue caused by this?
Regards,
Evan
-Original Message-
From: amd-gfx On Behalf Of Alex Deucher
Sent: Tuesday, February 18, 2020 5:38 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander
Subject: [PATCH] drm/amdgpu/smu: add an update table lock
The driver uses a staging buffer to update tables in the SMU.
Add a lock to make sure we don't try and do this concurrently
by accident.
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 7 ++-
drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 9f2428fd98f6..437a3e7b36b4 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -530,6 +530,7 @@ int smu_update_table(struct smu_context *smu, enum
smu_table_id table_index, int
table_size = smu_table->tables[table_index].size;
+ mutex_lock(>update_table_lock);
if (drv2smu) {
memcpy(table->cpu_addr, table_data, table_size);
/*
@@ -544,13 +545,16 @@ int smu_update_table(struct smu_context *smu, enum
smu_table_id table_index, int
SMU_MSG_TransferTableSmu2Dram,
table_id | ((argument & 0x) <<
16));
if (ret)
- return ret;
+ goto unlock;
if (!drv2smu) {
amdgpu_asic_flush_hdp(adev, NULL);
memcpy(table_data, table->cpu_addr, table_size);
}
+unlock:
+ mutex_unlock(>update_table_lock);
+
return ret;
}
@@ -900,6 +904,7 @@ static int smu_sw_init(void *handle)
mutex_init(>sensor_lock);
mutex_init(>metrics_lock);
+ mutex_init(>update_table_lock);
smu->watermarks_bitmap = 0;
smu->power_profile_mode = PP_SMC_POWER_PROFILE_BOOTUP_DEFAULT;
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index 97b6714e83e6..506288072e8e 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -362,6 +362,7 @@ struct smu_context
struct mutexmutex;
struct mutexsensor_lock;
struct mutexmetrics_lock;
+ struct mutexupdate_table_lock;
uint64_t pool_size;
struct smu_table_contextsmu_table;
--
2.24.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=02%7C01%7Cevan.quan%40amd.com%7Cbdb09232f91649c08af408d7b3f1ab89%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175722941118202sdata=2%2FNzcPGthFtelyfXNiiIL3BV3c%2Bvoy%2F2Cq1oFWuZ4%2Bc%3Dreserved=0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
