RE: [PATCH 17/17] drm/amd/powerplay: add control method to bypass metrics cache on Vega12

["Quan, Evan" <Evan.Quan@amd.com>]

[AMD Official Use Only - Internal Distribution Only]

The cache is useful for the case like sysfs "amdgpu_pm_info". Which inquires many metrics data in a very short period.
Without the cache, there will be multiple table transfers triggered(unnecessary as the PMFW sample interval is 1ms).

The unreasonable setting in our driver is the cache interval. For this special ASIC(vega12), 0.5S is used which is too big I think.
It should not be bigger than the PMFW sample internal setting(1ms). Otherwise we may get outdated data.

BR
Evan
-----Original Message-----
From: Alex Deucher <alexdeucher@gmail.com>
Sent: Wednesday, August 5, 2020 4:42 AM
To: Quan, Evan <Evan.Quan@amd.com>
Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Deucher, Alexander <Alexander.Deucher@amd.com>; Kuehling, Felix <Felix.Kuehling@amd.com>; Kasiviswanathan, Harish <Harish.Kasiviswanathan@amd.com>; Das, Nirmoy <Nirmoy.Das@amd.com>
Subject: Re: [PATCH 17/17] drm/amd/powerplay: add control method to bypass metrics cache on Vega12

Do we want the metrics cache at all? I can see arguments both ways.
Patches 12-17 are:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

On Thu, Jul 30, 2020 at 10:45 PM Evan Quan <evan.quan@amd.com> wrote:
>
> As for the gpu metric export, metrics cache makes no sense. It's up to
> user to decide how often the metrics should be retrieved.
>
> Change-Id: Ic2a27ebc90f0a7cf581d0697c121b6d7df030f3b
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> ---
>  .../drm/amd/powerplay/hwmgr/vega12_hwmgr.c    | 29 ++++++++++++-------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
> index 40bb0c2e4e8c..c70c30175801 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega12_hwmgr.c
> @@ -1262,22 +1262,29 @@ static uint32_t vega12_dpm_get_mclk(struct pp_hwmgr *hwmgr, bool low)
>         return (mem_clk * 100);
>  }
>
> -static int vega12_get_metrics_table(struct pp_hwmgr *hwmgr,
> SmuMetrics_t *metrics_table)
> +static int vega12_get_metrics_table(struct pp_hwmgr *hwmgr,
> +                                   SmuMetrics_t *metrics_table,
> +                                   bool bypass_cache)
>  {
>         struct vega12_hwmgr *data =
>                         (struct vega12_hwmgr *)(hwmgr->backend);
>         int ret = 0;
>
> -       if (!data->metrics_time || time_after(jiffies, data->metrics_time + HZ / 2)) {
> -               ret = smum_smc_table_manager(hwmgr, (uint8_t *)metrics_table,
> -                               TABLE_SMU_METRICS, true);
> +       if (bypass_cache ||
> +           !data->metrics_time ||
> +           time_after(jiffies, data->metrics_time + HZ / 2)) {
> +               ret = smum_smc_table_manager(hwmgr,
> +                                            (uint8_t *)(&data->metrics_table),
> +                                            TABLE_SMU_METRICS,
> +                                            true);
>                 if (ret) {
>                         pr_info("Failed to export SMU metrics table!\n");
>                         return ret;
>                 }
> -               memcpy(&data->metrics_table, metrics_table, sizeof(SmuMetrics_t));
>                 data->metrics_time = jiffies;
> -       } else
> +       }
> +
> +       if (metrics_table)
>                 memcpy(metrics_table, &data->metrics_table,
> sizeof(SmuMetrics_t));
>
>         return ret;
> @@ -1288,7 +1295,7 @@ static int vega12_get_gpu_power(struct pp_hwmgr *hwmgr, uint32_t *query)
>         SmuMetrics_t metrics_table;
>         int ret = 0;
>
> -       ret = vega12_get_metrics_table(hwmgr, &metrics_table);
> +       ret = vega12_get_metrics_table(hwmgr, &metrics_table, false);
>         if (ret)
>                 return ret;
>
> @@ -1339,7 +1346,7 @@ static int vega12_get_current_activity_percent(
>         SmuMetrics_t metrics_table;
>         int ret = 0;
>
> -       ret = vega12_get_metrics_table(hwmgr, &metrics_table);
> +       ret = vega12_get_metrics_table(hwmgr, &metrics_table, false);
>         if (ret)
>                 return ret;
>
> @@ -1387,7 +1394,7 @@ static int vega12_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>                 *size = 4;
>                 break;
>         case AMDGPU_PP_SENSOR_HOTSPOT_TEMP:
> -               ret = vega12_get_metrics_table(hwmgr, &metrics_table);
> +               ret = vega12_get_metrics_table(hwmgr, &metrics_table,
> + false);
>                 if (ret)
>                         return ret;
>
> @@ -1396,7 +1403,7 @@ static int vega12_read_sensor(struct pp_hwmgr *hwmgr, int idx,
>                 *size = 4;
>                 break;
>         case AMDGPU_PP_SENSOR_MEM_TEMP:
> -               ret = vega12_get_metrics_table(hwmgr, &metrics_table);
> +               ret = vega12_get_metrics_table(hwmgr, &metrics_table,
> + false);
>                 if (ret)
>                         return ret;
>
> @@ -2752,7 +2759,7 @@ static ssize_t vega12_get_gpu_metrics(struct pp_hwmgr *hwmgr,
>         uint32_t fan_speed_rpm;
>         int ret;
>
> -       ret = vega12_get_metrics_table(hwmgr, &metrics);
> +       ret = vega12_get_metrics_table(hwmgr, &metrics, true);
>         if (ret)
>                 return ret;
>
> --
> 2.28.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cev
> an.quan%40amd.com%7C1bca63d3f90048d72d9808d838b6dd64%7C3dd8961fe4884e6
> 08e11a82d994e183d%7C0%7C0%7C637321705333790290&amp;sdata=b%2FJEpeIXuqH
> H%2BkiBxqIYMGVyirYGsCs5RiUq%2Bqp64oE%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx