[PATCH 3/7] drm/amd/pm: drop unneeded smu->metrics_lock
Lazar, Lijo
lijo.lazar at amd.com
Wed Jan 12 07:03:49 UTC 2022
On 1/11/2022 6:51 PM, Quan, Evan wrote:
> [AMD Official Use Only]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>> Sent: Monday, January 10, 2022 3:03 PM
>> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
>> Subject: Re: [PATCH 3/7] drm/amd/pm: drop unneeded smu->metrics_lock
>>
>>
>>
>> On 1/7/2022 7:49 AM, Quan, Evan wrote:
>>> [AMD Official Use Only]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>> Sent: Thursday, January 6, 2022 4:29 PM
>>>> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
>>>> Subject: Re: [PATCH 3/7] drm/amd/pm: drop unneeded smu-
>>> metrics_lock
>>>>
>>>>
>>>>
>>>> On 1/6/2022 12:32 PM, Quan, Evan wrote:
>>>>> [AMD Official Use Only]
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>>>>> Sent: Thursday, January 6, 2022 2:17 PM
>>>>>> To: Quan, Evan <Evan.Quan at amd.com>; amd-
>> gfx at lists.freedesktop.org
>>>>>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
>>>>>> Subject: Re: [PATCH 3/7] drm/amd/pm: drop unneeded smu-
>>>>> metrics_lock
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 1/6/2022 11:27 AM, Evan Quan wrote:
>>>>>>> As all those related APIs are already well protected by
>>>>>>> adev->pm.mutex and smu->message_lock.
>>>>>>>
>>>>>>
>>>>>> This one may be widely used. Instead of relying on pm.mutex it's
>>>>>> better to keep metrics lock so that multiple clients can read data
>>>>>> without waiting on other APIs that use pm.mutex.
>>>>> [Quan, Evan] If I understand it correctly, what you wanted to
>>>>> express is to
>>>> use fine-grained lock instead of cross-grained one to avoid chasing
>>>> for the same lock.
>>>>> Yes, that was what we did before and that's why we have so many
>>>>> types of
>>>> locks. Below are my considerations for this:
>>>>> 1. We actually do not have such issue that many APIs/clients chasing
>>>>> for the
>>>> same lock. Thus fine-grained locks cannot bring much benefits.
>>>>> Take the metrics_lock here for example. The data protected by
>>>> metrics_lock are for those pm sysfs interfaces. Those sysfs interface
>>>> are not so frequently called. And almost all the time, they are called one
>> after one.
>>>> So, it's rarely they will chase for the same lock.
>>>>>
>>>>
>>>> It's not just sysfs, there are other interfaces like sensors, hwmons etc.
>>>> Basically, metrics table provides data like GFX activity or throttler
>>>> status that may be continuously monitored by app layer. So other APIs
>>>> could suffer. My thought is to just keep metrics under a separate
>>>> lock and not tie with pm.mutex.
>>> [Quan, Evan] I doubt about the guess that "other APIs could suffer if
>> metrics data is continuously polled by app layer".
>>> Since according to my previous test(watch -n 0.1 "cat
>> /sys/kernel/debug/dri/0/amdgpu_pm_info") which polled the metrics data
>> every 0.1 second, the smu busy percent was almost not affected.
>>
>> May be try close to real cases like running some videos/changing resolution
>> and run radeon profile or some monitoring app in the background?
> [Quan, Evan] There is actually not many interactions between the UMD and driver during the video palying and game running.
> My local verifications proved that. Although that seems surprising. It's kind of expected.
> As pmfw is self-governed and it can perform clocks arbitration without driver's involvement.
>
> Most of the loadings come from the monitoring command(watch -n 1 "cat/sys/kernel/debug/dri/0/amdgpu_pm_info") in the background during
> my verifications. In fact, different from the way taken by amdgpu_pm_info sysfs implementation, UMD uses a much more clever way which can reduce
> the loadings even further: via the gpu_metrics interface, UMD can get the all metrics data instead of each at one time.
Ok, then let's go ahead and remove this lock. If at all there is an
issue, we can revisit.
Thanks,
Lijo
>>
>>> So, even the metric data is polled every second by app layer, that should
>> not enforce much loading on the CPUs and SMU cores.
>>>
>>> Also, keeping a separate lock for metrics data does not help much with the
>> issue mentioned here(even if there is such).
>>> As they(app layer and other APIs) will still chase for another
>> lock(message_lock which is kept) for interaction with SMU.
>>>
>>
>> Yes, only the subsequent steps related to reading table data from VRAM.
>> Other APIs don't need to wait for this. Only thinking about profiling cases,
>> may be I'm overthinking.
> [Quan, Evan] As far as I can remember, the metrics_lock was introduced to resolve some racing issue. It did not aim at performance improvement.
> So, it should be quite safe to drop it.
>
> BR
> Evan
>>
>> Thanks,
>> Lijo
>>
>>> BR
>>> Evan
>>>>
>>>> Thanks,
>>>> Lijo
>>>>
>>>>> 2. Cross-grained lock can simplify our implementations. It's hard to
>>>>> believe,
>>>> there is 10+(actually 13 as I counted) different types of locks used
>>>> in our existing power code.
>>>>> By the cross-grained lock, we can simplify the code and protect us
>>>>> from
>>>> some unintentional dead-locks(I actually run into that several times
>>>> and it's really tricky).
>>>>>
>>>>> BR
>>>>> Evan
>>>>>>
>>>>>> Thanks,
>>>>>> Lijo
>>>>>>
>>>>>>> Signed-off-by: Evan Quan <evan.quan at amd.com>
>>>>>>> Change-Id: Ic75326ba7b4b67be8762d5407d02f6c514e1ad35
>>>>>>> ---
>>>>>>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 1 -
>>>>>>> drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 1 -
>>>>>>> .../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 14 +--
>>>>>>> .../amd/pm/swsmu/smu11/cyan_skillfish_ppt.c | 10 +-
>>>>>>> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 112 +++++----
>> ----
>>>> ----
>>>>>> -
>>>>>>> .../amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 27 ++---
>>>>>>> .../gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 28 ++---
>>>>>>> .../gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 14 +--
>>>>>>> .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 23 ++--
>>>>>>> .../drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c | 10 +-
>>>>>>> drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 21 +---
>>>>>>> drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h | 4 -
>>>>>>> 12 files changed, 70 insertions(+), 195 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>>>> index ecbc768dfe2f..f0136bf36533 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>>>>> @@ -956,7 +956,6 @@ static int smu_sw_init(void *handle)
>>>>>>> bitmap_zero(smu->smu_feature.allowed,
>> SMU_FEATURE_MAX);
>>>>>>>
>>>>>>> mutex_init(&smu->sensor_lock);
>>>>>>> - mutex_init(&smu->metrics_lock);
>>>>>>> mutex_init(&smu->message_lock);
>>>>>>>
>>>>>>> INIT_WORK(&smu->throttling_logging_work,
>>>>>>> smu_throttling_logging_work_fn); diff --git
>>>>>>> a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>>>>>>> index c3efe4fea5e0..63ed807c96f5 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
>>>>>>> @@ -487,7 +487,6 @@ struct smu_context
>>>>>>> const struct cmn2asic_mapping *pwr_src_map;
>>>>>>> const struct cmn2asic_mapping *workload_map;
>>>>>>> struct mutex sensor_lock;
>>>>>>> - struct mutex metrics_lock;
>>>>>>> struct mutex message_lock;
>>>>>>> uint64_t pool_size;
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>>>> index addb0472d040..3f7c1f23475b 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>>>> @@ -602,15 +602,11 @@ static int
>>>>>>> arcturus_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> SmuMetrics_t *metrics = (SmuMetrics_t *)smu_table-
>>>>>>> metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -693,8 +689,6 @@ static int
>>>>>>> arcturus_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> 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..7ae6b1bd648a 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
>>>>>>> @@ -150,13 +150,9 @@ cyan_skillfish_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> SmuMetrics_t *metrics = (SmuMetrics_t *)smu_table-
>>>>>>> metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu, NULL, false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu, NULL, false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -200,8 +196,6 @@ cyan_skillfish_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>>>> index fe17b3c1ece7..fdb059e7c6ba 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>>>> @@ -546,15 +546,11 @@ static int
>>>>>> navi10_get_legacy_smu_metrics_data(struct smu_context *smu,
>>>>>>> (SmuMetrics_legacy_t *)smu_table->metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -624,8 +620,6 @@ static int
>>>>>> navi10_get_legacy_smu_metrics_data(struct smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -638,15 +632,11 @@ static int
>>>>>>> navi10_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> (SmuMetrics_t *)smu_table->metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -719,8 +709,6 @@ static int navi10_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -733,15 +721,11 @@ static int
>>>>>> navi12_get_legacy_smu_metrics_data(struct smu_context *smu,
>>>>>>> (SmuMetrics_NV12_legacy_t *)smu_table-
>>> metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -811,8 +795,6 @@ static int
>>>>>> navi12_get_legacy_smu_metrics_data(struct smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -825,15 +807,11 @@ static int
>>>>>>> navi12_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> (SmuMetrics_NV12_t *)smu_table->metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -906,8 +884,6 @@ static int navi12_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -2708,20 +2684,14 @@ static ssize_t
>>>>>> navi10_get_legacy_gpu_metrics(struct smu_context *smu,
>>>>>>> SmuMetrics_legacy_t metrics;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - true);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + true);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> memcpy(&metrics, smu_table->metrics_table,
>>>>>>> sizeof(SmuMetrics_legacy_t));
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>>>>>>>
>>>>>>> gpu_metrics->temperature_edge =
>> metrics.TemperatureEdge; @@
>>>>>>> -2899,20 +2869,14 @@ static ssize_t navi10_get_gpu_metrics(struct
>>>>>> smu_context *smu,
>>>>>>> SmuMetrics_t metrics;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - true);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + true);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> memcpy(&metrics, smu_table->metrics_table,
>>>>>> sizeof(SmuMetrics_t));
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>>>>>>>
>>>>>>> gpu_metrics->temperature_edge =
>> metrics.TemperatureEdge; @@
>>>>>>> -2977,20 +2941,14 @@ static ssize_t
>>>>>>> navi12_get_legacy_gpu_metrics(struct
>>>>>> smu_context *smu,
>>>>>>> SmuMetrics_NV12_legacy_t metrics;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - true);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + true);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> memcpy(&metrics, smu_table->metrics_table,
>>>>>>> sizeof(SmuMetrics_NV12_legacy_t));
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>>>>>>>
>>>>>>> gpu_metrics->temperature_edge =
>> metrics.TemperatureEdge; @@
>>>>>>> -3058,20 +3016,14 @@ static ssize_t navi12_get_gpu_metrics(struct
>>>>>> smu_context *smu,
>>>>>>> SmuMetrics_NV12_t metrics;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - true);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + true);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> memcpy(&metrics, smu_table->metrics_table,
>>>>>>> sizeof(SmuMetrics_NV12_t));
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>>>>>>>
>>>>>>> gpu_metrics->temperature_edge =
>> metrics.TemperatureEdge; diff
>>>>>>> --
>>>>>> git
>>>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>>>> index 93caaf45a2db..2241250c2d2a 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>>>> @@ -525,15 +525,11 @@ static int
>>>>>> sienna_cichlid_get_smu_metrics_data(struct smu_context *smu,
>>>>>>> uint16_t average_gfx_activity;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -633,8 +629,6 @@ static int
>>>>>> sienna_cichlid_get_smu_metrics_data(struct smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>>
>>>>>>> }
>>>>>>> @@ -3564,14 +3558,11 @@ static ssize_t
>>>>>> sienna_cichlid_get_gpu_metrics(struct smu_context *smu,
>>>>>>> uint16_t average_gfx_activity;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - &metrics_external,
>>>>>>> - true);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + &metrics_external,
>>>>>>> + true);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> smu_cmn_init_soft_gpu_metrics(gpu_metrics, 1, 3);
>>>>>>>
>>>>>>> @@ -3661,8 +3652,6 @@ static ssize_t
>>>>>> sienna_cichlid_get_gpu_metrics(struct smu_context *smu,
>>>>>>>
>>>>>> smu_v11_0_get_current_pcie_link_speed(smu);
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> gpu_metrics->system_clock_counter =
>> ktime_get_boottime_ns();
>>>>>>>
>>>>>>> *table = (void *)gpu_metrics; diff --git
>>>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>>>> index 5cb07ed227fb..c736adca6fbb 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>>>> @@ -273,15 +273,11 @@ static int
>>>>>> vangogh_get_legacy_smu_metrics_data(struct smu_context *smu,
>>>>>>> SmuMetrics_legacy_t *metrics = (SmuMetrics_legacy_t
>>>>>> *)smu_table->metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -335,8 +331,6 @@ static int
>>>>>> vangogh_get_legacy_smu_metrics_data(struct smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -348,15 +342,11 @@ static int
>>>> vangogh_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> SmuMetrics_t *metrics = (SmuMetrics_t *)smu_table-
>>>>>>> metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -410,8 +400,6 @@ static int
>> vangogh_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>>>> index 25c4b135f830..d75508085578 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>>>> @@ -1128,15 +1128,11 @@ static int
>>>>>>> renoir_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> SmuMetrics_t *metrics = (SmuMetrics_t *)smu_table-
>>>>>>> metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_AVERAGE_GFXCLK:
>>>>>>> @@ -1201,8 +1197,6 @@ static int
>>>>>>> renoir_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>>>> index f065d95b117a..014fb88daa04 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>>>> @@ -571,15 +571,11 @@ static int
>>>>>> aldebaran_get_smu_metrics_data(struct smu_context *smu,
>>>>>>> SmuMetrics_t *metrics = (SmuMetrics_t *)smu_table-
>>>>>>> metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - NULL,
>>>>>>> - false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu,
>>>>>>> + NULL,
>>>>>>> + false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_CURR_GFXCLK:
>>>>>>> @@ -653,8 +649,6 @@ static int
>>>> aldebaran_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -1592,17 +1586,14 @@ static void
>> aldebaran_get_unique_id(struct
>>>>>> smu_context *smu)
>>>>>>> uint32_t upper32 = 0, lower32 = 0;
>>>>>>> int ret;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu, NULL, false);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu, NULL, false);
>>>>>>> if (ret)
>>>>>>> - goto out_unlock;
>>>>>>> + goto out;
>>>>>>>
>>>>>>> upper32 = metrics->PublicSerialNumUpper32;
>>>>>>> lower32 = metrics->PublicSerialNumLower32;
>>>>>>>
>>>>>>> -out_unlock:
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> +out:
>>>>>>> adev->unique_id = ((uint64_t)upper32 << 32) | lower32;
>>>>>>> if (adev->serial[0] == '\0')
>>>>>>> sprintf(adev->serial, "%016llx", adev->unique_id);
>> diff
>>>>>>> --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>>>> index caf1775d48ef..451d30dcc639 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>>>> @@ -310,13 +310,9 @@ static int
>>>>>> yellow_carp_get_smu_metrics_data(struct smu_context *smu,
>>>>>>> SmuMetrics_t *metrics = (SmuMetrics_t *)smu_table-
>>>>>>> metrics_table;
>>>>>>> int ret = 0;
>>>>>>>
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu, NULL, false);
>>>>>>> - if (ret) {
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> + ret = smu_cmn_get_metrics_table(smu, NULL, false);
>>>>>>> + if (ret)
>>>>>>> return ret;
>>>>>>> - }
>>>>>>>
>>>>>>> switch (member) {
>>>>>>> case METRICS_AVERAGE_GFXCLK:
>>>>>>> @@ -387,8 +383,6 @@ static int
>>>>>>> yellow_carp_get_smu_metrics_data(struct
>>>>>> smu_context *smu,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>>>>>> index 735e1a1e365d..d78e4f689a2a 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>>>>>> @@ -964,9 +964,9 @@ int smu_cmn_write_pptable(struct
>> smu_context
>>>>>> *smu)
>>>>>>> true);
>>>>>>> }
>>>>>>>
>>>>>>> -int smu_cmn_get_metrics_table_locked(struct smu_context *smu,
>>>>>>> - void *metrics_table,
>>>>>>> - bool bypass_cache)
>>>>>>> +int smu_cmn_get_metrics_table(struct smu_context *smu,
>>>>>>> + void *metrics_table,
>>>>>>> + bool bypass_cache)
>>>>>>> {
>>>>>>> struct smu_table_context *smu_table= &smu->smu_table;
>>>>>>> uint32_t table_size =
>>>>>>> @@ -994,21 +994,6 @@ int
>> smu_cmn_get_metrics_table_locked(struct
>>>>>> smu_context *smu,
>>>>>>> return 0;
>>>>>>> }
>>>>>>>
>>>>>>> -int smu_cmn_get_metrics_table(struct smu_context *smu,
>>>>>>> - void *metrics_table,
>>>>>>> - bool bypass_cache)
>>>>>>> -{
>>>>>>> - int ret = 0;
>>>>>>> -
>>>>>>> - mutex_lock(&smu->metrics_lock);
>>>>>>> - ret = smu_cmn_get_metrics_table_locked(smu,
>>>>>>> - metrics_table,
>>>>>>> - bypass_cache);
>>>>>>> - mutex_unlock(&smu->metrics_lock);
>>>>>>> -
>>>>>>> - return ret;
>>>>>>> -}
>>>>>>> -
>>>>>>> void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev,
>>>>>>> uint8_t
>>>>>> crev)
>>>>>>> {
>>>>>>> struct metrics_table_header *header = (struct
>>>>>>> metrics_table_header *)table; diff --git
>>>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>>>>>> index 67a25da79256..f0b4fb2a0960 100644
>>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>>>>>> @@ -101,10 +101,6 @@ int smu_cmn_write_watermarks_table(struct
>>>>>>> smu_context *smu);
>>>>>>>
>>>>>>> int smu_cmn_write_pptable(struct smu_context *smu);
>>>>>>>
>>>>>>> -int smu_cmn_get_metrics_table_locked(struct smu_context *smu,
>>>>>>> - void *metrics_table,
>>>>>>> - bool bypass_cache);
>>>>>>> -
>>>>>>> int smu_cmn_get_metrics_table(struct smu_context *smu,
>>>>>>> void *metrics_table,
>>>>>>> bool bypass_cache);
>>>>>>>
More information about the amd-gfx
mailing list