Re: [PATCH v3 2/2] drm/xe/xe_gt_idle: add debugfs entry for powergating info

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 20-08-2024 16:04, Riana Tauro wrote:
> 
> 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@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.
Ok, My point was CPG make sense only when GT is alive.
>> 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.
Ok.
>> 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 = &gt->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.
Ok. Not exactly. One posibility could be uc specific things failed but 
MMIO accesses are still working. But anyway you are reading fresh value 
of pg enable below.
>>>>> +        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.
Make sense.
>>>>> +            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.
Ok.

Regards,
Badal
> 
> 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_ */