[PATCH v2 1/2] drm/xe: Make read_perf_limit_reasons globally accessible
John Harrison
john.c.harrison at intel.com
Fri Feb 16 17:48:22 UTC 2024
On 2/15/2024 05:43, Rodrigo Vivi wrote:
> On Wed, Feb 14, 2024 at 12:21:36PM -0800, John Harrison wrote:
>> On 2/14/2024 05:54, Rodrigo Vivi wrote:
>>> On Tue, Feb 13, 2024 at 06:12:11PM -0800, John Harrison wrote:
>>>> On 2/13/2024 07:22, Rodrigo Vivi wrote:
>>>>> On Mon, Feb 12, 2024 at 04:34:25PM -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 | 15 +++++++++++
>>>>>> drivers/gpu/drm/xe/xe_gt_freq.h | 4 +++
>>>>>> drivers/gpu/drm/xe/xe_gt_throttle_sysfs.c | 31 ++++++++---------------
>>>>>> 3 files changed, 29 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_freq.c b/drivers/gpu/drm/xe/xe_gt_freq.c
>>>>>> index e5b0f4ecdbe8..51645a24009e 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_gt_freq.c
>>>>>> +++ b/drivers/gpu/drm/xe/xe_gt_freq.c
>>>>>> @@ -11,7 +11,10 @@
>>>>>> #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_sysfs.h"
>>>>>> #include "xe_gt_throttle_sysfs.h"
>>>>>> #include "xe_guc_pc.h"
>>>>>> @@ -220,3 +223,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)
>>>>>> +{
>>>>>> + 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;
>>>>>> +}
>>>>>> 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);
>>>>> this component is xe_gt_freq, not xe_ nor xe_read or xe_read_perf.
>>>>> Please use the right namespace prefixes.
>>>> Argh! Forgot to update that.
>>>>
>>>>> But besides the namespace, I don't believe that these limits
>>>>> belong to xe_gt_freq anyway... why are you taking that from the
>>>>> throttle reasons and moving here?
>>>>>
>>>>> What about removing the 'sysfs' name from the xe_gt_throttle
>>>>> component and making that to export a function
>>>>> xe_gt_throttle_get_limit_reasons(struct xe_gt *gt)
>>>>> ?
>>>> Because that seems like a lot of unnecessary churn. The intention is to
>>>> simply add the throttle register to debug prints so that CI bug reports can
>>>> be triaged more effectively. Re-writing the entire sysfs file to split it up
>>>> into multiple units is not worth the effort when all that is required is to
>>>> export the function to read a register.
>>> No, that's not what I meant... I don't believe we need to split the file.
>>> just remove the 'sysfs' portion of the name of the component itself,
>>> so we have a component responsible for providing throttle information, either
>>> with direct exposing the sysfs and also with the exported functions that can
>>> be accessed by any other component that might make usage of the throttle
>>> information.
>> Still not sure I follow. You mean rename the files to xe_gt_throttle.[ch]
>> and rename the exported init function to xe_gt_throttle_init() but don't
>> change anything else? Leave all the sysfs code in the single file? And have
>> xe_gt_throttle_init() as just a stub wrapper around a static
>> gt_throttle_sysfs_init() or keep it as a single level init function that is
>> generically named but only does sysfs initialisation?
> I mean something like this:
> https://paste.centos.org/view/d3d2cc77
This has vanished now, but I managed to have a quick look earlier. I
think that basically matched what I described above.
John.
>
>> John.
>>
>>>> John.
>>>>
>>>>
>>>>> Thanks,
>>>>> Rodrigo.
>>>>>
>>>>>> +
>>>>>> #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