| Message ID | 20250324132952.1075209-1-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | CCS static load balance | expand |
Acked-by: Michal Mrozek <michal.mrozek@intel.com>
Quoting Andi Shyti (2025-03-24 15:29:36) > Hi, > > Back in v3, this patch series was turned down due to community > policies regarding i915 GEM development. Since then, I have > received several requests from userspace developers, which I > initially declined in order to respect those policies. > > However, with the latest request from UMD users, I decided to > give this series another chance. I believe that when a feature > is genuinely needed, our goal should be to support it, not to > dismiss user and customer needs blindly. We had plenty of community bug reports when the move to fixed CCS mode was initially implemented with some bugs. After those bugs were fixed, nobody was reporting impactful performance regressions. Do you have a reference to some GitLab issues or maybe some external project issues where regressions around here are discussed? Regards, Joonas
The ccs mode setting support via sysfs is required by our customer.
Acked-by: Arshad Mehmood <arshad.mehmood@intel.com>
Hi Joonas, thanks a lot for your reply! On Tue, Mar 25, 2025 at 10:24:42AM +0200, Joonas Lahtinen wrote: > Quoting Andi Shyti (2025-03-24 15:29:36) > > Back in v3, this patch series was turned down due to community > > policies regarding i915 GEM development. Since then, I have > > received several requests from userspace developers, which I > > initially declined in order to respect those policies. > > > > However, with the latest request from UMD users, I decided to > > give this series another chance. I believe that when a feature > > is genuinely needed, our goal should be to support it, not to > > dismiss user and customer needs blindly. > > We had plenty of community bug reports when the move to fixed CCS mode > was initially implemented with some bugs. > > After those bugs were fixed, nobody was reporting impactful performance > regressions. > > Do you have a reference to some GitLab issues or maybe some external > project issues where regressions around here are discussed? AFAIK, there's no GitLab issue for this because we're not fixing a bug here; we're adding a new sysfs interface. All known issues and reports related to CCS load balancing have already been addressed. What we're still missing is a way for compute applications to tweak CCS load balancing settings. I already shared the link [1], but if you take a look at that code, you'll find 'execution_environment_drm.cpp' [2], where the new interface is used. If you're feeling lazy, I can point out the relevant parts, otherwise, feel free to skip to the final greetings :-) In 'void ExecutionEnvironment::configureCcsMode()', the app sets up the path like this: const std::string drmPath = "/sys/class/drm"; const std::string expectedFilePrefix = drmPath + "/card"; ... auto gtFiles = Directory::getFiles(gtPath.c_str()); auto expectedGtFilePrefix = gtPath + "/gt"; ... std::string ccsFile = gtFile + "/ccs_mode"; Then it writes the desired CCS mode value: uint32_t ccsValue = 0; ssize_t ret = SysCalls::read(fd, &ccsValue, sizeof(uint32_t)); ... do { ret = SysCalls::write(fd, &ccsMode, sizeof(uint32_t)); } while (ret == -1 && errno == -EBUSY); Arshad and Usha can definitely help if there are any technical questions about how the application uses the interface. Usha, would you please be able to share your use case? Thanks, Andi [1] https://github.com/intel/compute-runtime [2] https://github.com/intel/compute-runtime/blob/master/shared/source/execution_environment/execution_environment_drm.cpp
Quoting Andi Shyti (2025-03-25 12:52:58) > On Tue, Mar 25, 2025 at 10:24:42AM +0200, Joonas Lahtinen wrote: <SNIP> > > Do you have a reference to some GitLab issues or maybe some external > > project issues where regressions around here are discussed? > > AFAIK, there's no GitLab issue for this because we're not fixing > a bug here; we're adding a new sysfs interface. This sysfs interface was exactly designed to address performance regressions coming from limiting the number of CCS to 1. So unless we have a specific workload and end-user reporting a regression on it, there's no incentive to spend any further time here. <SNIP> > Arshad and Usha can definitely help if there are any technical > questions about how the application uses the interface. I don't have any technical questions as I specified the interface initially :) This is not about technical opens about how the interface works. To recap, when we initially implemented the 1CCS mode, we got active feedback on the community on regressions. We were careful to verify that all userspace would cleanly fall back to using 1CCS mode after it was implemented. And indeed, nobody has been asking for the 4CCS mode back after the 1CCS mode bugs were fixed. So as far as I see it, there are no users for this interface in upstream, and thus we should not spend the time on it. Regards, Joonas
Justification: To address a hardware bug causing stability issues when RCS and multiple CCS operate simultaneously in dynamic load balancing mode, CCG limited the CCS count to 1 as a software workaround. Many ECG customers run compute-only workloads without requiring rendering tasks. Therefore, it is important to provide customers with a runtime configuration option to increase the CCS count for compute-only workloads, in order to meet the performance requirements.
Acked-by: Usharani Ayyalasomayajula <usharani.ayyalasomayajula@intel.com>
Thanks,
Usha.
I’d like to provide additional context regarding the necessity of these patches. The shift from dynamic load balancing mode to fixed mode, with CCS usage restricted to a single unit, has led to a notable performance regression, with workloads experiencing an approximately 10% FPS drop. For example, on DG2, the ResNet-50 inference benchmark previously achieved ~10,500 FPS in dynamic load balancing mode. However, after limiting CCS to 1 in fixed mode, performance dropped to ~9,200 FPS. With these patches, enabling all 4 CCS units via sysfs (in fixed mode) restores performance back to nearly 10,500 FPS, effectively matching the previous dynamic mode results. Given customer expectations to maintain prior performance levels, these patches are essential to ensuring workloads utilizing multiple CCS units do not experience unnecessary degradation. The proposed sysfs interface provides configurability, allowing controlled re-enablement of all 4 CCS units while keeping fixed mode intact. Since fixed mode is now in use, having a configurable approach ensures flexibility to address different scenarios that may arise. Let me know if you need further details. Regards, Arshad
Hi, Back in v3, this patch series was turned down due to community policies regarding i915 GEM development. Since then, I have received several requests from userspace developers, which I initially declined in order to respect those policies. However, with the latest request from UMD users, I decided to give this series another chance. I believe that when a feature is genuinely needed, our goal should be to support it, not to dismiss user and customer needs blindly. Here is the link to the userspace counterpart, which depends on this series to function properly[*]. I've been refreshing and testing the series together with Arshad. This patchset introduces static load balancing for GPUs with multiple compute engines. It's a relatively long series. To help with review, I've broken the work down as much as possible in multiple patches. To summarise: - Patches 1 to 14 introduce no functional changes, aside from adding the 'num_cslices' interface. - Patch 15 contains the core of the CCS mode setting, building on the earlier groundwork. The updated approach focuses on managing the UABI engine list, which controls which engines are exposed to userspace. Instead of manipulating physical engines and their memory directly, we now control exposure via this list. Since v3, I've kept the changes in v4 to a minimum because there wasn't a real technical review on the previous posting. I would really appreciate it if this time all technical concerns could be raised and discussed on the mailing list. IGT tests for this work exist but haven't been submitted yet. Thanks to Chris for the reviews, to Arshad for the work we've done together over the past few weeks, and to Michal for his invaluable input from the userspace side. Thanks, Andi [*] https://github.com/intel/compute-runtime Changelog: ========== PATCHv3 -> PATCHv4 ------------------ - Rebase on top of the latest drm-tip - Do not call functions inside GEM_BUG_ONs, but call them explicitly (thanks Arshad). PATCHv2 -> PATCHv3 ------------------ - Fix a NULL pointer dereference during module unload. In i915_gem_driver_remove() I was accessing the gt after the gt was removed. Use the dev_priv, instead (obviously!). - Fix a lockdep issue: Some of the uabi_engines_mutex unlocks were not correctly placed in the exit paths. - Fix a checkpatch error for spaces after and before parenthesis in the for_each_enabled_engine() definition. PATCHv1 -> PATCHv2 ------------------ - Use uabi_mutex to protect the uabi_engines, not the engine itself. Rename it to uabi_engines_mutex. - Use kobject_add/kobject_del for adding and removing interfaces, this way we don't need to destroy and recreate the engines, anymore. Refactor intel_engine_add_single_sysfs() to reflect this scenario. - After adding engines to the rb_tree check that they have been added correctly. - Fix rb_find_add() compare function to take into accoung also the class, not just the instance. RFCv2 -> PATCHv1 ---------------- - Removed gt->ccs.mutex - Rename m -> width, ccs_id -> engine in intel_gt_apply_ccs_mode(). - In the CCS register value calculation (intel_gt_apply_ccs_mode()) the engine (ccs_id) needs to move along the ccs_mask (set by the user) instead of the cslice_mask. - Add GEM_BUG_ON after calculating the new ccs_mask (update_ccs_mask()) to make sure all angines have been evaluated (i.e. ccs_mask must be '0' at the end of the algorithm). - move wakeref lock before evaluating intel_gt_pm_is_awake() and fix exit path accordingly. - Use a more compact form in intel_gt_sysfs_ccs_init() and add_uabi_ccs_engines() when evaluating sysfs_create_file(): no need to store the return value to the err variable which is unused. Get rid of err. - Print a warnging instead of a debug message if we fail to create the sysfs files. - If engine files creation fails in intel_engine_add_single_sysfs(), print a warning, not an error. - Rename gt->ccs.ccs_mask to gt->ccs.id_mask and add a comment to explain its purpose. - During uabi engine creation, in intel_engines_driver_register(), the uabi_ccs_instance is redundant because the ccs_instances is already tracked in engine->uabi_instance. - Mark add_uabi_ccs_engines() and remove_uabi_ccs_engines() as __maybe_unused not to break bisectability. They wouldn't compile in their own commit. They will be used in the next patch and the __maybe_unused is removed. - Update engine's workaround every time a new mode is set in update_ccs_mask(). - Mark engines as valid or invalid using their status as rb_node. Invalid engines are marked as invalid using RB_CLEAR_NODE(). Execbufs will check for their validity when selecting the engine to be combined to a context. - Create for_each_enabled_engine() which skips the non valid engines and use it in selftests. RFCv1 -> RFCv2 -------------- Compared to the first version I've taken a completely different approach to adding and removing engines. in v1 physical engines were directly added and removed, along with the memory allocated to them, each time the user changed the CCS mode (from the previous cover letter). Andi Shyti (15): drm/i915/gt: Avoid using masked workaround for CCS_MODE setting drm/i915/gt: Move the CCS mode variable to a global position drm/i915/gt: Allow the creation of multi-mode CCS masks drm/i915/gt: Refactor uabi engine class/instance list creation drm/i915/gem: Mark and verify UABI engine validity drm/i915/gt: Introduce for_each_enabled_engine() and apply it in selftests drm/i915/gt: Manage CCS engine creation within UABI exposure drm/i915/gt: Remove cslices mask value from the CCS structure drm/i915/gt: Expose the number of total CCS slices drm/i915/gt: Store engine-related sysfs kobjects drm/i915/gt: Store active CCS mask drm/i915: Protect access to the UABI engines list with a mutex drm/i915/gt: Isolate single sysfs engine file creation drm/i915/gt: Implement creation and removal routines for CCS engines drm/i915/gt: Allow the user to change the CCS mode through sysfs drivers/gpu/drm/i915/gem/i915_gem_context.c | 3 + .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 28 +- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 23 -- drivers/gpu/drm/i915/gt/intel_engine_types.h | 2 + drivers/gpu/drm/i915/gt/intel_engine_user.c | 62 ++- drivers/gpu/drm/i915/gt/intel_gt.c | 3 + drivers/gpu/drm/i915/gt/intel_gt.h | 12 + drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c | 357 +++++++++++++++++- drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h | 5 +- drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 2 + drivers/gpu/drm/i915/gt/intel_gt_types.h | 19 +- drivers/gpu/drm/i915/gt/intel_workarounds.c | 8 +- drivers/gpu/drm/i915/gt/selftest_context.c | 6 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 4 +- .../drm/i915/gt/selftest_engine_heartbeat.c | 6 +- drivers/gpu/drm/i915/gt/selftest_engine_pm.c | 6 +- drivers/gpu/drm/i915/gt/selftest_execlists.c | 52 +-- drivers/gpu/drm/i915/gt/selftest_gt_pm.c | 2 +- drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 22 +- drivers/gpu/drm/i915/gt/selftest_lrc.c | 18 +- drivers/gpu/drm/i915/gt/selftest_mocs.c | 6 +- drivers/gpu/drm/i915/gt/selftest_rc6.c | 4 +- drivers/gpu/drm/i915/gt/selftest_reset.c | 8 +- .../drm/i915/gt/selftest_ring_submission.c | 2 +- drivers/gpu/drm/i915/gt/selftest_rps.c | 14 +- drivers/gpu/drm/i915/gt/selftest_timeline.c | 14 +- drivers/gpu/drm/i915/gt/selftest_tlb.c | 2 +- .../gpu/drm/i915/gt/selftest_workarounds.c | 14 +- drivers/gpu/drm/i915/gt/sysfs_engines.c | 80 ++-- drivers/gpu/drm/i915/gt/sysfs_engines.h | 2 + drivers/gpu/drm/i915/i915_cmd_parser.c | 2 + drivers/gpu/drm/i915/i915_debugfs.c | 4 + drivers/gpu/drm/i915/i915_drv.h | 5 + drivers/gpu/drm/i915/i915_gem.c | 4 + drivers/gpu/drm/i915/i915_perf.c | 8 +- drivers/gpu/drm/i915/i915_pmu.c | 11 +- drivers/gpu/drm/i915/i915_query.c | 21 +- 37 files changed, 648 insertions(+), 193 deletions(-)