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

[Riana Tauro <riana.tauro@intel.com>]

Hi Badal

Thanks for the review comments

On 7/30/2024 4:38 PM, Nilawar, Badal wrote:
> Hi Riana,
> 
> On 29-07-2024 15:59, 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)
>>
>> 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      | 83 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_gt_idle.h      |  2 +
>>   4 files changed, 106 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h 
>> b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> index 8a94a94d2267..fe774d0894fa 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> @@ -336,6 +336,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..c623295235d9 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"
> Header inclusion is out of order.
>>   #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));
>> +    ret = xe_gt_idle_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 67aba4140510..70fe76735621 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) {
>> @@ -145,6 +150,84 @@ 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_print - Xe gt idle print
>> + * @gt: GT object
>> + * @p: drm_printer.
>> + *
>> + * This function prints the powergating information
>> + *
>> + * Return: 0 on success, negative error code otherwise
>> + */
>> +int xe_gt_idle_print(struct xe_gt *gt, struct drm_printer *p)
> Prefix xe_gt_idle points towards RC6, better to incorporate _pg_ 
> sufix/prefix in function name.  
Kept the name general if anything else needs to be added.
will add pg prefix
> +{
>> +    struct xe_device *xe = gt_to_xe(gt);
>> +    u32 pg_enabled, pg_status;
>> +    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 Enabled: no\n");
>> +        return 0;
>> +    }
>> +
>> +    err = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> When device is in RC6 is there any significance in waking it up and 
> checking pg status. I think we should check PG status only when device 
> not in RC6, may be we should add RC6 check at the top.

This is required only for debug purposes, is the coupling wih rc6 
necessary? We cannot save the previous status as there is no park/unpark.

Any suggestions on what should be the output when in Rc6?
Should it dump Rc6 status instead of powergating
>> +    if (err)
> Decrease the refcount / xe_force_wake_put
If there is an err there is no need of forcewake put

Thanks,
Riana
> 
> Regards,
> Badal
>> +        return err;
>> +
>> +    pg_enabled = xe_mmio_read32(gt, POWERGATE_ENABLE);
>> +    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..4b8192d3aee0 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_print(struct xe_gt *gt, struct drm_printer *p);
>>   #endif /* _XE_GT_IDLE_H_ */