amd-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu/smu: add an update table lock
@ 2020-02-17 21:37 Alex Deucher
  2020-02-18  3:01 ` Quan, Evan
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Deucher @ 2020-02-17 21:37 UTC (permalink / raw)
  To: amd-gfx; +Cc: Alex Deucher

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 <alexander.deucher@amd.com>
---
 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(&smu->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 & 0xFFFF) << 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(&smu->update_table_lock);
+
 	return ret;
 }
 
@@ -900,6 +904,7 @@ static int smu_sw_init(void *handle)
 
 	mutex_init(&smu->sensor_lock);
 	mutex_init(&smu->metrics_lock);
+	mutex_init(&smu->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 mutex			mutex;
 	struct mutex			sensor_lock;
 	struct mutex			metrics_lock;
+	struct mutex			update_table_lock;
 	uint64_t pool_size;
 
 	struct smu_table_context	smu_table;
-- 
2.24.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* RE: [PATCH] drm/amdgpu/smu: add an update table lock
  2020-02-17 21:37 [PATCH] drm/amdgpu/smu: add an update table lock Alex Deucher
@ 2020-02-18  3:01 ` Quan, Evan
  2020-02-18 14:34   ` Alex Deucher
  0 siblings, 1 reply; 4+ messages in thread
From: Quan, Evan @ 2020-02-18  3:01 UTC (permalink / raw)
  To: Alex Deucher, amd-gfx; +Cc: Deucher, Alexander

Hi Alex,

Did you seen any issue caused by this?

Regards,
Evan
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex Deucher
Sent: Tuesday, February 18, 2020 5:38 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
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 <alexander.deucher@amd.com>
---
 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(&smu->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 & 0xFFFF) << 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(&smu->update_table_lock);
+
 	return ret;
 }
 
@@ -900,6 +904,7 @@ static int smu_sw_init(void *handle)
 
 	mutex_init(&smu->sensor_lock);
 	mutex_init(&smu->metrics_lock);
+	mutex_init(&smu->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 mutex			mutex;
 	struct mutex			sensor_lock;
 	struct mutex			metrics_lock;
+	struct mutex			update_table_lock;
 	uint64_t pool_size;
 
 	struct smu_table_context	smu_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-gfx&amp;data=02%7C01%7Cevan.quan%40amd.com%7Cbdb09232f91649c08af408d7b3f1ab89%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175722941118202&amp;sdata=2%2FNzcPGthFtelyfXNiiIL3BV3c%2Bvoy%2F2Cq1oFWuZ4%2Bc%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/amdgpu/smu: add an update table lock
  2020-02-18  3:01 ` Quan, Evan
@ 2020-02-18 14:34   ` Alex Deucher
  2020-02-19 10:43     ` Quan, Evan
  0 siblings, 1 reply; 4+ messages in thread
From: Alex Deucher @ 2020-02-18 14:34 UTC (permalink / raw)
  To: Quan, Evan; +Cc: Deucher, Alexander, amd-gfx

On Mon, Feb 17, 2020 at 10:01 PM Quan, Evan <Evan.Quan@amd.com> 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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex Deucher
> Sent: Tuesday, February 18, 2020 5:38 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> 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 <alexander.deucher@amd.com>
> ---
>  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(&smu->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 & 0xFFFF) << 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(&smu->update_table_lock);
> +
>         return ret;
>  }
>
> @@ -900,6 +904,7 @@ static int smu_sw_init(void *handle)
>
>         mutex_init(&smu->sensor_lock);
>         mutex_init(&smu->metrics_lock);
> +       mutex_init(&smu->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 mutex                    mutex;
>         struct mutex                    sensor_lock;
>         struct mutex                    metrics_lock;
> +       struct mutex                    update_table_lock;
>         uint64_t pool_size;
>
>         struct smu_table_context        smu_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-gfx&amp;data=02%7C01%7Cevan.quan%40amd.com%7Cbdb09232f91649c08af408d7b3f1ab89%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637175722941118202&amp;sdata=2%2FNzcPGthFtelyfXNiiIL3BV3c%2Bvoy%2F2Cq1oFWuZ4%2Bc%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread
* RE: [PATCH] drm/amdgpu/smu: add an update table lock
  2020-02-18 14:34   ` Alex Deucher
@ 2020-02-19 10:43     ` Quan, Evan
  0 siblings, 0 replies; 4+ messages in thread
From: Quan, Evan @ 2020-02-19 10:43 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Deucher, Alexander, amd-gfx

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 <alexdeucher@gmail.com> 
Sent: Tuesday, February 18, 2020 10:35 PM
To: Quan, Evan <Evan.Quan@amd.com>
Cc: amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>
Subject: Re: [PATCH] drm/amdgpu/smu: add an update table lock

On Mon, Feb 17, 2020 at 10:01 PM Quan, Evan <Evan.Quan@amd.com> 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%2F1047&amp;data=02%7C01%7CEvan.Quan%40amd.com%7C1266ea24bc2f4fff2cfb08d7b47fc095%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637176333114972316&amp;sdata=Lpts%2FYe%2Bq64ppyuzNIGWFYiGEXqzQVdAO2CiP6mSfFc%3D&amp;reserved=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 <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of 
> Alex Deucher
> Sent: Tuesday, February 18, 2020 5:38 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> 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 <alexander.deucher@amd.com>
> ---
>  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(&smu->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 & 0xFFFF) << 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(&smu->update_table_lock);
> +
>         return ret;
>  }
>
> @@ -900,6 +904,7 @@ static int smu_sw_init(void *handle)
>
>         mutex_init(&smu->sensor_lock);
>         mutex_init(&smu->metrics_lock);
> +       mutex_init(&smu->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 mutex                    mutex;
>         struct mutex                    sensor_lock;
>         struct mutex                    metrics_lock;
> +       struct mutex                    update_table_lock;
>         uint64_t pool_size;
>
>         struct smu_table_context        smu_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-gfx&amp;data=02%7C01%7CEv
> an.Quan%40amd.com%7C1266ea24bc2f4fff2cfb08d7b47fc095%7C3dd8961fe4884e6
> 08e11a82d994e183d%7C0%7C0%7C637176333114972316&amp;sdata=JiABwHLa0eLLp
> yiwKXU4nSU28OXBuxTnRbisgoC4uK0%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-19 10:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-17 21:37 [PATCH] drm/amdgpu/smu: add an update table lock Alex Deucher
2020-02-18  3:01 ` Quan, Evan
2020-02-18 14:34   ` Alex Deucher
2020-02-19 10:43     ` Quan, Evan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).