[PATCH 1/7] drm/amdgpu/powerplay: pp module only enable smu when dpm disabled.

Rex.Zhu@xxxxxxx (Zhu, Rex) · Thu, 3 Nov 2016 03:31:00 +0000

Hi GraÅ¾vydas,

Only in case dpm was disabled, we didn't initialize hwmgr and hwmgr->hwmgr_func.
In this case, we just use smu to load firmware. All the power feature will be disabled.
And the sysfs and debugfs files will not be initialized.

Best Regards
Rex
-----Original Message-----
From: Grazvydas Ignotas [mailto:notasas@xxxxxxxxx] 
Sent: Wednesday, November 02, 2016 9:17 PM
To: Zhu, Rex
Cc: amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 1/7] drm/amdgpu/powerplay: pp module only enable smu when dpm disabled.

On Wed, Nov 2, 2016 at 12:27 PM, Rex Zhu <Rex.Zhu at amd.com> wrote:
> Change-Id: I3288a5a4bbca122d59b81e7635be5e5aeda8abeb
> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c     |  6 +--
>  drivers/gpu/drm/amd/powerplay/amd_powerplay.c     | 51 +++++++++++++++++------
>  drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h |  2 +
>  3 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> index fa6baf3..e2f0507 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_powerplay.c
> @@ -155,9 +155,6 @@ static int amdgpu_pp_sw_init(void *handle)
>                 ret = adev->powerplay.ip_funcs->sw_init(
>                                         adev->powerplay.pp_handle);
>
> -       if (adev->pp_enabled)
> -               adev->pm.dpm_enabled = true;
> -
>         return ret;
>  }
>
> @@ -187,6 +184,9 @@ static int amdgpu_pp_hw_init(void *handle)
>                 ret = adev->powerplay.ip_funcs->hw_init(
>                                         adev->powerplay.pp_handle);
>
> +       if (amdgpu_dpm != 0)
> +               adev->pm.dpm_enabled = true;
> +
>         return ret;
>  }
>
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c 
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index 1f49764..4a4f97b 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -41,7 +41,7 @@
>  #define PP_CHECK_HW(hwmgr)                                             \
>         do {                                                            \
>                 if ((hwmgr) == NULL || (hwmgr)->hwmgr_func == NULL)     \
> -                       return -EINVAL;                                 \
> +                       return 0;                                       \

Is that really the right thing to do? With it functions like
pp_dpm_get_fan_speed_percent() pp_dpm_read_sensor() will succeed but not set the values and callers will use uninitialized data (leak kernel stack contents, so it can be considered a security issue even).

GraÅ¾vydas