[PATCH] drm/amd/powerplay: add smu message mutex

[PATCH] drm/amd/powerplay: add smu message mutex

[Xiao, Jack]

Add smu message mutex preventing against race condition issue.

Signed-off-by: Jack Xiao <Jack.Xiao@amd.com>
---
 drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
 drivers/gpu/drm/amd/powerplay/smu_v11_0.c      | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index 3eb1de9..735233e 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -374,6 +374,7 @@ struct smu_context
 	const struct smu_funcs		*funcs;
 	const struct pptable_funcs	*ppt_funcs;
 	struct mutex			mutex;
+	struct mutex			msg_mutex;
 	uint64_t pool_size;
 
 	struct smu_table_context	smu_table;
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
index d2eeb62..de737a0 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
@@ -104,6 +104,8 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	if (index < 0)
 		return index;
 
+	mutex_lock(&smu->msg_mutex);
+
 	smu_v11_0_wait_for_response(smu);
 
 	WREG32_SOC15(MP1, 0, mmMP1_SMN_C2PMSG_90, 0);
@@ -111,11 +113,11 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	smu_v11_0_send_msg_without_waiting(smu, (uint16_t)index);
 
 	ret = smu_v11_0_wait_for_response(smu);
-
 	if (ret)
 		pr_err("Failed to send message 0x%x, response 0x%x\n", index,
 		       ret);
 
+	mutex_unlock(&smu->msg_mutex);
 	return ret;
 
 }
@@ -132,6 +134,8 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	if (index < 0)
 		return index;
 
+	mutex_lock(&smu->msg_mutex);
+
 	ret = smu_v11_0_wait_for_response(smu);
 	if (ret)
 		pr_err("Failed to send message 0x%x, response 0x%x, param 0x%x\n",
@@ -148,6 +152,7 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 		pr_err("Failed to send message 0x%x, response 0x%x param 0x%x\n",
 		       index, ret, param);
 
+	mutex_unlock(&smu->msg_mutex);
 	return ret;
 }
 
-- 
1.9.1

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

RE: [PATCH] drm/amd/powerplay: add smu message mutex

[Xiao, Jack]

Drop it due to a modification was missing.
A new patch was sent out.

-----Original Message-----
From: Xiao, Jack <Jack.Xiao@amd.com> 
Sent: Monday, June 03, 2019 2:12 PM
To: amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>
Cc: Xiao, Jack <Jack.Xiao@amd.com>
Subject: [PATCH] drm/amd/powerplay: add smu message mutex

Add smu message mutex preventing against race condition issue.

Signed-off-by: Jack Xiao <Jack.Xiao@amd.com>
---
 drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
 drivers/gpu/drm/amd/powerplay/smu_v11_0.c      | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index 3eb1de9..735233e 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -374,6 +374,7 @@ struct smu_context
 	const struct smu_funcs		*funcs;
 	const struct pptable_funcs	*ppt_funcs;
 	struct mutex			mutex;
+	struct mutex			msg_mutex;
 	uint64_t pool_size;
 
 	struct smu_table_context	smu_table;
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
index d2eeb62..de737a0 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
@@ -104,6 +104,8 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	if (index < 0)
 		return index;
 
+	mutex_lock(&smu->msg_mutex);
+
 	smu_v11_0_wait_for_response(smu);
 
 	WREG32_SOC15(MP1, 0, mmMP1_SMN_C2PMSG_90, 0); @@ -111,11 +113,11 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	smu_v11_0_send_msg_without_waiting(smu, (uint16_t)index);
 
 	ret = smu_v11_0_wait_for_response(smu);
-
 	if (ret)
 		pr_err("Failed to send message 0x%x, response 0x%x\n", index,
 		       ret);
 
+	mutex_unlock(&smu->msg_mutex);
 	return ret;
 
 }
@@ -132,6 +134,8 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	if (index < 0)
 		return index;
 
+	mutex_lock(&smu->msg_mutex);
+
 	ret = smu_v11_0_wait_for_response(smu);
 	if (ret)
 		pr_err("Failed to send message 0x%x, response 0x%x, param 0x%x\n", @@ -148,6 +152,7 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 		pr_err("Failed to send message 0x%x, response 0x%x param 0x%x\n",
 		       index, ret, param);
 
+	mutex_unlock(&smu->msg_mutex);
 	return ret;
 }
 
--
1.9.1

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

Re: [PATCH] drm/amd/powerplay: add smu message mutex

[Wang, Kevin(Yang)]

[-- Attachment #1.1: Type: text/plain, Size: 5190 bytes --]


________________________________
From: Huang, Ray
Sent: Tuesday, June 4, 2019 1:37 PM
To: Xiao, Jack; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Deucher, Alexander; Zhang, Hawking
Cc: Xiao, Jack; Wang, Kevin(Yang); Quan, Evan; Gui, Jack; Gao, Likun
Subject: RE: [PATCH] drm/amd/powerplay: add smu message mutex

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> On Behalf Of Xiao,
> Jack
> Sent: Monday, June 03, 2019 3:02 PM
> To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; Deucher, Alexander
> <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>; Zhang, Hawking
> <Hawking.Zhang-5C7GfCeVMHo@public.gmane.org>
> Cc: Xiao, Jack <Jack.Xiao-5C7GfCeVMHo@public.gmane.org>
> Subject: [PATCH] drm/amd/powerplay: add smu message mutex
>
> Add smu message mutex preventing against race condition issue.
>
> Signed-off-by: Jack Xiao <Jack.Xiao-5C7GfCeVMHo@public.gmane.org>
> ---
>  drivers/gpu/drm/amd/powerplay/amdgpu_smu.c     | 1 +
>  drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
>  drivers/gpu/drm/amd/powerplay/smu_v11_0.c      | 7 ++++++-
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 3026c7e..db2bbec 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -350,6 +350,7 @@ static int smu_early_init(void *handle)
>        smu->adev = adev;
>        smu->pm_enabled = !!amdgpu_dpm;
>        mutex_init(&smu->mutex);
> +     mutex_init(&smu->msg_mutex);

As talked with you, we need use smu->mutex to protect the context in the thread instead of introducing the specific mutex of messages. Because msg_mutex cannot protect the case of multi-message pairing. And yes, this is the key issue of swSMU so far.

+ Linux power folks,
Kevin, could you please use the smu->mutex to protect below callbacks which will be called from gpu_info ioctl.

amdgpu_dpm_get_sclk
amdgpu_dpm_get_mclk

And we need smu->mutex to protect the smu_dpm_set_uvd_enable/ smu_dpm_set_vce_enable as well, because they will be called during VCN command submissions. We should look over all ioctl/sysfs interface in the driver, they all need the mutex.

Thanks,
Ray

[Kevin]

Hi Ray,

i think we should add msg_mutex lock to protect smu message function and regsiter change.
more elaborate locks should be used to protect this critical resource and reduce the probability of mutual exclusion.

Hi Jack,

The patch is
Reviewed-by: Kevin Wang <kevin1.wang-5C7GfCeVMHo@public.gmane.org>

Thanks,
Kevin

>
>        return smu_set_funcs(adev);
>  }
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> index 3eb1de9..735233e 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> @@ -374,6 +374,7 @@ struct smu_context
>        const struct smu_funcs          *funcs;
>        const struct pptable_funcs      *ppt_funcs;
>        struct mutex                    mutex;
> +     struct mutex                    msg_mutex;
>        uint64_t pool_size;
>
>        struct smu_table_context        smu_table;
> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> index d2eeb62..de737a0 100644
> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> @@ -104,6 +104,8 @@ static int smu_v11_0_send_msg(struct smu_context
> *smu, uint16_t msg)
>        if (index < 0)
>                return index;
>
> +     mutex_lock(&smu->msg_mutex);
> +
>        smu_v11_0_wait_for_response(smu);
>
>        WREG32_SOC15(MP1, 0, mmMP1_SMN_C2PMSG_90, 0); @@ -
> 111,11 +113,11 @@ static int smu_v11_0_send_msg(struct smu_context
> *smu, uint16_t msg)
>        smu_v11_0_send_msg_without_waiting(smu, (uint16_t)index);
>
>        ret = smu_v11_0_wait_for_response(smu);
> -
>        if (ret)
>                pr_err("Failed to send message 0x%x, response 0x%x\n",
> index,
>                       ret);
>
> +     mutex_unlock(&smu->msg_mutex);
>        return ret;
>
>  }
> @@ -132,6 +134,8 @@ static int smu_v11_0_send_msg(struct smu_context
> *smu, uint16_t msg)
>        if (index < 0)
>                return index;
>
> +     mutex_lock(&smu->msg_mutex);
> +
>        ret = smu_v11_0_wait_for_response(smu);
>        if (ret)
>                pr_err("Failed to send message 0x%x, response 0x%x, param
> 0x%x\n", @@ -148,6 +152,7 @@ static int smu_v11_0_send_msg(struct
> smu_context *smu, uint16_t msg)
>                pr_err("Failed to send message 0x%x, response 0x%x param
> 0x%x\n",
>                       index, ret, param);
>
> +     mutex_unlock(&smu->msg_mutex);
>        return ret;
>  }
>
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[-- Attachment #1.2: Type: text/html, Size: 9500 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

RE: [PATCH] drm/amd/powerplay: add smu message mutex

[Huang, Ray]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Xiao,
> Jack
> Sent: Monday, June 03, 2019 3:02 PM
> To: amd-gfx@lists.freedesktop.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Zhang, Hawking
> <Hawking.Zhang@amd.com>
> Cc: Xiao, Jack <Jack.Xiao@amd.com>
> Subject: [PATCH] drm/amd/powerplay: add smu message mutex
> 
> Add smu message mutex preventing against race condition issue.
> 
> Signed-off-by: Jack Xiao <Jack.Xiao@amd.com>
> ---
>  drivers/gpu/drm/amd/powerplay/amdgpu_smu.c     | 1 +
>  drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
>  drivers/gpu/drm/amd/powerplay/smu_v11_0.c      | 7 ++++++-
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 3026c7e..db2bbec 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -350,6 +350,7 @@ static int smu_early_init(void *handle)
>  	smu->adev = adev;
>  	smu->pm_enabled = !!amdgpu_dpm;
>  	mutex_init(&smu->mutex);
> +	mutex_init(&smu->msg_mutex);

As talked with you, we need use smu->mutex to protect the context in the thread instead of introducing the specific mutex of messages. Because msg_mutex cannot protect the case of multi-message pairing. And yes, this is the key issue of swSMU so far.

+ Linux power folks, 
Kevin, could you please use the smu->mutex to protect below callbacks which will be called from gpu_info ioctl. 

amdgpu_dpm_get_sclk
amdgpu_dpm_get_mclk

And we need smu->mutex to protect the smu_dpm_set_uvd_enable/ smu_dpm_set_vce_enable as well, because they will be called during VCN command submissions. We should look over all ioctl/sysfs interface in the driver, they all need the mutex.

Thanks,
Ray

> 
>  	return smu_set_funcs(adev);
>  }
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> index 3eb1de9..735233e 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> @@ -374,6 +374,7 @@ struct smu_context
>  	const struct smu_funcs		*funcs;
>  	const struct pptable_funcs	*ppt_funcs;
>  	struct mutex			mutex;
> +	struct mutex			msg_mutex;
>  	uint64_t pool_size;
> 
>  	struct smu_table_context	smu_table;
> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> index d2eeb62..de737a0 100644
> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> @@ -104,6 +104,8 @@ static int smu_v11_0_send_msg(struct smu_context
> *smu, uint16_t msg)
>  	if (index < 0)
>  		return index;
> 
> +	mutex_lock(&smu->msg_mutex);
> +
>  	smu_v11_0_wait_for_response(smu);
> 
>  	WREG32_SOC15(MP1, 0, mmMP1_SMN_C2PMSG_90, 0); @@ -
> 111,11 +113,11 @@ static int smu_v11_0_send_msg(struct smu_context
> *smu, uint16_t msg)
>  	smu_v11_0_send_msg_without_waiting(smu, (uint16_t)index);
> 
>  	ret = smu_v11_0_wait_for_response(smu);
> -
>  	if (ret)
>  		pr_err("Failed to send message 0x%x, response 0x%x\n",
> index,
>  		       ret);
> 
> +	mutex_unlock(&smu->msg_mutex);
>  	return ret;
> 
>  }
> @@ -132,6 +134,8 @@ static int smu_v11_0_send_msg(struct smu_context
> *smu, uint16_t msg)
>  	if (index < 0)
>  		return index;
> 
> +	mutex_lock(&smu->msg_mutex);
> +
>  	ret = smu_v11_0_wait_for_response(smu);
>  	if (ret)
>  		pr_err("Failed to send message 0x%x, response 0x%x, param
> 0x%x\n", @@ -148,6 +152,7 @@ static int smu_v11_0_send_msg(struct
> smu_context *smu, uint16_t msg)
>  		pr_err("Failed to send message 0x%x, response 0x%x param
> 0x%x\n",
>  		       index, ret, param);
> 
> +	mutex_unlock(&smu->msg_mutex);
>  	return ret;
>  }
> 
> --
> 1.9.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/powerplay: add smu message mutex

[Xiao, Jack]

Add smu message mutex preventing against race condition issue.

Signed-off-by: Jack Xiao <Jack.Xiao@amd.com>
---
 drivers/gpu/drm/amd/powerplay/amdgpu_smu.c     | 1 +
 drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 1 +
 drivers/gpu/drm/amd/powerplay/smu_v11_0.c      | 7 ++++++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 3026c7e..db2bbec 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -350,6 +350,7 @@ static int smu_early_init(void *handle)
 	smu->adev = adev;
 	smu->pm_enabled = !!amdgpu_dpm;
 	mutex_init(&smu->mutex);
+	mutex_init(&smu->msg_mutex);
 
 	return smu_set_funcs(adev);
 }
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
index 3eb1de9..735233e 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -374,6 +374,7 @@ struct smu_context
 	const struct smu_funcs		*funcs;
 	const struct pptable_funcs	*ppt_funcs;
 	struct mutex			mutex;
+	struct mutex			msg_mutex;
 	uint64_t pool_size;
 
 	struct smu_table_context	smu_table;
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
index d2eeb62..de737a0 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
@@ -104,6 +104,8 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	if (index < 0)
 		return index;
 
+	mutex_lock(&smu->msg_mutex);
+
 	smu_v11_0_wait_for_response(smu);
 
 	WREG32_SOC15(MP1, 0, mmMP1_SMN_C2PMSG_90, 0);
@@ -111,11 +113,11 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	smu_v11_0_send_msg_without_waiting(smu, (uint16_t)index);
 
 	ret = smu_v11_0_wait_for_response(smu);
-
 	if (ret)
 		pr_err("Failed to send message 0x%x, response 0x%x\n", index,
 		       ret);
 
+	mutex_unlock(&smu->msg_mutex);
 	return ret;
 
 }
@@ -132,6 +134,8 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 	if (index < 0)
 		return index;
 
+	mutex_lock(&smu->msg_mutex);
+
 	ret = smu_v11_0_wait_for_response(smu);
 	if (ret)
 		pr_err("Failed to send message 0x%x, response 0x%x, param 0x%x\n",
@@ -148,6 +152,7 @@ static int smu_v11_0_send_msg(struct smu_context *smu, uint16_t msg)
 		pr_err("Failed to send message 0x%x, response 0x%x param 0x%x\n",
 		       index, ret, param);
 
+	mutex_unlock(&smu->msg_mutex);
 	return ret;
 }
 
-- 
1.9.1

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