Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
On 24-04-2024 08:42, Aravind Iddamsetty wrote: On 23/04/24 20:34, Nilawar, Badal wrote: On 22-04-2024 12:27, Aravind Iddamsetty wrote: PCI subsystem provides callbacks to inform the driver about a request to do function level reset by user, initiated by writing to sysfs entry /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR without the need to do unbind and rebind as the driver needs to reinitialize the device afresh post FLR. v2: 1. separate out gt idle and pci save/restore to a separate patch (Lucas) 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini v3: declare xe_pci_err_handlers as static(Michal) Cc: Rodrigo Vivi Cc: Lucas De Marchi Cc: Michal Wajdeczko Reviewed-by: Rodrigo Vivi Signed-off-by: Aravind Iddamsetty --- drivers/gpu/drm/xe/Makefile | 1 + drivers/gpu/drm/xe/xe_device_types.h | 3 + drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++ drivers/gpu/drm/xe/xe_pci.c | 9 ++- drivers/gpu/drm/xe/xe_pci.h | 2 + drivers/gpu/drm/xe/xe_pci_err.c | 88 drivers/gpu/drm/xe/xe_pci_err.h | 13 7 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index 8bc62bfbc679..693971a1fac0 100644 --- a/drivers/gpu/drm/xe/Makefile +++ b/drivers/gpu/drm/xe/Makefile @@ -117,6 +117,7 @@ xe-y += xe_bb.o \ xe_module.o \ xe_pat.o \ xe_pci.o \ + xe_pci_err.o \ xe_pcode.o \ xe_pm.o \ xe_preempt_fence.o \ diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h index 0a66555229e9..8c749b378a92 100644 --- a/drivers/gpu/drm/xe/xe_device_types.h +++ b/drivers/gpu/drm/xe/xe_device_types.h @@ -465,6 +465,9 @@ struct xe_device { /** @pci_state: PCI state of device */ struct pci_saved_state *pci_state; + /** @pci_device_is_reset: device went through PCIe FLR */ + bool pci_device_is_reset; + /* private: */ #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY) diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c index 509649d0e65e..efba0fbe2f5c 100644 --- a/drivers/gpu/drm/xe/xe_guc_pc.c +++ b/drivers/gpu/drm/xe/xe_guc_pc.c @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg) return; } + /* We already have done this before going through a reset, so skip here */ + if (xe->pci_device_is_reset) + return; + XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL)); XE_WARN_ON(xe_guc_pc_gucrc_disable(pc)); XE_WARN_ON(xe_guc_pc_stop(pc)); diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c index a62300990e19..b5a582afc9e7 100644 --- a/drivers/gpu/drm/xe/xe_pci.c +++ b/drivers/gpu/drm/xe/xe_pci.c @@ -23,6 +23,7 @@ #include "xe_macros.h" #include "xe_mmio.h" #include "xe_module.h" +#include "xe_pci_err.h" #include "xe_pci_types.h" #include "xe_pm.h" #include "xe_sriov.h" @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev) pci_set_drvdata(pdev, NULL); } -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) { const struct xe_device_desc *desc = (const void *)ent->driver_data; const struct xe_subplatform_desc *subplatform_desc; @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = { }; #endif +static const struct pci_error_handlers xe_pci_err_handlers = { + .reset_prepare = xe_pci_reset_prepare, + .reset_done = xe_pci_reset_done, +}; + static struct pci_driver xe_pci_driver = { .name = DRIVER_NAME, .id_table = pciidlist, @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = { #ifdef CONFIG_PM_SLEEP .driver.pm = _pm_ops, #endif + .err_handler = _pci_err_handlers, }; int xe_register_pci_driver(void) diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h index 73b90a430d1f..9faf5380a09e 100644 --- a/drivers/gpu/drm/xe/xe_pci.h +++ b/drivers/gpu/drm/xe/xe_pci.h @@ -7,8 +7,10 @@ #define _XE_PCI_H_ struct pci_dev; +struct pci_device_id; int xe_register_pci_driver(void); void xe_unregister_pci_driver(void); void xe_load_pci_state(struct pci_dev *pdev); +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent); #endif diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c new file mode 100644 index ..5306925ea2fa --- /dev/null +++ b/drivers/gpu/drm/xe/xe_pci_err.c @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2024 Intel Corporation + */ + +#include +#include + +#include &quo
Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
On 22-04-2024 12:27, Aravind Iddamsetty wrote: PCI subsystem provides callbacks to inform the driver about a request to do function level reset by user, initiated by writing to sysfs entry /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR without the need to do unbind and rebind as the driver needs to reinitialize the device afresh post FLR. v2: 1. separate out gt idle and pci save/restore to a separate patch (Lucas) 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini v3: declare xe_pci_err_handlers as static(Michal) Cc: Rodrigo Vivi Cc: Lucas De Marchi Cc: Michal Wajdeczko Reviewed-by: Rodrigo Vivi Signed-off-by: Aravind Iddamsetty --- drivers/gpu/drm/xe/Makefile | 1 + drivers/gpu/drm/xe/xe_device_types.h | 3 + drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++ drivers/gpu/drm/xe/xe_pci.c | 9 ++- drivers/gpu/drm/xe/xe_pci.h | 2 + drivers/gpu/drm/xe/xe_pci_err.c | 88 drivers/gpu/drm/xe/xe_pci_err.h | 13 7 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile index 8bc62bfbc679..693971a1fac0 100644 --- a/drivers/gpu/drm/xe/Makefile +++ b/drivers/gpu/drm/xe/Makefile @@ -117,6 +117,7 @@ xe-y += xe_bb.o \ xe_module.o \ xe_pat.o \ xe_pci.o \ + xe_pci_err.o \ xe_pcode.o \ xe_pm.o \ xe_preempt_fence.o \ diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h index 0a66555229e9..8c749b378a92 100644 --- a/drivers/gpu/drm/xe/xe_device_types.h +++ b/drivers/gpu/drm/xe/xe_device_types.h @@ -465,6 +465,9 @@ struct xe_device { /** @pci_state: PCI state of device */ struct pci_saved_state *pci_state; + /** @pci_device_is_reset: device went through PCIe FLR */ + bool pci_device_is_reset; + /* private: */ #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY) diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c index 509649d0e65e..efba0fbe2f5c 100644 --- a/drivers/gpu/drm/xe/xe_guc_pc.c +++ b/drivers/gpu/drm/xe/xe_guc_pc.c @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg) return; } + /* We already have done this before going through a reset, so skip here */ + if (xe->pci_device_is_reset) + return; + XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL)); XE_WARN_ON(xe_guc_pc_gucrc_disable(pc)); XE_WARN_ON(xe_guc_pc_stop(pc)); diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c index a62300990e19..b5a582afc9e7 100644 --- a/drivers/gpu/drm/xe/xe_pci.c +++ b/drivers/gpu/drm/xe/xe_pci.c @@ -23,6 +23,7 @@ #include "xe_macros.h" #include "xe_mmio.h" #include "xe_module.h" +#include "xe_pci_err.h" #include "xe_pci_types.h" #include "xe_pm.h" #include "xe_sriov.h" @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev) pci_set_drvdata(pdev, NULL); } -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) { const struct xe_device_desc *desc = (const void *)ent->driver_data; const struct xe_subplatform_desc *subplatform_desc; @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = { }; #endif +static const struct pci_error_handlers xe_pci_err_handlers = { + .reset_prepare = xe_pci_reset_prepare, + .reset_done = xe_pci_reset_done, +}; + static struct pci_driver xe_pci_driver = { .name = DRIVER_NAME, .id_table = pciidlist, @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = { #ifdef CONFIG_PM_SLEEP .driver.pm = _pm_ops, #endif + .err_handler = _pci_err_handlers, }; int xe_register_pci_driver(void) diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h index 73b90a430d1f..9faf5380a09e 100644 --- a/drivers/gpu/drm/xe/xe_pci.h +++ b/drivers/gpu/drm/xe/xe_pci.h @@ -7,8 +7,10 @@ #define _XE_PCI_H_ struct pci_dev; +struct pci_device_id; int xe_register_pci_driver(void); void xe_unregister_pci_driver(void); void xe_load_pci_state(struct pci_dev *pdev); +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent); #endif diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c new file mode 100644 index ..5306925ea2fa --- /dev/null +++ b/drivers/gpu/drm/xe/xe_pci_err.c @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2024 Intel Corporation + */ + +#include +#include + +#include "xe_device.h" +#include "xe_gt.h" +#include "xe_gt_printk.h" +#include "xe_pci.h" +#include "xe_pci_err.h" +#include "xe_pm.h" +#include "xe_uc.h" + +/** +
Re: [PATCH v3 3/3] drm/i915/guc: Enable Wa_14019159160
Hi John, On 04-01-2024 23:35, john.c.harri...@intel.com wrote: From: John Harrison Use the new w/a KLV support to enable a MTL w/a. Note, this w/a is a super-set of Wa_16019325821, so requires turning that one as well as setting the new flag for Wa_14019159160 itself. Signed-off-by: John Harrison Reviewed-by: Vinay Belgaumkar --- drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 3 ++ drivers/gpu/drm/i915/gt/intel_engine_types.h | 1 + drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h | 7 drivers/gpu/drm/i915/gt/uc/intel_guc.c| 1 + drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c| 34 ++- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 1 + 6 files changed, 38 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index 9cccd60a5c41d..359b21fb02ab2 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -744,6 +744,7 @@ static u32 *gen12_emit_preempt_busywait(struct i915_request *rq, u32 *cs) /* Wa_14014475959:dg2 */ /* Wa_16019325821 */ +/* Wa_14019159160 */ #define HOLD_SWITCHOUT_SEMAPHORE_PPHWSP_OFFSET0x540 static u32 hold_switchout_semaphore_offset(struct i915_request *rq) { @@ -753,6 +754,7 @@ static u32 hold_switchout_semaphore_offset(struct i915_request *rq) /* Wa_14014475959:dg2 */ /* Wa_16019325821 */ +/* Wa_14019159160 */ static u32 *hold_switchout_emit_wa_busywait(struct i915_request *rq, u32 *cs) { int i; @@ -793,6 +795,7 @@ gen12_emit_fini_breadcrumb_tail(struct i915_request *rq, u32 *cs) /* Wa_14014475959:dg2 */ /* Wa_16019325821 */ + /* Wa_14019159160 */ if (intel_engine_uses_wa_hold_switchout(rq->engine)) cs = hold_switchout_emit_wa_busywait(rq, cs); diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index b519812ba120d..ba55c059063db 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -697,6 +697,7 @@ intel_engine_has_relative_mmio(const struct intel_engine_cs * const engine) /* Wa_14014475959:dg2 */ /* Wa_16019325821 */ +/* Wa_14019159160 */ static inline bool intel_engine_uses_wa_hold_switchout(struct intel_engine_cs *engine) { diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h index 58012edd4eb0e..bebf28e3c4794 100644 --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_klvs_abi.h @@ -101,4 +101,11 @@ enum { GUC_CONTEXT_POLICIES_KLV_NUM_IDS = 5, }; +/* + * Workaround keys: + */ +enum { + GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE = 0x9001, +}; + #endif /* _ABI_GUC_KLVS_ABI_H */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index d5c856be31491..db3cb628f40dc 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -295,6 +295,7 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc) flags |= GUC_WA_HOLD_CCS_SWITCHOUT; /* Wa_16019325821 */ + /* Wa_14019159160 */ if (IS_GFX_GT_IP_RANGE(gt, IP_VER(12, 70), IP_VER(12, 71))) flags |= GUC_WA_RCS_CCS_SWITCHOUT; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c index 6af3fa8b92e34..68d9e277eca8b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c @@ -815,6 +815,25 @@ guc_capture_prep_lists(struct intel_guc *guc) return PAGE_ALIGN(total_size); } +/* Wa_14019159160 */ +static u32 guc_waklv_ra_mode(struct intel_guc *guc, u32 offset, u32 remain) +{ How about making this function generic by passing KLV id as arg? + u32 size; + u32 klv_entry[] = { + /* 16:16 key/length */ + FIELD_PREP(GUC_KLV_0_KEY, GUC_WORKAROUND_KLV_SERIALIZED_RA_MODE) | + FIELD_PREP(GUC_KLV_0_LEN, 0), + /* 0 dwords data */ + }; + + size = sizeof(klv_entry); + GEM_BUG_ON(remain < size); + + iosys_map_memcpy_to(>ads_map, offset, klv_entry, size); Otherwise preparing and adding klv entry can be wrapped in generic function. Regards, Badal + + return size; +} + static void guc_waklv_init(struct intel_guc *guc) { struct intel_gt *gt = guc_to_gt(guc); @@ -830,15 +849,12 @@ static void guc_waklv_init(struct intel_guc *guc) offset = guc_ads_waklv_offset(guc); remain = guc_ads_waklv_size(guc); - /* -* Add workarounds here: -* -* if (want_wa_) { -* size = guc_waklv_(guc, offset, remain); -* offset += size; -* remain -= size; -* } -*/ + /* Wa_14019159160 */ + if (IS_GFX_GT_IP_RANGE(gt,
Re: [PATCH] drm/i915/hwmon: Accept writes of value 0 to power1_max_interval
On 28-02-2023 10:13, Ashutosh Dixit wrote: The value shown by power1_max_interval in millisec is essentially: ((1.x * power(2,y)) * 1000) >> 10 Where x and y are read from a HW register. On ATSM, x and y are 0 on power-up so the value shown is 0. Writes of 0 to power1_max_interval had previously been disallowed to avoid computing ilog2(0) but this resulted in the corner-case bug below. Therefore allow writes of 0 now but special case that write to x = y = 0. Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7754 Signed-off-by: Ashutosh Dixit --- drivers/gpu/drm/i915/i915_hwmon.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 7c20a6f47b92e..596dd2c070106 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -218,11 +218,15 @@ hwm_power1_max_interval_store(struct device *dev, /* val in hw units */ val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME); /* Convert to 1.x * power(2,y) */ - if (!val) - return -EINVAL; - y = ilog2(val); - /* x = (val - (1 << y)) >> (y - 2); */ - x = (val - (1ul << y)) << x_w >> y; + if (!val) { + /* Avoid ilog2(0) */ + y = 0; + x = 0; + } else { + y = ilog2(val); + /* x = (val - (1 << y)) >> (y - 2); */ + x = (val - (1ul << y)) << x_w >> y; + } Reviewed-by: Badal Nilawar rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
Re: [Intel-gfx] [PATCH] drm/i915/mtl: Apply Wa_14017073508 for MTL SoC Step
Hi Matt, On 24-02-2023 02:43, Matt Roper wrote: On Thu, Feb 23, 2023 at 03:20:28PM -0500, Rodrigo Vivi wrote: On Fri, Feb 24, 2023 at 12:11:40AM +0530, Badal Nilawar wrote: Apply Wa_14017073508 for MTL SoC die A step instead of graphics step. To get the SoC die stepping there is no direct interface so using revid as revid 0 aligns with SoC die A step. Bspec: 55420 This doesn't prove anything. It is just saying Die A0 with GT A0, die B0 with GT B0 and so on... Please help me to understand that better offline before we move forward... The definition of the workaround doesn't say anything about SoC steppings that I can see. The workaround itself is tagged as being being tied to Xe_LPM+ (i.e., the media IP), not to MTL as a platform and not to the Xe_LPG graphics IP. In relation to the media IP specifically, the bounds are listed as needed from A0, fixed in B0. So unless there's a belief that the workaround itself is incorrect, I think the bounds should be IS_MTL_MEDIA_STEP(i915, STEP_A0, STEP_B0) As discussed offline I will update the patch with above change and resend. Thanks, Badal Matt Fixes: 8f70f1ec587d ("drm/i915/mtl: Add Wa_14017073508 for SAMedia") Signed-off-by: Badal Nilawar --- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 4 ++-- drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index cef3d6f5c34e..4ba3c8c97ccc 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -29,7 +29,7 @@ static void mtl_media_busy(struct intel_gt *gt) { /* Wa_14017073508: mtl */ - if (IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0) && + if (IS_METEORLAKE(gt->i915) && INTEL_REVID(gt->i915) == 0 && gt->type == GT_MEDIA) snb_pcode_write_p(gt->uncore, PCODE_MBOX_GT_STATE, PCODE_MBOX_GT_STATE_MEDIA_BUSY, @@ -39,7 +39,7 @@ static void mtl_media_busy(struct intel_gt *gt) static void mtl_media_idle(struct intel_gt *gt) { /* Wa_14017073508: mtl */ - if (IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0) && + if (IS_METEORLAKE(gt->i915) && INTEL_REVID(gt->i915) == 0 && gt->type == GT_MEDIA) snb_pcode_write_p(gt->uncore, PCODE_MBOX_GT_STATE, PCODE_MBOX_GT_STATE_MEDIA_NOT_BUSY, diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c index fcf51614f9a4..7429c233ad45 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c @@ -19,7 +19,7 @@ static bool __guc_rc_supported(struct intel_guc *guc) * Do not enable gucrc to avoid additional interrupts which * may disrupt pcode wa. */ - if (IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0) && + if (IS_METEORLAKE(gt->i915) && INTEL_REVID(gt->i915) == 0 && gt->type == GT_MEDIA) return false; -- 2.25.1
Re: [PATCH v3] drm/i915/mtl: Enable Idle Messaging for GSC CS
On 19-11-2022 00:07, Vivi, Rodrigo wrote: On Sat, 2022-11-19 at 00:03 +0530, Badal Nilawar wrote: From: Vinay Belgaumkar By defaut idle messaging is disabled for GSC CS so to unblock RC6 entry on media tile idle messaging need to be enabled. v2: - Fix review comments (Vinay) - Set GSC idle hysteresis as per spec (Badal) v3: - Fix review comments (Rodrigo) Bspec: 71496 Cc: Daniele Ceraolo Spurio Signed-off-by: Vinay Belgaumkar Signed-off-by: Badal Nilawar Reviewed-by: Vinay Belgaumkar He is the author of the patch, no?! or you can remove this or change the author to be you and keep his reviewed-by... or I can just remove his rv-b while merging.. just let me know.. As he is original author I will prefer not to change it. You can remove his rv-b while merging. Regards, Badal Reviewed-by: Rodrigo Vivi --- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 18 ++ drivers/gpu/drm/i915/gt/intel_gt_regs.h | 4 2 files changed, 22 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index b0a4a2dbe3ee..e971b153fda9 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -15,6 +15,22 @@ #include "intel_rc6.h" #include "intel_ring.h" #include "shmem_utils.h" +#include "intel_gt_regs.h" + +static void intel_gsc_idle_msg_enable(struct intel_engine_cs *engine) +{ + struct drm_i915_private *i915 = engine->i915; + + if (IS_METEORLAKE(i915) && engine->id == GSC0) { + intel_uncore_write(engine->gt->uncore, + RC_PSMI_CTRL_GSCCS, + _MASKED_BIT_DISABLE(IDLE_MSG_DISABLE)); + /* hysteresis 0xA=5us as recommended in spec*/ + intel_uncore_write(engine->gt->uncore, + PWRCTX_MAXCNT_GSCCS, + 0xA); + } +} static void dbg_poison_ce(struct intel_context *ce) { @@ -275,6 +291,8 @@ void intel_engine_init__pm(struct intel_engine_cs *engine) intel_wakeref_init(>wakeref, rpm, _ops); intel_engine_init_heartbeat(engine); + + intel_gsc_idle_msg_enable(engine); } /** diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index c3cd92691795..80a979e6f6be 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -917,6 +917,10 @@ #define MSG_IDLE_FW_MASK REG_GENMASK(13, 9) #define MSG_IDLE_FW_SHIFT 9 +#defineRC_PSMI_CTRL_GSCCS _MMIO(0x11a050) +#define IDLE_MSG_DISABLE REG_BIT(0) +#definePWRCTX_MAXCNT_GSCCS _MMIO(0x11a054) + #define FORCEWAKE_MEDIA_GEN9 _MMIO(0xa270) #define FORCEWAKE_RENDER_GEN9 _MMIO(0xa278)
Re: [Intel-gfx] [PATCH 1/1] drm/i915/mtl: Enable Idle Messaging for GSC CS
On 18-11-2022 03:44, Rodrigo Vivi wrote: On Tue, Nov 15, 2022 at 07:14:40PM +0530, Badal Nilawar wrote: From: Vinay Belgaumkar By defaut idle mesaging is disabled for GSC CS so to unblock RC6 entry on media tile idle messaging need to be enabled. v2: - Fix review comments (Vinay) - Set GSC idle hysterisis to 5 us (Badal) Bspec: 71496 Cc: Daniele Ceraolo Spurio Signed-off-by: Vinay Belgaumkar Signed-off-by: Badal Nilawar --- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 18 ++ drivers/gpu/drm/i915/gt/intel_gt_regs.h | 4 2 files changed, 22 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index b0a4a2dbe3ee..5522885b2db0 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -15,6 +15,22 @@ #include "intel_rc6.h" #include "intel_ring.h" #include "shmem_utils.h" +#include "intel_gt_regs.h" + +static void intel_gsc_idle_msg_enable(struct intel_engine_cs *engine) +{ + struct drm_i915_private *i915 = engine->i915; + + if (IS_METEORLAKE(i915) && engine->id == GSC0) { + intel_uncore_write(engine->gt->uncore, + RC_PSMI_CTRL_GSCCS, + _MASKED_BIT_DISABLE(IDLE_MSG_DISABLE)); disable the disable? shouldn't be enable the disable? 1 = disable, no? + /* 5 us hysterisis */ could you please mention here in the comment that 0xA = 5 us per spec? I got confused again even though you had explained already... Sure I will add the comment "0xA=5 us as per spec" BTW, how reliable that spec is? Because according to that same line we should be setting the bit 16, not the bit 0 in the previous reg! Bit 16 is mask bit. Bit 0 need to be cleared to enable Idle messaging. Bit[0] = 1 Disable Idle Messaging / 0 Enable Idle Messaging. Regards, Badal + intel_uncore_write(engine->gt->uncore, + PWRCTX_MAXCNT_GSCCS, + 0xA); + } +} static void dbg_poison_ce(struct intel_context *ce) { @@ -275,6 +291,8 @@ void intel_engine_init__pm(struct intel_engine_cs *engine) intel_wakeref_init(>wakeref, rpm, _ops); intel_engine_init_heartbeat(engine); + + intel_gsc_idle_msg_enable(engine); } /** diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index 07031e03f80c..20472eb15364 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -913,6 +913,10 @@ #define MSG_IDLE_FW_MASK REG_GENMASK(13, 9) #define MSG_IDLE_FW_SHIFT9 +#define RC_PSMI_CTRL_GSCCS _MMIO(0x11a050) +#define IDLE_MSG_DISABLE BIT(0) +#define PWRCTX_MAXCNT_GSCCS_MMIO(0x11a054) + #define FORCEWAKE_MEDIA_GEN9 _MMIO(0xa270) #define FORCEWAKE_RENDER_GEN9 _MMIO(0xa278) -- 2.25.1
Re: [Intel-gfx] [PATCH 1/1] drm/i915/mtl: Enable Idle Messaging for GSC CS
Hi Rodrigo, On 15-11-2022 20:57, Rodrigo Vivi wrote: On Tue, Nov 15, 2022 at 07:14:40PM +0530, Badal Nilawar wrote: From: Vinay Belgaumkar By defaut idle mesaging is disabled for GSC CS so to unblock RC6 entry on media tile idle messaging need to be enabled. v2: - Fix review comments (Vinay) - Set GSC idle hysterisis to 5 us (Badal) btw, no need for the cover letter in single/standalone patches. This history here is enough. Sure, next revision I will take care of this. Bspec: 71496 Cc: Daniele Ceraolo Spurio Signed-off-by: Vinay Belgaumkar Signed-off-by: Badal Nilawar --- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 18 ++ drivers/gpu/drm/i915/gt/intel_gt_regs.h | 4 2 files changed, 22 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index b0a4a2dbe3ee..5522885b2db0 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -15,6 +15,22 @@ #include "intel_rc6.h" #include "intel_ring.h" #include "shmem_utils.h" +#include "intel_gt_regs.h" + +static void intel_gsc_idle_msg_enable(struct intel_engine_cs *engine) +{ + struct drm_i915_private *i915 = engine->i915; + + if (IS_METEORLAKE(i915) && engine->id == GSC0) { + intel_uncore_write(engine->gt->uncore, + RC_PSMI_CTRL_GSCCS, + _MASKED_BIT_DISABLE(IDLE_MSG_DISABLE)); + /* 5 us hysterisis */ typo + intel_uncore_write(engine->gt->uncore, + PWRCTX_MAXCNT_GSCCS, + 0xA); you said 5 above, but used 10 here, why? Bspec:71496 specifies 0xA = 5us. + } +} static void dbg_poison_ce(struct intel_context *ce) { @@ -275,6 +291,8 @@ void intel_engine_init__pm(struct intel_engine_cs *engine) intel_wakeref_init(>wakeref, rpm, _ops); intel_engine_init_heartbeat(engine); + + intel_gsc_idle_msg_enable(engine); } /** diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index 07031e03f80c..20472eb15364 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -913,6 +913,10 @@ #define MSG_IDLE_FW_MASK REG_GENMASK(13, 9) #define MSG_IDLE_FW_SHIFT9 +#define RC_PSMI_CTRL_GSCCS _MMIO(0x11a050) +#define IDLE_MSG_DISABLE BIT(0) REG_BIT should be preferred. I will take care of this and above comments in next revision. Regards, Badal +#define PWRCTX_MAXCNT_GSCCS_MMIO(0x11a054) + #define FORCEWAKE_MEDIA_GEN9 _MMIO(0xa270) #define FORCEWAKE_RENDER_GEN9 _MMIO(0xa278) -- 2.25.1
Re: [PATCH] drm/i915/mtl: Add MC6 Wa_14017210380 for SAMedia
Hi Matt, On 01-11-2022 20:47, Matt Roper wrote: On Sat, Oct 29, 2022 at 12:59:35AM +0530, Badal Nilawar wrote: This workaround is added for Media Tile of MTL A step. It is to help pcode workaround which handles the hardware bug seen on CXL splitter during package C2/C3 transitins due to MC6 entry/exit. As a part of workaround pcode expect kmd to send mailbox message "media busy" when components of Media tile is in use and "media not busy" when not in use. As per workaround description gucrc need to be disabled so enabled host based RC for Media tile. HSD: 14017210380 Cc: Rodrigo Vivi Cc: Radhakrishna Sripada Cc: Vinay Belgaumkar Cc: Chris Wilson Signed-off-by: Badal Nilawar --- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 33 +++ drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c | 13 - drivers/gpu/drm/i915/i915_drv.h | 4 +++ drivers/gpu/drm/i915/i915_reg.h | 9 +++ 4 files changed, 58 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index f553e2173bda..398dbeb298ca 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -19,10 +19,37 @@ #include "intel_rc6.h" #include "intel_rps.h" #include "intel_wakeref.h" +#include "intel_pcode.h" #include "pxp/intel_pxp_pm.h" #define I915_GT_SUSPEND_IDLE_TIMEOUT (HZ / 2) +/* + * Wa_14017210380: mtl + */ This doesn't appear to be a valid workaround number; workaround numbers are always supposed to be the "lineage" numbers from the workaround database. Wa_14017073508 seems to be related; is that the one you're implementing? Thanks for pointing out I will correct the workaround number in next revision. + +static bool mtl_needs_media_mc6_wa(struct intel_gt *gt) Drive-by comment: names like this aren't great since even though there's only one "media MC6" workaround today, that may not be true in the future. I think its better to drop this function and replace its calls below with (IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0) && gt->type == GT_MEDIA) Regards, Badal Matt +{ + return (IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0) && + gt->type == GT_MEDIA); +} + +static void mtl_mc6_wa_media_busy(struct intel_gt *gt) +{ + if (mtl_needs_media_mc6_wa(gt)) + snb_pcode_write_p(gt->uncore, PCODE_MBOX_GT_STATE, + PCODE_MBOX_GT_STATE_MEDIA_BUSY, + PCODE_MBOX_GT_STATE_DOMAIN_MEDIA, 0); +} + +static void mtl_mc6_wa_media_not_busy(struct intel_gt *gt) +{ + if (mtl_needs_media_mc6_wa(gt)) + snb_pcode_write_p(gt->uncore, PCODE_MBOX_GT_STATE, + PCODE_MBOX_GT_STATE_MEDIA_NOT_BUSY, + PCODE_MBOX_GT_STATE_DOMAIN_MEDIA, 0); +} + static void user_forcewake(struct intel_gt *gt, bool suspend) { int count = atomic_read(>user_wakeref); @@ -70,6 +97,9 @@ static int __gt_unpark(struct intel_wakeref *wf) GT_TRACE(gt, "\n"); + /* Wa_14017210380: mtl */ + mtl_mc6_wa_media_busy(gt); + /* * It seems that the DMC likes to transition between the DC states a lot * when there are no connected displays (no active power domains) during @@ -119,6 +149,9 @@ static int __gt_park(struct intel_wakeref *wf) GEM_BUG_ON(!wakeref); intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); + /* Wa_14017210380: mtl */ + mtl_mc6_wa_media_not_busy(gt); + return 0; } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c index 8f8dd05835c5..cc6356ff84a5 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c @@ -11,9 +11,20 @@ static bool __guc_rc_supported(struct intel_guc *guc) { + struct intel_gt *gt = guc_to_gt(guc); + + /* +* Wa_14017210380: mtl +* Do not enable gucrc to avoid additional interrupts which +* may disrupt pcode wa. +*/ + if (IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0) && + gt->type == GT_MEDIA) + return false; + /* GuC RC is unavailable for pre-Gen12 */ return guc->submission_supported && - GRAPHICS_VER(guc_to_gt(guc)->i915) >= 12; + GRAPHICS_VER(gt->i915) >= 12; } static bool __guc_rc_selected(struct intel_guc *guc) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 05b3300cc4ed..659b92382ff2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -740,6 +740,10 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, #define IS_XEHPSDV_GRAPHICS_STEP(__i915, since, until) \ (IS_XEHPSDV(__i915) && IS_GRAPHICS_STEP(__i915, since, until)) +#define IS_MTL_GRAPHICS_STEP(__i915, variant, since,
Re: [PATCH] drm/i915/mtl: Add MC6 Wa_14017210380 for SAMedia
Hi Rodrigo, On 31-10-2022 15:19, Rodrigo Vivi wrote: On Sat, Oct 29, 2022 at 12:59:35AM +0530, Badal Nilawar wrote: This workaround is added for Media Tile of MTL A step. It is to help pcode workaround which handles the hardware bug seen on CXL splitter during package C2/C3 transitins due to MC6 entry/exit. As a part of workaround pcode expect kmd to send mailbox message "media busy" when components of Media tile is in use and "media not busy" when not in use. As per workaround description gucrc need to be disabled so enabled host based RC for Media tile. HSD: 14017210380 Cc: Rodrigo Vivi Cc: Radhakrishna Sripada Cc: Vinay Belgaumkar Cc: Chris Wilson Signed-off-by: Badal Nilawar --- drivers/gpu/drm/i915/gt/intel_gt_pm.c | 33 +++ drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c | 13 - drivers/gpu/drm/i915/i915_drv.h | 4 +++ drivers/gpu/drm/i915/i915_reg.h | 9 +++ 4 files changed, 58 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index f553e2173bda..398dbeb298ca 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -19,10 +19,37 @@ #include "intel_rc6.h" #include "intel_rps.h" #include "intel_wakeref.h" +#include "intel_pcode.h" #include "pxp/intel_pxp_pm.h" #define I915_GT_SUSPEND_IDLE_TIMEOUT (HZ / 2) +/* + * Wa_14017210380: mtl + */ + +static bool mtl_needs_media_mc6_wa(struct intel_gt *gt) +Ashutosh since we recently discussed about the term "MC6". in that discussion we have concluded to not introduce a new term since it is not used in any kind of spec and might only bring more doubts then answers, although "Render" in the media gt makes no sense as well. In the end, most of the code is common and is called as rc6. so we should maybe s/mc6/media_rc6 here? Sure I will make above change and resend the patch. The rest of the patch looks good to me. But we need to check the IGT failure on a pm related test that failed... just to be sure. Failures reported in IGT so far are not related to this change. Regards, Badal +{ + return (IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0) && + gt->type == GT_MEDIA); +} + +static void mtl_mc6_wa_media_busy(struct intel_gt *gt) +{ + if (mtl_needs_media_mc6_wa(gt)) + snb_pcode_write_p(gt->uncore, PCODE_MBOX_GT_STATE, + PCODE_MBOX_GT_STATE_MEDIA_BUSY, + PCODE_MBOX_GT_STATE_DOMAIN_MEDIA, 0); +} + +static void mtl_mc6_wa_media_not_busy(struct intel_gt *gt) +{ + if (mtl_needs_media_mc6_wa(gt)) + snb_pcode_write_p(gt->uncore, PCODE_MBOX_GT_STATE, + PCODE_MBOX_GT_STATE_MEDIA_NOT_BUSY, + PCODE_MBOX_GT_STATE_DOMAIN_MEDIA, 0); +} + static void user_forcewake(struct intel_gt *gt, bool suspend) { int count = atomic_read(>user_wakeref); @@ -70,6 +97,9 @@ static int __gt_unpark(struct intel_wakeref *wf) GT_TRACE(gt, "\n"); + /* Wa_14017210380: mtl */ + mtl_mc6_wa_media_busy(gt); + /* * It seems that the DMC likes to transition between the DC states a lot * when there are no connected displays (no active power domains) during @@ -119,6 +149,9 @@ static int __gt_park(struct intel_wakeref *wf) GEM_BUG_ON(!wakeref); intel_display_power_put_async(i915, POWER_DOMAIN_GT_IRQ, wakeref); + /* Wa_14017210380: mtl */ + mtl_mc6_wa_media_not_busy(gt); + return 0; } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c index 8f8dd05835c5..cc6356ff84a5 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c @@ -11,9 +11,20 @@ static bool __guc_rc_supported(struct intel_guc *guc) { + struct intel_gt *gt = guc_to_gt(guc); + + /* +* Wa_14017210380: mtl +* Do not enable gucrc to avoid additional interrupts which +* may disrupt pcode wa. +*/ + if (IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0) && + gt->type == GT_MEDIA) + return false; + /* GuC RC is unavailable for pre-Gen12 */ return guc->submission_supported && - GRAPHICS_VER(guc_to_gt(guc)->i915) >= 12; + GRAPHICS_VER(gt->i915) >= 12; } static bool __guc_rc_selected(struct intel_guc *guc) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 05b3300cc4ed..659b92382ff2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -740,6 +740,10 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, #define IS_XEHPSDV_GRAPHICS_STEP(__i915, since, until) \ (IS_XEHPSDV(__i915) && IS_GRAPHICS_STEP(__i915, since, until)) +#define IS_MTL_GRAPHICS_STEP(__i915, variant,
Re: [PATCH 0/7] Add HWMON support
On 27-09-2022 02:39, Guenter Roeck wrote: On 9/26/22 10:52, Badal Nilawar wrote: This series adds the HWMON support for DGFX Test-with: 20220919144408.251981-1-riana.ta...@intel.com v2: - Reorganized series. Created first patch as infrastructure patch followed by feature patches. (Ashutosh) - Fixed review comments (Jani) - Fixed review comments (Ashutosh) v3: - Fixed review comments from Guenter - Exposed energy inferface as standard hwmon interface (Ashutosh) - For power interface added entries for critical power and maintained standard interface for all the entries except power1_max_interval - Extended support for XEHPSDV (Ashutosh) v4: - Fixed review comment from Guenter - Cleaned up unused code v5: - Fixed review comments (Jani) v6: - Fixed review comments (Ashutosh) - Updated date and kernel version in documentation v7: - Fixed review comments (Anshuman) - KernelVersion: 6.2, Date: February 2023 in doc (Tvrtko) v8: s/hwmon_device_register_with_info/ devm_hwmon_device_register_with_info/ (Ashutosh) Is there some reason for not actually versioning this patch series ? Just wondering. Sorry I miss typed cover letter title. I will correct the title as "drm/i915: Add HWMON support" and resend the series to maintain versioning. Please ignore this series. Regards, Badal Thanks, Guenter Ashutosh Dixit (2): drm/i915/hwmon: Expose card reactive critical power drm/i915/hwmon: Expose power1_max_interval Dale B Stimson (4): drm/i915/hwmon: Add HWMON infrastructure drm/i915/hwmon: Power PL1 limit and TDP setting drm/i915/hwmon: Show device level energy usage drm/i915/hwmon: Extend power/energy for XEHPSDV Riana Tauro (1): drm/i915/hwmon: Add HWMON current voltage support .../ABI/testing/sysfs-driver-intel-i915-hwmon | 75 ++ drivers/gpu/drm/i915/Makefile | 3 + drivers/gpu/drm/i915/gt/intel_gt_regs.h | 8 + drivers/gpu/drm/i915/i915_driver.c | 5 + drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_hwmon.c | 736 ++ drivers/gpu/drm/i915/i915_hwmon.h | 20 + drivers/gpu/drm/i915/i915_reg.h | 6 + drivers/gpu/drm/i915/intel_mchbar_regs.h | 21 + 9 files changed, 876 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon create mode 100644 drivers/gpu/drm/i915/i915_hwmon.c create mode 100644 drivers/gpu/drm/i915/i915_hwmon.h
Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure
On 21-09-2022 18:14, Andi Shyti wrote: Hi Badal, +struct hwm_reg { +}; + +struct hwm_drvdata { + struct i915_hwmon *hwmon; + struct intel_uncore *uncore; + struct device *hwmon_dev; + char name[12]; +}; + +struct i915_hwmon { + struct hwm_drvdata ddat; + struct mutex hwmon_lock;/* counter overflow logic and rmw */ + struct hwm_reg rg; +}; + +static const struct hwmon_channel_info *hwm_info[] = { + NULL +}; + +static umode_t +hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + default: + return 0; + } +} + +static int +hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, +int channel, long *val) +{ + switch (type) { + default: + return -EOPNOTSUPP; + } +} + +static int +hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, + int channel, long val) +{ + switch (type) { + default: + return -EOPNOTSUPP; + } +} + +static const struct hwmon_ops hwm_ops = { + .is_visible = hwm_is_visible, + .read = hwm_read, + .write = hwm_write, +}; + +static const struct hwmon_chip_info hwm_chip_info = { + .ops = _ops, + .info = hwm_info, +}; what's the point for splitting so much? Can't you just send the hwmon driver all at once? With this patch you are not actually doing anything useful. In my opinion this should be squashed with the next ones. During discussion in cover letter of rev0 series we decided to create separate infrastructure patch, as we wanted to keep kconfig, i915 hwmon structures and new file addition in separate patch. Further feature wise we kept adding new patches. +static void +hwm_get_preregistration_info(struct drm_i915_private *i915) +{ +} + +void i915_hwmon_register(struct drm_i915_private *i915) +{ + struct device *dev = i915->drm.dev; + struct i915_hwmon *hwmon; + struct device *hwmon_dev; + struct hwm_drvdata *ddat; + + /* hwmon is available only for dGfx */ + if (!IS_DGFX(i915)) + return; + + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL); why don't we use devm_kzalloc? + if (!hwmon) + return; + + i915->hwmon = hwmon; + mutex_init(>hwmon_lock); + ddat = >ddat; + + ddat->hwmon = hwmon; + ddat->uncore = >uncore; + snprintf(ddat->name, sizeof(ddat->name), "i915"); + + hwm_get_preregistration_info(i915); + + /* hwmon_dev points to device hwmon */ + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, + ddat, + _chip_info, + NULL); + if (IS_ERR(hwmon_dev)) { + mutex_destroy(>hwmon_lock); there is not such a big need to destroy the mutex. Destroying mutexes is more useful when you actually are creating/destroying and there is some debug need. I don't think that's the case. With the devm_kzalloc this would be just a return. I think we can switch to devm_kzalloc. Regards, Badal Andi + i915->hwmon = NULL; + kfree(hwmon); + return; + } + + ddat->hwmon_dev = hwmon_dev; +} + +void i915_hwmon_unregister(struct drm_i915_private *i915) +{ + struct i915_hwmon *hwmon; + struct hwm_drvdata *ddat; + + hwmon = fetch_and_zero(>hwmon); + if (!hwmon) + return; + + ddat = >ddat; + if (ddat->hwmon_dev) + hwmon_device_unregister(ddat->hwmon_dev); + + mutex_destroy(>hwmon_lock); + kfree(hwmon); +} diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h new file mode 100644 index ..7ca9cf2c34c9 --- /dev/null +++ b/drivers/gpu/drm/i915/i915_hwmon.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: MIT */ + +/* + * Copyright © 2022 Intel Corporation + */ + +#ifndef __I915_HWMON_H__ +#define __I915_HWMON_H__ + +struct drm_i915_private; + +#if IS_REACHABLE(CONFIG_HWMON) +void i915_hwmon_register(struct drm_i915_private *i915); +void i915_hwmon_unregister(struct drm_i915_private *i915); +#else +static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; +static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; +#endif + +#endif /* __I915_HWMON_H__ */ -- 2.25.1
Re: [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting
On 21-09-2022 17:15, Gupta, Anshuman wrote: On 9/16/2022 8:30 PM, Badal Nilawar wrote: From: Dale B Stimson Use i915 HWMON to display/modify dGfx power PL1 limit and TDP setting. v2: - Fix review comments (Ashutosh) - Do not restore power1_max upon module unload/load sequence because on production systems modules are always loaded and not unloaded/reloaded (Ashutosh) - Fix review comments (Jani) - Remove endianness conversion (Ashutosh) v3: Add power1_rated_max (Ashutosh) v4: - Use macro HWMON_CHANNEL_INFO to define power channel (Guenter) - Update the date and kernel version in Documentation (Badal) v5: Use hwm_ prefix for static functions (Ashutosh) v6: - Fix review comments (Ashutosh) - Update date, kernel version in documentation Cc: Guenter Roeck Signed-off-by: Dale B Stimson Signed-off-by: Ashutosh Dixit Signed-off-by: Riana Tauro Signed-off-by: Badal Nilawar Acked-by: Guenter Roeck --- .../ABI/testing/sysfs-driver-intel-i915-hwmon | 20 +++ drivers/gpu/drm/i915/i915_hwmon.c | 158 +- drivers/gpu/drm/i915/i915_reg.h | 5 + drivers/gpu/drm/i915/intel_mchbar_regs.h | 6 + 4 files changed, 187 insertions(+), 2 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon index e2974f928e58..bc061238e35c 100644 --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon @@ -5,3 +5,23 @@ Contact: dri-devel@lists.freedesktop.org Description: RO. Current Voltage in millivolt. Only supported for particular Intel i915 graphics platforms. + +What: /sys/devices/.../hwmon/hwmon/power1_max +Date: September 2022 +KernelVersion: 6 +Contact: dri-devel@lists.freedesktop.org +Description: RW. Card reactive sustained (PL1/Tau) power limit in microwatts. + + The power controller will throttle the operating frequency + if the power averaged over a window (typically seconds) + exceeds this limit. + + Only supported for particular Intel i915 graphics platforms. + +What: /sys/devices/.../hwmon/hwmon/power1_rated_max +Date: September 2022 +KernelVersion: 6 +Contact: dri-devel@lists.freedesktop.org +Description: RO. Card default power limit (default TDP setting). + + Only supported for particular Intel i915 graphics platforms. diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 45745afa5c5b..5183cf51a49b 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -16,11 +16,16 @@ /* * SF_* - scale factors for particular quantities according to hwmon spec. * - voltage - millivolts + * - power - microwatts */ #define SF_VOLTAGE 1000 +#define SF_POWER 100 struct hwm_reg { i915_reg_t gt_perf_status; + i915_reg_t pkg_power_sku_unit; + i915_reg_t pkg_power_sku; + i915_reg_t pkg_rapl_limit; }; struct hwm_drvdata { @@ -34,10 +39,68 @@ struct i915_hwmon { struct hwm_drvdata ddat; struct mutex hwmon_lock; /* counter overflow logic and rmw */ struct hwm_reg rg; + int scl_shift_power; }; +static void +hwm_locked_with_pm_intel_uncore_rmw(struct hwm_drvdata *ddat, + i915_reg_t reg, u32 clear, u32 set) +{ + struct i915_hwmon *hwmon = ddat->hwmon; + struct intel_uncore *uncore = ddat->uncore; + intel_wakeref_t wakeref; + + mutex_lock(>hwmon_lock); + + with_intel_runtime_pm(uncore->rpm, wakeref) + intel_uncore_rmw(uncore, reg, clear, set); + + mutex_unlock(>hwmon_lock); +} + +/* + * This function's return type of u64 allows for the case where the scaling + * of the field taken from the 32-bit register value might cause a result to + * exceed 32 bits. + */ +static u64 +hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr, + u32 field_msk, int nshift, u32 scale_factor) +{ + struct intel_uncore *uncore = ddat->uncore; + intel_wakeref_t wakeref; + u32 reg_value; + + with_intel_runtime_pm(uncore->rpm, wakeref) + reg_value = intel_uncore_read(uncore, rgadr); + + reg_value = REG_FIELD_GET(field_msk, reg_value); + + return mul_u64_u32_shr(reg_value, scale_factor, nshift); +} + +static void +hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr, + u32 field_msk, int nshift, + unsigned int scale_factor, long lval) +{ + u32 nval; + u32 bits_to_clear; + u32 bits_to_set; + + /* Computation in 64-bits to avoid overflow. Round to nearest. */ + nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor); + + bits_to_clear = field_msk; + bits_to_set = FIELD_PREP(field_msk, nval); + + hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr, + bits_to_clear, bits_to_set); +} +
Re: [Intel-gfx] [PATCH 1/2] drm/i915/mtl: Modify CAGF functions for MTL
On 19-09-2022 22:19, Andi Shyti wrote: Hi Badal, On Mon, Sep 19, 2022 at 05:29:05PM +0530, Badal Nilawar wrote: Updated the CAGF functions to get actual resolved frequency of 3D and SAMedia can you please use the imperative form? "Update" and not "Updated". Ok. Besides I don't really understand what you did from the commit, can you please bea bit more descriptive? Sure I will describe more. For MTL Current Actual GFX frequency (CAGF) can be obtained from regs 0xc60 (GT0) and 0x380c60 (GT1). So to support MTL I modified functions read_cagf and intel_rps_get_cagf. Bspec: 66300 Cc: Vinay Belgaumkar Cc: Ashutosh Dixit Signed-off-by: Badal Nilawar --- drivers/gpu/drm/i915/gt/intel_gt_regs.h | 8 drivers/gpu/drm/i915/gt/intel_rps.c | 6 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index 2275ee47da95..7819d32db956 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -1510,6 +1510,14 @@ #define VLV_RENDER_C0_COUNT _MMIO(0x138118) #define VLV_MEDIA_C0_COUNT_MMIO(0x13811c) +/* + * MTL: Workpoint reg to get Core C state and act freq of 3D, SAMedia/ + * 3D - 0x0C60 , SAMedia - 0x380C60 + * Intel uncore handler redirects transactions for SAMedia to MTL_MEDIA_GSI_BASE + */ This comment is not understandable... we don't have limits in space, you can be a bit more explicit :) Below I defined only 0x0C60, so I am trying to say that intel_uncore_read/write functions takes care of adding GSI offset i.e. 0x38000 if the access is for Gt1 (SAMEDIA). This patch gives more clarity about GSI offset https://patchwork.freedesktop.org/patch/502004/?series=107908=5 Regards, Badal Andi +#define MTL_MIRROR_TARGET_WP1 _MMIO(0x0C60) +#define MTL_CAGF_MASKREG_GENMASK(8, 0) + #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4)) #define GEN11_CSME (31) #define GEN11_GUNIT (28) diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 17b40b625e31..c2349949ebae 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2075,6 +2075,8 @@ u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat) if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) cagf = (rpstat >> 8) & 0xff; + else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) + cagf = rpstat & MTL_CAGF_MASK; else if (GRAPHICS_VER(i915) >= 9) cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT; else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) @@ -2098,7 +2100,9 @@ static u32 read_cagf(struct intel_rps *rps) vlv_punit_get(i915); freq = vlv_punit_read(i915, PUNIT_REG_GPU_FREQ_STS); vlv_punit_put(i915); - } else if (GRAPHICS_VER(i915) >= 6) { + } else if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) + freq = intel_uncore_read(rps_to_gt(rps)->uncore, MTL_MIRROR_TARGET_WP1); + else if (GRAPHICS_VER(i915) >= 6) { freq = intel_uncore_read(uncore, GEN6_RPSTAT1); } else { freq = intel_uncore_read(uncore, MEMSTAT_ILK); -- 2.25.1
Re: [PATCH 0/7] drm/i915: Add HWMON support
On 19-09-2022 15:45, Gupta, Anshuman wrote: -Original Message- From: Nilawar, Badal Sent: Friday, September 16, 2022 8:31 PM To: intel-...@lists.freedesktop.org Cc: Dixit, Ashutosh ; Tauro, Riana ; Gupta, Anshuman ; Ewins, Jon ; linux-hw...@vger.kernel.org; dri- de...@lists.freedesktop.org Subject: [PATCH 0/7] drm/i915: Add HWMON support This series adds the HWMON support for DGFX Test-with: 20220914140721.3500129-1-riana.ta...@intel.com CI-BAT is failing with this series, Could you please check the failures, whether related to this series ? Thanks for pointing out. I checked the failures, those are not related to this series. I responded to "✗ Fi.CI.BAT: failure" thread Regards, Badal Nilawar Thanks, Anshuman Gupta. v2: - Reorganized series. Created first patch as infrastructure patch followed by feature patches. (Ashutosh) - Fixed review comments (Jani) - Fixed review comments (Ashutosh) v3: - Fixed review comments from Guenter - Exposed energy inferface as standard hwmon interface (Ashutosh) - For power interface added entries for critical power and maintained standard interface for all the entries except power1_max_interval - Extended support for XEHPSDV (Ashutosh) v4: - Fixed review comment from Guenter - Cleaned up unused code v5: - Fixed review comments (Jani) v6: - Fixed review comments (Ashutosh) - Updated date and kernel version in documentation Ashutosh Dixit (2): drm/i915/hwmon: Expose card reactive critical power drm/i915/hwmon: Expose power1_max_interval Dale B Stimson (4): drm/i915/hwmon: Add HWMON infrastructure drm/i915/hwmon: Power PL1 limit and TDP setting drm/i915/hwmon: Show device level energy usage drm/i915/hwmon: Extend power/energy for XEHPSDV Riana Tauro (1): drm/i915/hwmon: Add HWMON current voltage support .../ABI/testing/sysfs-driver-intel-i915-hwmon | 75 ++ drivers/gpu/drm/i915/Makefile | 3 + drivers/gpu/drm/i915/gt/intel_gt_regs.h | 8 + drivers/gpu/drm/i915/i915_driver.c| 5 + drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_hwmon.c | 761 ++ drivers/gpu/drm/i915/i915_hwmon.h | 21 + drivers/gpu/drm/i915/i915_reg.h | 14 + drivers/gpu/drm/i915/intel_mchbar_regs.h | 12 + 9 files changed, 901 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon create mode 100644 drivers/gpu/drm/i915/i915_hwmon.c create mode 100644 drivers/gpu/drm/i915/i915_hwmon.h -- 2.25.1
Re: [PATCH 3/3] drm/i915/guc/slpc: Update boost sysfs hooks for SLPC
Please fix code style related warnings and errors from checkpatch result. On 21-10-2021 01:22, Vinay Belgaumkar wrote: Add a helper to sort through the SLPC/RPS cases of get/set methods. Boost frequency will be modified as long as it is within the constraints of RP0 and if it is different from the existing one. We will set min freq to boost only if there is an active waiter. Signed-off-by: Vinay Belgaumkar --- drivers/gpu/drm/i915/gt/intel_rps.c | 44 + drivers/gpu/drm/i915/gt/intel_rps.h | 2 + drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 18 + drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 1 + drivers/gpu/drm/i915/i915_sysfs.c | 21 ++ 5 files changed, 69 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 023e9c0b9f4a..19c57aac9553 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -935,6 +935,50 @@ void intel_rps_park(struct intel_rps *rps) GT_TRACE(rps_to_gt(rps), "park:%x\n", rps->cur_freq); } +u32 intel_rps_get_boost_frequency(struct intel_rps *rps) +{ + struct intel_guc_slpc *slpc = rps_to_slpc(rps); + + if (rps_uses_slpc(rps)) + return slpc->boost_freq; + else + return intel_gpu_freq(rps, rps->boost_freq); +} + +static int set_boost_freq(struct intel_rps *rps, u32 val) +{ + bool boost = false; + + /* Validate against (static) hardware limits */ + val = intel_freq_opcode(rps, val); + if (val < rps->min_freq || val > rps->max_freq) + return -EINVAL; + + mutex_lock(>lock); + if (val != rps->boost_freq) { + rps->boost_freq = val; + boost = atomic_read(>num_waiters); + } + mutex_unlock(>lock); + if (boost) + schedule_work(>work); + + return 0; +} + +int intel_rps_set_boost_frequency(struct intel_rps *rps, u32 freq) +{ + struct intel_guc_slpc *slpc; + + if (rps_uses_slpc(rps)) { + slpc = rps_to_slpc(rps); + + return intel_guc_slpc_set_boost_freq(slpc, freq); + } else { + return set_boost_freq(rps, freq); + } +} + void intel_rps_update_waiters(struct intel_rps *rps) { struct intel_guc_slpc *slpc = rps_to_slpc(rps); diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index 4ca9924cb5ed..ce81094cf58e 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -24,6 +24,8 @@ void intel_rps_park(struct intel_rps *rps); void intel_rps_unpark(struct intel_rps *rps); void intel_rps_boost(struct i915_request *rq); void intel_rps_update_waiters(struct intel_rps *rps); +u32 intel_rps_get_boost_frequency(struct intel_rps *rps); +int intel_rps_set_boost_frequency(struct intel_rps *rps, u32 freq); int intel_rps_set(struct intel_rps *rps, u8 val); void intel_rps_mark_interactive(struct intel_rps *rps, bool interactive); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index a104371a8b79..7881bc1a5af8 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -613,6 +613,24 @@ void intel_guc_slpc_boost(struct intel_guc_slpc *slpc) slpc->num_waiters++; } +int intel_guc_slpc_set_boost_freq(struct intel_guc_slpc *slpc, u32 val) +{ + if (val < slpc->min_freq || val > slpc->rp0_freq) + return -EINVAL; + + if (val != slpc->boost_freq) { + slpc->boost_freq = val; + + /* Apply only if there are active waiters */ + if (slpc->num_waiters) + return slpc_set_param(slpc, + SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, + slpc->boost_freq); As per comments from some other ML wakeref may be needed here. CC: jon.ew...@intel.com, ashutosh.di...@intel.com + } + + return 0; +} + void intel_guc_slpc_update_waiters(struct intel_guc_slpc *slpc) { /* Return min back to the softlimit. diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h index 25093dfdea0b..d8191f2b965b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -34,6 +34,7 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc); void intel_guc_slpc_fini(struct intel_guc_slpc *slpc); int intel_guc_slpc_set_max_freq(struct intel_guc_slpc *slpc, u32 val); int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val); +int intel_guc_slpc_set_boost_freq(struct intel_guc_slpc *slpc, u32 val); int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val); int intel_guc_slpc_get_min_freq(struct intel_guc_slpc *slpc,