RE: [PATCH] drm/amd/powerplay: update cached feature enablement status V3
Reviewed-by: Jack Gui
-Original Message-
From: Quan, Evan
Sent: Monday, September 2, 2019 4:16 PM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Cc: Gui, Jack
Subject: RE: [PATCH] drm/amd/powerplay: update cached feature enablement status
V3
Ping..
> -Original Message-
> From: Evan Quan
> Sent: 2019年8月23日 12:49
> To: amd-gfx@lists.freedesktop.org
> Cc: Quan, Evan
> Subject: [PATCH] drm/amd/powerplay: update cached feature enablement
> status V3
>
> Need to update in cache feature enablement status after pp_feature
> settings. Another fix for the commit below:
> drm/amd/powerplay: implment sysfs feature status function in smu
>
> V2: update smu_feature_update_enable_state() and relates
> V3: use bitmap_or and bitmap_andnot
>
> Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
> Signed-off-by: Evan Quan
> ---
> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c| 101 +
> -
> .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h| 1 -
> 2 files changed, 49 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 4df7fb6eaf3c..c8c00966a621 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -94,6 +94,52 @@ size_t smu_sys_get_pp_feature_mask(struct
> smu_context *smu, char *buf)
> return size;
> }
>
> +static int smu_feature_update_enable_state(struct smu_context *smu,
> +uint64_t feature_mask,
> +bool enabled)
> +{
> + struct smu_feature *feature = >smu_feature;
> + uint32_t feature_low = 0, feature_high = 0;
> + int ret = 0;
> +
> + if (!smu->pm_enabled)
> + return ret;
> +
> + feature_low = (feature_mask >> 0 ) & 0x;
> + feature_high = (feature_mask >> 32) & 0x;
> +
> + if (enabled) {
> + ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_EnableSmuFeaturesLow,
> + feature_low);
> + if (ret)
> + return ret;
> + ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_EnableSmuFeaturesHigh,
> + feature_high);
> + if (ret)
> + return ret;
> + } else {
> + ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_DisableSmuFeaturesLow,
> + feature_low);
> + if (ret)
> + return ret;
> + ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_DisableSmuFeaturesHigh,
> + feature_high);
> + if (ret)
> + return ret;
> + }
> +
> + mutex_lock(>mutex);
> + if (enabled)
> + bitmap_or(feature->enabled, feature->enabled,
> + (unsigned long *)(_mask),
> SMU_FEATURE_MAX);
> + else
> + bitmap_andnot(feature->enabled, feature->enabled,
> + (unsigned long *)(_mask),
> SMU_FEATURE_MAX);
> + mutex_unlock(>mutex);
> +
> + return ret;
> +}
> +
> int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t
> new_mask) {
> int ret = 0;
> @@ -591,41 +637,7 @@ int smu_feature_init_dpm(struct smu_context
> *smu)
>
> return ret;
> }
> -int smu_feature_update_enable_state(struct smu_context *smu, uint64_t
> feature_mask, bool enabled) -{
> - uint32_t feature_low = 0, feature_high = 0;
> - int ret = 0;
> -
> - if (!smu->pm_enabled)
> - return ret;
> -
> - feature_low = (feature_mask >> 0 ) & 0x;
> - feature_high = (feature_mask >> 32) & 0x;
> -
> - if (enabled) {
> - ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_EnableSmuFeaturesLow,
> - feature_low);
> - if (ret)
> - return ret;
> - ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_EnableSmuFeaturesHigh,
> - feature_high);
> - if (ret)
> - return ret;
> -
> - } else {
> - ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_DisableSmuFeaturesLow,
> - feature_low);
> - if (ret)
> - return ret;
> - ret = smu_send_smc_msg_with_param(smu,
>
RE: [PATCH] drm/amd/powerplay: update cached feature enablement status V3
Ping..
> -Original Message-
> From: Evan Quan
> Sent: 2019年8月23日 12:49
> To: amd-gfx@lists.freedesktop.org
> Cc: Quan, Evan
> Subject: [PATCH] drm/amd/powerplay: update cached feature enablement
> status V3
>
> Need to update in cache feature enablement status after pp_feature
> settings. Another fix for the commit below:
> drm/amd/powerplay: implment sysfs feature status function in smu
>
> V2: update smu_feature_update_enable_state() and relates
> V3: use bitmap_or and bitmap_andnot
>
> Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
> Signed-off-by: Evan Quan
> ---
> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c| 101 +
> -
> .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h| 1 -
> 2 files changed, 49 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 4df7fb6eaf3c..c8c00966a621 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -94,6 +94,52 @@ size_t smu_sys_get_pp_feature_mask(struct
> smu_context *smu, char *buf)
> return size;
> }
>
> +static int smu_feature_update_enable_state(struct smu_context *smu,
> +uint64_t feature_mask,
> +bool enabled)
> +{
> + struct smu_feature *feature = >smu_feature;
> + uint32_t feature_low = 0, feature_high = 0;
> + int ret = 0;
> +
> + if (!smu->pm_enabled)
> + return ret;
> +
> + feature_low = (feature_mask >> 0 ) & 0x;
> + feature_high = (feature_mask >> 32) & 0x;
> +
> + if (enabled) {
> + ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_EnableSmuFeaturesLow,
> + feature_low);
> + if (ret)
> + return ret;
> + ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_EnableSmuFeaturesHigh,
> + feature_high);
> + if (ret)
> + return ret;
> + } else {
> + ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_DisableSmuFeaturesLow,
> + feature_low);
> + if (ret)
> + return ret;
> + ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_DisableSmuFeaturesHigh,
> + feature_high);
> + if (ret)
> + return ret;
> + }
> +
> + mutex_lock(>mutex);
> + if (enabled)
> + bitmap_or(feature->enabled, feature->enabled,
> + (unsigned long *)(_mask),
> SMU_FEATURE_MAX);
> + else
> + bitmap_andnot(feature->enabled, feature->enabled,
> + (unsigned long *)(_mask),
> SMU_FEATURE_MAX);
> + mutex_unlock(>mutex);
> +
> + return ret;
> +}
> +
> int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t
> new_mask) {
> int ret = 0;
> @@ -591,41 +637,7 @@ int smu_feature_init_dpm(struct smu_context
> *smu)
>
> return ret;
> }
> -int smu_feature_update_enable_state(struct smu_context *smu, uint64_t
> feature_mask, bool enabled) -{
> - uint32_t feature_low = 0, feature_high = 0;
> - int ret = 0;
> -
> - if (!smu->pm_enabled)
> - return ret;
> -
> - feature_low = (feature_mask >> 0 ) & 0x;
> - feature_high = (feature_mask >> 32) & 0x;
> -
> - if (enabled) {
> - ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_EnableSmuFeaturesLow,
> - feature_low);
> - if (ret)
> - return ret;
> - ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_EnableSmuFeaturesHigh,
> - feature_high);
> - if (ret)
> - return ret;
> -
> - } else {
> - ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_DisableSmuFeaturesLow,
> - feature_low);
> - if (ret)
> - return ret;
> - ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_DisableSmuFeaturesHigh,
> - feature_high);
> - if (ret)
> - return ret;
>
> - }
> -
> - return ret;
> -}
>
> int smu_feature_is_enabled(struct smu_context *smu, enum
> smu_feature_mask mask) { @@ -651,8 +663,6 @@ int
> smu_feature_set_enabled(struct smu_context *smu, enum
> smu_feature_mask mask, {
> struct smu_feature *feature = >smu_feature;
> int feature_id;
> - uint64_t feature_mask = 0;
> - int ret = 0;
>
> feature_id = smu_feature_get_index(smu, mask);
> if (feature_id < 0)
> @@ -660,22 +670,9 @@ int smu_feature_set_enabled(struct smu_context
> *smu, enum
Re: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
a patch does only one thing.
in this patch,it will do 2 things:
1. fixed feature bitmap cached issue.
2. make api of smu_feature_update_enabled_state as static function.
for reason#2:
the driver has other apis which only used in amdgpu_smu.c, but it still
declaration in amdgpu_smu.h.
the intent is to make this a public API.
if you want to make it is a static function, please split it 2 patches. and
indicate the reason.
Best Regards,
Kevin
From: Quan, Evan
Sent: Friday, August 23, 2019 1:38 PM
To: Wang, Kevin(Yang) ; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
smu_feature_update_enable_state() is used only in amdgpu_smu.c.
As a common sense, these APIs should be declared as ‘static’.
Regards,
Evan
From: Wang, Kevin(Yang)
Sent: Friday, August 23, 2019 1:30 PM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
comment inline
From: Quan, Evan mailto:evan.q...@amd.com>>
Sent: Friday, August 23, 2019 12:50 PM
To: Wang, Kevin(Yang) mailto:kevin1.w...@amd.com>>;
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
mailto:amd-gfx@lists.freedesktop.org>>
Subject: RE: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
Comment inline
From: Wang, Kevin(Yang) mailto:kevin1.w...@amd.com>>
Sent: Thursday, August 22, 2019 8:00 PM
To: Quan, Evan mailto:evan.q...@amd.com>>;
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
comment inline.
From: amd-gfx
mailto:amd-gfx-boun...@lists.freedesktop.org>>
on behalf of Evan Quan mailto:evan.q...@amd.com>>
Sent: Thursday, August 22, 2019 6:18 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
mailto:amd-gfx@lists.freedesktop.org>>
Cc: Quan, Evan mailto:evan.q...@amd.com>>
Subject: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
Need to update in cache feature enablement status after pp_feature
settings. Another fix for the commit below:
drm/amd/powerplay: implment sysfs feature status function in smu
V2: update smu_feature_update_enable_state() and relates
Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
[kevin]: this information is not neccessary for public, please remove it.
git config gerrit.createchangeid=false
Signed-off-by: Evan Quan mailto:evan.q...@amd.com>>
---
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c| 104 +-
.../gpu/drm/amd/powerplay/inc/amdgpu_smu.h| 1 -
2 files changed, 52 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 4df7fb6eaf3c..3e1cd5d9c29e 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -94,6 +94,55 @@ size_t smu_sys_get_pp_feature_mask(struct smu_context *smu,
char *buf)
return size;
}
+static int smu_feature_update_enable_state(struct smu_context *smu,
+ uint64_t feature_mask,
+ bool enabled)
+{
+ struct smu_feature *feature = >smu_feature;
+ uint32_t feature_low = 0, feature_high = 0;
+ uint64_t feature_id;
+ int ret = 0;
+
+ if (!smu->pm_enabled)
+ return ret;
+
+ feature_low = (feature_mask >> 0 ) & 0x;
+ feature_high = (feature_mask >> 32) & 0x;
+
+ if (enabled) {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ } else {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ }
+
+ mutex_lock(>mutex);
+ for (feature_id = 0; feature_id < 64; feature_id++) {
+ if (feature_mask & (1ULL << feature_id)) {
+ if (enabled)
+ test_and_set_bit(feature_id, feature-
RE: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
smu_feature_update_enable_state() is used only in amdgpu_smu.c.
As a common sense, these APIs should be declared as 'static'.
Regards,
Evan
From: Wang, Kevin(Yang)
Sent: Friday, August 23, 2019 1:30 PM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
comment inline
From: Quan, Evan mailto:evan.q...@amd.com>>
Sent: Friday, August 23, 2019 12:50 PM
To: Wang, Kevin(Yang) mailto:kevin1.w...@amd.com>>;
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
mailto:amd-gfx@lists.freedesktop.org>>
Subject: RE: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
Comment inline
From: Wang, Kevin(Yang) mailto:kevin1.w...@amd.com>>
Sent: Thursday, August 22, 2019 8:00 PM
To: Quan, Evan mailto:evan.q...@amd.com>>;
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
comment inline.
From: amd-gfx
mailto:amd-gfx-boun...@lists.freedesktop.org>>
on behalf of Evan Quan mailto:evan.q...@amd.com>>
Sent: Thursday, August 22, 2019 6:18 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
mailto:amd-gfx@lists.freedesktop.org>>
Cc: Quan, Evan mailto:evan.q...@amd.com>>
Subject: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
Need to update in cache feature enablement status after pp_feature
settings. Another fix for the commit below:
drm/amd/powerplay: implment sysfs feature status function in smu
V2: update smu_feature_update_enable_state() and relates
Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
[kevin]: this information is not neccessary for public, please remove it.
git config gerrit.createchangeid=false
Signed-off-by: Evan Quan mailto:evan.q...@amd.com>>
---
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c| 104 +-
.../gpu/drm/amd/powerplay/inc/amdgpu_smu.h| 1 -
2 files changed, 52 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 4df7fb6eaf3c..3e1cd5d9c29e 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -94,6 +94,55 @@ size_t smu_sys_get_pp_feature_mask(struct smu_context *smu,
char *buf)
return size;
}
+static int smu_feature_update_enable_state(struct smu_context *smu,
+ uint64_t feature_mask,
+ bool enabled)
+{
+ struct smu_feature *feature = >smu_feature;
+ uint32_t feature_low = 0, feature_high = 0;
+ uint64_t feature_id;
+ int ret = 0;
+
+ if (!smu->pm_enabled)
+ return ret;
+
+ feature_low = (feature_mask >> 0 ) & 0x;
+ feature_high = (feature_mask >> 32) & 0x;
+
+ if (enabled) {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ } else {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ }
+
+ mutex_lock(>mutex);
+ for (feature_id = 0; feature_id < 64; feature_id++) {
+ if (feature_mask & (1ULL << feature_id)) {
+ if (enabled)
+ test_and_set_bit(feature_id, feature->enabled);
+ else
+ test_and_clear_bit(feature_id,
feature->enabled);
+ }
+ }
//[kevin]: the code logic is a little redundant.
could you use bellow macro to replace that?
header : linux/bitmap.h
* bitmap_and(dst, src1, src2, nbits) *dst = *src1 & *src2
* bitmap_or(dst, src1, src2, nbits) *dst = *src1 | *src2
* bitmap_xor(dst, src1, src2, nbits) *dst = *src1 ^ *src2
* bitmap_andnot(dst, src1, src2, nbits) *dst = *src1 & ~(*src2)
+ mutex_unlock(>mutex);
+
+ return ret;
+}
+
[Quan, Evan] updated in v3.
int smu_sys_set_pp_feature_mask(struct smu_context *smu
Re: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
comment inline
From: Quan, Evan
Sent: Friday, August 23, 2019 12:50 PM
To: Wang, Kevin(Yang) ; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
Comment inline
From: Wang, Kevin(Yang)
Sent: Thursday, August 22, 2019 8:00 PM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
comment inline.
From: amd-gfx
mailto:amd-gfx-boun...@lists.freedesktop.org>>
on behalf of Evan Quan mailto:evan.q...@amd.com>>
Sent: Thursday, August 22, 2019 6:18 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
mailto:amd-gfx@lists.freedesktop.org>>
Cc: Quan, Evan mailto:evan.q...@amd.com>>
Subject: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
Need to update in cache feature enablement status after pp_feature
settings. Another fix for the commit below:
drm/amd/powerplay: implment sysfs feature status function in smu
V2: update smu_feature_update_enable_state() and relates
Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
[kevin]: this information is not neccessary for public, please remove it.
git config gerrit.createchangeid=false
Signed-off-by: Evan Quan mailto:evan.q...@amd.com>>
---
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c| 104 +-
.../gpu/drm/amd/powerplay/inc/amdgpu_smu.h| 1 -
2 files changed, 52 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 4df7fb6eaf3c..3e1cd5d9c29e 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -94,6 +94,55 @@ size_t smu_sys_get_pp_feature_mask(struct smu_context *smu,
char *buf)
return size;
}
+static int smu_feature_update_enable_state(struct smu_context *smu,
+ uint64_t feature_mask,
+ bool enabled)
+{
+ struct smu_feature *feature = >smu_feature;
+ uint32_t feature_low = 0, feature_high = 0;
+ uint64_t feature_id;
+ int ret = 0;
+
+ if (!smu->pm_enabled)
+ return ret;
+
+ feature_low = (feature_mask >> 0 ) & 0x;
+ feature_high = (feature_mask >> 32) & 0x;
+
+ if (enabled) {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ } else {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ }
+
+ mutex_lock(>mutex);
+ for (feature_id = 0; feature_id < 64; feature_id++) {
+ if (feature_mask & (1ULL << feature_id)) {
+ if (enabled)
+ test_and_set_bit(feature_id, feature->enabled);
+ else
+ test_and_clear_bit(feature_id,
feature->enabled);
+ }
+ }
//[kevin]: the code logic is a little redundant.
could you use bellow macro to replace that?
header : linux/bitmap.h
* bitmap_and(dst, src1, src2, nbits) *dst = *src1 & *src2
* bitmap_or(dst, src1, src2, nbits) *dst = *src1 | *src2
* bitmap_xor(dst, src1, src2, nbits) *dst = *src1 ^ *src2
* bitmap_andnot(dst, src1, src2, nbits) *dst = *src1 & ~(*src2)
+ mutex_unlock(>mutex);
+
+ return ret;
+}
+
[Quan, Evan] updated in v3.
int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t new_mask)
{
int ret = 0;
@@ -591,41 +640,7 @@ int smu_feature_init_dpm(struct smu_context *smu)
return ret;
}
[kevin]:
in this patch, i know you only want to fix not cached feature cache issue,
but in v2 patch,
the patch adjust the order of code functions, it seems that this is a brand new
function,
I don't think it is necessary,
could you just reflect the modified content in the patch, which can facilitate
us to trace problems and review.
thanks.
[Quan, Evan] Move the API before the place it’s called. No problem here.
[kevin]: in this patch, you don't
RE: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
Comment inline
From: Wang, Kevin(Yang)
Sent: Thursday, August 22, 2019 8:00 PM
To: Quan, Evan ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/powerplay: update cached feature enablement status
V2
comment inline.
From: amd-gfx
mailto:amd-gfx-boun...@lists.freedesktop.org>>
on behalf of Evan Quan mailto:evan.q...@amd.com>>
Sent: Thursday, August 22, 2019 6:18 PM
To: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
mailto:amd-gfx@lists.freedesktop.org>>
Cc: Quan, Evan mailto:evan.q...@amd.com>>
Subject: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
Need to update in cache feature enablement status after pp_feature
settings. Another fix for the commit below:
drm/amd/powerplay: implment sysfs feature status function in smu
V2: update smu_feature_update_enable_state() and relates
Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
[kevin]: this information is not neccessary for public, please remove it.
git config gerrit.createchangeid=false
Signed-off-by: Evan Quan mailto:evan.q...@amd.com>>
---
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c| 104 +-
.../gpu/drm/amd/powerplay/inc/amdgpu_smu.h| 1 -
2 files changed, 52 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 4df7fb6eaf3c..3e1cd5d9c29e 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -94,6 +94,55 @@ size_t smu_sys_get_pp_feature_mask(struct smu_context *smu,
char *buf)
return size;
}
+static int smu_feature_update_enable_state(struct smu_context *smu,
+ uint64_t feature_mask,
+ bool enabled)
+{
+ struct smu_feature *feature = >smu_feature;
+ uint32_t feature_low = 0, feature_high = 0;
+ uint64_t feature_id;
+ int ret = 0;
+
+ if (!smu->pm_enabled)
+ return ret;
+
+ feature_low = (feature_mask >> 0 ) & 0x;
+ feature_high = (feature_mask >> 32) & 0x;
+
+ if (enabled) {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ } else {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ }
+
+ mutex_lock(>mutex);
+ for (feature_id = 0; feature_id < 64; feature_id++) {
+ if (feature_mask & (1ULL << feature_id)) {
+ if (enabled)
+ test_and_set_bit(feature_id, feature->enabled);
+ else
+ test_and_clear_bit(feature_id,
feature->enabled);
+ }
+ }
//[kevin]: the code logic is a little redundant.
could you use bellow macro to replace that?
header : linux/bitmap.h
* bitmap_and(dst, src1, src2, nbits) *dst = *src1 & *src2
* bitmap_or(dst, src1, src2, nbits) *dst = *src1 | *src2
* bitmap_xor(dst, src1, src2, nbits) *dst = *src1 ^ *src2
* bitmap_andnot(dst, src1, src2, nbits) *dst = *src1 & ~(*src2)
+ mutex_unlock(>mutex);
+
+ return ret;
+}
+
[Quan, Evan] updated in v3.
int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t new_mask)
{
int ret = 0;
@@ -591,41 +640,7 @@ int smu_feature_init_dpm(struct smu_context *smu)
return ret;
}
[kevin]:
in this patch, i know you only want to fix not cached feature cache issue,
but in v2 patch,
the patch adjust the order of code functions, it seems that this is a brand new
function,
I don't think it is necessary,
could you just reflect the modified content in the patch, which can facilitate
us to trace problems and review.
thanks.
[Quan, Evan] Move the API before the place it's called. No problem here.
-int smu_feature_update_enable_state(struct smu_context *smu, uint64_t
feature_mask, bool enabled)
-{
- uint32_t feature_low = 0, feature_high = 0;
- int ret = 0;
- if (!smu->pm_enabled)
- return ret;
-
- feature_low = (feature_mask >> 0 ) & 0x
Re: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
comment inline.
From: amd-gfx on behalf of Evan Quan
Sent: Thursday, August 22, 2019 6:18 PM
To: amd-gfx@lists.freedesktop.org
Cc: Quan, Evan
Subject: [PATCH] drm/amd/powerplay: update cached feature enablement status V2
Need to update in cache feature enablement status after pp_feature
settings. Another fix for the commit below:
drm/amd/powerplay: implment sysfs feature status function in smu
V2: update smu_feature_update_enable_state() and relates
Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
[kevin]: this information is not neccessary for public, please remove it.
git config gerrit.createchangeid=false
Signed-off-by: Evan Quan
---
drivers/gpu/drm/amd/powerplay/amdgpu_smu.c| 104 +-
.../gpu/drm/amd/powerplay/inc/amdgpu_smu.h| 1 -
2 files changed, 52 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
index 4df7fb6eaf3c..3e1cd5d9c29e 100644
--- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
@@ -94,6 +94,55 @@ size_t smu_sys_get_pp_feature_mask(struct smu_context *smu,
char *buf)
return size;
}
+static int smu_feature_update_enable_state(struct smu_context *smu,
+ uint64_t feature_mask,
+ bool enabled)
+{
+ struct smu_feature *feature = >smu_feature;
+ uint32_t feature_low = 0, feature_high = 0;
+ uint64_t feature_id;
+ int ret = 0;
+
+ if (!smu->pm_enabled)
+ return ret;
+
+ feature_low = (feature_mask >> 0 ) & 0x;
+ feature_high = (feature_mask >> 32) & 0x;
+
+ if (enabled) {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ } else {
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesLow,
+ feature_low);
+ if (ret)
+ return ret;
+ ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesHigh,
+ feature_high);
+ if (ret)
+ return ret;
+ }
+
+ mutex_lock(>mutex);
+ for (feature_id = 0; feature_id < 64; feature_id++) {
+ if (feature_mask & (1ULL << feature_id)) {
+ if (enabled)
+ test_and_set_bit(feature_id, feature->enabled);
+ else
+ test_and_clear_bit(feature_id,
feature->enabled);
+ }
+ }
//[kevin]: the code logic is a little redundant.
could you use bellow macro to replace that?
header : linux/bitmap.h
* bitmap_and(dst, src1, src2, nbits) *dst = *src1 & *src2
* bitmap_or(dst, src1, src2, nbits) *dst = *src1 | *src2
* bitmap_xor(dst, src1, src2, nbits) *dst = *src1 ^ *src2
* bitmap_andnot(dst, src1, src2, nbits) *dst = *src1 & ~(*src2)
+ mutex_unlock(>mutex);
+
+ return ret;
+}
+
int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t new_mask)
{
int ret = 0;
@@ -591,41 +640,7 @@ int smu_feature_init_dpm(struct smu_context *smu)
return ret;
}
[kevin]:
in this patch, i know you only want to fix not cached feature cache issue,
but in v2 patch,
the patch adjust the order of code functions, it seems that this is a brand new
function,
I don't think it is necessary,
could you just reflect the modified content in the patch, which can facilitate
us to trace problems and review.
thanks.
-int smu_feature_update_enable_state(struct smu_context *smu, uint64_t
feature_mask, bool enabled)
-{
- uint32_t feature_low = 0, feature_high = 0;
- int ret = 0;
- if (!smu->pm_enabled)
- return ret;
-
- feature_low = (feature_mask >> 0 ) & 0x;
- feature_high = (feature_mask >> 32) & 0x;
-
- if (enabled) {
- ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesLow,
- feature_low);
- if (ret)
- return ret;
- ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_EnableSmuFeaturesHigh,
- feature_high);
- if (ret)
- return ret;
-
- } else {
- ret = smu_send_smc_msg_with_param(smu,
SMU_MSG_DisableSmuFeaturesLow,
-
RE: [PATCH] drm/amd/powerplay: update cached feature enablement status
Please check V2.
> -Original Message-
> From: amd-gfx On Behalf Of
> Kevin Wang
> Sent: Wednesday, August 21, 2019 7:45 PM
> To: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/powerplay: update cached feature
> enablement status
>
> Hi Evan,
>
> this is know issue for me.
> i think we should add update feature mask cached operation into
> smu_feature_update_enable_state function.
>
> Best Regards,
> Kevin
>
> On 8/21/19 5:24 PM, Evan Quan wrote:
> > Need to update in cache feature enablement status after pp_feature
> > settings. Another fix for the commit below:
> > drm/amd/powerplay: implment sysfs feature status function in smu
> >
> > Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
> > Signed-off-by: Evan Quan
> > ---
> > drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 16
>
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > index c663d25db5ab..04867cafb322 100644
> > --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > @@ -96,11 +96,13 @@ size_t smu_sys_get_pp_feature_mask(struct
> > smu_context *smu, char *buf)
> >
> > int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t
> new_mask)
> > {
> > + struct smu_feature *feature = >smu_feature;
> > int ret = 0;
> > uint32_t feature_mask[2] = { 0 };
> > uint64_t feature_2_enabled = 0;
> > uint64_t feature_2_disabled = 0;
> > uint64_t feature_enables = 0;
> > + uint64_t feature_id;
> >
> > ret = smu_feature_get_enabled_mask(smu, feature_mask, 2);
> > if (ret)
> > @@ -115,11 +117,25 @@ int smu_sys_set_pp_feature_mask(struct
> smu_context *smu, uint64_t new_mask)
> > ret = smu_feature_update_enable_state(smu,
> feature_2_enabled, true);
> > if (ret)
> > return ret;
> > +
> > + mutex_lock(>mutex);
> > + for (feature_id = 0; feature_id < 64; feature_id++) {
> > + if (feature_2_enabled & (1ULL << feature_id))
> > + test_and_set_bit(feature_id, feature-
> >enabled);
> > + }
> > + mutex_unlock(>mutex);
> > }
> > if (feature_2_disabled) {
> > ret = smu_feature_update_enable_state(smu,
> feature_2_disabled, false);
> > if (ret)
> > return ret;
> > +
> > + mutex_lock(>mutex);
> > + for (feature_id = 0; feature_id < 64; feature_id++) {
> > + if (feature_2_disabled & (1ULL << feature_id))
> > + test_and_clear_bit(feature_id, feature-
> >enabled);
> > + }
> > + mutex_unlock(>mutex);
> > }
> >
> > return ret;
> ___
> 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] drm/amd/powerplay: update cached feature enablement status
Please check V2.
> -Original Message-
> From: amd-gfx On Behalf Of
> Kevin Wang
> Sent: Wednesday, August 21, 2019 7:45 PM
> To: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/powerplay: update cached feature
> enablement status
>
> Hi Evan,
>
> this is know issue for me.
> i think we should add update feature mask cached operation into
> smu_feature_update_enable_state function.
>
> Best Regards,
> Kevin
>
> On 8/21/19 5:24 PM, Evan Quan wrote:
> > Need to update in cache feature enablement status after pp_feature
> > settings. Another fix for the commit below:
> > drm/amd/powerplay: implment sysfs feature status function in smu
> >
> > Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
> > Signed-off-by: Evan Quan
> > ---
> > drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 16
>
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > index c663d25db5ab..04867cafb322 100644
> > --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> > @@ -96,11 +96,13 @@ size_t smu_sys_get_pp_feature_mask(struct
> > smu_context *smu, char *buf)
> >
> > int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t
> new_mask)
> > {
> > + struct smu_feature *feature = >smu_feature;
> > int ret = 0;
> > uint32_t feature_mask[2] = { 0 };
> > uint64_t feature_2_enabled = 0;
> > uint64_t feature_2_disabled = 0;
> > uint64_t feature_enables = 0;
> > + uint64_t feature_id;
> >
> > ret = smu_feature_get_enabled_mask(smu, feature_mask, 2);
> > if (ret)
> > @@ -115,11 +117,25 @@ int smu_sys_set_pp_feature_mask(struct
> smu_context *smu, uint64_t new_mask)
> > ret = smu_feature_update_enable_state(smu,
> feature_2_enabled, true);
> > if (ret)
> > return ret;
> > +
> > + mutex_lock(>mutex);
> > + for (feature_id = 0; feature_id < 64; feature_id++) {
> > + if (feature_2_enabled & (1ULL << feature_id))
> > + test_and_set_bit(feature_id, feature-
> >enabled);
> > + }
> > + mutex_unlock(>mutex);
> > }
> > if (feature_2_disabled) {
> > ret = smu_feature_update_enable_state(smu,
> feature_2_disabled, false);
> > if (ret)
> > return ret;
> > +
> > + mutex_lock(>mutex);
> > + for (feature_id = 0; feature_id < 64; feature_id++) {
> > + if (feature_2_disabled & (1ULL << feature_id))
> > + test_and_clear_bit(feature_id, feature-
> >enabled);
> > + }
> > + mutex_unlock(>mutex);
> > }
> >
> > return ret;
> ___
> 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] drm/amd/powerplay: update cached feature enablement status
Hi Evan,
this is know issue for me.
i think we should add update feature mask cached operation into
smu_feature_update_enable_state function.
Best Regards,
Kevin
On 8/21/19 5:24 PM, Evan Quan wrote:
> Need to update in cache feature enablement status after pp_feature
> settings. Another fix for the commit below:
> drm/amd/powerplay: implment sysfs feature status function in smu
>
> Change-Id: I90e29b0d839df26825d5993212f6097c7ad4bebf
> Signed-off-by: Evan Quan
> ---
> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index c663d25db5ab..04867cafb322 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -96,11 +96,13 @@ size_t smu_sys_get_pp_feature_mask(struct smu_context
> *smu, char *buf)
>
> int smu_sys_set_pp_feature_mask(struct smu_context *smu, uint64_t new_mask)
> {
> + struct smu_feature *feature = >smu_feature;
> int ret = 0;
> uint32_t feature_mask[2] = { 0 };
> uint64_t feature_2_enabled = 0;
> uint64_t feature_2_disabled = 0;
> uint64_t feature_enables = 0;
> + uint64_t feature_id;
>
> ret = smu_feature_get_enabled_mask(smu, feature_mask, 2);
> if (ret)
> @@ -115,11 +117,25 @@ int smu_sys_set_pp_feature_mask(struct smu_context
> *smu, uint64_t new_mask)
> ret = smu_feature_update_enable_state(smu, feature_2_enabled,
> true);
> if (ret)
> return ret;
> +
> + mutex_lock(>mutex);
> + for (feature_id = 0; feature_id < 64; feature_id++) {
> + if (feature_2_enabled & (1ULL << feature_id))
> + test_and_set_bit(feature_id, feature->enabled);
> + }
> + mutex_unlock(>mutex);
> }
> if (feature_2_disabled) {
> ret = smu_feature_update_enable_state(smu, feature_2_disabled,
> false);
> if (ret)
> return ret;
> +
> + mutex_lock(>mutex);
> + for (feature_id = 0; feature_id < 64; feature_id++) {
> + if (feature_2_disabled & (1ULL << feature_id))
> + test_and_clear_bit(feature_id,
> feature->enabled);
> + }
> + mutex_unlock(>mutex);
> }
>
> return ret;
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
