From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Huang Subject: Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI Date: Wed, 4 Apr 2018 12:00:02 -0400 Message-ID: References: <1522830304-15505-1-git-send-email-Rex.Zhu@amd.com> <1522830304-15505-2-git-send-email-Rex.Zhu@amd.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1886412757==" Return-path: In-Reply-To: Content-Language: en-US List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Zhu, Rex" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" This is a multi-part message in MIME format. --===============1886412757== Content-Type: multipart/alternative; boundary="------------73053F9E381F4509CD402EEC" Content-Language: en-US This is a multi-part message in MIME format. --------------73053F9E381F4509CD402EEC Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit So you didn't compare the result with AGT, but I did. The reality is not like your speculation. Regards, Eric On 2018-04-04 11:50 AM, Zhu, Rex wrote: > > If user send message PPSMC_MSG_GetCurrPkgPwr, firmware will return the > pkgpwr immediately as current power value. > > as no PPSMC_MSG_GetCurrPkgPwr support, so send message  let firmware > write pkgpwr  to ixSMU_PM_STATUS_94, > > and driver delay 10ms to read ixSMU_PM_STATUS_94. > > > I don't think there is any problem. otherwise, there is same issue on > polaris/vega. > > > I clean ixSMU_PM_STATUS_94 before let smu write it. > > The delay time is enough unless we got 0 from ixSMU_PM_STATUS_94 . > > > > Best Regards > > Rex > > > > ------------------------------------------------------------------------ > *From:* amd-gfx on behalf of > Eric Huang > *Sent:* Wednesday, April 4, 2018 11:36 PM > *To:* amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > *Subject:* Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI > Sampling period is too short. The power reading value will be not > aligned with AGT's. It will confuse user that why AMD provides two > different power results. > > Regards, > Eric > > On 2018-04-04 04:25 AM, Rex Zhu wrote: > > 1. On polaris10/11/12, Sending smu message PPSMC_MSG_GetCurrPkgPwr to > >     read currentpkgpwr directly. > > 2. On Fiji/tonga/bonaire/hawwii, no PPSMC_MSG_GetCurrPkgPwr support. > >     Send PPSMC_MSG_PmStatusLogStart to let smu write currentpkgpwr > >     to ixSMU_PM_STATUS_94. this is asynchronous. need to delay no less > >     than 1ms. > > 3. Clean ixSMU_PM_STATUS_94 immediately when send > PPSMC_MSG_PmStatusLogStart > >     to avoid read old value. > > 4. delay 10 ms instand of 20 ms. so the result will more similar to > >     the output of PPSMC_MSG_GetCurrPkgPwr. > > 5. remove max power/vddc/vddci output to keep consistent with vega. > > 6. for vddc/vddci power, we can calculate the average value per > >     [10ms, 4s] in other interface if needed. > > > > Signed-off-by: Rex Zhu > > --- > > drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 45 > +++++++++++------------- > >   1 file changed, 21 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c > b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c > > index 40f2f87..c0ce672 100644 > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c > > @@ -3363,30 +3363,27 @@ static int smu7_get_pp_table_entry(struct > pp_hwmgr *hwmgr, > >   static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr, > >                struct pp_gpu_power *query) > >   { > > - PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr, > > - PPSMC_MSG_PmStatusLogStart), > > -                     "Failed to start pm status log!", > > -                     return -1); > > - > > -     msleep_interruptible(20); > > - > > - PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr, > > - PPSMC_MSG_PmStatusLogSample), > > -                     "Failed to sample pm status log!", > > -                     return -1); > > - > > -     query->vddc_power = cgs_read_ind_register(hwmgr->device, > > -                     CGS_IND_REG__SMC, > > -                     ixSMU_PM_STATUS_40); > > -     query->vddci_power = cgs_read_ind_register(hwmgr->device, > > -                     CGS_IND_REG__SMC, > > -                     ixSMU_PM_STATUS_49); > > -     query->max_gpu_power = cgs_read_ind_register(hwmgr->device, > > -                     CGS_IND_REG__SMC, > > -                     ixSMU_PM_STATUS_94); > > -     query->average_gpu_power = cgs_read_ind_register(hwmgr->device, > > -                     CGS_IND_REG__SMC, > > -                     ixSMU_PM_STATUS_95); > > +     if (!query) > > +             return -EINVAL; > > + > > +     memset(query, 0, sizeof *query); > > + > > +     if (hwmgr->chip_id >= CHIP_POLARIS10) { > > +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr); > > +             query->average_gpu_power = > cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0); > > +     } else { > > +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart); > > + cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC, > > + ixSMU_PM_STATUS_94, 0); > > + > > +             msleep_interruptible(10); > > + > > +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample); > > + > > +             query->average_gpu_power = > cgs_read_ind_register(hwmgr->device, > > + CGS_IND_REG__SMC, > > + ixSMU_PM_STATUS_94); > > +     } > > > >        return 0; > >   } > > _______________________________________________ > amd-gfx mailing list > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > amd-gfx Info Page - freedesktop.org > > lists.freedesktop.org > Subscribing to amd-gfx: Subscribe to amd-gfx by filling out the > following form. Use of all freedesktop.org lists is subject to our > Code of Conduct. > > > --------------73053F9E381F4509CD402EEC Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit

So you didn't compare the result with AGT, but I did. The reality is not like your speculation.


Regards,

Eric


On 2018-04-04 11:50 AM, Zhu, Rex wrote:

If user send message PPSMC_MSG_GetCurrPkgPwr, firmware will return the pkgpwr immediately as current power value.

as no PPSMC_MSG_GetCurrPkgPwr support, so send message  let firmware write pkgpwr  to ixSMU_PM_STATUS_94,

and driver delay 10ms to read ixSMU_PM_STATUS_94.


I don't think there is any problem. otherwise, there is same issue on polaris/vega.


I clean ixSMU_PM_STATUS_94 before let smu write it.

The delay time is enough unless we got 0 from ixSMU_PM_STATUS_94 .



Best Regards

Rex



From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Eric Huang <jinhuieric.huang-5C7GfCeVMHo@public.gmane.org>
Sent: Wednesday, April 4, 2018 11:36 PM
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 2/3] drm/amd/pp: Refine get_gpu_power for VI
 
Sampling period is too short. The power reading value will be not
aligned with AGT's. It will confuse user that why AMD provides two
different power results.

Regards,
Eric

On 2018-04-04 04:25 AM, Rex Zhu wrote:
> 1. On polaris10/11/12, Sending smu message PPSMC_MSG_GetCurrPkgPwr to
>     read currentpkgpwr directly.
> 2. On Fiji/tonga/bonaire/hawwii, no PPSMC_MSG_GetCurrPkgPwr support.
>     Send PPSMC_MSG_PmStatusLogStart to let smu write currentpkgpwr
>     to ixSMU_PM_STATUS_94. this is asynchronous. need to delay no less
>     than 1ms.
> 3. Clean ixSMU_PM_STATUS_94 immediately when send PPSMC_MSG_PmStatusLogStart
>     to avoid read old value.
> 4. delay 10 ms instand of 20 ms. so the result will more similar to
>     the output of PPSMC_MSG_GetCurrPkgPwr.
> 5. remove max power/vddc/vddci output to keep consistent with vega.
> 6. for vddc/vddci power, we can calculate the average value per
>     [10ms, 4s] in other interface if needed.
>
> Signed-off-by: Rex Zhu <Rex.Zhu-5C7GfCeVMHo@public.gmane.org>
> ---
>   drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 45 +++++++++++-------------
>   1 file changed, 21 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> index 40f2f87..c0ce672 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c
> @@ -3363,30 +3363,27 @@ static int smu7_get_pp_table_entry(struct pp_hwmgr *hwmgr,
>   static int smu7_get_gpu_power(struct pp_hwmgr *hwmgr,
>                struct pp_gpu_power *query)
>   {
> -     PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                     PPSMC_MSG_PmStatusLogStart),
> -                     "Failed to start pm status log!",
> -                     return -1);
> -
> -     msleep_interruptible(20);
> -
> -     PP_ASSERT_WITH_CODE(!smum_send_msg_to_smc(hwmgr,
> -                     PPSMC_MSG_PmStatusLogSample),
> -                     "Failed to sample pm status log!",
> -                     return -1);
> -
> -     query->vddc_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_40);
> -     query->vddci_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_49);
> -     query->max_gpu_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_94);
> -     query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> -                     CGS_IND_REG__SMC,
> -                     ixSMU_PM_STATUS_95);
> +     if (!query)
> +             return -EINVAL;
> +
> +     memset(query, 0, sizeof *query);
> +
> +     if (hwmgr->chip_id >= CHIP_POLARIS10) {
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetCurrPkgPwr);
> +             query->average_gpu_power = cgs_read_register(hwmgr->device, mmSMC_MSG_ARG_0);
> +     } else {
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogStart);
> +             cgs_write_ind_register(hwmgr->device, CGS_IND_REG__SMC,
> +                                     ixSMU_PM_STATUS_94, 0);
> +
> +             msleep_interruptible(10);
> +
> +             smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PmStatusLogSample);
> +
> +             query->average_gpu_power = cgs_read_ind_register(hwmgr->device,
> +                                                     CGS_IND_REG__SMC,
> +                                                     ixSMU_PM_STATUS_94);
> +     }
>  
>        return 0;
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



--------------73053F9E381F4509CD402EEC-- --===============1886412757== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============1886412757==--