[PATCH v3 2/2] drm/xe/xe_gt_idle: add debugfs entry for powergating info
Riana Tauro
riana.tauro at intel.com
Tue Aug 20 10:34:56 UTC 2024
Hi Badal
On 8/6/2024 5:29 PM, Nilawar, Badal wrote:
>
>
> On 01-08-2024 18:46, Riana Tauro wrote:
>>
>>
>> On 8/1/2024 3:33 PM, Nilawar, Badal wrote:
>>>
>>>
>>> On 01-08-2024 15:23, Riana Tauro wrote:
>>>> Coarse Powergating is a power saving technique where Render and Media
>>>> can be power-gated independently irrespective of the rest of the GT.
>>>>
>>>> For debug purposes, it is useful to expose the powergating information.
>>>>
>>>> v2: move to debugfs
>>>> add details to commit message
>>>> add per-slice status for media
>>>> define reg bits in descending order (Matt Roper)
>>>>
>>>> v3: fix return statement
>>>> fix kernel-doc
>>>> use loop for media slices
>>>> use helper function for status (Michal)
>>>>
>>>> v4: add pg prefix
>>>> do not wake GT if in C6 (Badal)
>>>>
>>>> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/regs/xe_gt_regs.h | 8 +++
>>>> drivers/gpu/drm/xe/xe_gt_debugfs.c | 13 ++++
>>>> drivers/gpu/drm/xe/xe_gt_idle.c | 91
>>>> ++++++++++++++++++++++++++++
>>>> drivers/gpu/drm/xe/xe_gt_idle.h | 2 +
>>>> 4 files changed, 114 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>>> b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>>> index 3b87f95f9ecf..279d862c306a 100644
>>>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>>> @@ -337,6 +337,14 @@
>>>> #define CTC_SOURCE_DIVIDE_LOGIC REG_BIT(0)
>>>> #define FORCEWAKE_RENDER XE_REG(0xa278)
>>>> +
>>>> +#define POWERGATE_DOMAIN_STATUS XE_REG(0xa2a0)
>>>> +#define MEDIA_SLICE3_AWAKE_STATUS REG_BIT(4)
>>>> +#define MEDIA_SLICE2_AWAKE_STATUS REG_BIT(3)
>>>> +#define MEDIA_SLICE1_AWAKE_STATUS REG_BIT(2)
>>>> +#define RENDER_AWAKE_STATUS REG_BIT(1)
>>>> +#define MEDIA_SLICE0_AWAKE_STATUS REG_BIT(0)
>>>> +
>>>> #define FORCEWAKE_MEDIA_VDBOX(n) XE_REG(0xa540 + (n) * 4)
>>>> #define FORCEWAKE_MEDIA_VEBOX(n) XE_REG(0xa560 + (n) * 4)
>>>> #define FORCEWAKE_GSC XE_REG(0xa618)
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c
>>>> b/drivers/gpu/drm/xe/xe_gt_debugfs.c
>>>> index 5e7fd937917a..47e3a1ca2394 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
>>>> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
>>>> @@ -15,6 +15,7 @@
>>>> #include "xe_ggtt.h"
>>>> #include "xe_gt.h"
>>>> #include "xe_gt_mcr.h"
>>>> +#include "xe_gt_idle.h"
>>>> #include "xe_gt_sriov_pf_debugfs.h"
>>>> #include "xe_gt_sriov_vf_debugfs.h"
>>>> #include "xe_gt_topology.h"
>>>> @@ -107,6 +108,17 @@ static int hw_engines(struct xe_gt *gt, struct
>>>> drm_printer *p)
>>>> return 0;
>>>> }
>>>> +static int powergate_info(struct xe_gt *gt, struct drm_printer *p)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + xe_pm_runtime_get(gt_to_xe(gt));
>>> In suspend resume path I am seeing PG disabled and enabled. Will it
>>> cause any race while this debugfs entry is being exercised?
>>
>> I checked the suspend cases and the enable_pg is called before the print.
> Ok.
> But there might be a case where we check the idle_status and see
>> C6 but before dumping it might be in C0, reporting wrong status
> Yes this can happen. The commit message indicates that the intention is
> to view the PG status regardless of the GT status.
The commit message was only to give a brief of what CPG means.
> Therefore, it doesn’t
> make sense to force wake the GT when it is in C6.
Agreed. yeah the changes are added in the rev. Wanted to let you know
the above scenario regarding the race condition.
> However, if you believe we should always wake the GT regardless of its
> state to check the PG status feel free to do that change and rephrase
> the commit message accordingly.
>
> Regards,
> Badal
>>
>>>> + ret = xe_gt_idle_pg_print(gt, p);
>>>> + xe_pm_runtime_put(gt_to_xe(gt));
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> static int force_reset(struct xe_gt *gt, struct drm_printer *p)
>>>> {
>>>> xe_pm_runtime_get(gt_to_xe(gt));
>>>> @@ -277,6 +289,7 @@ static const struct drm_info_list debugfs_list[]
>>>> = {
>>>> {"topology", .show = xe_gt_debugfs_simple_show, .data =
>>>> topology},
>>>> {"steering", .show = xe_gt_debugfs_simple_show, .data =
>>>> steering},
>>>> {"ggtt", .show = xe_gt_debugfs_simple_show, .data = ggtt},
>>>> + {"powergate_info", .show = xe_gt_debugfs_simple_show, .data =
>>>> powergate_info},
>>>> {"register-save-restore", .show = xe_gt_debugfs_simple_show,
>>>> .data = register_save_restore},
>>>> {"workarounds", .show = xe_gt_debugfs_simple_show, .data =
>>>> workarounds},
>>>> {"pat", .show = xe_gt_debugfs_simple_show, .data = pat},
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.c
>>>> b/drivers/gpu/drm/xe/xe_gt_idle.c
>>>> index 7188542aea43..2ab0eaafa7d7 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt_idle.c
>>>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.c
>>>> @@ -53,6 +53,11 @@ pc_to_xe(struct xe_guc_pc *pc)
>>>> return gt_to_xe(gt);
>>>> }
>>>> +static inline const char *str_up_down(bool v)
>>>> +{
>>>> + return v ? "up" : "down";
>>>> +}
>>>> +
>>>> static const char *gt_idle_state_to_string(enum xe_gt_idle_state
>>>> state)
>>>> {
>>>> switch (state) {
>>>> @@ -147,6 +152,92 @@ void xe_gt_idle_disable_pg(struct xe_gt *gt)
>>>> XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>>>> }
>>>> +/**
>>>> + * xe_gt_idle_pg_print - Xe powergating info
>>>> + * @gt: GT object
>>>> + * @p: drm_printer.
>>>> + *
>>>> + * This function prints the powergating information
>>>> + *
>>>> + * Return: 0 on success, negative error code otherwise
>>>> + */
>>>> +int xe_gt_idle_pg_print(struct xe_gt *gt, struct drm_printer *p)
>>>> +{
>>>> + struct xe_gt_idle *gtidle = >->gtidle;
>>>> + struct xe_device *xe = gt_to_xe(gt);
>>>> + enum xe_gt_idle_state state;
>>>> + u32 pg_enabled, pg_status = 0;
>>>> + u32 vcs_mask, vecs_mask;
>>>> + int err, n;
>>>> + /*
>>>> + * Media Slices
>>>> + *
>>>> + * Slice 0: VCS0, VCS1, VECS0
>>>> + * Slice 1: VCS2, VCS3, VECS1
>>>> + * Slice 2: VCS4, VCS5, VECS2
>>>> + * Slice 3: VCS6, VCS7, VECS3
>>>> + */
>>>> + static const struct {
>>>> + u64 engines;
>>>> + u32 status_bit;
>>>> + } media_slices[] = {
>>>> + {(BIT(XE_HW_ENGINE_VCS0) | BIT(XE_HW_ENGINE_VCS1) |
>>>> + BIT(XE_HW_ENGINE_VECS0)), MEDIA_SLICE0_AWAKE_STATUS},
>>>> +
>>>> + {(BIT(XE_HW_ENGINE_VCS2) | BIT(XE_HW_ENGINE_VCS3) |
>>>> + BIT(XE_HW_ENGINE_VECS1)), MEDIA_SLICE1_AWAKE_STATUS},
>>>> +
>>>> + {(BIT(XE_HW_ENGINE_VCS4) | BIT(XE_HW_ENGINE_VCS5) |
>>>> + BIT(XE_HW_ENGINE_VECS2)), MEDIA_SLICE2_AWAKE_STATUS},
>>>> +
>>>> + {(BIT(XE_HW_ENGINE_VCS6) | BIT(XE_HW_ENGINE_VCS7) |
>>>> + BIT(XE_HW_ENGINE_VECS3)), MEDIA_SLICE3_AWAKE_STATUS},
>>>> + };
>>>> +
>>>> + if (xe->info.platform == XE_PVC) {
>>>> + drm_printf(p, "Power Gating not supported\n");
>>>> + return 0;
>>>> + }
>>>> +
>>>> + state = gtidle->idle_status(gtidle_to_pc(gtidle));
>>>> + pg_enabled = gtidle->powergate_enable;
>>>> +
>>>> + /* Do not wake the GT to read powergating status */
>>>> + if (state != GT_IDLE_C6) {
>>> How about if (pg_enabled && state != GT_IDLE_C6) ?
>> Always enabled so would be unnecessary check
> If gt_resume fails the PG will not be reenabled.
If gt resume fails, the device will be in a bad state.
>>>> + err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>>>> + if (err)
> I still think we should do force_wake_put here.
> Check the discussion here.
> https://patchwork.freedesktop.org/patch/596760/?series=134121&rev=4
This patch was not merged with the suggested changes.
https://patchwork.freedesktop.org/patch/596933/?series=134437&rev=1
still seems to be under discussion.
Will fix this once the approach is decided in another patch.
>>>> + return err;
>>>> +
>>>> + pg_enabled = xe_mmio_read32(gt, POWERGATE_ENABLE);
>>> Is this needed?
>> can remove this
Isn't it better to read this, the previous value would be saved
before the write.
Thanks
Riana Tauro
> Ok
>
> Regards,
> Badal
>>
>> Thanks,
>> Riana
>>>
>>> Regards,
>>> Badal
>>>> + pg_status = xe_mmio_read32(gt, POWERGATE_DOMAIN_STATUS);
>>>> +
>>>> + XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>>>> + }
>>>> +
>>>> + if (gt->info.engine_mask & XE_HW_ENGINE_RCS_MASK) {
>>>> + drm_printf(p, "Render Power Gating Enabled: %s\n",
>>>> + str_yes_no(pg_enabled & RENDER_POWERGATE_ENABLE));
>>>> +
>>>> + drm_printf(p, "Render Power Gate Status: %s\n",
>>>> + str_up_down(pg_status & RENDER_AWAKE_STATUS));
>>>> + }
>>>> +
>>>> + vcs_mask = xe_hw_engine_mask_per_class(gt,
>>>> XE_ENGINE_CLASS_VIDEO_DECODE);
>>>> + vecs_mask = xe_hw_engine_mask_per_class(gt,
>>>> XE_ENGINE_CLASS_VIDEO_ENHANCE);
>>>> +
>>>> + /* Print media CPG status only if media is present */
>>>> + if (vcs_mask || vecs_mask) {
>>>> + drm_printf(p, "Media Power Gating Enabled: %s\n",
>>>> + str_yes_no(pg_enabled & MEDIA_POWERGATE_ENABLE));
>>>> +
>>>> + for (n = 0; n < ARRAY_SIZE(media_slices); n++)
>>>> + if (gt->info.engine_mask & media_slices[n].engines)
>>>> + drm_printf(p, "Media Slice%d Power Gate Status:
>>>> %s\n", n,
>>>> + str_up_down(pg_status &
>>>> media_slices[n].status_bit));
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> static ssize_t name_show(struct device *dev,
>>>> struct device_attribute *attr, char *buff)
>>>> {
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.h
>>>> b/drivers/gpu/drm/xe/xe_gt_idle.h
>>>> index 554447b5d46d..4455a6501cb0 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt_idle.h
>>>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.h
>>>> @@ -8,6 +8,7 @@
>>>> #include "xe_gt_idle_types.h"
>>>> +struct drm_printer;
>>>> struct xe_gt;
>>>> int xe_gt_idle_init(struct xe_gt_idle *gtidle);
>>>> @@ -15,5 +16,6 @@ void xe_gt_idle_enable_c6(struct xe_gt *gt);
>>>> void xe_gt_idle_disable_c6(struct xe_gt *gt);
>>>> void xe_gt_idle_enable_pg(struct xe_gt *gt);
>>>> void xe_gt_idle_disable_pg(struct xe_gt *gt);
>>>> +int xe_gt_idle_pg_print(struct xe_gt *gt, struct drm_printer *p);
>>>> #endif /* _XE_GT_IDLE_H_ */
More information about the Intel-xe
mailing list