[PATCH 1/2] Revert "drm/amdgpu: fix compiler warnings for df perfmons"

[PATCH 1/2] Revert "drm/amdgpu: fix compiler warnings for df perfmons"

[Kim, Jonathan]

This reverts commit 7dd2eb31fcd564574e8efea6bf23cf504f9e2fd7.

Signed-off-by: Jonathan Kim <Jonathan.Kim@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
index e1cf7e9c616a..f403c62c944e 100644
--- a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
+++ b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
@@ -93,7 +93,7 @@ const struct attribute_group *df_v3_6_attr_groups[] = {
 		NULL
 };
 
-static int df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow)
+static df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow)
 {
 	int r = 0;
 
@@ -546,7 +546,7 @@ static void df_v3_6_pmc_get_count(struct amdgpu_device *adev,
 				  uint64_t config,
 				  uint64_t *count)
 {
-	uint32_t lo_base_addr, hi_base_addr, lo_val = 0, hi_val = 0;
+	uint32_t lo_base_addr, hi_base_addr, lo_val, hi_val;
 	*count = 0;
 
 	switch (adev->asic_type) {
-- 
2.17.1

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

[PATCH 2/2] Revert "drm/amdgpu: disable c-states on xgmi perfmons"

[Kim, Jonathan]

This reverts commit 54275cd1649f4034c6450b6c5a8358fcd4f7dda6.

Signed-off-by: Jonathan Kim <Jonathan.Kim@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 36 +---------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
index f403c62c944e..16fbd2bc8ad1 100644
--- a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
+++ b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
@@ -93,21 +93,6 @@ const struct attribute_group *df_v3_6_attr_groups[] = {
 		NULL
 };
 
-static df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow)
-{
-	int r = 0;
-
-	if (is_support_sw_smu(adev)) {
-		r = smu_set_df_cstate(&adev->smu, allow);
-	} else if (adev->powerplay.pp_funcs
-			&& adev->powerplay.pp_funcs->set_df_cstate) {
-		r = adev->powerplay.pp_funcs->set_df_cstate(
-			adev->powerplay.pp_handle, allow);
-	}
-
-	return r;
-}
-
 static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
 				 uint32_t ficaa_val)
 {
@@ -117,9 +102,6 @@ static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
-		return 0xFFFFFFFFFFFFFFFF;
-
 	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
 	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessAddress3);
 	WREG32(data, ficaa_val);
@@ -132,8 +114,6 @@ static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
 
 	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
 
-	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
-
 	return (((ficadh_val & 0xFFFFFFFFFFFFFFFF) << 32) | ficadl_val);
 }
 
@@ -145,9 +125,6 @@ static void df_v3_6_set_fica(struct amdgpu_device *adev, uint32_t ficaa_val,
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
-		return;
-
 	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
 	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessAddress3);
 	WREG32(data, ficaa_val);
@@ -157,9 +134,8 @@ static void df_v3_6_set_fica(struct amdgpu_device *adev, uint32_t ficaa_val,
 
 	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessDataHi3);
 	WREG32(data, ficadh_val);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
 
-	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
 }
 
 /*
@@ -177,17 +153,12 @@ static void df_v3_6_perfmon_rreg(struct amdgpu_device *adev,
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
-		return;
-
 	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
 	WREG32(address, lo_addr);
 	*lo_val = RREG32(data);
 	WREG32(address, hi_addr);
 	*hi_val = RREG32(data);
 	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-
-	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
 }
 
 /*
@@ -204,17 +175,12 @@ static void df_v3_6_perfmon_wreg(struct amdgpu_device *adev, uint32_t lo_addr,
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
-		return;
-
 	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
 	WREG32(address, lo_addr);
 	WREG32(data, lo_val);
 	WREG32(address, hi_addr);
 	WREG32(data, hi_val);
 	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-
-	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
 }
 
 /* get the number of df counters available */
-- 
2.17.1

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

RE: [PATCH 1/2] Revert "drm/amdgpu: fix compiler warnings for df perfmons"

[Russell, Kent]

I think the first thing that Alex will say is "please include a commit description". Why did you revert it? 

 Kent

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Kim, Jonathan
Sent: Friday, October 18, 2019 1:31 PM
To: amd-gfx@lists.freedesktop.org
Cc: Kuehling, Felix <Felix.Kuehling@amd.com>; Kim, Jonathan <Jonathan.Kim@amd.com>
Subject: [PATCH 1/2] Revert "drm/amdgpu: fix compiler warnings for df perfmons"

This reverts commit 7dd2eb31fcd564574e8efea6bf23cf504f9e2fd7.

Signed-off-by: Jonathan Kim <Jonathan.Kim@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
index e1cf7e9c616a..f403c62c944e 100644
--- a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
+++ b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
@@ -93,7 +93,7 @@ const struct attribute_group *df_v3_6_attr_groups[] = {
 		NULL
 };
 
-static int df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow)
+static df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow)
 {
 	int r = 0;
 
@@ -546,7 +546,7 @@ static void df_v3_6_pmc_get_count(struct amdgpu_device *adev,
 				  uint64_t config,
 				  uint64_t *count)
 {
-	uint32_t lo_base_addr, hi_base_addr, lo_val = 0, hi_val = 0;
+	uint32_t lo_base_addr, hi_base_addr, lo_val, hi_val;
 	*count = 0;
 
 	switch (adev->asic_type) {
-- 
2.17.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

Re: [PATCH 2/2] Revert "drm/amdgpu: disable c-states on xgmi perfmons"

[Kuehling, Felix]

You can squash the two reverts into a single commit so you avoid 
reintroducing a broken intermediate state. Mention both reverted commits 
in the squashed commit description. Checkpatch.pl prefers a different 
format for quoting reverted commits. Run checkpatch.pl on your commit to 
see a proper example.

Regards,
   Felix


On 2019-10-18 1:59 p.m., Kim, Jonathan wrote:
> This reverts commit 54275cd1649f4034c6450b6c5a8358fcd4f7dda6.
>
> incomplete solution to df c-state race condition.  smu msg in perf events
> causes deadlock.
>
> Change-Id: Ia85179df2bd167657e42a2d828c4a7c475c392ff
> Signed-off-by: Jonathan Kim <Jonathan.Kim@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 36 +---------------------------
>   1 file changed, 1 insertion(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
> index f403c62c944e..16fbd2bc8ad1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
> +++ b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
> @@ -93,21 +93,6 @@ const struct attribute_group *df_v3_6_attr_groups[] = {
>   		NULL
>   };
>   
> -static df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow)
> -{
> -	int r = 0;
> -
> -	if (is_support_sw_smu(adev)) {
> -		r = smu_set_df_cstate(&adev->smu, allow);
> -	} else if (adev->powerplay.pp_funcs
> -			&& adev->powerplay.pp_funcs->set_df_cstate) {
> -		r = adev->powerplay.pp_funcs->set_df_cstate(
> -			adev->powerplay.pp_handle, allow);
> -	}
> -
> -	return r;
> -}
> -
>   static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
>   				 uint32_t ficaa_val)
>   {
> @@ -117,9 +102,6 @@ static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
>   	address = adev->nbio.funcs->get_pcie_index_offset(adev);
>   	data = adev->nbio.funcs->get_pcie_data_offset(adev);
>   
> -	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
> -		return 0xFFFFFFFFFFFFFFFF;
> -
>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>   	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessAddress3);
>   	WREG32(data, ficaa_val);
> @@ -132,8 +114,6 @@ static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
>   
>   	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>   
> -	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
> -
>   	return (((ficadh_val & 0xFFFFFFFFFFFFFFFF) << 32) | ficadl_val);
>   }
>   
> @@ -145,9 +125,6 @@ static void df_v3_6_set_fica(struct amdgpu_device *adev, uint32_t ficaa_val,
>   	address = adev->nbio.funcs->get_pcie_index_offset(adev);
>   	data = adev->nbio.funcs->get_pcie_data_offset(adev);
>   
> -	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
> -		return;
> -
>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>   	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessAddress3);
>   	WREG32(data, ficaa_val);
> @@ -157,9 +134,8 @@ static void df_v3_6_set_fica(struct amdgpu_device *adev, uint32_t ficaa_val,
>   
>   	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessDataHi3);
>   	WREG32(data, ficadh_val);
> -	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>   
> -	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
> +	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
>   }
>   
>   /*
> @@ -177,17 +153,12 @@ static void df_v3_6_perfmon_rreg(struct amdgpu_device *adev,
>   	address = adev->nbio.funcs->get_pcie_index_offset(adev);
>   	data = adev->nbio.funcs->get_pcie_data_offset(adev);
>   
> -	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
> -		return;
> -
>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>   	WREG32(address, lo_addr);
>   	*lo_val = RREG32(data);
>   	WREG32(address, hi_addr);
>   	*hi_val = RREG32(data);
>   	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> -
> -	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
>   }
>   
>   /*
> @@ -204,17 +175,12 @@ static void df_v3_6_perfmon_wreg(struct amdgpu_device *adev, uint32_t lo_addr,
>   	address = adev->nbio.funcs->get_pcie_index_offset(adev);
>   	data = adev->nbio.funcs->get_pcie_data_offset(adev);
>   
> -	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
> -		return;
> -
>   	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
>   	WREG32(address, lo_addr);
>   	WREG32(data, lo_val);
>   	WREG32(address, hi_addr);
>   	WREG32(data, hi_val);
>   	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
> -
> -	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
>   }
>   
>   /* get the number of df counters available */
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] Revert "drm/amdgpu: disable c-states on xgmi perfmons"

[Kim, Jonathan]

This reverts commit 54275cd1649f4034c6450b6c5a8358fcd4f7dda6.

incomplete solution to df c-state race condition.  smu msg in perf events
causes deadlock.

Change-Id: Ia85179df2bd167657e42a2d828c4a7c475c392ff
Signed-off-by: Jonathan Kim <Jonathan.Kim@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 36 +---------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
index f403c62c944e..16fbd2bc8ad1 100644
--- a/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
+++ b/drivers/gpu/drm/amd/amdgpu/df_v3_6.c
@@ -93,21 +93,6 @@ const struct attribute_group *df_v3_6_attr_groups[] = {
 		NULL
 };
 
-static df_v3_6_set_df_cstate(struct amdgpu_device *adev, int allow)
-{
-	int r = 0;
-
-	if (is_support_sw_smu(adev)) {
-		r = smu_set_df_cstate(&adev->smu, allow);
-	} else if (adev->powerplay.pp_funcs
-			&& adev->powerplay.pp_funcs->set_df_cstate) {
-		r = adev->powerplay.pp_funcs->set_df_cstate(
-			adev->powerplay.pp_handle, allow);
-	}
-
-	return r;
-}
-
 static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
 				 uint32_t ficaa_val)
 {
@@ -117,9 +102,6 @@ static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
-		return 0xFFFFFFFFFFFFFFFF;
-
 	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
 	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessAddress3);
 	WREG32(data, ficaa_val);
@@ -132,8 +114,6 @@ static uint64_t df_v3_6_get_fica(struct amdgpu_device *adev,
 
 	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
 
-	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
-
 	return (((ficadh_val & 0xFFFFFFFFFFFFFFFF) << 32) | ficadl_val);
 }
 
@@ -145,9 +125,6 @@ static void df_v3_6_set_fica(struct amdgpu_device *adev, uint32_t ficaa_val,
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
-		return;
-
 	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
 	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessAddress3);
 	WREG32(data, ficaa_val);
@@ -157,9 +134,8 @@ static void df_v3_6_set_fica(struct amdgpu_device *adev, uint32_t ficaa_val,
 
 	WREG32(address, smnDF_PIE_AON_FabricIndirectConfigAccessDataHi3);
 	WREG32(data, ficadh_val);
-	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
 
-	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
+	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
 }
 
 /*
@@ -177,17 +153,12 @@ static void df_v3_6_perfmon_rreg(struct amdgpu_device *adev,
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
-		return;
-
 	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
 	WREG32(address, lo_addr);
 	*lo_val = RREG32(data);
 	WREG32(address, hi_addr);
 	*hi_val = RREG32(data);
 	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-
-	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
 }
 
 /*
@@ -204,17 +175,12 @@ static void df_v3_6_perfmon_wreg(struct amdgpu_device *adev, uint32_t lo_addr,
 	address = adev->nbio.funcs->get_pcie_index_offset(adev);
 	data = adev->nbio.funcs->get_pcie_data_offset(adev);
 
-	if (df_v3_6_set_df_cstate(adev, DF_CSTATE_DISALLOW))
-		return;
-
 	spin_lock_irqsave(&adev->pcie_idx_lock, flags);
 	WREG32(address, lo_addr);
 	WREG32(data, lo_val);
 	WREG32(address, hi_addr);
 	WREG32(data, hi_val);
 	spin_unlock_irqrestore(&adev->pcie_idx_lock, flags);
-
-	df_v3_6_set_df_cstate(adev, DF_CSTATE_ALLOW);
 }
 
 /* get the number of df counters available */
-- 
2.17.1

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