From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wang, Kevin(Yang)" Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in smu Date: Sat, 20 Jul 2019 02:58:01 +0000 Message-ID: References: <20190719112232.28485-1-kevin1.wang@amd.com> , Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1134797384==" 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: Alex Deucher Cc: "Huang, Ray" , "Feng, Kenneth" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" --===============1134797384== Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MN2PR12MB3296D05AA1EB26B988AB75EFA2CA0MN2PR12MB3296namp_" --_000_MN2PR12MB3296D05AA1EB26B988AB75EFA2CA0MN2PR12MB3296namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable ________________________________ From: Alex Deucher Sent: Saturday, July 20, 2019 1:53 AM To: Wang, Kevin(Yang) Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org ; Huang, R= ay ; Feng, Kenneth Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequence in = smu On Fri, Jul 19, 2019 at 1:03 PM Wang, Kevin(Yang) wro= te: > > The read sensor function is not same as other one, this function should b= e handle many different sensor type, I don=92t want to handle all sensor t= ype in asic file, because some sensor is very simple, only should be send a= message to smc, and some sensor should be parse Table with firmware. > > Eg: get gfx clk, > Only need send message to get value. > Because the message already mapped on each asic, so this sensor should be= handled in smu_v11.c. > > Eg: get gpu load, > this sensor should be get value from firmware table, so should must be ha= ndled in xxx_ppt.c > > Eg: get pstate clock, > It is full software sensor, so it is handled in amdgpu_smu.c > > In this patch, the smu only want to public one api of smu_read_sensor, an= d the other sensor macro is helper in smu internally. > > I want to reduce duplicate code in smu, it will be let bring up new asic = easy. I think it's still pretty straight forward. E.g., in the asic specific read_sensor() callback you do something like this: switch (sensor) { case SENSOR1: case SENSOR2: case SENSOR3: ret =3D smu_v11_read_sensor_helper(smu, sensor, value); breakl case SENSOR4: ret =3D vega20_get_sensor4(smu, value); break; default: ... } That way there is less confusion about following callbacks. Alex [kevin]: I will improve the logic of this part of code according to your suggestion.= thanks. > > Thanks > > > On Jul 20, 2019, at 12:14 AM, Alex Deucher wrot= e: > > > >> On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) wrote: > >> > >> > >> ________________________________ > >> From: Alex Deucher > >> Sent: Friday, July 19, 2019 11:17 PM > >> To: Wang, Kevin(Yang) > >> Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org ; Hua= ng, Ray ; Feng, Kenneth > >> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor sequenc= e in smu > >> > >>> On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) wrote: > >>> > >>> each asic maybe has different read sensor method. > >>> so change read sensor sequence in smu. > >>> > >>> read sensor sequence: > >>> asic sensor --> smc sensor (smu 11...) --> default_sensor (common) > >> > >> I think this makes sense. That said, the current swSMU callback > >> structures are really confusing. I think we should switch to a single > >> set of per asic callbacks and then add common helpers. Then for asics > >> where it makes sense we can just use the helper as the callback for > >> all relevant asics. If they need something asic specific, use the > >> asic specific function. That should avoid the current mix of > >> callbacks and make it clearer what code gets used when. > >> > >> Alex > >> > >> [kevin]: > >> > >> thanks review, in current code, the read sensor related function is no= t very clear, so i want to refine them. > >> but I'm not sure which way to write good code logic. > >> > >> way 1: > >> > >> provide a puiblic function named smu_read_sensor as public smu api for= other kenel module, like this patch. > >> this function will try to get value from asic or smu ip level or commo= n, call them in turn according to the rules. > >> > >> way 2: > >> > >> define a maco named smu_read_sensor as public api, implement it in xxx= _ppt.c file, > >> if can't handle sensor type in xxx_ppt.c, then call helper in smu_v11_= 0.c, then call amdgpu_smu.c helper. > >> > >> in this way, it means we must implement this callback function in xxx_= ppt.c. > >> if need to support new asic, we should add some dulicated code in xxx_= ppt.c, if not the smu_read_sensor api is not work well. > >> in smu module, use many macros as module public api, it is impossible = to tell at what level these macros implement specific code logic. > >> so i want to refine them. > >> > >> do you think which way is good for this case? > > > > I personally prefer way 2. With way 1, the common functions would > > just be a wrapper around the asic specific callbacks. The older > > powerplay code worked that way. If there is something common that > > needs to be done for all asics, I think that would make sense, but I > > don't know that we have any cases like that. If we do end up needing > > something like that, we can always revisit this. > > > > I was thinking something like the following: > > > > struct smu_asic_funcs { > > int (*get_current_clock_freq)(); > > int (*get_fan_speed_rpm)(); > > ... > > } > > > > Then for cases where two asics use the same SMU interface, you can > > create a common function. So for vega20, it might look like: > > > > static const struct smu_asic_funcs vega20_smu_asic_funcs =3D > > { > > .get_current_clock_freq =3D smu_v11_0_get_current_clock_freq, > > .get_fan_speed_rpm =3D vega20_get_fan_speed_rpm, > > ... > > }; > > > > and navi10 would look like: > > > > static const struct smu_asic_funcs navi10_smu_asic_funcs =3D > > { > > .get_current_clock_freq =3D smu_v11_0_get_current_clock_freq, > > .get_fan_speed_rpm =3D navi10_get_fan_speed_rpm, > > ... > > }; > > > > Alex > > > > > >> thanks. > >> > >>> > >>> Signed-off-by: Kevin Wang > >>> --- > >>> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 26 +++++++++++++++++-= - > >>> .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 9 ++++--- > >>> drivers/gpu/drm/amd/powerplay/navi10_ppt.c | 3 +++ > >>> drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 +++---- > >>> drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 3 +++ > >>> 5 files changed, 40 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b/drivers/gpu= /drm/amd/powerplay/amdgpu_smu.c > >>> index 05b91bc5054c..85269f86cae2 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > >>> @@ -284,11 +284,14 @@ int smu_get_power_num_states(struct smu_context= *smu, > >>> return 0; > >>> } > >>> > >>> -int smu_common_read_sensor(struct smu_context *smu, enum amd_pp_sens= ors sensor, > >>> - void *data, uint32_t *size) > >>> +int smu_default_read_sensor(struct smu_context *smu, enum amd_pp_sen= sors sensor, > >>> + void *data, uint32_t *size) > >>> { > >>> int ret =3D 0; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_STABLE_PSTATE_SCLK: > >>> *((uint32_t *)data) =3D smu->pstate_sclk; > >>> @@ -321,6 +324,25 @@ int smu_common_read_sensor(struct smu_context *s= mu, enum amd_pp_sensors sensor, > >>> return ret; > >>> } > >>> > >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sen= sor, > >>> + void *data, uint32_t *size) > >>> +{ > >>> + int ret =3D 0; > >>> + > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> + /* handle sensor sequence: asic --> ip level --> default */ > >>> + ret =3D smu_asic_read_sensor(smu, sensor, data, size); > >>> + if (ret) { > >>> + ret =3D smu_smc_read_sensor(smu, sensor, data, size); > >>> + if (ret) > >>> + ret =3D smu_default_read_sensor(smu, sensor, = data, size); > >>> + } > >>> + > >>> + return ret; > >>> +} > >>> + > >>> int smu_update_table(struct smu_context *smu, enum smu_table_id table= _index, int argument, > >>> void *table_data, bool drv2smu) > >>> { > >>> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h b/drivers= /gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> index 34093ddca105..462bae8d62aa 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > >>> @@ -820,10 +820,10 @@ struct smu_funcs > >>> ((smu)->ppt_funcs->set_thermal_fan_table ? (smu)->ppt_funcs->s= et_thermal_fan_table((smu)) : 0) > >>> #define smu_start_thermal_control(smu) \ > >>> ((smu)->funcs->start_thermal_control? (smu)->funcs->start_ther= mal_control((smu)) : 0) > >>> -#define smu_read_sensor(smu, sensor, data, size) \ > >>> - ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), = (sensor), (data), (size)) : 0) > >>> +#define smu_smc_read_sensor(smu, sensor, data, size) \ > >>> + ((smu)->funcs->read_sensor? (smu)->funcs->read_sensor((smu), = (sensor), (data), (size)) : -EINVAL) > >>> #define smu_asic_read_sensor(smu, sensor, data, size) \ > >>> - ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor= ((smu), (sensor), (data), (size)) : 0) > >>> + ((smu)->ppt_funcs->read_sensor? (smu)->ppt_funcs->read_sensor= ((smu), (sensor), (data), (size)) : -EINVAL) > >>> #define smu_get_power_profile_mode(smu, buf) \ > >>> ((smu)->ppt_funcs->get_power_profile_mode ? (smu)->ppt_funcs->= get_power_profile_mode((smu), buf) : 0) > >>> #define smu_set_power_profile_mode(smu, param, param_size) \ > >>> @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get_performance_lev= el(struct smu_context *smu); > >>> int smu_force_performance_level(struct smu_context *smu, enum amd_dpm= _forced_level level); > >>> int smu_set_display_count(struct smu_context *smu, uint32_t count); > >>> bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum smu_clk_typ= e clk_type); > >>> - > >>> +int smu_read_sensor(struct smu_context *smu, enum amd_pp_sensors sen= sor, > >>> + void *data, uint32_t *size); > >>> #endif > >>> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b/drivers/gpu= /drm/amd/powerplay/navi10_ppt.c > >>> index 46e2913e4af4..0a53695785b6 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > >>> @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(struct smu_contex= t *smu, > >>> struct smu_table_context *table_context =3D &smu->smu_table; > >>> PPTable_t *pptable =3D table_context->driver_pptable; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > >>> *(uint32_t *)data =3D pptable->FanMaximumRpm; > >>> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/drivers/gpu/= drm/amd/powerplay/smu_v11_0.c > >>> index 76bc157525d0..2679b6ff6ca3 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0.c > >>> @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sensor(struct smu_co= ntext *smu, > >>> void *data, uint32_t *size) > >>> { > >>> int ret =3D 0; > >>> + > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_GFX_MCLK: > >>> ret =3D smu_get_current_clk_freq(smu, SMU_UCLK, (uint3= 2_t *)data); > >>> @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sensor(struct smu_c= ontext *smu, > >>> *size =3D 4; > >>> break; > >>> default: > >>> - ret =3D smu_common_read_sensor(smu, sensor, data, siz= e); > >>> + ret =3D -EINVAL; > >>> break; > >>> } > >>> > >>> - /* try get sensor data by asic */ > >>> - if (ret) > >>> - ret =3D smu_asic_read_sensor(smu, sensor, data, size)= ; > >>> - > >>> if (ret) > >>> *size =3D 0; > >>> > >>> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b/drivers/gpu= /drm/amd/powerplay/vega20_ppt.c > >>> index bcd0efaf7bbd..b44ec7c670c5 100644 > >>> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > >>> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c > >>> @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(struct smu_contex= t *smu, > >>> struct smu_table_context *table_context =3D &smu->smu_table; > >>> PPTable_t *pptable =3D table_context->driver_pptable; > >>> > >>> + if (!data || !size) > >>> + return -EINVAL; > >>> + > >>> switch (sensor) { > >>> case AMDGPU_PP_SENSOR_MAX_FAN_RPM: > >>> *(uint32_t *)data =3D pptable->FanMaximumRpm; > >>> -- > >>> 2.22.0 > >>> > >>> _______________________________________________ > >>> amd-gfx mailing list > >>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx --_000_MN2PR12MB3296D05AA1EB26B988AB75EFA2CA0MN2PR12MB3296namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable


From: Alex Deucher <alex= deucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Sent: Saturday, July 20, 2019 1:53 AM
To: Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org>
Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org&= gt;; Huang, Ray <Ray.Huang-5C7GfCeVMHo@public.gmane.org>; Feng, Kenneth <Kenneth.Feng@a= md.com>
Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_sensor seque= nce in smu
 
On Fri, Jul 19, 2019 at 1:03 PM Wang, Kevin(Yang) = <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org> wrote:
>
> The read sensor function is not same as other one, this function shoul= d be handle many different sensor type,  I don=92t want to handle all = sensor type in asic file, because some sensor is very simple, only should b= e send a message to smc, and  some sensor should be parse Table with firmware.
>
> Eg: get gfx clk,
> Only need send message to get value.
> Because the message already mapped on each asic, so this sensor should= be handled in smu_v11.c.
>
> Eg: get gpu load,
> this sensor should be get value from firmware table, so should must be= handled in xxx_ppt.c
>
> Eg: get pstate clock,
> It is full software sensor, so it is handled in amdgpu_smu.c
>
> In this patch, the smu only want to public one api of smu_read_sensor,= and the other sensor macro is helper in smu internally.
>
> I want to reduce duplicate code in smu, it will be let bring up new as= ic easy.

I think it's still pretty straight forward.  E.g., in the asic
specific read_sensor() callback you do something like this:

switch (sensor) {
case SENSOR1:
case SENSOR2:
case SENSOR3:
      ret =3D smu_v11_read_sensor_helper(smu, sens= or, value);
      breakl
case SENSOR4:
     ret =3D vega20_get_sensor4(smu, value);
     break;
default:
    ...
}

That way there is less confusion about following callbacks.

Alex

[kevin]:
I will improve the logic of this part of code according to your sugges= tion. thanks.

>
> Thanks
>
> > On Jul 20, 2019, at 12:14 AM, Alex Deucher <alexdeucher@gmail.= com> wrote:
> >
> >> On Fri, Jul 19, 2019 at 12:01 PM Wang, Kevin(Yang) <Kevin1= .Wang-5C7GfCeVMHo@public.gmane.org> wrote:
> >>
> >>
> >> ________________________________
> >> From: Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Sent: Friday, July 19, 2019 11:17 PM
> >> To: Wang, Kevin(Yang) <Kevin1.Wang-5C7GfCeVMHo@public.gmane.org>
> >> Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-PD4FTy7X32nrcLQh/DF6ew@public.gmane.org= op.org>; Huang, Ray <Ray.Huang-5C7GfCeVMHo@public.gmane.org>; Feng, Kenneth <Kenneth= .Feng-5C7GfCeVMHo@public.gmane.org>
> >> Subject: Re: [PATCH] drm/amd/powerplay: change smu_read_senso= r sequence in smu
> >>
> >>> On Fri, Jul 19, 2019 at 7:23 AM Wang, Kevin(Yang) <Kev= in1.Wang-5C7GfCeVMHo@public.gmane.org> wrote:
> >>>
> >>> each asic maybe has different read sensor method.
> >>> so change read sensor sequence in smu.
> >>>
> >>> read sensor sequence:
> >>> asic sensor --> smc sensor (smu 11...) --> default_= sensor (common)
> >>
> >> I think this makes sense.  That said, the current swSMU = callback
> >> structures are really confusing.  I think we should swit= ch to a single
> >> set of per asic callbacks and then add common helpers.  = Then for asics
> >> where it makes sense we can just use the helper as the callba= ck for
> >> all relevant asics.  If they need something asic specifi= c, use the
> >> asic specific function.  That should avoid the current m= ix of
> >> callbacks and make it clearer what code gets used when.
> >>
> >> Alex
> >>
> >> [kevin]:
> >>
> >> thanks review, in current code, the read sensor related funct= ion is not very clear, so i want to refine them.
> >> but I'm not sure which way to write good code logic.
> >>
> >> way 1:
> >>
> >> provide a puiblic function named smu_read_sensor as public sm= u api for other kenel module, like this patch.
> >> this function will try to get value from asic or smu ip level= or common, call them in turn according to the rules.
> >>
> >> way 2:
> >>
> >> define a maco named smu_read_sensor as public api, implement = it in xxx_ppt.c file,
> >> if can't handle sensor type in xxx_ppt.c, then call helper in= smu_v11_0.c,  then call amdgpu_smu.c helper.
> >>
> >> in this way, it means we must implement this callback functio= n in xxx_ppt.c.
> >> if need to support new asic, we should add some dulicated cod= e in xxx_ppt.c, if not the smu_read_sensor api is not work well.
> >> in smu module, use many macros as module public api, it is im= possible to tell at what level these macros implement specific code logic.<= br> > >> so i want to refine them.
> >>
> >> do you think which way is good for this case?
> >
> > I personally prefer way 2.  With way 1, the common functions= would
> > just be a wrapper around the asic specific callbacks.  The o= lder
> > powerplay code worked that way.  If there is something commo= n that
> > needs to be done for all asics, I think that would make sense, bu= t I
> > don't know that we have any cases like that.  If we do end u= p needing
> > something like that, we can always revisit this.
> >
> > I was thinking something like the following:
> >
> > struct smu_asic_funcs {
> >    int (*get_current_clock_freq)();
> >    int (*get_fan_speed_rpm)();
> >    ...
> > }
> >
> > Then for cases where two asics use the same SMU interface, you ca= n
> > create a common function.  So for vega20, it might look like= :
> >
> > static const struct smu_asic_funcs vega20_smu_asic_funcs =3D
> > {
> >    .get_current_clock_freq =3D smu_v11_0_get_curre= nt_clock_freq,
> >    .get_fan_speed_rpm =3D vega20_get_fan_speed_rpm= ,
> >    ...
> > };
> >
> > and navi10 would look like:
> >
> > static const struct smu_asic_funcs navi10_smu_asic_funcs =3D
> > {
> >    .get_current_clock_freq =3D smu_v11_0_get_curre= nt_clock_freq,
> >    .get_fan_speed_rpm =3D navi10_get_fan_speed_rpm= ,
> >    ...
> > };
> >
> > Alex
> >
> >
> >> thanks.
> >>
> >>>
> >>> Signed-off-by: Kevin Wang <kevin1.wang-5C7GfCeVMHo@public.gmane.org>
> >>> ---
> >>> drivers/gpu/drm/amd/powerplay/amdgpu_smu.c  &nb= sp; | 26 +++++++++++++&= #43;+++--
> >>> .../gpu/drm/amd/powerplay/inc/amdgpu_smu.h  &nb= sp; |  9 ++++---
> >>> drivers/gpu/drm/amd/powerplay/navi10_ppt.c  &nb= sp; |  3 +++
> >>> drivers/gpu/drm/amd/powerplay/smu_v11_0.c | 10 ++= +----
> >>> drivers/gpu/drm/amd/powerplay/vega20_ppt.c  &nb= sp; |  3 +++
> >>> 5 files changed, 40 insertions(+), 11 deletions(-) > >>>
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c b= /drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> >>> index 05b91bc5054c..85269f86cae2 100644
> >>> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> >>> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_sm= u.c
> >>> @@ -284,11 +284,14 @@ int smu_get_power_num_states(st= ruct smu_context *smu,
> >>>        return 0;
> >>> }
> >>>
> >>> -int smu_common_read_sensor(struct smu_context *smu, enum= amd_pp_sensors sensor,
> >>> -         &n= bsp;            = ;    void *data, uint32_t *size)
> >>> +int smu_default_read_sensor(struct smu_context *smu,= enum amd_pp_sensors sensor,
> >>> +        &nbs= p;            &= nbsp;     void *data, uint32_t *size)
> >>> {
> >>>        int ret =3D 0;<= br> > >>>
> >>> +       if (!data || !s= ize)
> >>> +        &nbs= p;      return -EINVAL;
> >>> +
> >>>        switch (sensor)= {
> >>>        case AMDGPU_PP_= SENSOR_STABLE_PSTATE_SCLK:
> >>>         &nbs= p;      *((uint32_t *)data) =3D smu->pstate_scl= k;
> >>> @@ -321,6 +324,25 @@ int smu_common_read_sensor(struc= t smu_context *smu, enum amd_pp_sensors sensor,
> >>>        return ret;
> >>> }
> >>>
> >>> +int smu_read_sensor(struct smu_context *smu, enum am= d_pp_sensors sensor,
> >>> +        &nbs= p;          void *data, uint32= _t *size)
> >>> +{
> >>> +       int ret =3D 0;<= br> > >>> +
> >>> +       if (!data || !s= ize)
> >>> +        &nbs= p;      return -EINVAL;
> >>> +
> >>> +       /* handle senso= r sequence: asic --> ip level -->  default */
> >>> +       ret =3D smu_asi= c_read_sensor(smu, sensor, data, size);
> >>> +       if (ret) {
> >>> +        &nbs= p;      ret =3D smu_smc_read_sensor(smu, sensor, d= ata, size);
> >>> +        &nbs= p;      if (ret)
> >>> +        &nbs= p;            &= nbsp; ret =3D smu_default_read_sensor(smu, sensor, data, size);
> >>> +       }
> >>> +
> >>> +       return ret;
> >>> +}
> >>> +
> >>> int smu_update_table(struct smu_context *smu, enum smu_ta= ble_id table_index, int argument,
> >>>         &nbs= p;           void *table_= data, bool drv2smu)
> >>> {
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu= .h b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> >>> index 34093ddca105..462bae8d62aa 100644
> >>> --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h
> >>> +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgp= u_smu.h
> >>> @@ -820,10 +820,10 @@ struct smu_funcs
> >>>        ((smu)->ppt_= funcs->set_thermal_fan_table ? (smu)->ppt_funcs->set_thermal_fan_t= able((smu)) : 0)
> >>> #define smu_start_thermal_control(smu) \
> >>>        ((smu)->func= s->start_thermal_control? (smu)->funcs->start_thermal_control((smu= )) : 0)
> >>> -#define smu_read_sensor(smu, sensor, data, size) \
> >>> -       ((smu)->funcs-&g= t;read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data), (si= ze)) : 0)
> >>> +#define smu_smc_read_sensor(smu, sensor, data, size)= \
> >>> +       ((smu)->func= s->read_sensor? (smu)->funcs->read_sensor((smu), (sensor), (data),= (size)) : -EINVAL)
> >>> #define smu_asic_read_sensor(smu, sensor, data, size) \ > >>> -       ((smu)->ppt_func= s->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor), (da= ta), (size)) : 0)
> >>> +       ((smu)->ppt_= funcs->read_sensor? (smu)->ppt_funcs->read_sensor((smu), (sensor),= (data), (size)) : -EINVAL)
> >>> #define smu_get_power_profile_mode(smu, buf) \
> >>>        ((smu)->ppt_= funcs->get_power_profile_mode ? (smu)->ppt_funcs->get_power_profil= e_mode((smu), buf) : 0)
> >>> #define smu_set_power_profile_mode(smu, param, param_size= ) \
> >>> @@ -989,5 +989,6 @@ enum amd_dpm_forced_level smu_get= _performance_level(struct smu_context *smu);
> >>> int smu_force_performance_level(struct smu_context *smu, = enum amd_dpm_forced_level level);
> >>> int smu_set_display_count(struct smu_context *smu, uint32= _t count);
> >>> bool smu_clk_dpm_is_enabled(struct smu_context *smu, enum= smu_clk_type clk_type);
> >>> -
> >>> +int smu_read_sensor(struct smu_context *smu, enum am= d_pp_sensors sensor,
> >>> +        &nbs= p;          void *data, uint32= _t *size);
> >>> #endif
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c b= /drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> >>> index 46e2913e4af4..0a53695785b6 100644
> >>> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> >>> +++ b/drivers/gpu/drm/amd/powerplay/navi10_pp= t.c
> >>> @@ -1365,6 +1365,9 @@ static int navi10_read_sensor(s= truct smu_context *smu,
> >>>        struct smu_tabl= e_context *table_context =3D &smu->smu_table;
> >>>        PPTable_t *ppta= ble =3D table_context->driver_pptable;
> >>>
> >>> +       if (!data || !s= ize)
> >>> +        &nbs= p;      return -EINVAL;
> >>> +
> >>>        switch (sensor)= {
> >>>        case AMDGPU_PP_= SENSOR_MAX_FAN_RPM:
> >>>         &nbs= p;      *(uint32_t *)data =3D pptable->FanMaxim= umRpm;
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c b/= drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> >>> index 76bc157525d0..2679b6ff6ca3 100644
> >>> --- a/drivers/gpu/drm/amd/powerplay/smu_v11_0.c
> >>> +++ b/drivers/gpu/drm/amd/powerplay/smu_v11_0= .c
> >>> @@ -1267,6 +1267,10 @@ static int smu_v11_0_read_sens= or(struct smu_context *smu,
> >>>         &nbs= p;            &= nbsp;          void *data, uin= t32_t *size)
> >>> {
> >>>        int ret =3D 0;<= br> > >>> +
> >>> +       if (!data || !s= ize)
> >>> +        &nbs= p;      return -EINVAL;
> >>> +
> >>>        switch (sensor)= {
> >>>        case AMDGPU_PP_= SENSOR_GFX_MCLK:
> >>>         &nbs= p;      ret =3D smu_get_current_clk_freq(smu, SMU_= UCLK, (uint32_t *)data);
> >>> @@ -1285,14 +1289,10 @@ static int smu_v11_0_read_sen= sor(struct smu_context *smu,
> >>>         &nbs= p;      *size =3D 4;
> >>>         &nbs= p;      break;
> >>>        default:
> >>> -         &n= bsp;     ret =3D smu_common_read_sensor(smu, sensor, da= ta, size);
> >>> +        &nbs= p;      ret =3D -EINVAL;
> >>>         &nbs= p;      break;
> >>>        }
> >>>
> >>> -       /* try get sensor d= ata by asic */
> >>> -       if (ret)
> >>> -         &n= bsp;     ret =3D smu_asic_read_sensor(smu, sensor, data= , size);
> >>> -
> >>>        if (ret)
> >>>         &nbs= p;      *size =3D 0;
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c b= /drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> >>> index bcd0efaf7bbd..b44ec7c670c5 100644
> >>> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> >>> +++ b/drivers/gpu/drm/amd/powerplay/vega20_pp= t.c
> >>> @@ -3146,6 +3146,9 @@ static int vega20_read_sensor(s= truct smu_context *smu,
> >>>        struct smu_tabl= e_context *table_context =3D &smu->smu_table;
> >>>        PPTable_t *ppta= ble =3D table_context->driver_pptable;
> >>>
> >>> +       if (!data || !s= ize)
> >>> +        &nbs= p;      return -EINVAL;
> >>> +
> >>>        switch (sensor)= {
> >>>        case AMDGPU_PP_= SENSOR_MAX_FAN_RPM:
> >>>         &nbs= p;      *(uint32_t *)data =3D pptable->FanMaxim= umRpm;
> >>> --
> >>> 2.22.0
> >>>
> >>> _______________________________________________
> >>> amd-gfx mailing list
> >>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
--_000_MN2PR12MB3296D05AA1EB26B988AB75EFA2CA0MN2PR12MB3296namp_-- --===============1134797384== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4 --===============1134797384==--