From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wang, Kevin(Yang)" Subject: Re: [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled Date: Mon, 22 Jul 2019 08:10:44 +0000 Message-ID: References: <20190722063426.19597-1-evan.quan@amd.com> , Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1290231845==" Return-path: In-Reply-To: Content-Language: en-US List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Quan, Evan" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============1290231845== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN2PR12MB3296FC663977D86111AB364FA2C40MN2PR12MB3296namp_" --_000_MN2PR12MB3296FC663977D86111AB364FA2C40MN2PR12MB3296namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable in fact, the smu feature bitmap is cached of smc firmware hardware bitmap, most of time, when driver want to check whether enable is available. the driver should talk with smc to check featue enabled. i think it is very low efficiency. so the driver will provide a bitmap structure to store hardware feature sta= tus. every time, the driver update hardware feature state, also should be update= software bitmap to sync it. if only update bitmap and don't set firmware feature enabled, the smu_featu= re_is_enabeld is maybe not work correctly. in your previous patch: drm/amd/powerplay: correct Navi10 VCN powergate control. after your patch, the function smu_feature_is_enabled(smu, SMU_FEATURE_VCN_PG_BIT) will retrun true, but in the firmware, this feature maybe is not enabled. so the function smu_feature_is_enabled is not work well. and smu_feature_is_supported is full software feature, this is helper func= tion to set allowed feature mask when smu power on, but this function is not used in smu driver, i want to remove them long lon= g ago. Best Regards, Kevin ________________________________ From: Quan, Evan Sent: Monday, July 22, 2019 3:17:55 PM To: Wang, Kevin(Yang) ; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org = Subject: RE: [PATCH] drm/amd/powerplay: correct confusing naming for smu_fe= ature_set_enabled I cannot get your point. What do you mean =93pairs of functions=94? Yes, this patch does not bring real changes. But this helps for future maintain and fit common logic. 1. As in my previous patch(=93drm/amd/powerplay: correct Navi10 VCN powerga= te control=94 ), =93smu_feature_set_enabled(smu, SMU_FEATURE_VCN_PG_BIT, e= nable);=94 was mistakenly used. I thought and expected smu_feature_set_enab= led set the bitmask on only. I do not want it to enable or disable the feat= ure. 2. smu_feature_set_enabled should in the same logic as smu_feature_set_supp= orted. It updates only the saved bit mask. That=92s the expected behavior f= or some APIs named as _set_enabled/supported. 3. The original callers who want feature enablement/disablement are updated= to use smu_enable_smc_feature(). I do not see any problem with that. Regards, Evan From: Wang, Kevin(Yang) Sent: Monday, July 22, 2019 3:00 PM To: Quan, Evan ; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Subject: Re: [PATCH] drm/amd/powerplay: correct confusing naming for smu_fe= ature_set_enabled before this patch, we have 4 apis to manage smu feature bits. the patch add a new one in them, but it is not add any feature in smu. before your patch: smu_feature_is_enabled and smu_feature_set_enabled is pair of functions, after your patch: smu_feature_is_enabled and smu_enable_smc_feature is pair of functions; so the driver don't have scenario needs to use smu_feature_set_enabeld, do you agree it? most of the time we update SMC feature, we must update software bitmap in s= mu_feature structure. if not, the smu_feature_is_enabled funciton is not work well. extern int smu_feature_init_dpm(struct smu_context *smu); extern int smu_feature_is_enabled(struct smu_context *smu, enum smu_feature_mask mask); extern int smu_feature_set_enabled(struct smu_context *smu, enum smu_feature_mask mask, bool enable); extern int smu_feature_is_supported(struct smu_context *smu, enum smu_feature_mask mask); extern int smu_feature_set_supported(struct smu_context *smu, enum smu_feature_mask mask, bool enable); Best Regards, Kevin ________________________________ From: amd-gfx > on behalf of Evan Quan > Sent: Monday, July 22, 2019 2:34:26 PM To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > Cc: Quan, Evan > Subject: [PATCH] drm/amd/powerplay: correct confusing naming for smu_featur= e_set_enabled It does more than updating the bitmask. In fact it enables also the feature. That's confusing. As for this, a new API is added for the feature enablement job. And smu_feature_set_enabled is updated to setting the bitmask only(as smu_feature_set_supported). Change-Id: I758e4461be34c0fcbdf19448e34180a5251926c4 Signed-off-by: Evan Quan > --- drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 29 +++++++++++++------ .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 2 ++ drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 2 +- drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 4 +-- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/a= md/powerplay/amdgpu_smu.c index 4e18f33a1bab..9262883d4031 100644 --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c @@ -511,28 +511,39 @@ int smu_feature_set_enabled(struct smu_context *smu, = enum smu_feature_mask mask, { struct smu_feature *feature =3D &smu->smu_feature; int feature_id; - int ret =3D 0; feature_id =3D smu_feature_get_index(smu, mask); if (feature_id < 0) return -EINVAL; - WARN_ON(feature_id > feature->feature_num); - mutex_lock(&feature->mutex); - ret =3D smu_feature_update_enable_state(smu, feature_id, enable); - if (ret) - goto failed; if (enable) test_and_set_bit(feature_id, feature->enabled); else test_and_clear_bit(feature_id, feature->enabled); -failed: mutex_unlock(&feature->mutex); - return ret; + return 0; +} + +int smu_enable_smc_feature(struct smu_context *smu, + enum smu_feature_mask mask, + bool enable) +{ + int feature_id; + int ret =3D 0; + + feature_id =3D smu_feature_get_index(smu, mask); + if (feature_id < 0) + return -EINVAL; + + ret =3D smu_feature_update_enable_state(smu, feature_id, enable); + if (ret) + return ret; + + return smu_feature_set_enabled(smu, mask, enable); } int smu_feature_is_supported(struct smu_context *smu, enum smu_feature_mas= k mask) @@ -1186,7 +1197,7 @@ static int smu_suspend(void *handle) return ret; if (adev->in_gpu_reset && baco_feature_is_enabled) { - ret =3D smu_feature_set_enabled(smu, SMU_FEATURE_BACO_BIT, = true); + ret =3D smu_enable_smc_feature(smu, SMU_FEATURE_BACO_BIT, t= rue); if (ret) { pr_warn("set BACO feature enabled failed, return %= d\n", ret); return ret; diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/d= rm/amd/powerplay/inc/amdgpu_smu.h index b702c9ee975f..267b879796f9 100644 --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h @@ -946,6 +946,8 @@ extern int smu_feature_is_enabled(struct smu_context *s= mu, enum smu_feature_mask mask); extern int smu_feature_set_enabled(struct smu_context *smu, enum smu_feature_mask mask, bool enable= ); +extern int smu_enable_smc_feature(struct smu_context *smu, + enum smu_feature_mask mask, bool enable)= ; extern int smu_feature_is_supported(struct smu_context *smu, enum smu_feature_mask mask); extern int smu_feature_set_supported(struct smu_context *smu, diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/am= d/powerplay/smu_v11_0.c index e3a178403546..0f59d2178d01 100644 --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c @@ -1419,7 +1419,7 @@ smu_v11_0_smc_fan_control(struct smu_context *smu, bo= ol start) if (smu_feature_is_supported(smu, SMU_FEATURE_FAN_CONTROL_BIT)) return 0; - ret =3D smu_feature_set_enabled(smu, SMU_FEATURE_FAN_CONTROL_BIT, s= tart); + ret =3D smu_enable_smc_feature(smu, SMU_FEATURE_FAN_CONTROL_BIT, st= art); if (ret) pr_err("[%s]%s smc FAN CONTROL feature failed!", __func__, (start ? "Start" : "Stop")); diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/a= md/powerplay/vega20_ppt.c index 9ead36192787..536ff884ddca 100644 --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c @@ -2845,7 +2845,7 @@ static int vega20_dpm_set_uvd_enable(struct smu_conte= xt *smu, bool enable) if (enable =3D=3D smu_feature_is_enabled(smu, SMU_FEATURE_DPM_UVD_= BIT)) return 0; - return smu_feature_set_enabled(smu, SMU_FEATURE_DPM_UVD_BIT, enable= ); + return smu_enable_smc_feature(smu, SMU_FEATURE_DPM_UVD_BIT, enable)= ; } static int vega20_dpm_set_vce_enable(struct smu_context *smu, bool enable) @@ -2856,7 +2856,7 @@ static int vega20_dpm_set_vce_enable(struct smu_conte= xt *smu, bool enable) if (enable =3D=3D smu_feature_is_enabled(smu, SMU_FEATURE_DPM_VCE_= BIT)) return 0; - return smu_feature_set_enabled(smu, SMU_FEATURE_DPM_VCE_BIT, enable= ); + return smu_enable_smc_feature(smu, SMU_FEATURE_DPM_VCE_BIT, enable)= ; } static int vega20_get_enabled_smc_features(struct smu_context *smu, -- 2.22.0 _______________________________________________ amd-gfx mailing list amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx --_000_MN2PR12MB3296FC663977D86111AB364FA2C40MN2PR12MB3296namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

in fact, the smu feature bitmap i= s cached of smc firmware hardware bitmap,

most of time, when driver want to check whether enable is available.=

the driver should talk with smc to ch= eck featue enabled. 
i think it is very low efficiency.

so the driver will provide a bitm= ap structure to store hardware feature status.

every time, the driver update har= dware feature state, also should be update software bitmap to sync it.


if only update bitmap and don't s= et firmware feature enabled, the smu_feature_is_enabeld is maybe not work c= orrectly.


in your previous patch:

drm/amd/powerplay: correct Navi10 VCN powergate control.

after your patch, 

the function smu_feature_is_= enabled(smu, SMU_FEATURE_VCN_PG_BIT)

will retrun true, but in the firm= ware, this feature maybe is not enabled.


so the function smu_feature_= is_enabled is not work well.


and smu_feature_is_supported = ; is full software feature, this is helper function to set allowe= d feature mask when smu power on,

but this function is not used in = smu driver, i want to remove them lon= g long ago.


Best Regards,
Kevin

=


From: Quan, Evan <Evan.Q= uan-5C7GfCeVMHo@public.gmane.org>
Sent: Monday, July 22, 2019 3:17:55 PM
To: Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32mzQB+pC5nmwQ@public.gmane.org= edesktop.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: RE: [PATCH] drm/amd/powerplay: correct confusing naming for= smu_feature_set_enabled
 

I cannot get your point. What do you mean =93pairs= of functions=94?

Yes, this patch does not bring real changes.

But this helps for future maintain and fit common = logic.

1. As in my previous patch(=93drm/amd/powerplay: c= orrect Navi10 VCN powergate control=94 ),  =93smu_feature_set_enabled(= smu, SMU_FEATURE_VCN_PG_BIT, enable);=94 was mistakenly used. I thought and= expected smu_feature_set_enabled set the bitmask on only. I do not want it to enable or disable the feature.

2. smu_feature_set_enabled should in the same logi= c as smu_feature_set_supported. It updates only the saved bit mask. That=92= s the expected behavior for some APIs named as _set_enabled/supporte= d.

3. The original callers who want feature enablemen= t/disablement are updated to use smu_enable_smc_feature(). I do not see any= problem with that.

 

Regards,

Evan

From: Wang, Kevin(Yang) <Kevin1.Wang@amd= .com>
Sent: Monday, July 22, 2019 3:00 PM
To: Quan, Evan <Evan.Quan-5C7GfCeVMHo@public.gmane.org>; amd-gfx-PD4FTy7X32lNgt0PjOBp9/rsn8yoX9R0@public.gmane.org= org
Subject: Re: [PATCH] drm/amd/powerplay: correct confusing naming for= smu_feature_set_enabled

 

befo= re this patch, we have 4 apis to manage smu feature bits.

the = patch add a new one in them, but it is not add any feature in smu.

&nbs= p;

befo= re your patch:

smu_= feature_is_enabled and smu_feature_set_enabled is pair of functions,=

afte= r your patch:

smu_= feature_is_enabled and smu_enable_smc_feature is pair of functions;<= /p>

&nbs= p;

so t= he driver don't have scenario needs to use smu_feature_set_enabeld,&nb= sp;

do y= ou agree it?

&nbs= p;

most= of the time we update SMC feature, we must update software bitmap in smu_f= eature structure.

if n= ot, the smu_feature_is_enabled funciton is not work well.

&nbs= p;

exter= n int smu_feature_init_dpm(struct smu_context *smu);

exter= n int smu_feature_is_enabled(struct smu_context *smu,

 = ; enum smu_feature_mask mask);

exter= n int smu_feature_set_enabled(struct smu_context *smu,

 = ;  enum smu_feature_mask mask, bool enable);

exter= n int smu_feature_is_supported(struct smu_context *smu,

 = ;   enum smu_feature_mask mask);

exter= n int smu_feature_set_supported(struct smu_context *smu,

 = ;    enum smu_feature_mask mask, bool enable);

&nbs= p;

Best= Regards,
Kevin

From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Evan Quan <evan.quan-5C7GfCeVMHo@public.gmane.org>
Sent: Monday, July 22, 2019 2:34:26 PM
To: amd-gfx@= lists.freedesktop.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Cc: Quan, Evan <Evan.= Quan-5C7GfCeVMHo@public.gmane.org>
Subject: [PATCH] drm/amd/powerplay: correct confusing naming for smu= _feature_set_enabled

 

It does more than updating the bitmask. In fact it= enables also the
feature. That's confusing. As for this, a new API is added for the
feature enablement job. And smu_feature_set_enabled is updated to
setting the bitmask only(as smu_feature_set_supported).

Change-Id: I758e4461be34c0fcbdf19448e34180a5251926c4
Signed-off-by: Evan Quan <evan.quan= @amd.com>
---
 drivers/gpu/drm/amd/powerplay/amdgpu_smu.c    | 29 = 3;++++++++++++------
 .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h    |  = 2 ++
 drivers/gpu/drm/amd/powerplay/smu_v11_0.c     |&n= bsp; 2 +-
 drivers/gpu/drm/amd/powerplay/vega20_ppt.c    |  = 4 +--
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu/drm/a= md/powerplay/amdgpu_smu.c
index 4e18f33a1bab..9262883d4031 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -511,28 +511,39 @@ int smu_feature_set_enabled(struct smu_context *s= mu, enum smu_feature_mask mask,
 {
         struct smu_feature *featur= e =3D &smu->smu_feature;
         int feature_id;
-       int ret =3D 0;
 
         feature_id =3D smu_feature= _get_index(smu, mask);
         if (feature_id < 0)
            &nb= sp;    return -EINVAL;
 
-       WARN_ON(feature_id > feature->f= eature_num);
-
         mutex_lock(&feature-&g= t;mutex);
-       ret =3D smu_feature_update_enable_sta= te(smu, feature_id, enable);
-       if (ret)
-            &n= bsp;  goto failed;
 
         if (enable)
            &nb= sp;    test_and_set_bit(feature_id, feature->enabled);          else
            &nb= sp;    test_and_clear_bit(feature_id, feature->enabled);<= br>  
-failed:
         mutex_unlock(&feature-= >mutex);
 
-       return ret;
+       return 0;
+}
+
+int smu_enable_smc_feature(struct smu_context *smu,
+           &nbs= p;            &= nbsp; enum smu_feature_mask mask,
+           &nbs= p;            &= nbsp; bool enable)
+{
+       int feature_id;
+       int ret =3D 0;
+
+       feature_id =3D smu_feature_get_in= dex(smu, mask);
+       if (feature_id < 0)
+           &nbs= p;   return -EINVAL;
+
+       ret =3D smu_feature_update_enable= _state(smu, feature_id, enable);
+       if (ret)
+           &nbs= p;   return ret;
+
+       return smu_feature_set_enabled(sm= u, mask, enable);
 }
 
 int smu_feature_is_supported(struct smu_context *smu, enum smu_featur= e_mask mask)
@@ -1186,7 +1197,7 @@ static int smu_suspend(void *handle)
            &nb= sp;    return ret;
 
         if (adev->in_gpu_reset = && baco_feature_is_enabled) {
-            &n= bsp;  ret =3D smu_feature_set_enabled(smu, SMU_FEATURE_BACO_BIT, true)= ;
+           &nbs= p;   ret =3D smu_enable_smc_feature(smu, SMU_FEATURE_BACO_BIT, tr= ue);
            &nb= sp;    if (ret) {
            &nb= sp;            pr_wa= rn("set BACO feature enabled failed, return %d\n", ret);
            &nb= sp;            retur= n ret;
diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers/gpu/d= rm/amd/powerplay/inc/amdgpu_smu.h
index b702c9ee975f..267b879796f9 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
@@ -946,6 +946,8 @@ extern int smu_feature_is_enabled(struct smu_contex= t *smu,
            &nb= sp;            =           enum smu_feature_mas= k mask);
 extern int smu_feature_set_enabled(struct smu_context *smu,
            &nb= sp;            =            enum smu_featu= re_mask mask, bool enable);
+extern int smu_enable_smc_feature(struct smu_context *smu,
+           &nbs= p;            &= nbsp;         enum smu_feature_mask= mask, bool enable);
 extern int smu_feature_is_supported(struct smu_context *smu,
            &nb= sp;            =             enum smu= _feature_mask mask);
 extern int smu_feature_set_supported(struct smu_context *smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/drm/am= d/powerplay/smu_v11_0.c
index e3a178403546..0f59d2178d01 100644
--- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
@@ -1419,7 +1419,7 @@ smu_v11_0_smc_fan_control(struct smu_context *smu= , bool start)
         if (smu_feature_is_support= ed(smu, SMU_FEATURE_FAN_CONTROL_BIT))
            &nb= sp;    return 0;
 
-       ret =3D smu_feature_set_enabled(smu, = SMU_FEATURE_FAN_CONTROL_BIT, start);
+       ret =3D smu_enable_smc_feature(sm= u, SMU_FEATURE_FAN_CONTROL_BIT, start);
         if (ret)
            &nb= sp;    pr_err("[%s]%s smc FAN CONTROL feature failed!&q= uot;,
            &nb= sp;           __func__, (= start ? "Start" : "Stop"));
diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu/drm/a= md/powerplay/vega20_ppt.c
index 9ead36192787..536ff884ddca 100644
--- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
@@ -2845,7 +2845,7 @@ static int vega20_dpm_set_uvd_enable(struct smu_c= ontext *smu, bool enable)
         if (enable =3D=3D smu_feat= ure_is_enabled(smu, SMU_FEATURE_DPM_UVD_BIT))
            &nb= sp;    return 0;
 
-       return smu_feature_set_enabled(smu, S= MU_FEATURE_DPM_UVD_BIT, enable);
+       return smu_enable_smc_feature(smu= , SMU_FEATURE_DPM_UVD_BIT, enable);
 }
 
 static int vega20_dpm_set_vce_enable(struct smu_context *smu, bool en= able)
@@ -2856,7 +2856,7 @@ static int vega20_dpm_set_vce_enable(struct smu_c= ontext *smu, bool enable)
         if (enable =3D=3D smu_feat= ure_is_enabled(smu, SMU_FEATURE_DPM_VCE_BIT))
            &nb= sp;    return 0;
 
-       return smu_feature_set_enabled(smu, S= MU_FEATURE_DPM_VCE_BIT, enable);
+       return smu_enable_smc_feature(smu= , SMU_FEATURE_DPM_VCE_BIT, enable);
 }
 
 static int vega20_get_enabled_smc_features(struct smu_context *smu, --
2.22.0

_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9/rsn8yoX9R0@public.gmane.org= org
https://= lists.freedesktop.org/mailman/listinfo/amd-gfx

--_000_MN2PR12MB3296FC663977D86111AB364FA2C40MN2PR12MB3296namp_-- --===============1290231845== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4 --===============1290231845==--