[PATCH 1/2] drm/amd/pm: add get_dpm_ultimate_freq function for cyan skillfish

[PATCH 1/2] drm/amd/pm: add get_dpm_ultimate_freq function for cyan skillfish

[Lang Yu]

Some clients(e.g., kfd) query sclk/mclk through this function.

Before this patch:
 # /opt/rocm/opencl/bin/clinfo

 Max clock frequency:                           0Mhz

After this patch:
 # /opt/rocm/opencl/bin/clinfo

 Max clock frequency:                           1500Mhz

Signed-off-by: Lang Yu <Lang.Yu@amd.com>
---
 drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
index 2238ee19c222..665905a568eb 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
@@ -552,6 +552,14 @@ static int cyan_skillfish_od_edit_dpm_table(struct smu_context *smu,
 	return ret;
 }
 
+static int cyan_skillfish_get_dpm_ultimate_freq(struct smu_context *smu,
+						enum smu_clk_type clk_type,
+						uint32_t *min,
+						uint32_t *max)
+{
+	return cyan_skillfish_get_current_clk_freq(smu, clk_type, min ? min : max);
+}
+
 static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
 
 	.check_fw_status = smu_v11_0_check_fw_status,
@@ -565,6 +573,7 @@ static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
 	.is_dpm_running = cyan_skillfish_is_dpm_running,
 	.get_gpu_metrics = cyan_skillfish_get_gpu_metrics,
 	.od_edit_dpm_table = cyan_skillfish_od_edit_dpm_table,
+	.get_dpm_ultimate_freq = cyan_skillfish_get_dpm_ultimate_freq,
 	.register_irq_handler = smu_v11_0_register_irq_handler,
 	.notify_memory_pool_location = smu_v11_0_notify_memory_pool_location,
 	.send_smc_msg_with_param = smu_cmn_send_smc_msg_with_param,
-- 
2.25.1

[PATCH 2/2] drm/amd/pm: use existing fini_smc_tables function for cyan skillfish

[Lang Yu]

Remove redundant code and use general smu_v11_0_fini_smc_tables function.

Signed-off-by: Lang Yu <Lang.Yu@amd.com>
---
 .../amd/pm/swsmu/smu11/cyan_skillfish_ppt.c    | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
index 665905a568eb..a2b72d19a400 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
@@ -125,22 +125,6 @@ static int cyan_skillfish_init_smc_tables(struct smu_context *smu)
 	return smu_v11_0_init_smc_tables(smu);
 }
 
-static int cyan_skillfish_finit_smc_tables(struct smu_context *smu)
-{
-	struct smu_table_context *smu_table = &smu->smu_table;
-
-	kfree(smu_table->metrics_table);
-	smu_table->metrics_table = NULL;
-
-	kfree(smu_table->gpu_metrics_table);
-	smu_table->gpu_metrics_table = NULL;
-	smu_table->gpu_metrics_table_size = 0;
-
-	smu_table->metrics_time = 0;
-
-	return 0;
-}
-
 static int
 cyan_skillfish_get_smu_metrics_data(struct smu_context *smu,
 					MetricsMember_t member,
@@ -567,7 +551,7 @@ static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
 	.init_power = smu_v11_0_init_power,
 	.fini_power = smu_v11_0_fini_power,
 	.init_smc_tables = cyan_skillfish_init_smc_tables,
-	.fini_smc_tables = cyan_skillfish_finit_smc_tables,
+	.fini_smc_tables = smu_v11_0_fini_smc_tables,
 	.read_sensor = cyan_skillfish_read_sensor,
 	.print_clk_levels = cyan_skillfish_print_clk_levels,
 	.is_dpm_running = cyan_skillfish_is_dpm_running,
-- 
2.25.1

Re: [PATCH 1/2] drm/amd/pm: add get_dpm_ultimate_freq function for cyan skillfish

[Huang Rui]

On Mon, Jan 24, 2022 at 02:43:41PM +0800, Yu, Lang wrote:
> Some clients(e.g., kfd) query sclk/mclk through this function.
> 
> Before this patch:
>  # /opt/rocm/opencl/bin/clinfo
> 
>  Max clock frequency:                           0Mhz
> 
> After this patch:
>  # /opt/rocm/opencl/bin/clinfo
> 
>  Max clock frequency:                           1500Mhz
> 
> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
> ---
>  drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> index 2238ee19c222..665905a568eb 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> @@ -552,6 +552,14 @@ static int cyan_skillfish_od_edit_dpm_table(struct smu_context *smu,
>  	return ret;
>  }
>  
> +static int cyan_skillfish_get_dpm_ultimate_freq(struct smu_context *smu,
> +						enum smu_clk_type clk_type,
> +						uint32_t *min,
> +						uint32_t *max)
> +{

We need to add comment here to explain in Cyan Skillfish, the clock is
constant and not changed. So here, min = max = current clock.

With that updated, patch is Reviewed-by: Huang Rui <ray.huang@amd.com>

> +	return cyan_skillfish_get_current_clk_freq(smu, clk_type, min ? min : max);
> +}
> +
>  static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
>  
>  	.check_fw_status = smu_v11_0_check_fw_status,
> @@ -565,6 +573,7 @@ static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
>  	.is_dpm_running = cyan_skillfish_is_dpm_running,
>  	.get_gpu_metrics = cyan_skillfish_get_gpu_metrics,
>  	.od_edit_dpm_table = cyan_skillfish_od_edit_dpm_table,
> +	.get_dpm_ultimate_freq = cyan_skillfish_get_dpm_ultimate_freq,
>  	.register_irq_handler = smu_v11_0_register_irq_handler,
>  	.notify_memory_pool_location = smu_v11_0_notify_memory_pool_location,
>  	.send_smc_msg_with_param = smu_cmn_send_smc_msg_with_param,
> -- 
> 2.25.1
> 

Re: [PATCH 2/2] drm/amd/pm: use existing fini_smc_tables function for cyan skillfish

[Huang Rui]

On Mon, Jan 24, 2022 at 02:43:42PM +0800, Yu, Lang wrote:
> Remove redundant code and use general smu_v11_0_fini_smc_tables function.
> 
> Signed-off-by: Lang Yu <Lang.Yu@amd.com>

Reviewed-by: Huang Rui <ray.huang@amd.com>

> ---
>  .../amd/pm/swsmu/smu11/cyan_skillfish_ppt.c    | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> index 665905a568eb..a2b72d19a400 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> @@ -125,22 +125,6 @@ static int cyan_skillfish_init_smc_tables(struct smu_context *smu)
>  	return smu_v11_0_init_smc_tables(smu);
>  }
>  
> -static int cyan_skillfish_finit_smc_tables(struct smu_context *smu)
> -{
> -	struct smu_table_context *smu_table = &smu->smu_table;
> -
> -	kfree(smu_table->metrics_table);
> -	smu_table->metrics_table = NULL;
> -
> -	kfree(smu_table->gpu_metrics_table);
> -	smu_table->gpu_metrics_table = NULL;
> -	smu_table->gpu_metrics_table_size = 0;
> -
> -	smu_table->metrics_time = 0;
> -
> -	return 0;
> -}
> -
>  static int
>  cyan_skillfish_get_smu_metrics_data(struct smu_context *smu,
>  					MetricsMember_t member,
> @@ -567,7 +551,7 @@ static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
>  	.init_power = smu_v11_0_init_power,
>  	.fini_power = smu_v11_0_fini_power,
>  	.init_smc_tables = cyan_skillfish_init_smc_tables,
> -	.fini_smc_tables = cyan_skillfish_finit_smc_tables,
> +	.fini_smc_tables = smu_v11_0_fini_smc_tables,
>  	.read_sensor = cyan_skillfish_read_sensor,
>  	.print_clk_levels = cyan_skillfish_print_clk_levels,
>  	.is_dpm_running = cyan_skillfish_is_dpm_running,
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] drm/amd/pm: add get_dpm_ultimate_freq function for cyan skillfish

[Lazar, Lijo]

On 1/24/2022 12:13 PM, Lang Yu wrote:
> Some clients(e.g., kfd) query sclk/mclk through this function.
> 
> Before this patch:
>   # /opt/rocm/opencl/bin/clinfo
> 
>   Max clock frequency:                           0Mhz
> 
> After this patch:
>   # /opt/rocm/opencl/bin/clinfo
> 
>   Max clock frequency:                           1500Mhz
> 
> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> index 2238ee19c222..665905a568eb 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> @@ -552,6 +552,14 @@ static int cyan_skillfish_od_edit_dpm_table(struct smu_context *smu,
>   	return ret;
>   }
>   
> +static int cyan_skillfish_get_dpm_ultimate_freq(struct smu_context *smu,
> +						enum smu_clk_type clk_type,
> +						uint32_t *min,
> +						uint32_t *max)
> +{
> +	return cyan_skillfish_get_current_clk_freq(smu, clk_type, min ? min : max);
> +}
> +

I see the below logic already there and this patch doesn't match with that.

         case SMU_GFXCLK:
                 ret = cyan_skillfish_get_current_clk_freq(smu, 
clk_type, &cur_value);
                 if (ret)
                         return ret;
                 if (cur_value  == CYAN_SKILLFISH_SCLK_MAX)
                         i = 2;
                 else if (cur_value == CYAN_SKILLFISH_SCLK_MIN)
                         i = 0;
                 else
                         i = 1;
                 size += sysfs_emit_at(buf, size, "0: %uMhz %s\n", 
CYAN_SKILLFISH_SCLK_MIN,
                                 i == 0 ? "*" : "");
                 size += sysfs_emit_at(buf, size, "1: %uMhz %s\n",
                                 i == 1 ? cur_value : 
cyan_skillfish_sclk_default,
                                 i == 1 ? "*" : "");
                 size += sysfs_emit_at(buf, size, "2: %uMhz %s\n", 
CYAN_SKILLFISH_SCLK_MAX,
                                 i == 2 ? "*" : "");
                 break;


Thanks,
Lijo

>   static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
>   
>   	.check_fw_status = smu_v11_0_check_fw_status,
> @@ -565,6 +573,7 @@ static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
>   	.is_dpm_running = cyan_skillfish_is_dpm_running,
>   	.get_gpu_metrics = cyan_skillfish_get_gpu_metrics,
>   	.od_edit_dpm_table = cyan_skillfish_od_edit_dpm_table,
> +	.get_dpm_ultimate_freq = cyan_skillfish_get_dpm_ultimate_freq,
>   	.register_irq_handler = smu_v11_0_register_irq_handler,
>   	.notify_memory_pool_location = smu_v11_0_notify_memory_pool_location,
>   	.send_smc_msg_with_param = smu_cmn_send_smc_msg_with_param,
> 

Re: [PATCH 1/2] drm/amd/pm: add get_dpm_ultimate_freq function for cyan skillfish

[Lang Yu]

On 01/24/ , Lazar, Lijo wrote:
> 
> 
> On 1/24/2022 12:13 PM, Lang Yu wrote:
> > Some clients(e.g., kfd) query sclk/mclk through this function.
> > 
> > Before this patch:
> >   # /opt/rocm/opencl/bin/clinfo
> > 
> >   Max clock frequency:                           0Mhz
> > 
> > After this patch:
> >   # /opt/rocm/opencl/bin/clinfo
> > 
> >   Max clock frequency:                           1500Mhz
> > 
> > Signed-off-by: Lang Yu <Lang.Yu@amd.com>
> > ---
> >   drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> > index 2238ee19c222..665905a568eb 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
> > @@ -552,6 +552,14 @@ static int cyan_skillfish_od_edit_dpm_table(struct smu_context *smu,
> >   	return ret;
> >   }
> > +static int cyan_skillfish_get_dpm_ultimate_freq(struct smu_context *smu,
> > +						enum smu_clk_type clk_type,
> > +						uint32_t *min,
> > +						uint32_t *max)
> > +{
> > +	return cyan_skillfish_get_current_clk_freq(smu, clk_type, min ? min : max);
> > +}
> > +
> 
> I see the below logic already there and this patch doesn't match with that.
> 
>         case SMU_GFXCLK:
>                 ret = cyan_skillfish_get_current_clk_freq(smu, clk_type,
> &cur_value);
>                 if (ret)
>                         return ret;
>                 if (cur_value  == CYAN_SKILLFISH_SCLK_MAX)
>                         i = 2;
>                 else if (cur_value == CYAN_SKILLFISH_SCLK_MIN)
>                         i = 0;
>                 else
>                         i = 1;
>                 size += sysfs_emit_at(buf, size, "0: %uMhz %s\n",
> CYAN_SKILLFISH_SCLK_MIN,
>                                 i == 0 ? "*" : "");
>                 size += sysfs_emit_at(buf, size, "1: %uMhz %s\n",
>                                 i == 1 ? cur_value :
> cyan_skillfish_sclk_default,
>                                 i == 1 ? "*" : "");
>                 size += sysfs_emit_at(buf, size, "2: %uMhz %s\n",
> CYAN_SKILLFISH_SCLK_MAX,
>                                 i == 2 ? "*" : "");
>                 break;

Thanks for your comments. To maintain the logic, for sclk,
just set min/max to CYAN_SKILLFISH_SCLK_MIN/CYAN_SKILLFISH_SCLK_MAX.
For others, set min=max=current. What do you think? Thanks!

Regards,
Lang

> 
> Thanks,
> Lijo
> 
> >   static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
> >   	.check_fw_status = smu_v11_0_check_fw_status,
> > @@ -565,6 +573,7 @@ static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
> >   	.is_dpm_running = cyan_skillfish_is_dpm_running,
> >   	.get_gpu_metrics = cyan_skillfish_get_gpu_metrics,
> >   	.od_edit_dpm_table = cyan_skillfish_od_edit_dpm_table,
> > +	.get_dpm_ultimate_freq = cyan_skillfish_get_dpm_ultimate_freq,
> >   	.register_irq_handler = smu_v11_0_register_irq_handler,
> >   	.notify_memory_pool_location = smu_v11_0_notify_memory_pool_location,
> >   	.send_smc_msg_with_param = smu_cmn_send_smc_msg_with_param,
> > 

Re: [PATCH 1/2] drm/amd/pm: add get_dpm_ultimate_freq function for cyan skillfish

[Lazar, Lijo]

On 1/24/2022 1:48 PM, Lang Yu wrote:
> On 01/24/ , Lazar, Lijo wrote:
>>
>>
>> On 1/24/2022 12:13 PM, Lang Yu wrote:
>>> Some clients(e.g., kfd) query sclk/mclk through this function.
>>>
>>> Before this patch:
>>>    # /opt/rocm/opencl/bin/clinfo
>>>
>>>    Max clock frequency:                           0Mhz
>>>
>>> After this patch:
>>>    # /opt/rocm/opencl/bin/clinfo
>>>
>>>    Max clock frequency:                           1500Mhz
>>>
>>> Signed-off-by: Lang Yu <Lang.Yu@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c | 9 +++++++++
>>>    1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
>>> index 2238ee19c222..665905a568eb 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/cyan_skillfish_ppt.c
>>> @@ -552,6 +552,14 @@ static int cyan_skillfish_od_edit_dpm_table(struct smu_context *smu,
>>>    	return ret;
>>>    }
>>> +static int cyan_skillfish_get_dpm_ultimate_freq(struct smu_context *smu,
>>> +						enum smu_clk_type clk_type,
>>> +						uint32_t *min,
>>> +						uint32_t *max)
>>> +{
>>> +	return cyan_skillfish_get_current_clk_freq(smu, clk_type, min ? min : max);
>>> +}
>>> +
>>
>> I see the below logic already there and this patch doesn't match with that.
>>
>>          case SMU_GFXCLK:
>>                  ret = cyan_skillfish_get_current_clk_freq(smu, clk_type,
>> &cur_value);
>>                  if (ret)
>>                          return ret;
>>                  if (cur_value  == CYAN_SKILLFISH_SCLK_MAX)
>>                          i = 2;
>>                  else if (cur_value == CYAN_SKILLFISH_SCLK_MIN)
>>                          i = 0;
>>                  else
>>                          i = 1;
>>                  size += sysfs_emit_at(buf, size, "0: %uMhz %s\n",
>> CYAN_SKILLFISH_SCLK_MIN,
>>                                  i == 0 ? "*" : "");
>>                  size += sysfs_emit_at(buf, size, "1: %uMhz %s\n",
>>                                  i == 1 ? cur_value :
>> cyan_skillfish_sclk_default,
>>                                  i == 1 ? "*" : "");
>>                  size += sysfs_emit_at(buf, size, "2: %uMhz %s\n",
>> CYAN_SKILLFISH_SCLK_MAX,
>>                                  i == 2 ? "*" : "");
>>                  break;
> 
> Thanks for your comments. To maintain the logic, for sclk,
> just set min/max to CYAN_SKILLFISH_SCLK_MIN/CYAN_SKILLFISH_SCLK_MAX.
> For others, set min=max=current. What do you think? Thanks!
> 

Should be fine. Also, from API perspective - make sure to check both 
min/max args for non-null and assign values to both when they are not 
null (doesn't matter whether min=max).

Thanks,
Lijo

> Regards,
> Lang
> 
>>
>> Thanks,
>> Lijo
>>
>>>    static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
>>>    	.check_fw_status = smu_v11_0_check_fw_status,
>>> @@ -565,6 +573,7 @@ static const struct pptable_funcs cyan_skillfish_ppt_funcs = {
>>>    	.is_dpm_running = cyan_skillfish_is_dpm_running,
>>>    	.get_gpu_metrics = cyan_skillfish_get_gpu_metrics,
>>>    	.od_edit_dpm_table = cyan_skillfish_od_edit_dpm_table,
>>> +	.get_dpm_ultimate_freq = cyan_skillfish_get_dpm_ultimate_freq,
>>>    	.register_irq_handler = smu_v11_0_register_irq_handler,
>>>    	.notify_memory_pool_location = smu_v11_0_notify_memory_pool_location,
>>>    	.send_smc_msg_with_param = smu_cmn_send_smc_msg_with_param,
>>>