[PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled
Quan, Evan
Evan.Quan at amd.com
Tue Jul 23 01:31:06 UTC 2019
smu_feature_set_enabled() is refreshed to update the stored copy only because there do has the use case.
Another example is vega20_set_ppfeature_status() which needs also updating the stored feature masks only. That is missing now and is a bug.
I have noticed that for a while and do not have time to fix that yet.
A new API smu_enable_smc_feature() is provided if you want to enable the hw feature also(plus updating stored copy). The naming is much more straightforward.
I cannot see any problem with these changes and cannot understand why you are so resistant to them.
Regards,
Evan
From: Wang, Kevin(Yang) <Kevin1.Wang at amd.com>
Sent: Monday, July 22, 2019 9:21 PM
To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled
it seems this feature is specific feature on some asic in smc firmware.
why not create a specific function to handle this case,
the apis of smu_feature_is_enabled and smu_feature_set_enabled is public api for all smu asic.
how can you be sure that message must have the same logic on other asics in SMC firwamre.
may be this case not work on other smu or other asic.
if you still do, it means that when developer use smu_feature_is_enabled api,
the user must know the message detail information in smc firmware.
sw-bitmap and smc firwmare bitmap must be same in smu driver.
if not, this sw-bitmap is meaningless.
Best Regards,
Kevin
________________________________
From: Quan, Evan <Evan.Quan at amd.com<mailto:Evan.Quan at amd.com>>
Sent: Monday, July 22, 2019 8:45:15 PM
To: Wang, Kevin(Yang) <Kevin1.Wang at amd.com<mailto:Kevin1.Wang at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org> <amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>>
Subject: RE: [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled
The implementation of SMU_MSG_PowerDown[UP]Vcn (in smu fw) already includes the identical hardware enablement/disablement of SMU_FEATURE_VCN_PG_BIT.
Thus after that, only update the bitmask is enough. This is done on purpose.
Regards,
Evan
From: Wang, Kevin(Yang) <Kevin1.Wang at amd.com<mailto:Kevin1.Wang at amd.com>>
Sent: Monday, July 22, 2019 4:11 PM
To: Quan, Evan <Evan.Quan at amd.com<mailto:Evan.Quan at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
Subject: Re: [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled
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 status.
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_feature_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 function to set allowed feature mask when smu power on,
but this function is not used in smu driver, i want to remove them long long ago.
Best Regards,
Kevin
________________________________
From: Quan, Evan <Evan.Quan at amd.com<mailto:Evan.Quan at amd.com>>
Sent: Monday, July 22, 2019 3:17:55 PM
To: Wang, Kevin(Yang) <Kevin1.Wang at amd.com<mailto:Kevin1.Wang at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org> <amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>>
Subject: RE: [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_set_enabled
I cannot get your point. What do you mean "pairs of functions"?
Yes, this patch does not bring real changes.
But this helps for future maintain and fit common logic.
1. As in my previous patch("drm/amd/powerplay: correct Navi10 VCN powergate control" ), "smu_feature_set_enabled(smu, SMU_FEATURE_VCN_PG_BIT, enable);" 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 logic as smu_feature_set_supported. It updates only the saved bit mask. That's the expected behavior for 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) <Kevin1.Wang at amd.com<mailto:Kevin1.Wang at amd.com>>
Sent: Monday, July 22, 2019 3:00 PM
To: Quan, Evan <Evan.Quan at amd.com<mailto:Evan.Quan at amd.com>>; amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
Subject: Re: [PATCH] drm/amd/powerplay: correct confusing naming for smu_feature_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 smu_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 <amd-gfx-bounces at lists.freedesktop.org<mailto:amd-gfx-bounces at lists.freedesktop.org>> on behalf of Evan Quan <evan.quan at amd.com<mailto:evan.quan at amd.com>>
Sent: Monday, July 22, 2019 2:34:26 PM
To: amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org> <amd-gfx at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>>
Cc: Quan, Evan <Evan.Quan at amd.com<mailto:Evan.Quan at amd.com>>
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 at amd.com<mailto:evan.quan at amd.com>>
---
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/amd/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 = &smu->smu_feature;
int feature_id;
- int ret = 0;
feature_id = smu_feature_get_index(smu, mask);
if (feature_id < 0)
return -EINVAL;
- WARN_ON(feature_id > feature->feature_num);
-
mutex_lock(&feature->mutex);
- ret = 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 = 0;
+
+ feature_id = smu_feature_get_index(smu, mask);
+ if (feature_id < 0)
+ return -EINVAL;
+
+ ret = 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_mask mask)
@@ -1186,7 +1197,7 @@ static int smu_suspend(void *handle)
return ret;
if (adev->in_gpu_reset && baco_feature_is_enabled) {
- ret = smu_feature_set_enabled(smu, SMU_FEATURE_BACO_BIT, true);
+ ret = smu_enable_smc_feature(smu, SMU_FEATURE_BACO_BIT, true);
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/drm/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 *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_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/amd/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_supported(smu, SMU_FEATURE_FAN_CONTROL_BIT))
return 0;
- ret = smu_feature_set_enabled(smu, SMU_FEATURE_FAN_CONTROL_BIT, start);
+ ret = smu_enable_smc_feature(smu, SMU_FEATURE_FAN_CONTROL_BIT, start);
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/amd/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_context *smu, bool enable)
if (enable == 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_context *smu, bool enable)
if (enable == 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 at lists.freedesktop.org<mailto:amd-gfx at lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20190723/fb09a696/attachment-0001.html>
More information about the amd-gfx
mailing list