| Message ID | 20250108141318.63823-3-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable GuC SLPC default balancing strategies v2 | expand |
On 1/8/2025 6:13 AM, Rodrigo Vivi wrote: > The Balancer and DCC strategies were left off on a fear that > these strategies would conflict with the i915's waitboost. > However, these strategies are only active in certain conditions where > the system is TDP limited. So, they don't conflict, but help the > waitboost by guaranteeing a bit more of GT frequency. > > Without these strategies we were likely leaving some performance > behind on some scenarious. s/scenarious/scenarios We should also mention that the platform defaults for enabling/disabling DCC/Balancer will now be chosen by GuC. Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 17 ----------------- > 1 file changed, 17 deletions(-) > > 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 706fffca698b..722da8a7f852 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c > @@ -76,17 +76,6 @@ static void slpc_mem_set_enabled(struct slpc_shared_data *data, > slpc_mem_set_param(data, disable_id, 0); > } > > -static void slpc_mem_set_disabled(struct slpc_shared_data *data, > - u8 enable_id, u8 disable_id) > -{ > - /* > - * Disabling a param involves setting the enable_id > - * to 0 and disable_id to 1. > - */ > - slpc_mem_set_param(data, disable_id, 1); > - slpc_mem_set_param(data, enable_id, 0); > -} > - > static u32 slpc_get_state(struct intel_guc_slpc *slpc) > { > struct slpc_shared_data *data; > @@ -366,12 +355,6 @@ static void slpc_shared_data_reset(struct slpc_shared_data *data) > /* Enable only GTPERF task, disable others */ > slpc_mem_set_enabled(data, SLPC_PARAM_TASK_ENABLE_GTPERF, > SLPC_PARAM_TASK_DISABLE_GTPERF); > - > - slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_BALANCER, > - SLPC_PARAM_TASK_DISABLE_BALANCER); > - > - slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_DCC, > - SLPC_PARAM_TASK_DISABLE_DCC); > } > > /**
On Wed, 08 Jan 2025 11:11:53 -0800, Belgaumkar, Vinay wrote: > > > On 1/8/2025 6:13 AM, Rodrigo Vivi wrote: > > The Balancer and DCC strategies were left off on a fear that > > these strategies would conflict with the i915's waitboost. > > However, these strategies are only active in certain conditions where > > the system is TDP limited. So, they don't conflict, but help the > > waitboost by guaranteeing a bit more of GT frequency. > > > > Without these strategies we were likely leaving some performance > > behind on some scenarious. > > s/scenarious/scenarios > > We should also mention that the platform defaults for enabling/disabling > DCC/Balancer will now be chosen by GuC. > > Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Does this need a Fixes: and Cc:stable?
On Wed, Jan 08, 2025 at 11:30:59AM -0800, Dixit, Ashutosh wrote: > On Wed, 08 Jan 2025 11:11:53 -0800, Belgaumkar, Vinay wrote: > > > > > > On 1/8/2025 6:13 AM, Rodrigo Vivi wrote: > > > The Balancer and DCC strategies were left off on a fear that > > > these strategies would conflict with the i915's waitboost. > > > However, these strategies are only active in certain conditions where > > > the system is TDP limited. So, they don't conflict, but help the > > > waitboost by guaranteeing a bit more of GT frequency. > > > > > > Without these strategies we were likely leaving some performance > > > behind on some scenarious. > > > > s/scenarious/scenarios I do need to re-enable my spell checker during git commit :) > > > > We should also mention that the platform defaults for enabling/disabling > > DCC/Balancer will now be chosen by GuC. > > > > Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Does this need a Fixes: and Cc:stable? nope... this is more like enabling a new feature...
On Wed, 08 Jan 2025 13:19:16 -0800, Rodrigo Vivi wrote: > Hi Rodrigo, > On Wed, Jan 08, 2025 at 11:30:59AM -0800, Dixit, Ashutosh wrote: > > On Wed, 08 Jan 2025 11:11:53 -0800, Belgaumkar, Vinay wrote: > > > > > > > > > On 1/8/2025 6:13 AM, Rodrigo Vivi wrote: > > > > The Balancer and DCC strategies were left off on a fear that > > > > these strategies would conflict with the i915's waitboost. > > > > However, these strategies are only active in certain conditions where > > > > the system is TDP limited. So, they don't conflict, but help the > > > > waitboost by guaranteeing a bit more of GT frequency. > > > > > > > > Without these strategies we were likely leaving some performance > > > > behind on some scenarious. > > > > > > s/scenarious/scenarios > > I do need to re-enable my spell checker during git commit :) > > > > > > > We should also mention that the platform defaults for enabling/disabling > > > DCC/Balancer will now be chosen by GuC. > > > > > > Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > > > > > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Does this need a Fixes: and Cc:stable? > > nope... this is more like enabling a new feature... Hmm, if this really improves performance, as the commit message seems to suggest, why wouldn't we want this in stable, or at least LTS, kernels? So why isn't this a performance bug fix? Ashutosh
On Wed, Jan 08, 2025 at 09:18:41PM -0800, Dixit, Ashutosh wrote: > On Wed, 08 Jan 2025 13:19:16 -0800, Rodrigo Vivi wrote: > > > > Hi Rodrigo, > > > On Wed, Jan 08, 2025 at 11:30:59AM -0800, Dixit, Ashutosh wrote: > > > On Wed, 08 Jan 2025 11:11:53 -0800, Belgaumkar, Vinay wrote: > > > > > > > > > > > > On 1/8/2025 6:13 AM, Rodrigo Vivi wrote: > > > > > The Balancer and DCC strategies were left off on a fear that > > > > > these strategies would conflict with the i915's waitboost. > > > > > However, these strategies are only active in certain conditions where > > > > > the system is TDP limited. So, they don't conflict, but help the > > > > > waitboost by guaranteeing a bit more of GT frequency. > > > > > > > > > > Without these strategies we were likely leaving some performance > > > > > behind on some scenarious. > > > > > > > > s/scenarious/scenarios > > > > I do need to re-enable my spell checker during git commit :) > > > > > > > > > > We should also mention that the platform defaults for enabling/disabling > > > > DCC/Balancer will now be chosen by GuC. > > > > > > > > Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > > > > > > > > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Does this need a Fixes: and Cc:stable? > > > > nope... this is more like enabling a new feature... > > Hmm, if this really improves performance, as the commit message seems to > suggest, why wouldn't we want this in stable, or at least LTS, kernels? So > why isn't this a performance bug fix? hmm... performance is not listed in the stable-rules as a obvious case, and it is not a regression, is really a new feature, that has the potential to find some later corner cases. So, safer to just enable as a new feature... Also a feature that is really a corner case for the end user... needs to be using both cpu and gpu with high utilization and a limited power budget. General benchmarks and usages don't hit this scenario very easily. > > Ashutosh
On Thu, 09 Jan 2025 11:00:36 -0800, Rodrigo Vivi wrote: > > On Wed, Jan 08, 2025 at 09:18:41PM -0800, Dixit, Ashutosh wrote: > > On Wed, 08 Jan 2025 13:19:16 -0800, Rodrigo Vivi wrote: > > > > > > > Hi Rodrigo, > > > > > On Wed, Jan 08, 2025 at 11:30:59AM -0800, Dixit, Ashutosh wrote: > > > > On Wed, 08 Jan 2025 11:11:53 -0800, Belgaumkar, Vinay wrote: > > > > > > > > > > > > > > > On 1/8/2025 6:13 AM, Rodrigo Vivi wrote: > > > > > > The Balancer and DCC strategies were left off on a fear that > > > > > > these strategies would conflict with the i915's waitboost. > > > > > > However, these strategies are only active in certain conditions where > > > > > > the system is TDP limited. So, they don't conflict, but help the > > > > > > waitboost by guaranteeing a bit more of GT frequency. > > > > > > > > > > > > Without these strategies we were likely leaving some performance > > > > > > behind on some scenarious. > > > > > > > > > > s/scenarious/scenarios > > > > > > I do need to re-enable my spell checker during git commit :) > > > > > > > > > > > > > We should also mention that the platform defaults for enabling/disabling > > > > > DCC/Balancer will now be chosen by GuC. > > > > > > > > > > Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > > > > > > > > > > > > > > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > > Does this need a Fixes: and Cc:stable? > > > > > > nope... this is more like enabling a new feature... > > > > Hmm, if this really improves performance, as the commit message seems to > > suggest, why wouldn't we want this in stable, or at least LTS, kernels? So > > why isn't this a performance bug fix? > > hmm... performance is not listed in the stable-rules as a obvious case, and > it is not a regression, is really a new feature, that has the potential to > find some later corner cases. So, safer to just enable as a new feature... > > Also a feature that is really a corner case for the end user... needs to > be using both cpu and gpu with high utilization and a limited power budget. > General benchmarks and usages don't hit this scenario very easily. OK, fair enough :)
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 706fffca698b..722da8a7f852 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -76,17 +76,6 @@ static void slpc_mem_set_enabled(struct slpc_shared_data *data, slpc_mem_set_param(data, disable_id, 0); } -static void slpc_mem_set_disabled(struct slpc_shared_data *data, - u8 enable_id, u8 disable_id) -{ - /* - * Disabling a param involves setting the enable_id - * to 0 and disable_id to 1. - */ - slpc_mem_set_param(data, disable_id, 1); - slpc_mem_set_param(data, enable_id, 0); -} - static u32 slpc_get_state(struct intel_guc_slpc *slpc) { struct slpc_shared_data *data; @@ -366,12 +355,6 @@ static void slpc_shared_data_reset(struct slpc_shared_data *data) /* Enable only GTPERF task, disable others */ slpc_mem_set_enabled(data, SLPC_PARAM_TASK_ENABLE_GTPERF, SLPC_PARAM_TASK_DISABLE_GTPERF); - - slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_BALANCER, - SLPC_PARAM_TASK_DISABLE_BALANCER); - - slpc_mem_set_disabled(data, SLPC_PARAM_TASK_ENABLE_DCC, - SLPC_PARAM_TASK_DISABLE_DCC); } /**
The Balancer and DCC strategies were left off on a fear that these strategies would conflict with the i915's waitboost. However, these strategies are only active in certain conditions where the system is TDP limited. So, they don't conflict, but help the waitboost by guaranteeing a bit more of GT frequency. Without these strategies we were likely leaving some performance behind on some scenarious. Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 17 ----------------- 1 file changed, 17 deletions(-)