[PATCH 1/2] drm/xe: Make read_perf_limit_reasons globally accessible
Lucas De Marchi
lucas.demarchi at intel.com
Thu Feb 8 05:53:53 UTC 2024
On Tue, Feb 06, 2024 at 05:26:28PM -0800, John Harrison wrote:
>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
the origin of "just sort the headers alphabetically" also has another
requirement: the compilation unit using that header includes it first.
Otherwise you are not guaranteeing the headers are self-contained and
can be included in any order.... you are just guaranteeing they can be
include in alphabetical order. Or better, in the specific sequence
available by compilation units including it.
i915 end up including the hdrtest some time ago to overcome the problem
of not adhering to a convention like that. xe is in fact using that
check (it's the only other module in the kernel), so it is redundant.
The hdrtest is nice because some people don't follow the
rules/conventions (probably because they are not aware of such), but
it's also nice not needing it and maintain consistency.
We are not very consistent, but the majority of xe still uses that:
$ git grep -l 'include "xe_.*h"' drivers/gpu/drm/xe/xe_*.c | \
while read f; do \
h=$(basename $f); h=${h/.c/}\\.h; \
git grep -n "#include.*$h" $f; \
done | wc -l
88
Considering the SPDX at the beginning has the same number of lines,
which is true in 99% of the code:
$ git grep -l 'include "xe_.*h"' drivers/gpu/drm/xe/xe_*.c | \
while read f; do \
h=$(basename $f); \
h=${h/.c/}\\.h; \
git grep -n "#include.*$h" $f; \
done | grep :6 | wc -l
63
I had to double check online where that came from originally (since I
only memorized the reason for it) and I think it's from Google style
guide and that spread over time through lints like cpplint,
clang-format, etc. See here a more extensive explanation:
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
My main problem with this when I saw was because it was moved, in the
middle of a unrelated changes.
>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.
fair enough
>I was assuming that the code itself was already reviewed and perfect
great joke on perfect code ;)
>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.
It seems a very raw interface to other compilation units that won't
survive many platforms down the road. However I don't have a better
suggestion now, so.... ok
Lucas De Marchi
>
>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
>>>
>
More information about the Intel-xe
mailing list