[PATCH 1/2] drm/xe: Make read_perf_limit_reasons globally accessible

Wed Feb 7 01:26:28 UTC 2024

On 2/6/2024 13:26, Lucas De Marchi wrote:
> On Tue, Feb 06, 2024 at 12:11:50PM -0800, John.C.Harrison at Intel.com 
> wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> Other driver code beyond the sysfs interface wants to know about
>> throttling. So move the query function out of sysfs.
>>
>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_gt_freq.c           | 18 +++++++++++--
>> drivers/gpu/drm/xe/xe_gt_freq.h           |  4 +++
>> drivers/gpu/drm/xe/xe_gt_throttle_sysfs.c | 31 ++++++++---------------
>> 3 files changed, 30 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_freq.c 
>> b/drivers/gpu/drm/xe/xe_gt_freq.c
>> index e5b0f4ecdbe8..4cf7772c387f 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_freq.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_freq.c
>> @@ -3,15 +3,17 @@
>>  * Copyright © 2023 Intel Corporation
>>  */
>>
>> -#include "xe_gt_freq.h"
>> -
>
> foo.c should always include foo.h as the first thing.
My understanding is that local header files must be includable in any 
order and therefore should always be kept sorted alphabetically. Also 
that system includes always come first and local includes after 
otherwise you risk doing bad things to the system headers if you have 
something dodgy in your local include.

>
>> #include <linux/kobject.h>
>> #include <linux/sysfs.h>
>>
>> #include <drm/drm_managed.h>
>> #include <drm/drm_print.h>
>>
>> +#include "regs/xe_gt_regs.h"
>> #include "xe_device_types.h"
>> +#include "xe_mmio.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_freq.h"
>> #include "xe_gt_sysfs.h"
>> #include "xe_gt_throttle_sysfs.h"
>> #include "xe_guc_pc.h"
>> @@ -220,3 +222,15 @@ void xe_gt_freq_init(struct xe_gt *gt)
>>
>>     xe_gt_throttle_sysfs_init(gt);
>> }
>> +
>> +u32 xe_read_perf_limit_reasons(struct xe_gt *gt)
>
>
> any function available to other compilation units should keep the prefix
>
> xe_gt_freq.c ->  xe_gt_freq_XXXXXXXX()
>
>> +{
>> +    u32 reg;
>
> this gives the impression you are getting the register offset, not its
> value.
Not my code, I just moved it from the sysfs file to the generic file. I 
was assuming that the code itself was already reviewed and perfect and 
should not be modified.

>
>
>> +
>> +    if (xe_gt_is_media_type(gt))
>> +        reg = xe_mmio_read32(gt, MTL_MEDIA_PERF_LIMIT_REASONS);
>> +    else
>> +        reg = xe_mmio_read32(gt, GT0_PERF_LIMIT_REASONS);
>> +
>> +    return reg;
>
> does it make sense to share the function to read a register, but then
> only return the raw value? What if the register change in the next
> platform or what you are reading changes?

For the sake of a debug print, I didn't feel like it is worth exporting 
accessor functions for every single field within the register. The 
fields are pretty meaningless to an end user and are only of use when 
the user logs a bug report and someone here is trying to work out what 
happened. At which point, the developer can look up the register meaning 
in the specs for the appropriate platform.

John.

>
> Lucas De Marchi
>
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_gt_freq.h 
>> b/drivers/gpu/drm/xe/xe_gt_freq.h
>> index f3fe3c90491a..89be518b4967 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_freq.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_freq.h
>> @@ -6,8 +6,12 @@
>> #ifndef _XE_GT_FREQ_H_
>> #define _XE_GT_FREQ_H_
>>
>> +#include <linux/types.h>
>> +
>> struct xe_gt;
>>
>> void xe_gt_freq_init(struct xe_gt *gt);
>>
>> +u32 xe_read_perf_limit_reasons(struct xe_gt *gt);
>> +
>> #endif
>> diff --git a/drivers/gpu/drm/xe/xe_gt_throttle_sysfs.c 
>> b/drivers/gpu/drm/xe/xe_gt_throttle_sysfs.c
>> index 63d640591a52..89d9f89962ad 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_throttle_sysfs.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_throttle_sysfs.c
>> @@ -8,6 +8,7 @@
>> #include <regs/xe_gt_regs.h>
>> #include "xe_device.h"
>> #include "xe_gt.h"
>> +#include "xe_gt_freq.h"
>> #include "xe_gt_sysfs.h"
>> #include "xe_gt_throttle_sysfs.h"
>> #include "xe_mmio.h"
>> @@ -34,77 +35,65 @@ dev_to_gt(struct device *dev)
>>     return kobj_to_gt(dev->kobj.parent);
>> }
>>
>> -static u32 read_perf_limit_reasons(struct xe_gt *gt)
>> -{
>> -    u32 reg;
>> -
>> -    if (xe_gt_is_media_type(gt))
>> -        reg = xe_mmio_read32(gt, MTL_MEDIA_PERF_LIMIT_REASONS);
>> -    else
>> -        reg = xe_mmio_read32(gt, GT0_PERF_LIMIT_REASONS);
>> -
>> -    return reg;
>> -}
>> -
>> static u32 read_status(struct xe_gt *gt)
>> {
>> -    u32 status = read_perf_limit_reasons(gt) & 
>> GT0_PERF_LIMIT_REASONS_MASK;
>> +    u32 status = xe_read_perf_limit_reasons(gt) & 
>> GT0_PERF_LIMIT_REASONS_MASK;
>>
>>     return status;
>> }
>>
>> static u32 read_reason_pl1(struct xe_gt *gt)
>> {
>> -    u32 pl1 = read_perf_limit_reasons(gt) & POWER_LIMIT_1_MASK;
>> +    u32 pl1 = xe_read_perf_limit_reasons(gt) & POWER_LIMIT_1_MASK;
>>
>>     return pl1;
>> }
>>
>> static u32 read_reason_pl2(struct xe_gt *gt)
>> {
>> -    u32 pl2 = read_perf_limit_reasons(gt) & POWER_LIMIT_2_MASK;
>> +    u32 pl2 = xe_read_perf_limit_reasons(gt) & POWER_LIMIT_2_MASK;
>>
>>     return pl2;
>> }
>>
>> static u32 read_reason_pl4(struct xe_gt *gt)
>> {
>> -    u32 pl4 = read_perf_limit_reasons(gt) & POWER_LIMIT_4_MASK;
>> +    u32 pl4 = xe_read_perf_limit_reasons(gt) & POWER_LIMIT_4_MASK;
>>
>>     return pl4;
>> }
>>
>> static u32 read_reason_thermal(struct xe_gt *gt)
>> {
>> -    u32 thermal = read_perf_limit_reasons(gt) & THERMAL_LIMIT_MASK;
>> +    u32 thermal = xe_read_perf_limit_reasons(gt) & THERMAL_LIMIT_MASK;
>>
>>     return thermal;
>> }
>>
>> static u32 read_reason_prochot(struct xe_gt *gt)
>> {
>> -    u32 prochot = read_perf_limit_reasons(gt) & PROCHOT_MASK;
>> +    u32 prochot = xe_read_perf_limit_reasons(gt) & PROCHOT_MASK;
>>
>>     return prochot;
>> }
>>
>> static u32 read_reason_ratl(struct xe_gt *gt)
>> {
>> -    u32 ratl = read_perf_limit_reasons(gt) & RATL_MASK;
>> +    u32 ratl = xe_read_perf_limit_reasons(gt) & RATL_MASK;
>>
>>     return ratl;
>> }
>>
>> static u32 read_reason_vr_thermalert(struct xe_gt *gt)
>> {
>> -    u32 thermalert = read_perf_limit_reasons(gt) & VR_THERMALERT_MASK;
>> +    u32 thermalert = xe_read_perf_limit_reasons(gt) & 
>> VR_THERMALERT_MASK;
>>
>>     return thermalert;
>> }
>>
>> static u32 read_reason_vr_tdc(struct xe_gt *gt)
>> {
>> -    u32 tdc = read_perf_limit_reasons(gt) & VR_TDC_MASK;
>> +    u32 tdc = xe_read_perf_limit_reasons(gt) & VR_TDC_MASK;
>>
>>     return tdc;
>> }
>> -- 
>> 2.43.0
>>