Re: [PATCH v2 2/2] drm/xe: Enable Coarse Power Gating

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

Hi Vinay

Thanks for the review.

On 5/22/2024 1:59 AM, Belgaumkar, Vinay wrote:
> 
> On 5/21/2024 2:22 AM, Riana Tauro wrote:
>> Enable power gating for all units and sub-pipes that
>> are disabled by default.
>>
>> v2: change the init function name
>>      use symmetric calls for enable/disable pg
>>      re-pharase commit message (Rodrigo)
>>      modify the sub-pipe power gating condition
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>>   drivers/gpu/drm/xe/regs/xe_gt_regs.h |  2 ++
>>   drivers/gpu/drm/xe/xe_gt.c           | 12 +++++---
>>   drivers/gpu/drm/xe/xe_gt_idle.c      | 43 +++++++++++++++++++++++++---
>>   drivers/gpu/drm/xe/xe_gt_idle.h      |  4 ++-
>>   4 files changed, 52 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h 
>> b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> index 7c173db7d585..1cb0343ab581 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> @@ -317,6 +317,8 @@
>>   #define FORCEWAKE_GT                XE_REG(0xa188)
>>   #define POWERGATE_ENABLE            XE_REG(0xa210)
>> +#define   RENDER_POWERGATE_ENABLE        REG_BIT(0)
>> +#define   MEDIA_POWERGATE_ENABLE        REG_BIT(1)
>>   #define   VDN_HCP_POWERGATE_ENABLE(n)        REG_BIT(3 + 2 * (n))
>>   #define   VDN_MFXVDENC_POWERGATE_ENABLE(n)    REG_BIT(4 + 2 * (n))
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index e69a03ddd255..1ed6309270cb 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -362,10 +362,6 @@ static int gt_fw_domain_init(struct xe_gt *gt)
>>               xe_lmtt_init(&gt_to_tile(gt)->sriov.pf.lmtt);
>>       }
>> -    err = xe_gt_idle_sysfs_init(&gt->gtidle);
>> -    if (err)
>> -        goto err_force_wake;
>> -
>>       /* Enable per hw engine IRQs */
>>       xe_irq_enable_hwe(gt);
>> @@ -550,6 +546,10 @@ int xe_gt_init(struct xe_gt *gt)
>>       if (err)
>>           return err;
>> +    err = xe_gt_idle_init(&gt->gtidle);
>> +    if (err)
>> +        return err;
>> +
>>       err = xe_gt_freq_init(gt);
>>       if (err)
>>           return err;
>> @@ -733,6 +733,8 @@ int xe_gt_suspend(struct xe_gt *gt)
>>       if (err)
>>           goto err_force_wake;
>> +    xe_gt_idle_disable_pg(gt);
>> +
>>       XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
>>       xe_gt_dbg(gt, "suspended\n");
>> @@ -759,6 +761,8 @@ int xe_gt_resume(struct xe_gt *gt)
>>       if (err)
>>           goto err_force_wake;
>> +    xe_gt_idle_enable_pg(gt);
>> +
>>       XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
>>       xe_gt_dbg(gt, "resumed\n");
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.c 
>> b/drivers/gpu/drm/xe/xe_gt_idle.c
>> index 4384f7e80258..a056e00b4ab9 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_idle.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.c
>> @@ -12,6 +12,7 @@
>>   #include "xe_gt_sysfs.h"
>>   #include "xe_guc_pc.h"
>>   #include "regs/xe_gt_regs.h"
>> +#include "xe_macros.h"
>>   #include "xe_mmio.h"
>>   #include "xe_pm.h"
>> @@ -93,6 +94,36 @@ static u64 get_residency_ms(struct xe_gt_idle 
>> *gtidle, u64 cur_residency)
>>       return cur_residency;
>>   }
>> +void xe_gt_idle_enable_pg(struct xe_gt *gt)
>> +{
>> +    int i, j;
>> +    u32 pg_enable;
>> +
>> +    xe_device_assert_mem_access(gt_to_xe(gt));
>> +
>> +    pg_enable = RENDER_POWERGATE_ENABLE | MEDIA_POWERGATE_ENABLE;
>> +
>> +    for (i = XE_HW_ENGINE_VCS0, j = 0; i <= XE_HW_ENGINE_VCS7; ++i, 
>> ++j) {
>> +        if ((gt->info.engine_mask & BIT(i)))
>> +            pg_enable |= (VDN_HCP_POWERGATE_ENABLE(j) |
>> +                      VDN_MFXVDENC_POWERGATE_ENABLE(j));
>> +    }
>> +
>> +    XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FW_GT));
>> +    xe_mmio_write32(gt, POWERGATE_ENABLE, pg_enable);
>> +    XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
> 
> One other thing we typically setup here is the media/render hysteresis 
> (0xa0c4, 0xa0c8). When SLPC is enabled, GuC will set this up. However, 
> if skip_guc_pc is set, we will need to set these.
Sure will add this.
> 
> Also, we need to ensure CPG is not enabled for PVC as we do not setup c6 
> either -
>          if (xe->info.platform == XE_PVC) {
>                  xe_guc_pc_gucrc_disable(pc);
>                  ret = 0;
>                  goto out;
>          }
> 
Will skip it for PVC
> It will also be good to have a patch that adds a sysfs that can display 
> CPG enable and status bits. This can be a separate series.

Will send a separate series for this.

Thanks,
Riana
> 
> Thanks,
> 
> Vinay.
> 
>> +}
>> +
>> +void xe_gt_idle_disable_pg(struct xe_gt *gt)
>> +{
>> +    xe_device_assert_mem_access(gt_to_xe(gt));
>> +    XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FW_GT));
>> +
>> +    xe_mmio_write32(gt, POWERGATE_ENABLE, 0);
>> +
>> +    XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FW_GT));
>> +}
>> +
>>   static ssize_t name_show(struct device *dev,
>>                struct device_attribute *attr, char *buff)
>>   {
>> @@ -145,15 +176,18 @@ static const struct attribute *gt_idle_attrs[] = {
>>       NULL,
>>   };
>> -static void gt_idle_sysfs_fini(struct drm_device *drm, void *arg)
>> +static void gt_idle_fini(struct drm_device *drm, void *arg)
>>   {
>>       struct kobject *kobj = arg;
>> +    struct xe_gt *gt = kobj_to_gt(kobj->parent);
>> +
>> +    xe_gt_idle_disable_pg(gt);
>>       sysfs_remove_files(kobj, gt_idle_attrs);
>>       kobject_put(kobj);
>>   }
>> -int xe_gt_idle_sysfs_init(struct xe_gt_idle *gtidle)
>> +int xe_gt_idle_init(struct xe_gt_idle *gtidle)
>>   {
>>       struct xe_gt *gt = gtidle_to_gt(gtidle);
>>       struct xe_device *xe = gt_to_xe(gt);
>> @@ -182,7 +216,9 @@ int xe_gt_idle_sysfs_init(struct xe_gt_idle *gtidle)
>>           return err;
>>       }
>> -    return drmm_add_action_or_reset(&xe->drm, gt_idle_sysfs_fini, kobj);
>> +    xe_gt_idle_enable_pg(gt);
>> +
>> +    return drmm_add_action_or_reset(&xe->drm, gt_idle_fini, kobj);
>>   }
>>   void xe_gt_idle_enable_c6(struct xe_gt *gt)
>> @@ -202,7 +238,6 @@ void xe_gt_idle_disable_c6(struct xe_gt *gt)
>>       xe_device_assert_mem_access(gt_to_xe(gt));
>>       xe_force_wake_assert_held(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> -    xe_mmio_write32(gt, POWERGATE_ENABLE, 0);
>>       xe_mmio_write32(gt, RC_CONTROL, 0);
>>       xe_mmio_write32(gt, RC_STATE, 0);
>>   }
>> diff --git a/drivers/gpu/drm/xe/xe_gt_idle.h 
>> b/drivers/gpu/drm/xe/xe_gt_idle.h
>> index 75bd99659b1b..554447b5d46d 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_idle.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_idle.h
>> @@ -10,8 +10,10 @@
>>   struct xe_gt;
>> -int xe_gt_idle_sysfs_init(struct xe_gt_idle *gtidle);
>> +int xe_gt_idle_init(struct xe_gt_idle *gtidle);
>>   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);
>>   #endif /* _XE_GT_IDLE_H_ */