Re: [PATCH -next] drm/amdgpu: Fix return value check in amdgpu_allocate_static_csa()
Hi Yongjun,
This issue has been fixed.
Thanks.
Best Regards
Rex
commit 51f1f6f51712aade68cabb145ed8bab4a6c3997e
Author: Rex Zhu
Date: Fri Nov 23 18:52:21 2018 +0800
drm/amdgpu: Fix static checker warning
drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c:49 amdgpu_allocate_static_csa()
error: uninitialized symbol 'ptr'.
the test if (!bo) doesn't work, as the bo is a pointer to a pointer.
if bo create failed, the *bo will be set to NULL.
so change to test *bo.
Reviewed-by: Christian König
Signed-off-by: Rex Zhu
Signed-off-by: Alex Deucher
From: Wei Yongjun
Sent: Tuesday, December 4, 2018 2:39 PM
To: Deucher, Alexander; Koenig, Christian; Zhou, David(ChunMing);
airl...@linux.ie; Zhu, Rex; Liu, Monk
Cc: Wei Yongjun; amd-...@lists.freedesktop.org;
dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org;
kernel-janit...@vger.kernel.org
Subject: [PATCH -next] drm/amdgpu: Fix return value check in
amdgpu_allocate_static_csa()
Fix the return value check which testing the wrong variable
in amdgpu_allocate_static_csa().
Fixes: 7946340fa389 ("drm/amdgpu: Move csa related code to separate file")
Signed-off-by: Wei Yongjun
---
drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
index 0c590dd..a5fbc6f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_csa.c
@@ -43,7 +43,7 @@ int amdgpu_allocate_static_csa(struct amdgpu_device *adev,
struct amdgpu_bo **bo
r = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
domain, bo,
NULL, );
- if (!bo)
+ if (!r)
return -ENOMEM;
memset(ptr, 0, size);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu/pm: Remove VLA usage
Patch has been applied to our internal amd-staging-drm-next tree.
Thanks.
Best Regards
Rex
From: Christian K?nig
Sent: Tuesday, July 17, 2018 2:58:21 PM
To: Zhu, Rex; Kees Cook; Deucher, Alexander
Cc: Zhou, David(ChunMing); David Airlie; Kuehling, Felix; LKML; amd-gfx list;
Huang, Ray; Maling list - DRI developers; Koenig, Christian
Subject: Re: [PATCH] drm/amdgpu/pm: Remove VLA usage
Who's tree should this go through?
To answer the question: When Rex is ok with that he pushes it to our internal
amd-staging-drm-next tree.
Alex then pushes that tree to a public server and at some point sends a pull
request for inclusion in drm-next.
Regards,
Christian.
Am 17.07.2018 um 08:23 schrieb Zhu, Rex:
Patch is:
Reviewed-by: Rex Zhumailto:re...@amd.com>>
Best Regards
Rex
From: keesc...@google.com<mailto:keesc...@google.com>
<mailto:keesc...@google.com> on behalf of Kees Cook
<mailto:keesc...@chromium.org>
Sent: Tuesday, July 17, 2018 11:59 AM
To: Deucher, Alexander
Cc: LKML; Koenig, Christian; Zhou, David(ChunMing); David Airlie; Zhu, Rex;
Huang, Ray; Kuehling, Felix; amd-gfx list; Maling list - DRI developers
Subject: Re: [PATCH] drm/amdgpu/pm: Remove VLA usage
On Wed, Jun 20, 2018 at 11:26 AM, Kees Cook
<mailto:keesc...@chromium.org> wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> uses the maximum sane buffer size and removes copy/paste code.
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook <mailto:keesc...@chromium.org>
Friendly ping! Who's tree should this go through?
Thanks!
-Kees
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 100 +++--
> 1 file changed, 42 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index b455da487782..5eb98cde22ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -593,40 +593,59 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device
> *dev,
> return snprintf(buf, PAGE_SIZE, "\n");
> }
>
> -static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
> - struct device_attribute *attr,
> - const char *buf,
> - size_t count)
> +/*
> + * Worst case: 32 bits individually specified, in octal at 12 characters
> + * per line (+1 for \n).
> + */
> +#define AMDGPU_MASK_BUF_MAX(32 * 13)
> +
> +static ssize_t amdgpu_read_mask(const char *buf, size_t count, uint32_t
> *mask)
> {
> - struct drm_device *ddev = dev_get_drvdata(dev);
> - struct amdgpu_device *adev = ddev->dev_private;
> int ret;
> long level;
> - uint32_t mask = 0;
> char *sub_str = NULL;
> char *tmp;
> - char buf_cpy[count];
> + char buf_cpy[AMDGPU_MASK_BUF_MAX + 1];
> const char delimiter[3] = {' ', '\n', '\0'};
> + size_t bytes;
>
> - memcpy(buf_cpy, buf, count+1);
> + *mask = 0;
> +
> + bytes = min(count, sizeof(buf_cpy) - 1);
> + memcpy(buf_cpy, buf, bytes);
> + buf_cpy[bytes] = '\0';
> tmp = buf_cpy;
> while (tmp[0]) {
> - sub_str = strsep(, delimiter);
> + sub_str = strsep(, delimiter);
> if (strlen(sub_str)) {
> ret = kstrtol(sub_str, 0, );
> -
> - if (ret) {
> - count = -EINVAL;
> - goto fail;
> - }
> - mask |= 1 << level;
> + if (ret)
> + return -EINVAL;
> + *mask |= 1 << level;
> } else
> break;
> }
> +
> + return 0;
> +}
> +
> +static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> + int ret;
> + uint32_t mask = 0;
> +
> + ret = amdgpu_read_mask(buf, count, );
> + if (ret)
> + return ret;
> +
> if (adev->powerplay.pp_funcs->force_clock_level)
> amdgpu_dpm_force_clock_level(adev, PP_SCLK, mask);
>
> -fail:
> return count;
> }
>
> @@ -651,32 +670,15 @@ static ssize_t amdgpu_set_pp_dpm_mclk(struct device
&
Re: [PATCH] drm/amdgpu/pm: Remove VLA usage
Patch is:
Reviewed-by: Rex Zhumailto:re...@amd.com>>
Best Regards
Rex
From: keesc...@google.com on behalf of Kees Cook
Sent: Tuesday, July 17, 2018 11:59 AM
To: Deucher, Alexander
Cc: LKML; Koenig, Christian; Zhou, David(ChunMing); David Airlie; Zhu, Rex;
Huang, Ray; Kuehling, Felix; amd-gfx list; Maling list - DRI developers
Subject: Re: [PATCH] drm/amdgpu/pm: Remove VLA usage
On Wed, Jun 20, 2018 at 11:26 AM, Kees Cook wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> uses the maximum sane buffer size and removes copy/paste code.
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook
Friendly ping! Who's tree should this go through?
Thanks!
-Kees
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 100 +++--
> 1 file changed, 42 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index b455da487782..5eb98cde22ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -593,40 +593,59 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device
> *dev,
> return snprintf(buf, PAGE_SIZE, "\n");
> }
>
> -static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
> - struct device_attribute *attr,
> - const char *buf,
> - size_t count)
> +/*
> + * Worst case: 32 bits individually specified, in octal at 12 characters
> + * per line (+1 for \n).
> + */
> +#define AMDGPU_MASK_BUF_MAX(32 * 13)
> +
> +static ssize_t amdgpu_read_mask(const char *buf, size_t count, uint32_t
> *mask)
> {
> - struct drm_device *ddev = dev_get_drvdata(dev);
> - struct amdgpu_device *adev = ddev->dev_private;
> int ret;
> long level;
> - uint32_t mask = 0;
> char *sub_str = NULL;
> char *tmp;
> - char buf_cpy[count];
> + char buf_cpy[AMDGPU_MASK_BUF_MAX + 1];
> const char delimiter[3] = {' ', '\n', '\0'};
> + size_t bytes;
>
> - memcpy(buf_cpy, buf, count+1);
> + *mask = 0;
> +
> + bytes = min(count, sizeof(buf_cpy) - 1);
> + memcpy(buf_cpy, buf, bytes);
> + buf_cpy[bytes] = '\0';
> tmp = buf_cpy;
> while (tmp[0]) {
> - sub_str = strsep(, delimiter);
> + sub_str = strsep(, delimiter);
> if (strlen(sub_str)) {
> ret = kstrtol(sub_str, 0, );
> -
> - if (ret) {
> - count = -EINVAL;
> - goto fail;
> - }
> - mask |= 1 << level;
> + if (ret)
> + return -EINVAL;
> + *mask |= 1 << level;
> } else
> break;
> }
> +
> + return 0;
> +}
> +
> +static ssize_t amdgpu_set_pp_dpm_sclk(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> + int ret;
> + uint32_t mask = 0;
> +
> + ret = amdgpu_read_mask(buf, count, );
> + if (ret)
> + return ret;
> +
> if (adev->powerplay.pp_funcs->force_clock_level)
> amdgpu_dpm_force_clock_level(adev, PP_SCLK, mask);
>
> -fail:
> return count;
> }
>
> @@ -651,32 +670,15 @@ static ssize_t amdgpu_set_pp_dpm_mclk(struct device
> *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = ddev->dev_private;
> int ret;
> - long level;
> uint32_t mask = 0;
> - char *sub_str = NULL;
> - char *tmp;
> - char buf_cpy[count];
> - const char delimiter[3] = {' ', '\n', '\0'};
>
> - memcpy(buf_cpy, buf, count+1);
> - tmp = buf_cpy;
> - while (tmp[0]) {
> - sub_str = strsep(, delimiter);
> - if (strlen(sub_str)) {
> - ret = kstrtol(sub_str, 0, );
> + ret = amdgpu_read_mask(buf, count, );
> + if (ret)
> + return ret;
>
> - if (ret) {
> - count = -EINVAL;
> - goto fail;
> -
Re: [PATCH] drm/amd/pp: Fix uninitialized variable
Applied. Thanks.
Best Regards
Rex
From: rajan.v...@gmail.com
Sent: Monday, June 18, 2018 3:31 PM
To: Deucher, Alexander; Koenig, Christian; Zhou, David(ChunMing); Zhu, Rex;
StDenis, Tom
Cc: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
linux-ker...@vger.kernel.org; Rajan Vaja
Subject: [PATCH] drm/amd/pp: Fix uninitialized variable
From: Rajan Vaja
Initialize variable to 0 before performing logical OR operation.
Signed-off-by: Rajan Vaja
---
drivers/gpu/drm/amd/powerplay/hwmgr/vega10_powertune.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_powertune.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_powertune.c
index a9efd855..bde01d4 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_powertune.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_powertune.c
@@ -1090,7 +1090,7 @@ static int vega10_disable_se_edc_config(struct pp_hwmgr
*hwmgr)
static int vega10_enable_psm_gc_edc_config(struct pp_hwmgr *hwmgr)
{
struct amdgpu_device *adev = hwmgr->adev;
- int result;
+ int result = 0;
uint32_t num_se = 0;
uint32_t count, data;
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH][next] drm/amd/pp: Fix spelling mistake: "suppported" -> "supported"
Applied and thanks.
Best Regards
Rex
From: Colin King <colin.k...@canonical.com>
Sent: Tuesday, March 27, 2018 4:32 PM
To: Deucher, Alexander; Koenig, Christian; Zhou, David(ChunMing); David Airlie;
Zhu, Rex; Quan, Evan; amd-...@lists.freedesktop.org;
dri-devel@lists.freedesktop.org
Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Subject: [PATCH][next] drm/amd/pp: Fix spelling mistake: "suppported" ->
"supported"
From: Colin Ian King <colin.k...@canonical.com>
Trivial fix to spelling mistake in pr_warn warning message text
Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
drivers/gpu/drm/amd/powerplay/hwmgr/pp_psm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/pp_psm.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/pp_psm.c
index 0f2851b5b368..308bff2b5d1d 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/pp_psm.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/pp_psm.c
@@ -46,7 +46,7 @@ int psm_init_power_state_table(struct pp_hwmgr *hwmgr)
sizeof(struct pp_power_state);
if (table_entries == 0 || size == 0) {
- pr_warn("Please check whether power state management is
suppported on this asic\n");
+ pr_warn("Please check whether power state management is
supported on this asic\n");
return 0;
}
--
2.15.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/pp: use mlck_table.count for array loop index limit
Thanks. Will apply the patch to drm-next.
Best Regards
Rex
From: Joe Perches <j...@perches.com>
Sent: Thursday, March 22, 2018 3:02 AM
To: Colin King; Koenig, Christian; Zhou, David(ChunMing); David Airlie; Zhu,
Rex; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH] drm/amd/pp: use mlck_table.count for array loop index limit
On Wed, 2018-03-21 at 18:26 +, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> The for-loops process data in the mclk_table but use slck_table.count
> as the loop index limit. I believe these are cut-n-paste errors from
> the previous almost identical loops as indicated by static analysis.
> Fix these.
Nice tool.
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
[]
> @@ -855,7 +855,7 @@ static int smu7_odn_initial_default_setting(struct
> pp_hwmgr *hwmgr)
>
>odn_table->odn_memory_clock_dpm_levels.num_of_pl =
>
> data->golden_dpm_table.mclk_table.count;
> - for (i=0; igolden_dpm_table.sclk_table.count; i++) {
> + for (i=0; igolden_dpm_table.mclk_table.count; i++) {
>odn_table->odn_memory_clock_dpm_levels.entries[i].clock =
>
> data->golden_dpm_table.mclk_table.dpm_levels[i].value;
>odn_table->odn_memory_clock_dpm_levels.entries[i].enabled =
> true;
Probably more sensible to use temporaries too.
Maybe something like the below (also trivially reduces object size)
---
drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
index df2a312ca6c9..339b897146af 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
@@ -834,6 +834,7 @@ static int smu7_odn_initial_default_setting(struct pp_hwmgr
*hwmgr)
struct phm_ppt_v1_clock_voltage_dependency_table *dep_sclk_table;
struct phm_ppt_v1_clock_voltage_dependency_table *dep_mclk_table;
+ struct phm_odn_performance_level *entries;
if (table_info == NULL)
return -EINVAL;
@@ -843,11 +844,11 @@ static int smu7_odn_initial_default_setting(struct
pp_hwmgr *hwmgr)
odn_table->odn_core_clock_dpm_levels.num_of_pl =
data->golden_dpm_table.sclk_table.count;
+ entries = odn_table->odn_core_clock_dpm_levels.entries;
for (i=0; igolden_dpm_table.sclk_table.count; i++) {
- odn_table->odn_core_clock_dpm_levels.entries[i].clock =
-
data->golden_dpm_table.sclk_table.dpm_levels[i].value;
- odn_table->odn_core_clock_dpm_levels.entries[i].enabled = true;
- odn_table->odn_core_clock_dpm_levels.entries[i].vddc =
dep_sclk_table->entries[i].vddc;
+ entries[i].clock =
data->golden_dpm_table.sclk_table.dpm_levels[i].value;
+ entries[i].enabled = true;
+ entries[i].vddc = dep_sclk_table->entries[i].vddc;
}
smu7_get_voltage_dependency_table(dep_sclk_table,
@@ -855,11 +856,11 @@ static int smu7_odn_initial_default_setting(struct
pp_hwmgr *hwmgr)
odn_table->odn_memory_clock_dpm_levels.num_of_pl =
data->golden_dpm_table.mclk_table.count;
- for (i=0; igolden_dpm_table.sclk_table.count; i++) {
- odn_table->odn_memory_clock_dpm_levels.entries[i].clock =
-
data->golden_dpm_table.mclk_table.dpm_levels[i].value;
- odn_table->odn_memory_clock_dpm_levels.entries[i].enabled =
true;
- odn_table->odn_memory_clock_dpm_levels.entries[i].vddc =
dep_mclk_table->entries[i].vddc;
+ entries = odn_table->odn_memory_clock_dpm_levels.entries;
+ for (i=0; igolden_dpm_table.mclk_table.count; i++) {
+ entries[i].clock =
data->golden_dpm_table.mclk_table.dpm_levels[i].value;
+ entries[i].enabled = true;
+ entries[i].vddc = dep_mclk_table->entries[i].vddc;
}
smu7_get_voltage_dependency_table(dep_mclk_table,
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH][next] drm/amd/pp: remove redundant pointer internal_buf
Apply it and thanks.
Best Regards
Rex
From: Colin King <colin.k...@canonical.com>
Sent: Tuesday, March 13, 2018 9:22 PM
To: Deucher, Alexander; Koenig, Christian; Zhou, David(ChunMing); David Airlie;
Zhu, Rex; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Subject: [PATCH][next] drm/amd/pp: remove redundant pointer internal_buf
From: Colin Ian King <colin.k...@canonical.com>
The pointer internal_buf is assigned a value but the pointer is never
read, hence it is redundant and can be removed.
Cleans up clang warning:
drivers/gpu/drm/amd/amdgpu/../powerplay/smumgr/smu7_smumgr.c:630:2:
warning: Value stored to 'internal_buf' is never read
Signed-off-by: Colin Ian King <colin.k...@canonical.com>
---
drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
index 7394bb46b8b2..dcb151cabc00 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu7_smumgr.c
@@ -585,7 +585,6 @@ int smu7_setup_pwr_virus(struct pp_hwmgr *hwmgr)
int smu7_init(struct pp_hwmgr *hwmgr)
{
struct smu7_smumgr *smu_data;
- uint8_t *internal_buf;
uint64_t mc_addr = 0;
int r;
/* Allocate memory for backend private data */
@@ -627,7 +626,6 @@ int smu7_init(struct pp_hwmgr *hwmgr)
_data->header_buffer.kaddr);
return -EINVAL;
}
- internal_buf = smu_data->smu_buffer.kaddr;
smu_data->smu_buffer.mc_addr = mc_addr;
if (smum_is_hw_avfs_present(hwmgr))
--
2.15.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [amd-staging-drm-next] regression - no fan info (sensors) on RX580
Yes, caused by the commit e37a7b4088da
("drm/amd/powerplay: tidy up ret checks in amd_powerplay.c")
Replace error when split patches.
Have sent the fix patch.
Please review.
Best Regards
Rex
-Original Message-
From: Alex Deucher [mailto:alexdeuc...@gmail.com]
Sent: Friday, September 29, 2017 10:11 PM
To: Dieter Nützel; Zhu, Rex
Cc: amd-devel; DRI Devel; Wentland, Harry; Michel Dänzer
Subject: Re: [amd-staging-drm-next] regression - no fan info (sensors) on RX580
Rex, probably related to the recent cleanups in powerplay.
On Fri, Sep 29, 2017 at 10:09 AM, Dieter Nützel <die...@nuetzel-hh.de> wrote:
> Hello all,
>
> since latest update
>
> 1d7da702e70d3c27408a3bb312c71d6be9f7bebe
> drm/amd/powerplay: fix spelling mistake: "dividable" -> "divisible"
>
> I didn't get fan info with my RX580 (Polaris21) any longer.
>
> Worked with this commit:
>
> 786df0b89fe5a0b405d4de0a1ce03003c0743ec3
> drm/amd/display: fix pflip irq registor for raven
>
> Sorry, I do not have full time for bisect, because we are on way to
> our vacation.
>
> Maybe in the evening (only a few commits).
>
> Greetings,
> Dieter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/powerplay/hwmgr: Remove null check before kfree
Reviewed-by: Rex Zhu <rex@amd.com<mailto:rex@amd.com>>
Best Regards
Rex
From: Wentland, Harry
Sent: Tuesday, August 29, 2017 9:34:01 PM
To: Himanshu Jha; airl...@linux.ie
Cc: linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org;
amd-...@lists.freedesktop.org; Deucher, Alexander; Zhu, Rex; Koenig, Christian
Subject: Re: [PATCH] drm/amd/powerplay/hwmgr: Remove null check before kfree
On 2017-08-29 09:12 AM, Himanshu Jha wrote:
> kfree on NULL pointer is a no-op and therefore checking is redundant.
>
> Signed-off-by: Himanshu Jha <himanshujha199...@gmail.com>
Reviewed-by: Harry Wentland <harry.wentl...@amd.com>
Harry
> ---
> drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 6 +-
> .../gpu/drm/amd/powerplay/hwmgr/processpptables.c | 96
> --
> drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c | 52
> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 12 +--
> 4 files changed, 56 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> index bc839ff..9f2c037 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> @@ -1225,10 +1225,8 @@ static int cz_hwmgr_backend_fini(struct pp_hwmgr
> *hwmgr)
>phm_destroy_table(hwmgr, &(hwmgr->power_down_asic));
>phm_destroy_table(hwmgr, &(hwmgr->setup_asic));
>
> - if (NULL != hwmgr->dyn_state.vddc_dep_on_dal_pwrl) {
> - kfree(hwmgr->dyn_state.vddc_dep_on_dal_pwrl);
> - hwmgr->dyn_state.vddc_dep_on_dal_pwrl = NULL;
> - }
> + kfree(hwmgr->dyn_state.vddc_dep_on_dal_pwrl);
> + hwmgr->dyn_state.vddc_dep_on_dal_pwrl = NULL;
>
>kfree(hwmgr->backend);
>hwmgr->backend = NULL;
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c
> index 2716721..a6dbc55 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/processpptables.c
> @@ -1615,85 +1615,53 @@ static int pp_tables_uninitialize(struct pp_hwmgr
> *hwmgr)
>if (hwmgr->chip_id == CHIP_RAVEN)
>return 0;
>
> - if (NULL != hwmgr->dyn_state.vddc_dependency_on_sclk) {
> - kfree(hwmgr->dyn_state.vddc_dependency_on_sclk);
> - hwmgr->dyn_state.vddc_dependency_on_sclk = NULL;
> - }
> + kfree(hwmgr->dyn_state.vddc_dependency_on_sclk);
> + hwmgr->dyn_state.vddc_dependency_on_sclk = NULL;
>
> - if (NULL != hwmgr->dyn_state.vddci_dependency_on_mclk) {
> - kfree(hwmgr->dyn_state.vddci_dependency_on_mclk);
> - hwmgr->dyn_state.vddci_dependency_on_mclk = NULL;
> - }
> + kfree(hwmgr->dyn_state.vddci_dependency_on_mclk);
> + hwmgr->dyn_state.vddci_dependency_on_mclk = NULL;
>
> - if (NULL != hwmgr->dyn_state.vddc_dependency_on_mclk) {
> - kfree(hwmgr->dyn_state.vddc_dependency_on_mclk);
> - hwmgr->dyn_state.vddc_dependency_on_mclk = NULL;
> - }
> + kfree(hwmgr->dyn_state.vddc_dependency_on_mclk);
> + hwmgr->dyn_state.vddc_dependency_on_mclk = NULL;
>
> - if (NULL != hwmgr->dyn_state.mvdd_dependency_on_mclk) {
> - kfree(hwmgr->dyn_state.mvdd_dependency_on_mclk);
> - hwmgr->dyn_state.mvdd_dependency_on_mclk = NULL;
> - }
> + kfree(hwmgr->dyn_state.mvdd_dependency_on_mclk);
> + hwmgr->dyn_state.mvdd_dependency_on_mclk = NULL;
>
> - if (NULL != hwmgr->dyn_state.valid_mclk_values) {
> - kfree(hwmgr->dyn_state.valid_mclk_values);
> - hwmgr->dyn_state.valid_mclk_values = NULL;
> - }
> + kfree(hwmgr->dyn_state.valid_mclk_values);
> + hwmgr->dyn_state.valid_mclk_values = NULL;
>
> - if (NULL != hwmgr->dyn_state.valid_sclk_values) {
> - kfree(hwmgr->dyn_state.valid_sclk_values);
> - hwmgr->dyn_state.valid_sclk_values = NULL;
> - }
> + kfree(hwmgr->dyn_state.valid_sclk_values);
> + hwmgr->dyn_state.valid_sclk_values = NULL;
>
> - if (NULL != hwmgr->dyn_state.cac_leakage_table) {
> - kfree(hwmgr->dyn_state.cac_leakage_table);
> - hwmgr->dyn_state.cac_leakage_table = NULL;
> - }
> + kfree(hwmgr->dyn_state.cac_leakage_table);
> + hwmgr->dyn_state.cac_leakage_table = NULL;
>
> - if
Re: [PATCH] drm/amd/powerplay: fix a signedness bugs
Patches has been applied.
Thanks.
Best Regards
Rex
From: Huang, JinHuiEric
Sent: Friday, May 19, 2017 12:28:09 AM
To: Dan Carpenter; Deucher, Alexander; Zhu, Rex
Cc: Koenig, Christian; David Airlie; Wang, Ken; Huang, Ray;
amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
kernel-janit...@vger.kernel.org
Subject: Re: [PATCH] drm/amd/powerplay: fix a signedness bugs
Reviewed-by: Eric Huang <jinhuieric.hu...@amd.com>
On 2017-05-16 10:42 AM, Dan Carpenter wrote:
> Smatch complains about a signedness bug here:
>
>vega10_hwmgr.c:4202 vega10_force_clock_level()
>warn: always true condition '(i >= 0) => (0-u32max >= 0)'
>
> Fixes: 7b52db39a4c2 ("drm/amd/powerplay: fix bug sclk/mclk level can't be set
> on vega10.")
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index ad30f5d3a10d..2614af2f553f 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> @@ -4186,7 +4186,7 @@ static int vega10_force_clock_level(struct pp_hwmgr
> *hwmgr,
>enum pp_clock_type type, uint32_t mask)
> {
>struct vega10_hwmgr *data = (struct vega10_hwmgr *)(hwmgr->backend);
> - uint32_t i;
> + int i;
>
>if (hwmgr->dpm_level != AMD_DPM_FORCED_LEVEL_MANUAL)
>return -EINVAL;
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/powerplay: header should be defining _SMU7_CLOCK_POWER_GATING_H_
Thanks. Reviewed-by: Rex Zhu <rex@amd.com> Best Regards Rex From: Colin King <colin.k...@canonical.com> Sent: Wednesday, January 25, 2017 8:07 PM To: Deucher, Alexander; Koenig, Christian; David Airlie; Wang, Ken; Daenzer, Michel; Zhu, Rex; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org Subject: [PATCH] drm/amd/powerplay: header should be defining _SMU7_CLOCK_POWER_GATING_H_ From: Colin Ian King <colin.k...@canonical.com> _SMU7_CLOCK_POWER_GATING_H_ is being used as a header guard, followed by a #define of a different macro. Define _SMU7_CLOCK_POWER_GATING_H_ instead to fix this. Signed-off-by: Colin Ian King <colin.k...@canonical.com> --- drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h index d52a28c..c96ed9e 100644 --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h @@ -22,7 +22,7 @@ */ #ifndef _SMU7_CLOCK_POWER_GATING_H_ -#define _SMU7_CLOCK__POWER_GATING_H_ +#define _SMU7_CLOCK_POWER_GATING_H_ #include "smu7_hwmgr.h" #include "pp_asicblocks.h" -- 2.10.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/powerplay: don't give up if DPM is already running
Hi Gražvydas, In GPU passthrough case, I can't find better solution than yours from driver. So will apply your patch and just update reproduce step. Thanks. Best Regards Rex -Original Message- From: Zhu, Rex Sent: Wednesday, October 19, 2016 10:00 PM To: 'Grazvydas Ignotas' Cc: Alex Deucher; amd-gfx list; Maling list - DRI developers Subject: RE: [PATCH] drm/amd/powerplay: don't give up if DPM is already running Hi Gražvydas, Sorry for so late response. I can reproduce this issue in gpu passthrough case. When we forced power off the VM or forced reset the VM without shutting down the Os, the rmmod will not be called. So dpm was still running. If we skipped the enable dpm tasks, some clock tables were not be initialized, so kernel panic when we visited those tables in set power state. I will send the fix patch for review tomorrow. Thanks. Best Regards Rex -Original Message- From: Grazvydas Ignotas [mailto:nota...@gmail.com] Sent: Sunday, October 16, 2016 2:55 AM To: Zhu, Rex Cc: Alex Deucher; amd-gfx list; Maling list - DRI developers Subject: Re: [PATCH] drm/amd/powerplay: don't give up if DPM is already running Hi, On Thu, Oct 13, 2016 at 10:45 AM, Zhu, Rex wrote: > > The attached patches were also for this issue. > Disable dpm when rmmod amdgpu. It works for modprobe-rmmod-modprobe test, thanks. However with GPU passthrough (giving control of the GPU to a Windows virtual machine using iommu, then shutting down the VM and loading amdgpu) the problem is still there, same backtrace as in my commit message. It seems the Windows driver leaves DPM enabled on shutdown. With my patch the crash goes away. It would be nice to have GPU passthrough working, this is the only issue that breaks it. Windows can drive the GPU after amdgpu fine already, only amdgpu has problems to run after Windows. Gražvydas -- next part -- A non-text attachment was scrubbed... Name: 0001-drm-amd-powerplay-don-t-give-up-if-DPM-is-already-ru.patch Type: application/octet-stream Size: 1642 bytes Desc: 0001-drm-amd-powerplay-don-t-give-up-if-DPM-is-already-ru.patch URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161020/275e3a07/attachment.obj>
[PATCH] drm/amd/powerplay: don't give up if DPM is already running
Hi Gražvydas, Sorry for so late response. I can reproduce this issue in gpu passthrough case. When we forced power off the VM or forced reset the VM without shutting down the Os, the rmmod will not be called. So dpm was still running. If we skipped the enable dpm tasks, some clock tables were not be initialized, so kernel panic when we visited those tables in set power state. I will send the fix patch for review tomorrow. Thanks. Best Regards Rex -Original Message- From: Grazvydas Ignotas [mailto:nota...@gmail.com] Sent: Sunday, October 16, 2016 2:55 AM To: Zhu, Rex Cc: Alex Deucher; amd-gfx list; Maling list - DRI developers Subject: Re: [PATCH] drm/amd/powerplay: don't give up if DPM is already running Hi, On Thu, Oct 13, 2016 at 10:45 AM, Zhu, Rex wrote: > > The attached patches were also for this issue. > Disable dpm when rmmod amdgpu. It works for modprobe-rmmod-modprobe test, thanks. However with GPU passthrough (giving control of the GPU to a Windows virtual machine using iommu, then shutting down the VM and loading amdgpu) the problem is still there, same backtrace as in my commit message. It seems the Windows driver leaves DPM enabled on shutdown. With my patch the crash goes away. It would be nice to have GPU passthrough working, this is the only issue that breaks it. Windows can drive the GPU after amdgpu fine already, only amdgpu has problems to run after Windows. Gražvydas
[bug report] drm/amd/powerplay: add vce state tables initialize for ppt v1.
Thanks, please help to review the attached patch.
Best Regards
Rex
-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
Sent: Thursday, October 13, 2016 9:26 PM
To: Zhu, Rex
Cc: dri-devel at lists.freedesktop.org
Subject: [bug report] drm/amd/powerplay: add vce state tables initialize for
ppt v1.
Hello Rex Zhu,
The patch 48d7b759a8bc: "drm/amd/powerplay: add vce state tables initialize for
ppt v1." from Aug 31, 2016, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/process_pptables_v1_0.c:1211
ppt_get_num_of_vce_state_table_entries_v1_0()
warn: 'vce_state_table' can't be NULL.
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/process_pptables_v1_0.c
1200
1201 static int ppt_get_num_of_vce_state_table_entries_v1_0(struct pp_hwmgr
*hwmgr)
1202 {
1203 const ATOM_Tonga_POWERPLAYTABLE *pp_table =
get_powerplay_table(hwmgr);
1204 const ATOM_Tonga_VCE_State_Table *vce_state_table =
1205 (ATOM_Tonga_VCE_State_Table
*)(((unsigned long)pp_table) + le16_to_cpu(pp_table->usVCEStateTableOffset));
pp_table can't be NULL because we're
dereferencing it here. That means vce_state_table can't be NULL either.
1206
1207 if (vce_state_table == NULL)
1208 return 0;
1209
1210 return vce_state_table->ucNumEntries;
1211 }
1212
A cleaner way to write this is maybe:
static int ppt_get_num_of_vce_state_table_entries_v1_0(struct pp_hwmgr *hwmgr) {
const ATOM_Tonga_POWERPLAYTABLE *pp_table = get_powerplay_table(hwmgr);
const ATOM_Tonga_VCE_State_Table *vce_state_table;
if (!pp_table)
return 0;
vce_state_table = (void *)pp_table +
le16_to_cpu(pp_table->usVCEStateTableOffset);
return vce_state_table->ucNumEntries;
}
regards,
dan carpenter
-- next part --
A non-text attachment was scrubbed...
Name: 0001-drm-amd-powerplay-fix-static-checker-warning-in-proc.patch
Type: application/octet-stream
Size: 1439 bytes
Desc: 0001-drm-amd-powerplay-fix-static-checker-warning-in-proc.patch
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161017/cf338c79/attachment-0001.obj>
[bug report] drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7.
Thanks, Alex.
Patch was Reviewed-by: Rex Zhu
Best Regards
Rex
-Original Message-
From: Alex Deucher [mailto:alexdeuc...@gmail.com]
Sent: Friday, October 14, 2016 11:14 PM
To: Dan Carpenter
Cc: Zhu, Rex; Maling list - DRI developers
Subject: Re: [bug report] drm/amd/powerplay: implement smu7 hwmgr to manager
asics with smu ip version 7.
On Fri, Oct 14, 2016 at 10:32 AM, Dan Carpenter
wrote:
> Hello Rex Zhu,
>
> The patch 599a7e9fe1b6: "drm/amd/powerplay: implement smu7 hwmgr to
> manager asics with smu ip version 7." from Sep 9, 2016, leads to the
> following static checker warning:
>
> drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:2125
> smu7_patch_limits_vddc()
> warn: passing casted pointer '>vddc' to
> 'smu7_patch_ppt_v0_with_vdd_leakage()' 16 vs 32.
>
> drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c
> 2119 static int smu7_patch_limits_vddc(struct pp_hwmgr *hwmgr,
> 2120 struct
> phm_clock_and_voltage_limits *tab)
> 2121 {
> 2122 struct smu7_hwmgr *data = (struct smu7_hwmgr
> *)(hwmgr->backend);
> 2123
> 2124 if (tab) {
> 2125 smu7_patch_ppt_v0_with_vdd_leakage(hwmgr, (uint32_t
> *)>vddc,
> 2126
> >vddc_leakage);
>
> This call corrupts vddci.
>
> 2127 smu7_patch_ppt_v0_with_vdd_leakage(hwmgr, (uint32_t
> *)>vddci,
> 2128
> >vddci_leakage);
>
> But that's fine since we immediately overwrite it here. But
> unfortunately this call corrupt tab->vddgfx.
Thanks. Should be fixed in the attached patch.
Alex
>
> 2129 }
> 2130
> 2131 return 0;
> 2132 }
>
> regards,
> dan carpenter
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/powerplay: don't give up if DPM is already running
Hi Alex,
Your new attached patch is Tested-by and Reviewed-by: Rex Zhu
Just one question,
Do we need to set clock_gate for smu?
Best Regards
Rex
-Original Message-
From: Alex Deucher [mailto:alexdeuc...@gmail.com]
Sent: Thursday, October 13, 2016 11:29 PM
To: Zhu, Rex
Cc: Grazvydas Ignotas; amd-gfx list; Maling list - DRI developers
Subject: Re: [PATCH] drm/amd/powerplay: don't give up if DPM is already running
On Thu, Oct 13, 2016 at 3:45 AM, Zhu, Rex wrote:
> Hi all,
>
> The attached patches were also for this issue.
> Disable dpm when rmmod amdgpu.
>
> Please help to review.
Patch 1:
+r = adev->ip_blocks[AMD_IP_BLOCK_TYPE_SMC].funcs->hw_fini((void *)adev);
+if (r)
+DRM_DEBUG("hw_fini of IP block <%s> failed %d\n",
+adev->ip_blocks[AMD_IP_BLOCK_TYPE_SMC].funcs->name, r);
+
+adev->ip_block_status[AMD_IP_BLOCK_TYPE_SMC].hw = false;
You can't use AMD_IP_BLOCK_TYPE_* as index. Some chips may not have the IP
block. Maybe something like the attached patch. That said, I think longer
term it makes more sense to allows the SOCs to specify the order for init,
fini, etc. otherwise we'll have lots of variable logic in the common code.
Patch 2:
+if (1 == PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device,
CGS_IND_REG__SMC, FEATURE_STATUS, AVS_ON)) {
+PP_ASSERT_WITH_CODE((0 == smum_send_msg_to_smc(hwmgr->smumgr,
PPSMC_MSG_DisableAvfs)),
+"Failed to disable AVFS!",
+return -1;);
+}
Use a proper error code there like -EINVAL or something. With that fixed:
Reviewed-by: Alex Deucher
Alex
>
> Best Regards
> Rex
>
> -Original Message-
> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
> Of Zhu, Rex
> Sent: Wednesday, October 12, 2016 9:45 PM
> To: Alex Deucher; Grazvydas Ignotas
> Cc: amd-gfx list; Maling list - DRI developers
> Subject: RE: [PATCH] drm/amd/powerplay: don't give up if DPM is
> already running
>
> Hi Grazvydas and Alex,
>
> We needed to disable dpm when rmmod amdgpu for this issue.
>
> I am checking the function of disable dpm task.
>
> Best Regards
> Rex
>
> -Original Message-----
> From: Alex Deucher [mailto:alexdeucher at gmail.com]
> Sent: Wednesday, October 12, 2016 4:01 AM
> To: Grazvydas Ignotas; Zhu, Rex
> Cc: Maling list - DRI developers; amd-gfx list
> Subject: Re: [PATCH] drm/amd/powerplay: don't give up if DPM is
> already running
>
> +Rex to review this.
>
> Alex
>
> On Sun, Oct 9, 2016 at 3:23 PM, Grazvydas Ignotas
> wrote:
>> Currently the driver crashes if smu7_enable_dpm_tasks() returns
>> early, which it does if DPM is already active. It seems to be better
>> just to continue anyway, at least I haven't noticed any ill effects.
>> It's also unclear at what state the hardware was left by the previous
>> driver, so IMO it's better to always fully initialize.
>>
>> Way to reproduce:
>> $ modprobe amdgpu
>> $ rmmod amdgpu
>> $ modprobe amdgpu
>> ...
>> DPM is already running right now, no need to enable DPM!
>> ...
>> failed to send message 18b ret is 0
>> BUG: unable to handle kernel paging request at ed01fc9ab21f Call
>> Trace:
>> smu7_set_power_state_tasks+0x499/0x1940 [amdgpu]
>> phm_set_power_state+0xcb/0x120 [amdgpu]
>> psm_adjust_power_state_dynamic+0x11e/0x1b0 [amdgpu]
>> pem_task_adjust_power_state+0xb9/0xd0 [amdgpu]
>> pem_excute_event_chain+0x7d/0xe0 [amdgpu]
>> pem_handle_event_unlocked+0x49/0x60 [amdgpu]
>> pem_handle_event+0xe/0x10 [amdgpu]
>> pp_dpm_dispatch_tasks+0xe0/0x190 [amdgpu]
>> amdgpu_pm_compute_clocks+0x10c/0xc60 [amdgpu]
>> dce_v11_0_crtc_dpms+0x7d/0x150 [amdgpu]
>> dce_v11_0_crtc_disable+0x90/0x4a0 [amdgpu]
>> drm_helper_disable_unused_functions+0x67/0x80 [drm_kms_helper]
>> amdgpu_fbdev_init+0x13e/0x170 [amdgpu]
>> amdgpu_device_init+0x1aeb/0x2490 [amdgpu]
>>
>> Signed-off-by: Grazvydas Ignotas
>> ---
>> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> index f6afa6a..327030b 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
>> @@ -1166,8 +1166,8 @@ static int smu7_enable_dpm_tasks(struct
>> pp_hwmgr
>> *hwmgr)
>>
>> tmp_result = (!smum_is_dpm_running(hwmgr)) ? 0 : -1;
>> PP_ASSERT_WITH_CODE(tmp_result == 0,
>> - "DPM is already runnin
[PATCH] drm/amd/powerplay: don't give up if DPM is already running
Hi all,
The attached patches were also for this issue.
Disable dpm when rmmod amdgpu.
Please help to review.
Best Regards
Rex
-Original Message-
From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of Zhu,
Rex
Sent: Wednesday, October 12, 2016 9:45 PM
To: Alex Deucher; Grazvydas Ignotas
Cc: amd-gfx list; Maling list - DRI developers
Subject: RE: [PATCH] drm/amd/powerplay: don't give up if DPM is already running
Hi Grazvydas and Alex,
We needed to disable dpm when rmmod amdgpu for this issue.
I am checking the function of disable dpm task.
Best Regards
Rex
-Original Message-
From: Alex Deucher [mailto:alexdeuc...@gmail.com]
Sent: Wednesday, October 12, 2016 4:01 AM
To: Grazvydas Ignotas; Zhu, Rex
Cc: Maling list - DRI developers; amd-gfx list
Subject: Re: [PATCH] drm/amd/powerplay: don't give up if DPM is already running
+Rex to review this.
Alex
On Sun, Oct 9, 2016 at 3:23 PM, Grazvydas Ignotas wrote:
> Currently the driver crashes if smu7_enable_dpm_tasks() returns early,
> which it does if DPM is already active. It seems to be better just to
> continue anyway, at least I haven't noticed any ill effects. It's also
> unclear at what state the hardware was left by the previous driver, so
> IMO it's better to always fully initialize.
>
> Way to reproduce:
> $ modprobe amdgpu
> $ rmmod amdgpu
> $ modprobe amdgpu
> ...
> DPM is already running right now, no need to enable DPM!
> ...
> failed to send message 18b ret is 0
> BUG: unable to handle kernel paging request at ed01fc9ab21f Call
> Trace:
> smu7_set_power_state_tasks+0x499/0x1940 [amdgpu]
> phm_set_power_state+0xcb/0x120 [amdgpu]
> psm_adjust_power_state_dynamic+0x11e/0x1b0 [amdgpu]
> pem_task_adjust_power_state+0xb9/0xd0 [amdgpu]
> pem_excute_event_chain+0x7d/0xe0 [amdgpu]
> pem_handle_event_unlocked+0x49/0x60 [amdgpu]
> pem_handle_event+0xe/0x10 [amdgpu]
> pp_dpm_dispatch_tasks+0xe0/0x190 [amdgpu]
> amdgpu_pm_compute_clocks+0x10c/0xc60 [amdgpu]
> dce_v11_0_crtc_dpms+0x7d/0x150 [amdgpu]
> dce_v11_0_crtc_disable+0x90/0x4a0 [amdgpu]
> drm_helper_disable_unused_functions+0x67/0x80 [drm_kms_helper]
> amdgpu_fbdev_init+0x13e/0x170 [amdgpu]
> amdgpu_device_init+0x1aeb/0x2490 [amdgpu]
>
> Signed-off-by: Grazvydas Ignotas
> ---
> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index f6afa6a..327030b 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -1166,8 +1166,8 @@ static int smu7_enable_dpm_tasks(struct pp_hwmgr
> *hwmgr)
>
> tmp_result = (!smum_is_dpm_running(hwmgr)) ? 0 : -1;
> PP_ASSERT_WITH_CODE(tmp_result == 0,
> - "DPM is already running right now, no need to enable
> DPM!",
> - return 0);
> + "DPM is already running",
> + );
>
> if (smu7_voltage_control(hwmgr)) {
> tmp_result = smu7_enable_voltage_control(hwmgr);
> --
> 2.7.4
>
> ___
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
-- next part --
A non-text attachment was scrubbed...
Name: 0001-drm-amdgpu-need-to-disable-dpm-first-when-rmmod-amdg.patch
Type: application/octet-stream
Size: 1469 bytes
Desc: 0001-drm-amdgpu-need-to-disable-dpm-first-when-rmmod-amdg.patch
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/c2213003/attachment.obj>
-- next part --
A non-text attachment was scrubbed...
Name: 0002-drm-amd-powerplay-fix-bug-stop-dpm-can-t-work-on-Vi.patch
Type: application/octet-stream
Size: 3840 bytes
Desc: 0002-drm-amd-powerplay-fix-bug-stop-dpm-can-t-work-on-Vi.patch
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/c2213003/attachment-0001.obj>
[bug report] drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7.
Please Review.
Best Regards
Rex
-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
Sent: Wednesday, October 12, 2016 2:12 PM
To: Zhu, Rex
Cc: dri-devel at lists.freedesktop.org
Subject: [bug report] drm/amd/powerplay: implement smu7 hwmgr to manager asics
with smu ip version 7.
Hello Rex Zhu,
This is a semi-automatic email about new static checker warnings.
The patch 599a7e9fe1b6: "drm/amd/powerplay: implement smu7 hwmgr to manager
asics with smu ip version 7." from Sep 9, 2016, leads to the following Smatch
complaint:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:1463
smu7_get_evv_voltages()
error: we previously assumed 'table_info' could be null (see line 1455)
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c
1454
1455 if (table_info != NULL)
^
Check for non-NULL.
1456 sclk_table = table_info->vdd_dep_on_sclk;
1457
1458 for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) {
1459 vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i;
1460
1461 if (data->vdd_gfx_control ==
SMU7_VOLTAGE_CONTROL_BY_SVID2) {
1462 if (0 == phm_get_sclk_for_voltage_evv(hwmgr,
1463
table_info->vddgfx_lookup_table, vv_id, )) {
^^^ Unchecked dereference.
1464 if
(phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
1465
PHM_PlatformCaps_ClockStretcher)) {
regards,
dan carpenter
-- next part --
A non-text attachment was scrubbed...
Name: 0001-drm-amd-powerplay-fix-static-checker-warnings-in-smu.patch
Type: application/octet-stream
Size: 1105 bytes
Desc: 0001-drm-amd-powerplay-fix-static-checker-warnings-in-smu.patch
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/7bf264a5/attachment-0001.obj>
[PATCH] drm/amd/powerplay: don't give up if DPM is already running
Hi Grazvydas and Alex,
We needed to disable dpm when rmmod amdgpu for this issue.
I am checking the function of disable dpm task.
Best Regards
Rex
-Original Message-
From: Alex Deucher [mailto:alexdeuc...@gmail.com]
Sent: Wednesday, October 12, 2016 4:01 AM
To: Grazvydas Ignotas; Zhu, Rex
Cc: Maling list - DRI developers; amd-gfx list
Subject: Re: [PATCH] drm/amd/powerplay: don't give up if DPM is already running
+Rex to review this.
Alex
On Sun, Oct 9, 2016 at 3:23 PM, Grazvydas Ignotas wrote:
> Currently the driver crashes if smu7_enable_dpm_tasks() returns early,
> which it does if DPM is already active. It seems to be better just to
> continue anyway, at least I haven't noticed any ill effects. It's also
> unclear at what state the hardware was left by the previous driver, so
> IMO it's better to always fully initialize.
>
> Way to reproduce:
> $ modprobe amdgpu
> $ rmmod amdgpu
> $ modprobe amdgpu
> ...
> DPM is already running right now, no need to enable DPM!
> ...
> failed to send message 18b ret is 0
> BUG: unable to handle kernel paging request at ed01fc9ab21f Call
> Trace:
> smu7_set_power_state_tasks+0x499/0x1940 [amdgpu]
> phm_set_power_state+0xcb/0x120 [amdgpu]
> psm_adjust_power_state_dynamic+0x11e/0x1b0 [amdgpu]
> pem_task_adjust_power_state+0xb9/0xd0 [amdgpu]
> pem_excute_event_chain+0x7d/0xe0 [amdgpu]
> pem_handle_event_unlocked+0x49/0x60 [amdgpu]
> pem_handle_event+0xe/0x10 [amdgpu]
> pp_dpm_dispatch_tasks+0xe0/0x190 [amdgpu]
> amdgpu_pm_compute_clocks+0x10c/0xc60 [amdgpu]
> dce_v11_0_crtc_dpms+0x7d/0x150 [amdgpu]
> dce_v11_0_crtc_disable+0x90/0x4a0 [amdgpu]
> drm_helper_disable_unused_functions+0x67/0x80 [drm_kms_helper]
> amdgpu_fbdev_init+0x13e/0x170 [amdgpu]
> amdgpu_device_init+0x1aeb/0x2490 [amdgpu]
>
> Signed-off-by: Grazvydas Ignotas
> ---
> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index f6afa6a..327030b 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -1166,8 +1166,8 @@ static int smu7_enable_dpm_tasks(struct pp_hwmgr
> *hwmgr)
>
> tmp_result = (!smum_is_dpm_running(hwmgr)) ? 0 : -1;
> PP_ASSERT_WITH_CODE(tmp_result == 0,
> - "DPM is already running right now, no need to enable
> DPM!",
> - return 0);
> + "DPM is already running",
> + );
>
> if (smu7_voltage_control(hwmgr)) {
> tmp_result = smu7_enable_voltage_control(hwmgr);
> --
> 2.7.4
>
> ___
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7.
Thanks.
Please review the attached patches.
Best Regards
Rex
-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
Sent: Tuesday, October 11, 2016 3:11 PM
To: Zhu, Rex
Cc: dri-devel at lists.freedesktop.org
Subject: re: drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu
ip version 7.
Hello Rex Zhu,
This is a semi-automatic email about new static checker warnings.
The patch 599a7e9fe1b6: "drm/amd/powerplay: implement smu7 hwmgr to manager
asics with smu ip version 7." from Sep 9, 2016, leads to the following Smatch
complaint:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:3809
smu7_check_states_equal()
warn: variable dereferenced before check 'pstate1' (see line 3805)
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c
3804 {
3805 const struct smu7_power_state *psa =
cast_const_phw_smu7_power_state(pstate1);
^^^
3806 const struct smu7_power_state *psb =
cast_const_phw_smu7_power_state(pstate2);
^^^ New dereferences.
3807 int i;
3808
3809 if (pstate1 == NULL || pstate2 == NULL || equal == NULL)
^^
Checked too late.
Just as an aside. People really don't review code in initializers.
I've commented on this before, and other people have noted it as well but other
people are like "In that case, those people shouldn't be kernel devs!" which is
so ignorant. Everyone consistently makes the same mistakes. I've spent so
many hours looking at basically this same bug over and over and everyone does
it.
That's nothing to do with your code, in particular, I just wanted to update you
on how my morning has been going. (Pretty well, thanks, although I'm due for a
second cup of coffee).
3810 return -EINVAL;
3811
regards,
dan carpenter
-- next part --
A non-text attachment was scrubbed...
Name: 0001-drm-amd-powerplay-add-array-length-check-to-avoid-bu.patch
Type: application/octet-stream
Size: 1189 bytes
Desc: 0001-drm-amd-powerplay-add-array-length-check-to-avoid-bu.patch
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161011/4cb46a8f/attachment-0002.obj>
-- next part --
A non-text attachment was scrubbed...
Name: 0002-drm-amd-powerplay-fix-bug-variable-dereferenced-befo.patch
Type: application/octet-stream
Size: 1518 bytes
Desc: 0002-drm-amd-powerplay-fix-bug-variable-dereferenced-befo.patch
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161011/4cb46a8f/attachment-0003.obj>
[PATCH 1/2] drm/amd/powerplay: mark symbols static where possible
Hi Alex and Baoyou,
I will implement and use most of the functions in Patch2 in the future.
and I will refine the code and fix the build warning.
Thanks very much.
Best Regards
Rex
-Original Message-
From: Alex Deucher [mailto:alexdeuc...@gmail.com]
Sent: Saturday, October 01, 2016 1:32 AM
To: Christian König
Cc: Baoyou Xie; Deucher, Alexander; Dave Airlie; Zhu, Rex; Huang, JinHuiEric;
Zhou, Jammy; StDenis, Tom; Nath, Arindam; Nils Wallménius; Yang, Eric;
funfunctor at folklore1984.net; Yang, Young; Dan Carpenter; Huang, Ray; Arnd
Bergmann; Cui, Flora; Wang, Ken; Liu, Monk; Min, Frank; tang.qiang007 at
zte.com.cn; xie.baoyou at zte.com.cn; han.fei at zte.com.cn; LKML; Maling list
- DRI developers
Subject: Re: [PATCH 1/2] drm/amd/powerplay: mark symbols static where possible
On Fri, Sep 30, 2016 at 8:16 AM, Christian König
wrote:
> Both patches are Acked-by: Christian König .
Applied patch 1. I'd like to get Rex's ack on patch 2 before applying it.
Alex
>
> Regards,
> Christian.
>
>
> Am 30.09.2016 um 11:58 schrieb Baoyou Xie:
>>
>> We get a few warnings when building kernel with W=1:
>> drivers/gpu/drm/amd/amdgpu/../powerplay/smumgr/fiji_smumgr.c:162:5:
>> warning: no previous prototype for 'fiji_setup_pwr_virus'
>> [-Wmissing-prototypes]
>> drivers/gpu/drm/amd/amdgpu/../powerplay/smumgr/fiji_smc.c:2052:5: warning:
>> no previous prototype for 'fiji_program_mem_timing_parameters'
>> [-Wmissing-prototypes]
>> drivers/gpu/drm/amd/amdgpu/../powerplay/smumgr/polaris10_smumgr.c:175:5:
>> warning: no previous prototype for 'polaris10_avfs_event_mgr'
>> [-Wmissing-prototypes]
>> drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:69:10: warning:
>> no previous prototype for 'cz_get_eclk_level' [-Wmissing-prototypes]
>> drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:92:26: warning:
>> no previous prototype for 'cast_phw_smu7_power_state'
>> [-Wmissing-prototypes]
>>
>> In fact, these functions are only used in the file in which they are
>> declared and don't need a declaration, but can be made static.
>> So this patch marks these functions with 'static'.
>>
>> Signed-off-by: Baoyou Xie
>> ---
>> drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 5 ++-
>> drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 12 +++---
>> .../amd/powerplay/hwmgr/process_pptables_v1_0.c| 6 +--
>> .../gpu/drm/amd/powerplay/hwmgr/processpptables.c | 4 +-
>> .../amd/powerplay/hwmgr/smu7_clockpowergating.c| 10 ++---
>> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 45
>> --
>> drivers/gpu/drm/amd/powerplay/smumgr/fiji_smc.c| 2 +-
>> drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c | 12 +++---
>> .../drm/amd/powerplay/smumgr/polaris10_smumgr.c| 5 ++-
>> 9 files changed, 54 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> index 7174f7a..bb8a345 100644
>> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
>> @@ -436,7 +436,8 @@ static enum PP_StateUILabel
>> power_state_convert(enum amd_pm_state_type state)
>> }
>> }
>> -int pp_dpm_dispatch_tasks(void *handle, enum amd_pp_event
>> event_id, void *input, void *output)
>> +static int pp_dpm_dispatch_tasks(void *handle, enum amd_pp_event
>> event_id,
>> + void *input, void *output)
>> {
>> int ret = 0;
>> struct pp_instance *pp_handle; @@ -475,7 +476,7 @@ int
>> pp_dpm_dispatch_tasks(void *handle, enum amd_pp_event event_id, void
>> *input,
>> return ret;
>> }
>> -enum amd_pm_state_type pp_dpm_get_current_power_state(void
>> *handle)
>> +static enum amd_pm_state_type pp_dpm_get_current_power_state(void
>> *handle)
>> {
>> struct pp_hwmgr *hwmgr;
>> struct pp_power_state *state; diff --git
>> a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
>> b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
>> index 7e4fcbb..b48d00f 100644
>> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
>> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
>> @@ -66,7 +66,7 @@ static const struct cz_power_state
>> *cast_const_PhwCzPowerState(
>> return (struct cz_power_state *)hw_ps;
>> }
>> -uint32_t cz_get_eclk_level(struct pp_hwmgr *hwmgr,
>> +static uint32_t cz_get_eclk_level(struct pp_hwmgr *hwmgr,
>> uint32_t clock, uint32_t msg)
&
[PATCH] drivers: gpu: drm: amd: powerplay: hwmgr: Remove unused variable
Yes, stretch_amount2 was not used on Polaris.
Patch was Reviewed-by: Rex Zhu
Thanks.
Best Regards
Rex
From: Matthias Beyer <m...@beyermatthias.de>
Sent: Friday, July 1, 2016 12:38:49 AM
To: linuxdev.baldrick at gmail.com
Cc: Deucher, Alexander; Koenig, Christian; airlied at linux.ie; dri-devel at
lists.freedesktop.org; dcb314 at hotmail.com; linux-kernel at vger.kernel.org;
Zhu, Rex; Huang, JinHuiEric; nils.wallmenius at gmail.com; Matthias Beyer
Subject: [PATCH] drivers: gpu: drm: amd: powerplay: hwmgr: Remove unused
variable
Signed-off-by: Matthias Beyer
---
drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c
index 64ee78f..1dcd52d 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c
@@ -1761,7 +1761,7 @@ static int
polaris10_populate_clock_stretcher_data_table(struct pp_hwmgr *hwmgr)
{
uint32_t ro, efuse, volt_without_cks, volt_with_cks, value, max, min;
struct polaris10_hwmgr *data = (struct polaris10_hwmgr
*)(hwmgr->backend);
- uint8_t i, stretch_amount, stretch_amount2, volt_offset = 0;
+ uint8_t i, stretch_amount, volt_offset = 0;
struct phm_ppt_v1_information *table_info =
(struct phm_ppt_v1_information *)(hwmgr->pptable);
struct phm_ppt_v1_clock_voltage_dependency_table *sclk_table =
@@ -1806,11 +1806,8 @@ static int
polaris10_populate_clock_stretcher_data_table(struct pp_hwmgr *hwmgr)
}
/* Populate CKS Lookup Table */
- if (stretch_amount == 1 || stretch_amount == 2 || stretch_amount == 5)
- stretch_amount2 = 0;
- else if (stretch_amount == 3 || stretch_amount == 4)
- stretch_amount2 = 1;
- else {
+ if (stretch_amount != 1 && stretch_amount != 2 && stretch_amount != 3 &&
+ stretch_amount != 4 && stretch_amount != 5) {
phm_cap_unset(hwmgr->platform_descriptor.platformCaps,
PHM_PlatformCaps_ClockStretcher);
PP_ASSERT_WITH_CODE(false,
--
2.9.0
-- next part --
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160705/5d090dab/attachment.html>
drm/amd/powerplay: enable dpm for baffin.
It is a bug. I will fix it.
Best Regars
Rex
-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
Sent: Tuesday, May 10, 2016 3:42 AM
To: Zhu, Rex
Cc: dri-devel at lists.freedesktop.org
Subject: re: drm/amd/powerplay: enable dpm for baffin.
Hello Rex Zhu,
This is a semi-automatic email about new static checker warnings.
The patch a23eefa2f461: "drm/amd/powerplay: enable dpm for baffin."
from Nov 19, 2015, leads to the following Smatch complaint:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/polaris10_hwmgr.c:206
phm_apply_dal_min_voltage_request()
error: we previously assumed 'table' could be null (see line 202)
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/polaris10_hwmgr.c
201
202 if (!table && !(dal_power_level >= PP_DAL_POWERLEVEL_ULTRALOW &&
^^
Should this be an ||?
203 dal_power_level <=
PP_DAL_POWERLEVEL_PERFORMANCE))
204 return;
205
206 for (i = 0; i < table->count; i++) {
Because if table is ever NULL then
we're toasted on the next line.
207 if (dal_power_level == table->entries[i].clk) {
208 req_vddc = table->entries[i].v;
regards,
dan carpenter
[PATCH] amdgpu: fix NULL pointer dereference at tonga_check_states_equal
Reviewed-by: Rex Zhu
Best Regards
Rex Zhu
-Original Message-
From: Bradley Pankow [mailto:btpan...@gmail.com]
Sent: Tuesday, February 23, 2016 9:12 AM
To: Deucher, Alexander; Zhu, Rex; Zhou, Jammy
Cc: dri-devel at lists.freedesktop.org; linux-kernel at vger.kernel.org;
Bradley Pankow
Subject: [PATCH] amdgpu: fix NULL pointer dereference at
tonga_check_states_equal
The event_data passed from pem_fini was not cleared upon initialization.
This caused NULL checks to pass and cast_const_phw_tonga_power_state to attempt
to dereference an invalid pointer. Clear the event_data in pem_init and
pem_fini before calling pem_handle_event.
Signed-off-by: Bradley Pankow
---
drivers/gpu/drm/amd/powerplay/eventmgr/eventmgr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/eventmgr/eventmgr.c
b/drivers/gpu/drm/amd/powerplay/eventmgr/eventmgr.c
index 52a3efc..46410e3 100644
--- a/drivers/gpu/drm/amd/powerplay/eventmgr/eventmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/eventmgr/eventmgr.c
@@ -31,7 +31,7 @@
static int pem_init(struct pp_eventmgr *eventmgr) {
int result = 0;
- struct pem_event_data event_data;
+ struct pem_event_data event_data = { {0} };
/* Initialize PowerPlay feature info */
pem_init_feature_info(eventmgr);
@@ -52,7 +52,7 @@ static int pem_init(struct pp_eventmgr *eventmgr)
static void pem_fini(struct pp_eventmgr *eventmgr) {
- struct pem_event_data event_data;
+ struct pem_event_data event_data = { {0} };
pem_uninit_featureInfo(eventmgr);
pem_unregister_interrupts(eventmgr);
--
2.7.1
drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)
Hi Dan,
I see, I will fix this warning.
Best Regards
Rex
-Original Message-
From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
Sent: Friday, January 08, 2016 4:39 AM
To: Zhu, Rex
Cc: dri-devel at lists.freedesktop.org
Subject: re: drm/amd/powerplay: show gpu load when print gpu performance for
Cz. (v2)
Hello Rex Zhu,
The patch 605ed21929fe: "drm/amd/powerplay: show gpu load when print gpu
performance for Cz. (v2)" from Dec 17, 2015, leads to the following static
checker warning:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1545
cz_print_current_perforce_level()
warn: 'result' can be either negative or positive
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c
1535 }
1536
1537 result = smum_send_msg_to_smc(hwmgr->smumgr,
PPSMC_MSG_GetAverageGraphicsActivity);
^^^
1538 if (0 == result) {
^^
smum_send_msg_to_smc() returns either a negative error code or zero or one.
There is no documentation. What does it mean when it returns 1?
Btw, Yoda code is just makes it harder to understand for no reason. It
triggers a checkpatch.pl warning these days.
I studied this back in 2002 and == vs = bugs were very rare even then.
These days the tools are much stricter so normally we get about 2 bugs caused
by == vs = typos per year. We might not have had any in 2015?
Let's say you write it as:
if (result = 0)
1) First we get a GCC warning:
CC [M] drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.o
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c: In function
âcz_print_current_perforce_levelâ:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538:2: warning:
suggest parentheses around assignment used as truth value [-Wparentheses]
if (result = 0) {
It's true that some people use extra parenthesis around every condition which
disables the GCC warning. Don't do that.
2) Second that code will also trigger a checkpatch.pl warning.
ERROR: do not use assignment in if condition
#1538: FILE: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538:
+ if (result = 0) {
It can be easy to miss that warning if your code has a ton of warnings like
this driver does. If you care about bugs, you should probably take the energy
from writing in Yoda code and use it fix checkpatch.pl warnings instead.
3) Third it triggers a Smatch warning:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/cz_hwmgr.c:1538
cz_print_current_perforce_level() warn: was '== 0' instead of '='
We miss Smatch warnings for non-x86 arches. So yes it's still possible to have
a == vs = bug in 2016 era but it's unlikely and it's sort of your fault if that
happens because it means you write messy code to foil GCC, have checkpatch
warnings and don't run Smatch. Also testing, I suppose.
1539 active_percent = cgs_read_register(hwmgr->device,
mmSMU_MP1_SRBM2P_ARG_0);
1540 active_percent = active_percent > 100 ? 100 :
active_percent;
1541 } else {
1542 active_percent = 50;
1543 }
1544
1545 seq_printf(m, "\n [GPU load]: %u %%\n\n", active_percent);
regards,
dan carpenter
drm/amd/powerplay: show gpu load when print gpu performance for Cz. (v2)
Hi Dan, It is (result == 0).
