| Message ID | 20180530143355.6611-8-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30/05/2018 15:33, Lionel Landwerlin wrote: > There are concerns about denial of service around the per context sseu > configuration capability. In a previous commit introducing the > capability we allowed it only for capable users. This changes adds a > new debugfs entry to let any user configure its own context > powergating setup. As far as I understood it, Joonas' concerns here are: 1) That in the containers use case individual containers wouldn't be able to turn on the sysfs toggle for them. 2) That also in the containers use case if box admin turns on the feature, some containers would potentially start negatively affecting the others (via the accumulated cost of slice re-configuration on context switching). I am not familiar with typical container setups to be authoritative here, but intuitively I find it reasonable that a low-level hardware switch like this would be under the control of a master domain administrator. ("If you are installing our product in the container environment, make sure your system administrator enables this hardware feature.", "Note to system administrators: Enabling this features may negatively affect the performance of other containers.") Alternative proposal is for the i915 to apply an "or" filter on all requested masks and in that way ensure dynamic re-configuration doesn't happen on context switches, but driven from userspace via ioctls. In other words, should _all_ userspace agree between themselves that they want to turn off a slice, they would then need to send out a concerted ioctl storm, where number of needed ioctls equals the number of currently active contexts. (This may have its own performance consequences caused by the barriers needed to modify all context images.) This was deemed acceptable the the media use case, but my concern is the approach is not elegant and will tie us with the "or" policy in the ABI. (Performance concerns I haven't evaluated yet, but they also may be significant.) If we go back thinking about the containers use case, then it transpires that even though the "or" policy does prevent one container from affecting the other from one angle, it also prevents one container from exercising the feature unless all containers co-operate. As such, we can view the original problem statement where we have an issue if not everyone co-operates, as conceptually the same just from an opposite angle. (Rather than one container incurring the increased cost of context switches to the rest, we would have one container preventing the optimized slice configuration to the other.) From this follows that both proposals require complete co-operation from all running userspace to avoid complete control of the feature. Since the balance between the benefit of optimized slice configuration (or penalty of suboptimal one), versus the penalty of increased context switch times, cannot be know by the driver (barring venturing into the heuristics territory), that is another reason why I find the "or" policy in the driver questionable. We can also ask a question of - If we go with the "or" policy, why require N per-context ioctls to modify the global GPU configuration and not instead add a global driver ioctl to modify the state? If a future hardware requires, or enables, the per-context behaviour in a more efficient way, we could then revisit the problem space. In the mean time I see the "or" policy solution as adding some ABI which doesn't do anything for many use cases without any way for the sysadmin to enable it. At the same time master sysfs knob at least enables the sysadmin to make a decision. Here I am thinking about a random client environment where not all userspace co-operates, but for instance user is running the feature aware media stack, and non-feature aware OpenCL/3d stack. I guess the complete story boils down to - is the master sysfs knob really a problem in container use cases. Regards, Tvrtko > > v2: Rename sysfs entry (Tvrtko) > Lock interruptible the device in sysfs (Tvrtko) > Fix dropped error code in getting dynamic sseu value (Tvrtko) > s/dev_priv/i915/ (Tvrtko) > > v3: Drop locked function suffix (Tvrtko) > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 16 +++++++--- > drivers/gpu/drm/i915/i915_gem_context.c | 38 +++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_sysfs.c | 40 +++++++++++++++++++++++++ > 3 files changed, 90 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b2386c37437d..5aa96a6650b4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1842,6 +1842,8 @@ struct drm_i915_private { > struct ida hw_ida; > #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */ > #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */ > + > + bool dynamic_sseu; > } contexts; > > u32 fdi_rx_config; > @@ -3259,6 +3261,10 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id) > return ctx; > } > > +int i915_gem_contexts_set_dynamic_sseu(struct drm_i915_private *i915, > + bool allowed); > +bool i915_gem_contexts_get_dynamic_sseu(struct drm_i915_private *i915); > + > int i915_perf_open_ioctl(struct drm_device *dev, void *data, > struct drm_file *file); > int i915_perf_add_config_ioctl(struct drm_device *dev, void *data, > @@ -3859,11 +3865,13 @@ intel_engine_prepare_sseu(struct intel_engine_cs *engine, > struct drm_i915_private *i915 = engine->i915; > > /* > - * If i915/perf is active, we want a stable powergating configuration > - * on the system. The most natural configuration to take in that case > - * is the default (i.e maximum the hardware can do). > + * If i915/perf is active or dynamic sseu configuration is not allowed > + * (through sysfs), we want a stable powergating configuration on the > + * system. The most natural configuration to take in that case is the > + * default (i.e maximum the hardware can do). > */ > - return i915->perf.oa.exclusive_stream ? > + return (i915->perf.oa.exclusive_stream || > + !i915->contexts.dynamic_sseu) ? > intel_device_default_sseu(i915) : sseu; > } > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 79029815c21f..453bdc976be3 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -1118,6 +1118,44 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, > return ret; > } > > +int i915_gem_contexts_set_dynamic_sseu(struct drm_i915_private *i915, > + bool allowed) > +{ > + struct intel_engine_cs *engine = i915->engine[RCS]; > + struct i915_gem_context *ctx; > + int ret = 0; > + > + lockdep_assert_held(&i915->drm.struct_mutex); > + > + if (!engine->emit_rpcs_config) > + return -ENODEV; > + > + if (allowed == i915->contexts.dynamic_sseu) > + return 0; > + > + i915->contexts.dynamic_sseu = allowed; > + > + list_for_each_entry(ctx, &i915->contexts.list, link) { > + struct intel_context *ce = to_intel_context(ctx, engine); > + > + ret = i915_gem_context_reconfigure_sseu(ctx, engine, ce->sseu); > + if (ret) > + break; > + } > + > + return ret; > +} > + > +bool i915_gem_contexts_get_dynamic_sseu(struct drm_i915_private *i915) > +{ > + struct intel_engine_cs *engine = i915->engine[RCS]; > + > + if (!engine->emit_rpcs_config) > + return false; > + > + return i915->contexts.dynamic_sseu; > +} > + > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > #include "selftests/mock_context.c" > #include "selftests/i915_gem_context.c" > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > index e5e6f6bb2b05..d895054d245e 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -483,6 +483,44 @@ static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr > return snprintf(buf, PAGE_SIZE, "%d\n", val); > } > > +static ssize_t allow_dynamic_sseu_show(struct device *kdev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); > + bool value = i915_gem_contexts_get_dynamic_sseu(i915); > + > + return snprintf(buf, PAGE_SIZE, "%d\n", value); > +} > + > +static ssize_t allow_dynamic_sseu_store(struct device *kdev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); > + ssize_t ret; > + u32 val; > + > + ret = kstrtou32(buf, 0, &val); > + if (ret) > + return ret; > + > + if (val != 0 && val != 1) > + return -EINVAL; > + > + ret = mutex_lock_interruptible(&i915->drm.struct_mutex); > + if (ret) > + return ret; > + > + ret = i915_gem_contexts_set_dynamic_sseu(i915, val != 0); > + > + mutex_unlock(&i915->drm.struct_mutex); > + > + return ret ?: count; > +} > + > +static DEVICE_ATTR_RW(allow_dynamic_sseu); > + > static const struct attribute *gen6_attrs[] = { > &dev_attr_gt_act_freq_mhz.attr, > &dev_attr_gt_cur_freq_mhz.attr, > @@ -492,6 +530,7 @@ static const struct attribute *gen6_attrs[] = { > &dev_attr_gt_RP0_freq_mhz.attr, > &dev_attr_gt_RP1_freq_mhz.attr, > &dev_attr_gt_RPn_freq_mhz.attr, > + &dev_attr_allow_dynamic_sseu.attr, > NULL, > }; > > @@ -505,6 +544,7 @@ static const struct attribute *vlv_attrs[] = { > &dev_attr_gt_RP1_freq_mhz.attr, > &dev_attr_gt_RPn_freq_mhz.attr, > &dev_attr_vlv_rpe_freq_mhz.attr, > + &dev_attr_allow_dynamic_sseu.attr, > NULL, > }; > >
On 11/06/18 13:10, Tvrtko Ursulin wrote: > > On 30/05/2018 15:33, Lionel Landwerlin wrote: >> There are concerns about denial of service around the per context sseu >> configuration capability. In a previous commit introducing the >> capability we allowed it only for capable users. This changes adds a >> new debugfs entry to let any user configure its own context >> powergating setup. > > As far as I understood it, Joonas' concerns here are: > > 1) That in the containers use case individual containers wouldn't be > able to turn on the sysfs toggle for them. > > 2) That also in the containers use case if box admin turns on the > feature, some containers would potentially start negatively affecting > the others (via the accumulated cost of slice re-configuration on > context switching). > > I am not familiar with typical container setups to be authoritative > here, but intuitively I find it reasonable that a low-level hardware > switch like this would be under the control of a master domain > administrator. ("If you are installing our product in the container > environment, make sure your system administrator enables this hardware > feature.", "Note to system administrators: Enabling this features may > negatively affect the performance of other containers.") > > Alternative proposal is for the i915 to apply an "or" filter on all > requested masks and in that way ensure dynamic re-configuration > doesn't happen on context switches, but driven from userspace via ioctls. > > In other words, should _all_ userspace agree between themselves that > they want to turn off a slice, they would then need to send out a > concerted ioctl storm, where number of needed ioctls equals the number > of currently active contexts. (This may have its own performance > consequences caused by the barriers needed to modify all context images.) > > This was deemed acceptable the the media use case, but my concern is > the approach is not elegant and will tie us with the "or" policy in > the ABI. (Performance concerns I haven't evaluated yet, but they also > may be significant.) > > If we go back thinking about the containers use case, then it > transpires that even though the "or" policy does prevent one container > from affecting the other from one angle, it also prevents one > container from exercising the feature unless all containers co-operate. > > As such, we can view the original problem statement where we have an > issue if not everyone co-operates, as conceptually the same just from > an opposite angle. (Rather than one container incurring the increased > cost of context switches to the rest, we would have one container > preventing the optimized slice configuration to the other.) > > From this follows that both proposals require complete co-operation > from all running userspace to avoid complete control of the feature. > > Since the balance between the benefit of optimized slice configuration > (or penalty of suboptimal one), versus the penalty of increased > context switch times, cannot be know by the driver (barring venturing > into the heuristics territory), that is another reason why I find the > "or" policy in the driver questionable. > > We can also ask a question of - If we go with the "or" policy, why > require N per-context ioctls to modify the global GPU configuration > and not instead add a global driver ioctl to modify the state? > > If a future hardware requires, or enables, the per-context behaviour > in a more efficient way, we could then revisit the problem space. > > In the mean time I see the "or" policy solution as adding some ABI > which doesn't do anything for many use cases without any way for the > sysadmin to enable it. At the same time master sysfs knob at least > enables the sysadmin to make a decision. Here I am thinking about a > random client environment where not all userspace co-operates, but for > instance user is running the feature aware media stack, and > non-feature aware OpenCL/3d stack. > > I guess the complete story boils down to - is the master sysfs knob > really a problem in container use cases. > > Regards, > > Tvrtko Hey Tvrtko, Thanks for summarizing a bunch of discussions. Essentially I agree with every you wrote above. If we have a global setting (determined by the OR policy), what's the point of per context settings? In Dmitry's scenario, all userspace applications will work together to reach the consensus so it sounds like we're reimplementing the policy that is already existing in userspace. Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else than me pick one or the other :) Cheers, - Lionel
Quoting Lionel Landwerlin (2018-06-11 14:46:07) > On 11/06/18 13:10, Tvrtko Ursulin wrote: > > > > On 30/05/2018 15:33, Lionel Landwerlin wrote: > >> There are concerns about denial of service around the per context sseu > >> configuration capability. In a previous commit introducing the > >> capability we allowed it only for capable users. This changes adds a > >> new debugfs entry to let any user configure its own context > >> powergating setup. > > > > As far as I understood it, Joonas' concerns here are: > > > > 1) That in the containers use case individual containers wouldn't be > > able to turn on the sysfs toggle for them. > > > > 2) That also in the containers use case if box admin turns on the > > feature, some containers would potentially start negatively affecting > > the others (via the accumulated cost of slice re-configuration on > > context switching). > > > > I am not familiar with typical container setups to be authoritative > > here, but intuitively I find it reasonable that a low-level hardware > > switch like this would be under the control of a master domain > > administrator. ("If you are installing our product in the container > > environment, make sure your system administrator enables this hardware > > feature.", "Note to system administrators: Enabling this features may > > negatively affect the performance of other containers.") > > > > Alternative proposal is for the i915 to apply an "or" filter on all > > requested masks and in that way ensure dynamic re-configuration > > doesn't happen on context switches, but driven from userspace via ioctls. > > > > In other words, should _all_ userspace agree between themselves that > > they want to turn off a slice, they would then need to send out a > > concerted ioctl storm, where number of needed ioctls equals the number > > of currently active contexts. (This may have its own performance > > consequences caused by the barriers needed to modify all context images.) > > > > This was deemed acceptable the the media use case, but my concern is > > the approach is not elegant and will tie us with the "or" policy in > > the ABI. (Performance concerns I haven't evaluated yet, but they also > > may be significant.) > > > > If we go back thinking about the containers use case, then it > > transpires that even though the "or" policy does prevent one container > > from affecting the other from one angle, it also prevents one > > container from exercising the feature unless all containers co-operate. > > > > As such, we can view the original problem statement where we have an > > issue if not everyone co-operates, as conceptually the same just from > > an opposite angle. (Rather than one container incurring the increased > > cost of context switches to the rest, we would have one container > > preventing the optimized slice configuration to the other.) > > > > From this follows that both proposals require complete co-operation > > from all running userspace to avoid complete control of the feature. > > > > Since the balance between the benefit of optimized slice configuration > > (or penalty of suboptimal one), versus the penalty of increased > > context switch times, cannot be know by the driver (barring venturing > > into the heuristics territory), that is another reason why I find the > > "or" policy in the driver questionable. > > > > We can also ask a question of - If we go with the "or" policy, why > > require N per-context ioctls to modify the global GPU configuration > > and not instead add a global driver ioctl to modify the state? > > > > If a future hardware requires, or enables, the per-context behaviour > > in a more efficient way, we could then revisit the problem space. > > > > In the mean time I see the "or" policy solution as adding some ABI > > which doesn't do anything for many use cases without any way for the > > sysadmin to enable it. At the same time master sysfs knob at least > > enables the sysadmin to make a decision. Here I am thinking about a > > random client environment where not all userspace co-operates, but for > > instance user is running the feature aware media stack, and > > non-feature aware OpenCL/3d stack. > > > > I guess the complete story boils down to - is the master sysfs knob > > really a problem in container use cases. > > > > Regards, > > > > Tvrtko > > Hey Tvrtko, > > Thanks for summarizing a bunch of discussions. > Essentially I agree with every you wrote above. > > If we have a global setting (determined by the OR policy), what's the > point of per context settings? > > In Dmitry's scenario, all userspace applications will work together to > reach the consensus so it sounds like we're reimplementing the policy > that is already existing in userspace. > > Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else > than me pick one or the other :) I'll just mention the voting/consensus approach to see if anyone else likes it. Each context has a CONTEXT_PARAM_HINT_SSEU { small, dontcare, large } (or some other abstract names). Then whenever the host cares, they can evaluate the set of hints provided and make a choice on sseu config. One presumes a simple greater good method (but you could extends that to include batch frequency/duration to try and determine system impact on one setting or another). Keeping it a hint helps reduce the effect of policy, though it may still be policy and merit a switch for different implementations (or BPF!). -Chris
Quoting Chris Wilson (2018-06-11 18:02:37) > Quoting Lionel Landwerlin (2018-06-11 14:46:07) > > On 11/06/18 13:10, Tvrtko Ursulin wrote: > > > > > > On 30/05/2018 15:33, Lionel Landwerlin wrote: > > >> There are concerns about denial of service around the per context sseu > > >> configuration capability. In a previous commit introducing the > > >> capability we allowed it only for capable users. This changes adds a > > >> new debugfs entry to let any user configure its own context > > >> powergating setup. > > > > > > As far as I understood it, Joonas' concerns here are: > > > > > > 1) That in the containers use case individual containers wouldn't be > > > able to turn on the sysfs toggle for them. > > > > > > 2) That also in the containers use case if box admin turns on the > > > feature, some containers would potentially start negatively affecting > > > the others (via the accumulated cost of slice re-configuration on > > > context switching). > > > > > > I am not familiar with typical container setups to be authoritative > > > here, but intuitively I find it reasonable that a low-level hardware > > > switch like this would be under the control of a master domain > > > administrator. ("If you are installing our product in the container > > > environment, make sure your system administrator enables this hardware > > > feature.", "Note to system administrators: Enabling this features may > > > negatively affect the performance of other containers.") > > > > > > Alternative proposal is for the i915 to apply an "or" filter on all > > > requested masks and in that way ensure dynamic re-configuration > > > doesn't happen on context switches, but driven from userspace via ioctls. > > > > > > In other words, should _all_ userspace agree between themselves that > > > they want to turn off a slice, they would then need to send out a > > > concerted ioctl storm, where number of needed ioctls equals the number > > > of currently active contexts. (This may have its own performance > > > consequences caused by the barriers needed to modify all context images.) > > > > > > This was deemed acceptable the the media use case, but my concern is > > > the approach is not elegant and will tie us with the "or" policy in > > > the ABI. (Performance concerns I haven't evaluated yet, but they also > > > may be significant.) > > > > > > If we go back thinking about the containers use case, then it > > > transpires that even though the "or" policy does prevent one container > > > from affecting the other from one angle, it also prevents one > > > container from exercising the feature unless all containers co-operate. > > > > > > As such, we can view the original problem statement where we have an > > > issue if not everyone co-operates, as conceptually the same just from > > > an opposite angle. (Rather than one container incurring the increased > > > cost of context switches to the rest, we would have one container > > > preventing the optimized slice configuration to the other.) > > > > > > From this follows that both proposals require complete co-operation > > > from all running userspace to avoid complete control of the feature. > > > > > > Since the balance between the benefit of optimized slice configuration > > > (or penalty of suboptimal one), versus the penalty of increased > > > context switch times, cannot be know by the driver (barring venturing > > > into the heuristics territory), that is another reason why I find the > > > "or" policy in the driver questionable. > > > > > > We can also ask a question of - If we go with the "or" policy, why > > > require N per-context ioctls to modify the global GPU configuration > > > and not instead add a global driver ioctl to modify the state? > > > > > > If a future hardware requires, or enables, the per-context behaviour > > > in a more efficient way, we could then revisit the problem space. > > > > > > In the mean time I see the "or" policy solution as adding some ABI > > > which doesn't do anything for many use cases without any way for the > > > sysadmin to enable it. At the same time master sysfs knob at least > > > enables the sysadmin to make a decision. Here I am thinking about a > > > random client environment where not all userspace co-operates, but for > > > instance user is running the feature aware media stack, and > > > non-feature aware OpenCL/3d stack. > > > > > > I guess the complete story boils down to - is the master sysfs knob > > > really a problem in container use cases. > > > > > > Regards, > > > > > > Tvrtko > > > > Hey Tvrtko, > > > > Thanks for summarizing a bunch of discussions. > > Essentially I agree with every you wrote above. > > > > If we have a global setting (determined by the OR policy), what's the > > point of per context settings? > > > > In Dmitry's scenario, all userspace applications will work together to > > reach the consensus so it sounds like we're reimplementing the policy > > that is already existing in userspace. > > > > Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else > > than me pick one or the other :) > > I'll just mention the voting/consensus approach to see if anyone else > likes it. > > Each context has a CONTEXT_PARAM_HINT_SSEU { small, dontcare, large } > (or some other abstract names). Yeah, the param name should have the word _HINT_ in it when it's not a definitive set. There's no global setter across containers, only a scenario when everyone agrees or not. Tallying up the votes and going with a majority vote might be an option, too. Regards, Joonas > > Then whenever the host cares, they can evaluate the set of hints > provided and make a choice on sseu config. One presumes a simple greater > good method (but you could extends that to include batch > frequency/duration to try and determine system impact on one setting or > another). Keeping it a hint helps reduce the effect of policy, though it > may still be policy and merit a switch for different implementations (or > BPF!). > -Chris > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/06/18 10:20, Joonas Lahtinen wrote: > Quoting Chris Wilson (2018-06-11 18:02:37) >> Quoting Lionel Landwerlin (2018-06-11 14:46:07) >>> On 11/06/18 13:10, Tvrtko Ursulin wrote: >>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: >>>>> There are concerns about denial of service around the per context sseu >>>>> configuration capability. In a previous commit introducing the >>>>> capability we allowed it only for capable users. This changes adds a >>>>> new debugfs entry to let any user configure its own context >>>>> powergating setup. >>>> As far as I understood it, Joonas' concerns here are: >>>> >>>> 1) That in the containers use case individual containers wouldn't be >>>> able to turn on the sysfs toggle for them. >>>> >>>> 2) That also in the containers use case if box admin turns on the >>>> feature, some containers would potentially start negatively affecting >>>> the others (via the accumulated cost of slice re-configuration on >>>> context switching). >>>> >>>> I am not familiar with typical container setups to be authoritative >>>> here, but intuitively I find it reasonable that a low-level hardware >>>> switch like this would be under the control of a master domain >>>> administrator. ("If you are installing our product in the container >>>> environment, make sure your system administrator enables this hardware >>>> feature.", "Note to system administrators: Enabling this features may >>>> negatively affect the performance of other containers.") >>>> >>>> Alternative proposal is for the i915 to apply an "or" filter on all >>>> requested masks and in that way ensure dynamic re-configuration >>>> doesn't happen on context switches, but driven from userspace via ioctls. >>>> >>>> In other words, should _all_ userspace agree between themselves that >>>> they want to turn off a slice, they would then need to send out a >>>> concerted ioctl storm, where number of needed ioctls equals the number >>>> of currently active contexts. (This may have its own performance >>>> consequences caused by the barriers needed to modify all context images.) >>>> >>>> This was deemed acceptable the the media use case, but my concern is >>>> the approach is not elegant and will tie us with the "or" policy in >>>> the ABI. (Performance concerns I haven't evaluated yet, but they also >>>> may be significant.) >>>> >>>> If we go back thinking about the containers use case, then it >>>> transpires that even though the "or" policy does prevent one container >>>> from affecting the other from one angle, it also prevents one >>>> container from exercising the feature unless all containers co-operate. >>>> >>>> As such, we can view the original problem statement where we have an >>>> issue if not everyone co-operates, as conceptually the same just from >>>> an opposite angle. (Rather than one container incurring the increased >>>> cost of context switches to the rest, we would have one container >>>> preventing the optimized slice configuration to the other.) >>>> >>>> From this follows that both proposals require complete co-operation >>>> from all running userspace to avoid complete control of the feature. >>>> >>>> Since the balance between the benefit of optimized slice configuration >>>> (or penalty of suboptimal one), versus the penalty of increased >>>> context switch times, cannot be know by the driver (barring venturing >>>> into the heuristics territory), that is another reason why I find the >>>> "or" policy in the driver questionable. >>>> >>>> We can also ask a question of - If we go with the "or" policy, why >>>> require N per-context ioctls to modify the global GPU configuration >>>> and not instead add a global driver ioctl to modify the state? >>>> >>>> If a future hardware requires, or enables, the per-context behaviour >>>> in a more efficient way, we could then revisit the problem space. >>>> >>>> In the mean time I see the "or" policy solution as adding some ABI >>>> which doesn't do anything for many use cases without any way for the >>>> sysadmin to enable it. At the same time master sysfs knob at least >>>> enables the sysadmin to make a decision. Here I am thinking about a >>>> random client environment where not all userspace co-operates, but for >>>> instance user is running the feature aware media stack, and >>>> non-feature aware OpenCL/3d stack. >>>> >>>> I guess the complete story boils down to - is the master sysfs knob >>>> really a problem in container use cases. >>>> >>>> Regards, >>>> >>>> Tvrtko >>> Hey Tvrtko, >>> >>> Thanks for summarizing a bunch of discussions. >>> Essentially I agree with every you wrote above. >>> >>> If we have a global setting (determined by the OR policy), what's the >>> point of per context settings? >>> >>> In Dmitry's scenario, all userspace applications will work together to >>> reach the consensus so it sounds like we're reimplementing the policy >>> that is already existing in userspace. >>> >>> Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else >>> than me pick one or the other :) >> I'll just mention the voting/consensus approach to see if anyone else >> likes it. >> >> Each context has a CONTEXT_PARAM_HINT_SSEU { small, dontcare, large } >> (or some other abstract names). > Yeah, the param name should have the word _HINT_ in it when it's not a > definitive set. > > There's no global setter across containers, only a scenario when > everyone agrees or not. Tallying up the votes and going with a majority > vote might be an option, too. > > Regards, Joonas Trying to test the "everyone agrees" approach here. There are a number of processes that can hold onto a gem context and therefore prevent agreement. On my system plymouthd & systemd-login have a number of contexts opened. A process simply opening&closing a render node could flip the system back and forth between 2 configurations. Does that change people's mind about how we should go about this? - Lionel > >> Then whenever the host cares, they can evaluate the set of hints >> provided and make a choice on sseu config. One presumes a simple greater >> good method (but you could extends that to include batch >> frequency/duration to try and determine system impact on one setting or >> another). Keeping it a hint helps reduce the effect of policy, though it >> may still be policy and merit a switch for different implementations (or >> BPF!). >> -Chris >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Lionel Landwerlin (2018-06-12 11:33:34) > On 12/06/18 10:20, Joonas Lahtinen wrote: > > Quoting Chris Wilson (2018-06-11 18:02:37) > >> Quoting Lionel Landwerlin (2018-06-11 14:46:07) > >>> On 11/06/18 13:10, Tvrtko Ursulin wrote: > >>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: > >>>>> There are concerns about denial of service around the per context sseu > >>>>> configuration capability. In a previous commit introducing the > >>>>> capability we allowed it only for capable users. This changes adds a > >>>>> new debugfs entry to let any user configure its own context > >>>>> powergating setup. > >>>> As far as I understood it, Joonas' concerns here are: > >>>> > >>>> 1) That in the containers use case individual containers wouldn't be > >>>> able to turn on the sysfs toggle for them. > >>>> > >>>> 2) That also in the containers use case if box admin turns on the > >>>> feature, some containers would potentially start negatively affecting > >>>> the others (via the accumulated cost of slice re-configuration on > >>>> context switching). > >>>> > >>>> I am not familiar with typical container setups to be authoritative > >>>> here, but intuitively I find it reasonable that a low-level hardware > >>>> switch like this would be under the control of a master domain > >>>> administrator. ("If you are installing our product in the container > >>>> environment, make sure your system administrator enables this hardware > >>>> feature.", "Note to system administrators: Enabling this features may > >>>> negatively affect the performance of other containers.") > >>>> > >>>> Alternative proposal is for the i915 to apply an "or" filter on all > >>>> requested masks and in that way ensure dynamic re-configuration > >>>> doesn't happen on context switches, but driven from userspace via ioctls. > >>>> > >>>> In other words, should _all_ userspace agree between themselves that > >>>> they want to turn off a slice, they would then need to send out a > >>>> concerted ioctl storm, where number of needed ioctls equals the number > >>>> of currently active contexts. (This may have its own performance > >>>> consequences caused by the barriers needed to modify all context images.) > >>>> > >>>> This was deemed acceptable the the media use case, but my concern is > >>>> the approach is not elegant and will tie us with the "or" policy in > >>>> the ABI. (Performance concerns I haven't evaluated yet, but they also > >>>> may be significant.) > >>>> > >>>> If we go back thinking about the containers use case, then it > >>>> transpires that even though the "or" policy does prevent one container > >>>> from affecting the other from one angle, it also prevents one > >>>> container from exercising the feature unless all containers co-operate. > >>>> > >>>> As such, we can view the original problem statement where we have an > >>>> issue if not everyone co-operates, as conceptually the same just from > >>>> an opposite angle. (Rather than one container incurring the increased > >>>> cost of context switches to the rest, we would have one container > >>>> preventing the optimized slice configuration to the other.) > >>>> > >>>> From this follows that both proposals require complete co-operation > >>>> from all running userspace to avoid complete control of the feature. > >>>> > >>>> Since the balance between the benefit of optimized slice configuration > >>>> (or penalty of suboptimal one), versus the penalty of increased > >>>> context switch times, cannot be know by the driver (barring venturing > >>>> into the heuristics territory), that is another reason why I find the > >>>> "or" policy in the driver questionable. > >>>> > >>>> We can also ask a question of - If we go with the "or" policy, why > >>>> require N per-context ioctls to modify the global GPU configuration > >>>> and not instead add a global driver ioctl to modify the state? > >>>> > >>>> If a future hardware requires, or enables, the per-context behaviour > >>>> in a more efficient way, we could then revisit the problem space. > >>>> > >>>> In the mean time I see the "or" policy solution as adding some ABI > >>>> which doesn't do anything for many use cases without any way for the > >>>> sysadmin to enable it. At the same time master sysfs knob at least > >>>> enables the sysadmin to make a decision. Here I am thinking about a > >>>> random client environment where not all userspace co-operates, but for > >>>> instance user is running the feature aware media stack, and > >>>> non-feature aware OpenCL/3d stack. > >>>> > >>>> I guess the complete story boils down to - is the master sysfs knob > >>>> really a problem in container use cases. > >>>> > >>>> Regards, > >>>> > >>>> Tvrtko > >>> Hey Tvrtko, > >>> > >>> Thanks for summarizing a bunch of discussions. > >>> Essentially I agree with every you wrote above. > >>> > >>> If we have a global setting (determined by the OR policy), what's the > >>> point of per context settings? > >>> > >>> In Dmitry's scenario, all userspace applications will work together to > >>> reach the consensus so it sounds like we're reimplementing the policy > >>> that is already existing in userspace. > >>> > >>> Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else > >>> than me pick one or the other :) > >> I'll just mention the voting/consensus approach to see if anyone else > >> likes it. > >> > >> Each context has a CONTEXT_PARAM_HINT_SSEU { small, dontcare, large } > >> (or some other abstract names). > > Yeah, the param name should have the word _HINT_ in it when it's not a > > definitive set. > > > > There's no global setter across containers, only a scenario when > > everyone agrees or not. Tallying up the votes and going with a majority > > vote might be an option, too. > > > > Regards, Joonas > > Trying to test the "everyone agrees" approach here. It's not everyone agrees, but the greater good. > There are a number of processes that can hold onto a gem context and > therefore prevent agreement. > On my system plymouthd & systemd-login have a number of contexts opened. But they should be dontcare? There should only be a few processes that insist on a particular configuration, afui. -Chris
On 12/06/18 11:37, Chris Wilson wrote: > Quoting Lionel Landwerlin (2018-06-12 11:33:34) >> On 12/06/18 10:20, Joonas Lahtinen wrote: >>> Quoting Chris Wilson (2018-06-11 18:02:37) >>>> Quoting Lionel Landwerlin (2018-06-11 14:46:07) >>>>> On 11/06/18 13:10, Tvrtko Ursulin wrote: >>>>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: >>>>>>> There are concerns about denial of service around the per context sseu >>>>>>> configuration capability. In a previous commit introducing the >>>>>>> capability we allowed it only for capable users. This changes adds a >>>>>>> new debugfs entry to let any user configure its own context >>>>>>> powergating setup. >>>>>> As far as I understood it, Joonas' concerns here are: >>>>>> >>>>>> 1) That in the containers use case individual containers wouldn't be >>>>>> able to turn on the sysfs toggle for them. >>>>>> >>>>>> 2) That also in the containers use case if box admin turns on the >>>>>> feature, some containers would potentially start negatively affecting >>>>>> the others (via the accumulated cost of slice re-configuration on >>>>>> context switching). >>>>>> >>>>>> I am not familiar with typical container setups to be authoritative >>>>>> here, but intuitively I find it reasonable that a low-level hardware >>>>>> switch like this would be under the control of a master domain >>>>>> administrator. ("If you are installing our product in the container >>>>>> environment, make sure your system administrator enables this hardware >>>>>> feature.", "Note to system administrators: Enabling this features may >>>>>> negatively affect the performance of other containers.") >>>>>> >>>>>> Alternative proposal is for the i915 to apply an "or" filter on all >>>>>> requested masks and in that way ensure dynamic re-configuration >>>>>> doesn't happen on context switches, but driven from userspace via ioctls. >>>>>> >>>>>> In other words, should _all_ userspace agree between themselves that >>>>>> they want to turn off a slice, they would then need to send out a >>>>>> concerted ioctl storm, where number of needed ioctls equals the number >>>>>> of currently active contexts. (This may have its own performance >>>>>> consequences caused by the barriers needed to modify all context images.) >>>>>> >>>>>> This was deemed acceptable the the media use case, but my concern is >>>>>> the approach is not elegant and will tie us with the "or" policy in >>>>>> the ABI. (Performance concerns I haven't evaluated yet, but they also >>>>>> may be significant.) >>>>>> >>>>>> If we go back thinking about the containers use case, then it >>>>>> transpires that even though the "or" policy does prevent one container >>>>>> from affecting the other from one angle, it also prevents one >>>>>> container from exercising the feature unless all containers co-operate. >>>>>> >>>>>> As such, we can view the original problem statement where we have an >>>>>> issue if not everyone co-operates, as conceptually the same just from >>>>>> an opposite angle. (Rather than one container incurring the increased >>>>>> cost of context switches to the rest, we would have one container >>>>>> preventing the optimized slice configuration to the other.) >>>>>> >>>>>> From this follows that both proposals require complete co-operation >>>>>> from all running userspace to avoid complete control of the feature. >>>>>> >>>>>> Since the balance between the benefit of optimized slice configuration >>>>>> (or penalty of suboptimal one), versus the penalty of increased >>>>>> context switch times, cannot be know by the driver (barring venturing >>>>>> into the heuristics territory), that is another reason why I find the >>>>>> "or" policy in the driver questionable. >>>>>> >>>>>> We can also ask a question of - If we go with the "or" policy, why >>>>>> require N per-context ioctls to modify the global GPU configuration >>>>>> and not instead add a global driver ioctl to modify the state? >>>>>> >>>>>> If a future hardware requires, or enables, the per-context behaviour >>>>>> in a more efficient way, we could then revisit the problem space. >>>>>> >>>>>> In the mean time I see the "or" policy solution as adding some ABI >>>>>> which doesn't do anything for many use cases without any way for the >>>>>> sysadmin to enable it. At the same time master sysfs knob at least >>>>>> enables the sysadmin to make a decision. Here I am thinking about a >>>>>> random client environment where not all userspace co-operates, but for >>>>>> instance user is running the feature aware media stack, and >>>>>> non-feature aware OpenCL/3d stack. >>>>>> >>>>>> I guess the complete story boils down to - is the master sysfs knob >>>>>> really a problem in container use cases. >>>>>> >>>>>> Regards, >>>>>> >>>>>> Tvrtko >>>>> Hey Tvrtko, >>>>> >>>>> Thanks for summarizing a bunch of discussions. >>>>> Essentially I agree with every you wrote above. >>>>> >>>>> If we have a global setting (determined by the OR policy), what's the >>>>> point of per context settings? >>>>> >>>>> In Dmitry's scenario, all userspace applications will work together to >>>>> reach the consensus so it sounds like we're reimplementing the policy >>>>> that is already existing in userspace. >>>>> >>>>> Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else >>>>> than me pick one or the other :) >>>> I'll just mention the voting/consensus approach to see if anyone else >>>> likes it. >>>> >>>> Each context has a CONTEXT_PARAM_HINT_SSEU { small, dontcare, large } >>>> (or some other abstract names). >>> Yeah, the param name should have the word _HINT_ in it when it's not a >>> definitive set. >>> >>> There's no global setter across containers, only a scenario when >>> everyone agrees or not. Tallying up the votes and going with a majority >>> vote might be an option, too. >>> >>> Regards, Joonas >> Trying to test the "everyone agrees" approach here. > It's not everyone agrees, but the greater good. I'm looking forward to the definition of the greater good :) Tvrtko wanted to avoid the heuristic territory, it seems like we're just stepping into it. - Lionel >> There are a number of processes that can hold onto a gem context and >> therefore prevent agreement. >> On my system plymouthd & systemd-login have a number of contexts opened. > But they should be dontcare? > > There should only be a few processes that insist on a particular > configuration, afui. > -Chris >
On 12/06/2018 11:52, Lionel Landwerlin wrote: > On 12/06/18 11:37, Chris Wilson wrote: >> Quoting Lionel Landwerlin (2018-06-12 11:33:34) >>> On 12/06/18 10:20, Joonas Lahtinen wrote: >>>> Quoting Chris Wilson (2018-06-11 18:02:37) >>>>> Quoting Lionel Landwerlin (2018-06-11 14:46:07) >>>>>> On 11/06/18 13:10, Tvrtko Ursulin wrote: >>>>>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: >>>>>>>> There are concerns about denial of service around the per >>>>>>>> context sseu >>>>>>>> configuration capability. In a previous commit introducing the >>>>>>>> capability we allowed it only for capable users. This changes >>>>>>>> adds a >>>>>>>> new debugfs entry to let any user configure its own context >>>>>>>> powergating setup. >>>>>>> As far as I understood it, Joonas' concerns here are: >>>>>>> >>>>>>> 1) That in the containers use case individual containers wouldn't be >>>>>>> able to turn on the sysfs toggle for them. >>>>>>> >>>>>>> 2) That also in the containers use case if box admin turns on the >>>>>>> feature, some containers would potentially start negatively >>>>>>> affecting >>>>>>> the others (via the accumulated cost of slice re-configuration on >>>>>>> context switching). >>>>>>> >>>>>>> I am not familiar with typical container setups to be authoritative >>>>>>> here, but intuitively I find it reasonable that a low-level hardware >>>>>>> switch like this would be under the control of a master domain >>>>>>> administrator. ("If you are installing our product in the container >>>>>>> environment, make sure your system administrator enables this >>>>>>> hardware >>>>>>> feature.", "Note to system administrators: Enabling this features >>>>>>> may >>>>>>> negatively affect the performance of other containers.") >>>>>>> >>>>>>> Alternative proposal is for the i915 to apply an "or" filter on all >>>>>>> requested masks and in that way ensure dynamic re-configuration >>>>>>> doesn't happen on context switches, but driven from userspace via >>>>>>> ioctls. >>>>>>> >>>>>>> In other words, should _all_ userspace agree between themselves that >>>>>>> they want to turn off a slice, they would then need to send out a >>>>>>> concerted ioctl storm, where number of needed ioctls equals the >>>>>>> number >>>>>>> of currently active contexts. (This may have its own performance >>>>>>> consequences caused by the barriers needed to modify all context >>>>>>> images.) >>>>>>> >>>>>>> This was deemed acceptable the the media use case, but my concern is >>>>>>> the approach is not elegant and will tie us with the "or" policy in >>>>>>> the ABI. (Performance concerns I haven't evaluated yet, but they >>>>>>> also >>>>>>> may be significant.) >>>>>>> >>>>>>> If we go back thinking about the containers use case, then it >>>>>>> transpires that even though the "or" policy does prevent one >>>>>>> container >>>>>>> from affecting the other from one angle, it also prevents one >>>>>>> container from exercising the feature unless all containers >>>>>>> co-operate. >>>>>>> >>>>>>> As such, we can view the original problem statement where we have an >>>>>>> issue if not everyone co-operates, as conceptually the same just >>>>>>> from >>>>>>> an opposite angle. (Rather than one container incurring the >>>>>>> increased >>>>>>> cost of context switches to the rest, we would have one container >>>>>>> preventing the optimized slice configuration to the other.) >>>>>>> >>>>>>> From this follows that both proposals require complete >>>>>>> co-operation >>>>>>> from all running userspace to avoid complete control of the feature. >>>>>>> >>>>>>> Since the balance between the benefit of optimized slice >>>>>>> configuration >>>>>>> (or penalty of suboptimal one), versus the penalty of increased >>>>>>> context switch times, cannot be know by the driver (barring >>>>>>> venturing >>>>>>> into the heuristics territory), that is another reason why I find >>>>>>> the >>>>>>> "or" policy in the driver questionable. >>>>>>> >>>>>>> We can also ask a question of - If we go with the "or" policy, why >>>>>>> require N per-context ioctls to modify the global GPU configuration >>>>>>> and not instead add a global driver ioctl to modify the state? >>>>>>> >>>>>>> If a future hardware requires, or enables, the per-context behaviour >>>>>>> in a more efficient way, we could then revisit the problem space. >>>>>>> >>>>>>> In the mean time I see the "or" policy solution as adding some ABI >>>>>>> which doesn't do anything for many use cases without any way for the >>>>>>> sysadmin to enable it. At the same time master sysfs knob at least >>>>>>> enables the sysadmin to make a decision. Here I am thinking about a >>>>>>> random client environment where not all userspace co-operates, >>>>>>> but for >>>>>>> instance user is running the feature aware media stack, and >>>>>>> non-feature aware OpenCL/3d stack. >>>>>>> >>>>>>> I guess the complete story boils down to - is the master sysfs knob >>>>>>> really a problem in container use cases. >>>>>>> >>>>>>> Regards, >>>>>>> >>>>>>> Tvrtko >>>>>> Hey Tvrtko, >>>>>> >>>>>> Thanks for summarizing a bunch of discussions. >>>>>> Essentially I agree with every you wrote above. >>>>>> >>>>>> If we have a global setting (determined by the OR policy), what's the >>>>>> point of per context settings? >>>>>> >>>>>> In Dmitry's scenario, all userspace applications will work >>>>>> together to >>>>>> reach the consensus so it sounds like we're reimplementing the policy >>>>>> that is already existing in userspace. >>>>>> >>>>>> Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else >>>>>> than me pick one or the other :) >>>>> I'll just mention the voting/consensus approach to see if anyone else >>>>> likes it. >>>>> >>>>> Each context has a CONTEXT_PARAM_HINT_SSEU { small, dontcare, large } >>>>> (or some other abstract names). >>>> Yeah, the param name should have the word _HINT_ in it when it's not a >>>> definitive set. >>>> >>>> There's no global setter across containers, only a scenario when >>>> everyone agrees or not. Tallying up the votes and going with a majority >>>> vote might be an option, too. >>>> >>>> Regards, Joonas >>> Trying to test the "everyone agrees" approach here. >> It's not everyone agrees, but the greater good. > > I'm looking forward to the definition of the greater good :) > Tvrtko wanted to avoid the heuristic territory, it seems like we're just > stepping into it. I am not sure that "small, dontcare, large" models brings much. No one would probably set "dontcare" since we have to default new contexts to large to be compatible. Don't know, I still prefer the master knob option. Honestly don't yet see the containers use case as a problem. There is always a master domain in any system where the knob can be enabled if the customers on the system are such to warrant it. On mixed systems enable it or not someone always suffers. And with the knob we are free to add heuristics later, keep the uapi, and just default the knob to on. I think the focus should be reaching a consensus whether the containers use case is a problem with the master knob or not. Regards, Tvrtko
Quoting Lionel Landwerlin (2018-06-12 11:52:10) > I'm looking forward to the definition of the greater good :) > Tvrtko wanted to avoid the heuristic territory, it seems like we're just > stepping into it. If we have to make any choice, we have not just stepped, but dived head first into it. Tbh, I quite like the idea of having BPF policy hooks. (Devil will definitely be in the details!) -Chris
Quoting Tvrtko Ursulin (2018-06-12 15:02:07) > > On 12/06/2018 11:52, Lionel Landwerlin wrote: > > On 12/06/18 11:37, Chris Wilson wrote: > >> Quoting Lionel Landwerlin (2018-06-12 11:33:34) > >>> On 12/06/18 10:20, Joonas Lahtinen wrote: > >>>> Quoting Chris Wilson (2018-06-11 18:02:37) > >>>>> Quoting Lionel Landwerlin (2018-06-11 14:46:07) > >>>>>> On 11/06/18 13:10, Tvrtko Ursulin wrote: > >>>>>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: > >>>>>>>> There are concerns about denial of service around the per > >>>>>>>> context sseu > >>>>>>>> configuration capability. In a previous commit introducing the > >>>>>>>> capability we allowed it only for capable users. This changes > >>>>>>>> adds a > >>>>>>>> new debugfs entry to let any user configure its own context > >>>>>>>> powergating setup. > >>>>>>> As far as I understood it, Joonas' concerns here are: > >>>>>>> > >>>>>>> 1) That in the containers use case individual containers wouldn't be > >>>>>>> able to turn on the sysfs toggle for them. > >>>>>>> > >>>>>>> 2) That also in the containers use case if box admin turns on the > >>>>>>> feature, some containers would potentially start negatively > >>>>>>> affecting > >>>>>>> the others (via the accumulated cost of slice re-configuration on > >>>>>>> context switching). > >>>>>>> > >>>>>>> I am not familiar with typical container setups to be authoritative > >>>>>>> here, but intuitively I find it reasonable that a low-level hardware > >>>>>>> switch like this would be under the control of a master domain > >>>>>>> administrator. ("If you are installing our product in the container > >>>>>>> environment, make sure your system administrator enables this > >>>>>>> hardware > >>>>>>> feature.", "Note to system administrators: Enabling this features > >>>>>>> may > >>>>>>> negatively affect the performance of other containers.") > >>>>>>> > >>>>>>> Alternative proposal is for the i915 to apply an "or" filter on all > >>>>>>> requested masks and in that way ensure dynamic re-configuration > >>>>>>> doesn't happen on context switches, but driven from userspace via > >>>>>>> ioctls. > >>>>>>> > >>>>>>> In other words, should _all_ userspace agree between themselves that > >>>>>>> they want to turn off a slice, they would then need to send out a > >>>>>>> concerted ioctl storm, where number of needed ioctls equals the > >>>>>>> number > >>>>>>> of currently active contexts. (This may have its own performance > >>>>>>> consequences caused by the barriers needed to modify all context > >>>>>>> images.) > >>>>>>> > >>>>>>> This was deemed acceptable the the media use case, but my concern is > >>>>>>> the approach is not elegant and will tie us with the "or" policy in > >>>>>>> the ABI. (Performance concerns I haven't evaluated yet, but they > >>>>>>> also > >>>>>>> may be significant.) > >>>>>>> > >>>>>>> If we go back thinking about the containers use case, then it > >>>>>>> transpires that even though the "or" policy does prevent one > >>>>>>> container > >>>>>>> from affecting the other from one angle, it also prevents one > >>>>>>> container from exercising the feature unless all containers > >>>>>>> co-operate. > >>>>>>> > >>>>>>> As such, we can view the original problem statement where we have an > >>>>>>> issue if not everyone co-operates, as conceptually the same just > >>>>>>> from > >>>>>>> an opposite angle. (Rather than one container incurring the > >>>>>>> increased > >>>>>>> cost of context switches to the rest, we would have one container > >>>>>>> preventing the optimized slice configuration to the other.) > >>>>>>> > >>>>>>> From this follows that both proposals require complete > >>>>>>> co-operation > >>>>>>> from all running userspace to avoid complete control of the feature. > >>>>>>> > >>>>>>> Since the balance between the benefit of optimized slice > >>>>>>> configuration > >>>>>>> (or penalty of suboptimal one), versus the penalty of increased > >>>>>>> context switch times, cannot be know by the driver (barring > >>>>>>> venturing > >>>>>>> into the heuristics territory), that is another reason why I find > >>>>>>> the > >>>>>>> "or" policy in the driver questionable. > >>>>>>> > >>>>>>> We can also ask a question of - If we go with the "or" policy, why > >>>>>>> require N per-context ioctls to modify the global GPU configuration > >>>>>>> and not instead add a global driver ioctl to modify the state? > >>>>>>> > >>>>>>> If a future hardware requires, or enables, the per-context behaviour > >>>>>>> in a more efficient way, we could then revisit the problem space. > >>>>>>> > >>>>>>> In the mean time I see the "or" policy solution as adding some ABI > >>>>>>> which doesn't do anything for many use cases without any way for the > >>>>>>> sysadmin to enable it. At the same time master sysfs knob at least > >>>>>>> enables the sysadmin to make a decision. Here I am thinking about a > >>>>>>> random client environment where not all userspace co-operates, > >>>>>>> but for > >>>>>>> instance user is running the feature aware media stack, and > >>>>>>> non-feature aware OpenCL/3d stack. > >>>>>>> > >>>>>>> I guess the complete story boils down to - is the master sysfs knob > >>>>>>> really a problem in container use cases. > >>>>>>> > >>>>>>> Regards, > >>>>>>> > >>>>>>> Tvrtko > >>>>>> Hey Tvrtko, > >>>>>> > >>>>>> Thanks for summarizing a bunch of discussions. > >>>>>> Essentially I agree with every you wrote above. > >>>>>> > >>>>>> If we have a global setting (determined by the OR policy), what's the > >>>>>> point of per context settings? > >>>>>> > >>>>>> In Dmitry's scenario, all userspace applications will work > >>>>>> together to > >>>>>> reach the consensus so it sounds like we're reimplementing the policy > >>>>>> that is already existing in userspace. > >>>>>> > >>>>>> Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else > >>>>>> than me pick one or the other :) > >>>>> I'll just mention the voting/consensus approach to see if anyone else > >>>>> likes it. > >>>>> > >>>>> Each context has a CONTEXT_PARAM_HINT_SSEU { small, dontcare, large } > >>>>> (or some other abstract names). > >>>> Yeah, the param name should have the word _HINT_ in it when it's not a > >>>> definitive set. > >>>> > >>>> There's no global setter across containers, only a scenario when > >>>> everyone agrees or not. Tallying up the votes and going with a majority > >>>> vote might be an option, too. > >>>> > >>>> Regards, Joonas > >>> Trying to test the "everyone agrees" approach here. > >> It's not everyone agrees, but the greater good. > > > > I'm looking forward to the definition of the greater good :) > > Tvrtko wanted to avoid the heuristic territory, it seems like we're just > > stepping into it. > > I am not sure that "small, dontcare, large" models brings much. No one > would probably set "dontcare" since we have to default new contexts to > large to be compatible. > > Don't know, I still prefer the master knob option. Honestly don't yet > see the containers use case as a problem. There is always a master > domain in any system where the knob can be enabled if the customers on > the system are such to warrant it. On mixed systems enable it or not > someone always suffers. And with the knob we are free to add heuristics > later, keep the uapi, and just default the knob to on. Master knob effectively means dead code behind a switch, that's not very upstream friendly. Regards, Joonas
On 13/06/2018 13:49, Joonas Lahtinen wrote: > Quoting Tvrtko Ursulin (2018-06-12 15:02:07) >> >> On 12/06/2018 11:52, Lionel Landwerlin wrote: >>> On 12/06/18 11:37, Chris Wilson wrote: >>>> Quoting Lionel Landwerlin (2018-06-12 11:33:34) >>>>> On 12/06/18 10:20, Joonas Lahtinen wrote: >>>>>> Quoting Chris Wilson (2018-06-11 18:02:37) >>>>>>> Quoting Lionel Landwerlin (2018-06-11 14:46:07) >>>>>>>> On 11/06/18 13:10, Tvrtko Ursulin wrote: >>>>>>>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: >>>>>>>>>> There are concerns about denial of service around the per >>>>>>>>>> context sseu >>>>>>>>>> configuration capability. In a previous commit introducing the >>>>>>>>>> capability we allowed it only for capable users. This changes >>>>>>>>>> adds a >>>>>>>>>> new debugfs entry to let any user configure its own context >>>>>>>>>> powergating setup. >>>>>>>>> As far as I understood it, Joonas' concerns here are: >>>>>>>>> >>>>>>>>> 1) That in the containers use case individual containers wouldn't be >>>>>>>>> able to turn on the sysfs toggle for them. >>>>>>>>> >>>>>>>>> 2) That also in the containers use case if box admin turns on the >>>>>>>>> feature, some containers would potentially start negatively >>>>>>>>> affecting >>>>>>>>> the others (via the accumulated cost of slice re-configuration on >>>>>>>>> context switching). >>>>>>>>> >>>>>>>>> I am not familiar with typical container setups to be authoritative >>>>>>>>> here, but intuitively I find it reasonable that a low-level hardware >>>>>>>>> switch like this would be under the control of a master domain >>>>>>>>> administrator. ("If you are installing our product in the container >>>>>>>>> environment, make sure your system administrator enables this >>>>>>>>> hardware >>>>>>>>> feature.", "Note to system administrators: Enabling this features >>>>>>>>> may >>>>>>>>> negatively affect the performance of other containers.") >>>>>>>>> >>>>>>>>> Alternative proposal is for the i915 to apply an "or" filter on all >>>>>>>>> requested masks and in that way ensure dynamic re-configuration >>>>>>>>> doesn't happen on context switches, but driven from userspace via >>>>>>>>> ioctls. >>>>>>>>> >>>>>>>>> In other words, should _all_ userspace agree between themselves that >>>>>>>>> they want to turn off a slice, they would then need to send out a >>>>>>>>> concerted ioctl storm, where number of needed ioctls equals the >>>>>>>>> number >>>>>>>>> of currently active contexts. (This may have its own performance >>>>>>>>> consequences caused by the barriers needed to modify all context >>>>>>>>> images.) >>>>>>>>> >>>>>>>>> This was deemed acceptable the the media use case, but my concern is >>>>>>>>> the approach is not elegant and will tie us with the "or" policy in >>>>>>>>> the ABI. (Performance concerns I haven't evaluated yet, but they >>>>>>>>> also >>>>>>>>> may be significant.) >>>>>>>>> >>>>>>>>> If we go back thinking about the containers use case, then it >>>>>>>>> transpires that even though the "or" policy does prevent one >>>>>>>>> container >>>>>>>>> from affecting the other from one angle, it also prevents one >>>>>>>>> container from exercising the feature unless all containers >>>>>>>>> co-operate. >>>>>>>>> >>>>>>>>> As such, we can view the original problem statement where we have an >>>>>>>>> issue if not everyone co-operates, as conceptually the same just >>>>>>>>> from >>>>>>>>> an opposite angle. (Rather than one container incurring the >>>>>>>>> increased >>>>>>>>> cost of context switches to the rest, we would have one container >>>>>>>>> preventing the optimized slice configuration to the other.) >>>>>>>>> >>>>>>>>> From this follows that both proposals require complete >>>>>>>>> co-operation >>>>>>>>> from all running userspace to avoid complete control of the feature. >>>>>>>>> >>>>>>>>> Since the balance between the benefit of optimized slice >>>>>>>>> configuration >>>>>>>>> (or penalty of suboptimal one), versus the penalty of increased >>>>>>>>> context switch times, cannot be know by the driver (barring >>>>>>>>> venturing >>>>>>>>> into the heuristics territory), that is another reason why I find >>>>>>>>> the >>>>>>>>> "or" policy in the driver questionable. >>>>>>>>> >>>>>>>>> We can also ask a question of - If we go with the "or" policy, why >>>>>>>>> require N per-context ioctls to modify the global GPU configuration >>>>>>>>> and not instead add a global driver ioctl to modify the state? >>>>>>>>> >>>>>>>>> If a future hardware requires, or enables, the per-context behaviour >>>>>>>>> in a more efficient way, we could then revisit the problem space. >>>>>>>>> >>>>>>>>> In the mean time I see the "or" policy solution as adding some ABI >>>>>>>>> which doesn't do anything for many use cases without any way for the >>>>>>>>> sysadmin to enable it. At the same time master sysfs knob at least >>>>>>>>> enables the sysadmin to make a decision. Here I am thinking about a >>>>>>>>> random client environment where not all userspace co-operates, >>>>>>>>> but for >>>>>>>>> instance user is running the feature aware media stack, and >>>>>>>>> non-feature aware OpenCL/3d stack. >>>>>>>>> >>>>>>>>> I guess the complete story boils down to - is the master sysfs knob >>>>>>>>> really a problem in container use cases. >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> >>>>>>>>> Tvrtko >>>>>>>> Hey Tvrtko, >>>>>>>> >>>>>>>> Thanks for summarizing a bunch of discussions. >>>>>>>> Essentially I agree with every you wrote above. >>>>>>>> >>>>>>>> If we have a global setting (determined by the OR policy), what's the >>>>>>>> point of per context settings? >>>>>>>> >>>>>>>> In Dmitry's scenario, all userspace applications will work >>>>>>>> together to >>>>>>>> reach the consensus so it sounds like we're reimplementing the policy >>>>>>>> that is already existing in userspace. >>>>>>>> >>>>>>>> Anyway, I'm implementing Joonas' suggestion. Hopefully somebody else >>>>>>>> than me pick one or the other :) >>>>>>> I'll just mention the voting/consensus approach to see if anyone else >>>>>>> likes it. >>>>>>> >>>>>>> Each context has a CONTEXT_PARAM_HINT_SSEU { small, dontcare, large } >>>>>>> (or some other abstract names). >>>>>> Yeah, the param name should have the word _HINT_ in it when it's not a >>>>>> definitive set. >>>>>> >>>>>> There's no global setter across containers, only a scenario when >>>>>> everyone agrees or not. Tallying up the votes and going with a majority >>>>>> vote might be an option, too. >>>>>> >>>>>> Regards, Joonas >>>>> Trying to test the "everyone agrees" approach here. >>>> It's not everyone agrees, but the greater good. >>> >>> I'm looking forward to the definition of the greater good :) >>> Tvrtko wanted to avoid the heuristic territory, it seems like we're just >>> stepping into it. >> >> I am not sure that "small, dontcare, large" models brings much. No one >> would probably set "dontcare" since we have to default new contexts to >> large to be compatible. >> >> Don't know, I still prefer the master knob option. Honestly don't yet >> see the containers use case as a problem. There is always a master >> domain in any system where the knob can be enabled if the customers on >> the system are such to warrant it. On mixed systems enable it or not >> someone always suffers. And with the knob we are free to add heuristics >> later, keep the uapi, and just default the knob to on. > > Master knob effectively means dead code behind a switch, that's not very > upstream friendly. Hey at least I wasn't proposing a modparam! :))) Yes it is not the best software practice, upstream or not, however I am trying to be pragmatical here and choose the simplest, smallest, good enough, and least headache inducing in the future solution. One way of of looking at the master switch could be like tune your system for XYZ - change CPU frequency governor, disable SATA link saving, allow i915 media optimized mode. Some live code out, some dead code in. But perhaps discussion is moot since we don't have userspace anyway. Regards, Tvrtko
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > Tvrtko Ursulin > Sent: Thursday, June 14, 2018 1:29 AM > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Chris Wilson > <chris@chris-wilson.co.uk>; Landwerlin, Lionel G > <lionel.g.landwerlin@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs entry to let users > set sseu configs > > > On 13/06/2018 13:49, Joonas Lahtinen wrote: > > Quoting Tvrtko Ursulin (2018-06-12 15:02:07) > >> > >> On 12/06/2018 11:52, Lionel Landwerlin wrote: > >>> On 12/06/18 11:37, Chris Wilson wrote: > >>>> Quoting Lionel Landwerlin (2018-06-12 11:33:34) > >>>>> On 12/06/18 10:20, Joonas Lahtinen wrote: > >>>>>> Quoting Chris Wilson (2018-06-11 18:02:37) > >>>>>>> Quoting Lionel Landwerlin (2018-06-11 14:46:07) > >>>>>>>> On 11/06/18 13:10, Tvrtko Ursulin wrote: > >>>>>>>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: > >>>>>>>>>> There are concerns about denial of service around the per > >>>>>>>>>> context sseu > >>>>>>>>>> configuration capability. In a previous commit introducing the > >>>>>>>>>> capability we allowed it only for capable users. This changes > >>>>>>>>>> adds a > >>>>>>>>>> new debugfs entry to let any user configure its own context > >>>>>>>>>> powergating setup. > >>>>>>>>> As far as I understood it, Joonas' concerns here are: > >>>>>>>>> > >>>>>>>>> 1) That in the containers use case individual containers wouldn't > be > >>>>>>>>> able to turn on the sysfs toggle for them. > >>>>>>>>> > >>>>>>>>> 2) That also in the containers use case if box admin turns on the > >>>>>>>>> feature, some containers would potentially start negatively > >>>>>>>>> affecting > >>>>>>>>> the others (via the accumulated cost of slice re-configuration on > >>>>>>>>> context switching). > >>>>>>>>> > >>>>>>>>> I am not familiar with typical container setups to be authoritative > >>>>>>>>> here, but intuitively I find it reasonable that a low-level hardware > >>>>>>>>> switch like this would be under the control of a master domain > >>>>>>>>> administrator. ("If you are installing our product in the container > >>>>>>>>> environment, make sure your system administrator enables this > >>>>>>>>> hardware > >>>>>>>>> feature.", "Note to system administrators: Enabling this features > >>>>>>>>> may > >>>>>>>>> negatively affect the performance of other containers.") > >>>>>>>>> > >>>>>>>>> Alternative proposal is for the i915 to apply an "or" filter on all > >>>>>>>>> requested masks and in that way ensure dynamic re- > configuration > >>>>>>>>> doesn't happen on context switches, but driven from userspace > via > >>>>>>>>> ioctls. > >>>>>>>>> > >>>>>>>>> In other words, should _all_ userspace agree between > themselves that > >>>>>>>>> they want to turn off a slice, they would then need to send out a > >>>>>>>>> concerted ioctl storm, where number of needed ioctls equals > the > >>>>>>>>> number > >>>>>>>>> of currently active contexts. (This may have its own performance > >>>>>>>>> consequences caused by the barriers needed to modify all > context > >>>>>>>>> images.) > >>>>>>>>> > >>>>>>>>> This was deemed acceptable the the media use case, but my > concern is > >>>>>>>>> the approach is not elegant and will tie us with the "or" policy in > >>>>>>>>> the ABI. (Performance concerns I haven't evaluated yet, but > they > >>>>>>>>> also > >>>>>>>>> may be significant.) > >>>>>>>>> > >>>>>>>>> If we go back thinking about the containers use case, then it > >>>>>>>>> transpires that even though the "or" policy does prevent one > >>>>>>>>> container > >>>>>>>>> from affecting the other from one angle, it also prevents one > >>>>>>>>> container from exercising the feature unless all containers > >>>>>>>>> co-operate. > >>>>>>>>> > >>>>>>>>> As such, we can view the original problem statement where we > have an > >>>>>>>>> issue if not everyone co-operates, as conceptually the same just > >>>>>>>>> from > >>>>>>>>> an opposite angle. (Rather than one container incurring the > >>>>>>>>> increased > >>>>>>>>> cost of context switches to the rest, we would have one > container > >>>>>>>>> preventing the optimized slice configuration to the other.) > >>>>>>>>> > >>>>>>>>> From this follows that both proposals require complete > >>>>>>>>> co-operation > >>>>>>>>> from all running userspace to avoid complete control of the > feature. > >>>>>>>>> > >>>>>>>>> Since the balance between the benefit of optimized slice > >>>>>>>>> configuration > >>>>>>>>> (or penalty of suboptimal one), versus the penalty of increased > >>>>>>>>> context switch times, cannot be know by the driver (barring > >>>>>>>>> venturing > >>>>>>>>> into the heuristics territory), that is another reason why I find > >>>>>>>>> the > >>>>>>>>> "or" policy in the driver questionable. > >>>>>>>>> > >>>>>>>>> We can also ask a question of - If we go with the "or" policy, why > >>>>>>>>> require N per-context ioctls to modify the global GPU > configuration > >>>>>>>>> and not instead add a global driver ioctl to modify the state? > >>>>>>>>> > >>>>>>>>> If a future hardware requires, or enables, the per-context > behaviour > >>>>>>>>> in a more efficient way, we could then revisit the problem space. > >>>>>>>>> > >>>>>>>>> In the mean time I see the "or" policy solution as adding some > ABI > >>>>>>>>> which doesn't do anything for many use cases without any way > for the > >>>>>>>>> sysadmin to enable it. At the same time master sysfs knob at > least > >>>>>>>>> enables the sysadmin to make a decision. Here I am thinking > about a > >>>>>>>>> random client environment where not all userspace co- > operates, > >>>>>>>>> but for > >>>>>>>>> instance user is running the feature aware media stack, and > >>>>>>>>> non-feature aware OpenCL/3d stack. > >>>>>>>>> > >>>>>>>>> I guess the complete story boils down to - is the master sysfs > knob > >>>>>>>>> really a problem in container use cases. > >>>>>>>>> > >>>>>>>>> Regards, > >>>>>>>>> > >>>>>>>>> Tvrtko > >>>>>>>> Hey Tvrtko, > >>>>>>>> > >>>>>>>> Thanks for summarizing a bunch of discussions. > >>>>>>>> Essentially I agree with every you wrote above. > >>>>>>>> > >>>>>>>> If we have a global setting (determined by the OR policy), what's > the > >>>>>>>> point of per context settings? > >>>>>>>> > >>>>>>>> In Dmitry's scenario, all userspace applications will work > >>>>>>>> together to > >>>>>>>> reach the consensus so it sounds like we're reimplementing the > policy > >>>>>>>> that is already existing in userspace. > >>>>>>>> > >>>>>>>> Anyway, I'm implementing Joonas' suggestion. Hopefully > somebody else > >>>>>>>> than me pick one or the other :) > >>>>>>> I'll just mention the voting/consensus approach to see if anyone > else > >>>>>>> likes it. > >>>>>>> > >>>>>>> Each context has a CONTEXT_PARAM_HINT_SSEU { small, > dontcare, large } > >>>>>>> (or some other abstract names). > >>>>>> Yeah, the param name should have the word _HINT_ in it when it's > not a > >>>>>> definitive set. > >>>>>> > >>>>>> There's no global setter across containers, only a scenario when > >>>>>> everyone agrees or not. Tallying up the votes and going with a > majority > >>>>>> vote might be an option, too. > >>>>>> > >>>>>> Regards, Joonas > >>>>> Trying to test the "everyone agrees" approach here. > >>>> It's not everyone agrees, but the greater good. > >>> > >>> I'm looking forward to the definition of the greater good :) > >>> Tvrtko wanted to avoid the heuristic territory, it seems like we're just > >>> stepping into it. > >> > >> I am not sure that "small, dontcare, large" models brings much. No one > >> would probably set "dontcare" since we have to default new contexts to > >> large to be compatible. > >> > >> Don't know, I still prefer the master knob option. Honestly don't yet > >> see the containers use case as a problem. There is always a master > >> domain in any system where the knob can be enabled if the customers on > >> the system are such to warrant it. On mixed systems enable it or not > >> someone always suffers. And with the knob we are free to add heuristics > >> later, keep the uapi, and just default the knob to on. > > > > Master knob effectively means dead code behind a switch, that's not very > > upstream friendly. > > Hey at least I wasn't proposing a modparam! :))) > > Yes it is not the best software practice, upstream or not, however I am > trying to be pragmatical here and choose the simplest, smallest, good > enough, and least headache inducing in the future solution. > > One way of of looking at the master switch could be like tune your > system for XYZ - change CPU frequency governor, disable SATA link > saving, allow i915 media optimized mode. Some live code out, some dead > code in. > > But perhaps discussion is moot since we don't have userspace anyway. > > Regards, > > Tvrtko I'm surprised by the "no deadcode behind knobs comment". We do this all the time - "display=0" anyone? Or "enable_cmdparser=false"? Allowing user space to reduce EU performance for the system as a whole is not a great idea imho. Only sysadmin should have the right to let that happen, and an admin switch (I WOULD go with a modparam personally) is a good way to ensure that we can get the optimal media configuration for dedicated media systems, while for general systems we get the best overall performance (not just favouring media). Why would we want to over engineer this? Jon
Quoting Bloomfield, Jon (2018-07-18 19:44:14) > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > > Tvrtko Ursulin > > Sent: Thursday, June 14, 2018 1:29 AM > > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Chris Wilson > > <chris@chris-wilson.co.uk>; Landwerlin, Lionel G > > <lionel.g.landwerlin@intel.com>; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs entry to let users > > set sseu configs > > > > > > On 13/06/2018 13:49, Joonas Lahtinen wrote: > > > Quoting Tvrtko Ursulin (2018-06-12 15:02:07) > > >> > > >> On 12/06/2018 11:52, Lionel Landwerlin wrote: > > >>> On 12/06/18 11:37, Chris Wilson wrote: > > >>>> Quoting Lionel Landwerlin (2018-06-12 11:33:34) > > >>>>> On 12/06/18 10:20, Joonas Lahtinen wrote: > > >>>>>> Quoting Chris Wilson (2018-06-11 18:02:37) > > >>>>>>> Quoting Lionel Landwerlin (2018-06-11 14:46:07) > > >>>>>>>> On 11/06/18 13:10, Tvrtko Ursulin wrote: > > >>>>>>>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: > > >>>>>>>>>> There are concerns about denial of service around the per > > >>>>>>>>>> context sseu > > >>>>>>>>>> configuration capability. In a previous commit introducing the > > >>>>>>>>>> capability we allowed it only for capable users. This changes > > >>>>>>>>>> adds a > > >>>>>>>>>> new debugfs entry to let any user configure its own context > > >>>>>>>>>> powergating setup. > > >>>>>>>>> As far as I understood it, Joonas' concerns here are: > > >>>>>>>>> > > >>>>>>>>> 1) That in the containers use case individual containers wouldn't > > be > > >>>>>>>>> able to turn on the sysfs toggle for them. > > >>>>>>>>> > > >>>>>>>>> 2) That also in the containers use case if box admin turns on the > > >>>>>>>>> feature, some containers would potentially start negatively > > >>>>>>>>> affecting > > >>>>>>>>> the others (via the accumulated cost of slice re-configuration on > > >>>>>>>>> context switching). > > >>>>>>>>> > > >>>>>>>>> I am not familiar with typical container setups to be authoritative > > >>>>>>>>> here, but intuitively I find it reasonable that a low-level hardware > > >>>>>>>>> switch like this would be under the control of a master domain > > >>>>>>>>> administrator. ("If you are installing our product in the container > > >>>>>>>>> environment, make sure your system administrator enables this > > >>>>>>>>> hardware > > >>>>>>>>> feature.", "Note to system administrators: Enabling this features > > >>>>>>>>> may > > >>>>>>>>> negatively affect the performance of other containers.") > > >>>>>>>>> > > >>>>>>>>> Alternative proposal is for the i915 to apply an "or" filter on all > > >>>>>>>>> requested masks and in that way ensure dynamic re- > > configuration > > >>>>>>>>> doesn't happen on context switches, but driven from userspace > > via > > >>>>>>>>> ioctls. > > >>>>>>>>> > > >>>>>>>>> In other words, should _all_ userspace agree between > > themselves that > > >>>>>>>>> they want to turn off a slice, they would then need to send out a > > >>>>>>>>> concerted ioctl storm, where number of needed ioctls equals > > the > > >>>>>>>>> number > > >>>>>>>>> of currently active contexts. (This may have its own performance > > >>>>>>>>> consequences caused by the barriers needed to modify all > > context > > >>>>>>>>> images.) > > >>>>>>>>> > > >>>>>>>>> This was deemed acceptable the the media use case, but my > > concern is > > >>>>>>>>> the approach is not elegant and will tie us with the "or" policy in > > >>>>>>>>> the ABI. (Performance concerns I haven't evaluated yet, but > > they > > >>>>>>>>> also > > >>>>>>>>> may be significant.) > > >>>>>>>>> > > >>>>>>>>> If we go back thinking about the containers use case, then it > > >>>>>>>>> transpires that even though the "or" policy does prevent one > > >>>>>>>>> container > > >>>>>>>>> from affecting the other from one angle, it also prevents one > > >>>>>>>>> container from exercising the feature unless all containers > > >>>>>>>>> co-operate. > > >>>>>>>>> > > >>>>>>>>> As such, we can view the original problem statement where we > > have an > > >>>>>>>>> issue if not everyone co-operates, as conceptually the same just > > >>>>>>>>> from > > >>>>>>>>> an opposite angle. (Rather than one container incurring the > > >>>>>>>>> increased > > >>>>>>>>> cost of context switches to the rest, we would have one > > container > > >>>>>>>>> preventing the optimized slice configuration to the other.) > > >>>>>>>>> > > >>>>>>>>> From this follows that both proposals require complete > > >>>>>>>>> co-operation > > >>>>>>>>> from all running userspace to avoid complete control of the > > feature. > > >>>>>>>>> > > >>>>>>>>> Since the balance between the benefit of optimized slice > > >>>>>>>>> configuration > > >>>>>>>>> (or penalty of suboptimal one), versus the penalty of increased > > >>>>>>>>> context switch times, cannot be know by the driver (barring > > >>>>>>>>> venturing > > >>>>>>>>> into the heuristics territory), that is another reason why I find > > >>>>>>>>> the > > >>>>>>>>> "or" policy in the driver questionable. > > >>>>>>>>> > > >>>>>>>>> We can also ask a question of - If we go with the "or" policy, why > > >>>>>>>>> require N per-context ioctls to modify the global GPU > > configuration > > >>>>>>>>> and not instead add a global driver ioctl to modify the state? > > >>>>>>>>> > > >>>>>>>>> If a future hardware requires, or enables, the per-context > > behaviour > > >>>>>>>>> in a more efficient way, we could then revisit the problem space. > > >>>>>>>>> > > >>>>>>>>> In the mean time I see the "or" policy solution as adding some > > ABI > > >>>>>>>>> which doesn't do anything for many use cases without any way > > for the > > >>>>>>>>> sysadmin to enable it. At the same time master sysfs knob at > > least > > >>>>>>>>> enables the sysadmin to make a decision. Here I am thinking > > about a > > >>>>>>>>> random client environment where not all userspace co- > > operates, > > >>>>>>>>> but for > > >>>>>>>>> instance user is running the feature aware media stack, and > > >>>>>>>>> non-feature aware OpenCL/3d stack. > > >>>>>>>>> > > >>>>>>>>> I guess the complete story boils down to - is the master sysfs > > knob > > >>>>>>>>> really a problem in container use cases. > > >>>>>>>>> > > >>>>>>>>> Regards, > > >>>>>>>>> > > >>>>>>>>> Tvrtko > > >>>>>>>> Hey Tvrtko, > > >>>>>>>> > > >>>>>>>> Thanks for summarizing a bunch of discussions. > > >>>>>>>> Essentially I agree with every you wrote above. > > >>>>>>>> > > >>>>>>>> If we have a global setting (determined by the OR policy), what's > > the > > >>>>>>>> point of per context settings? > > >>>>>>>> > > >>>>>>>> In Dmitry's scenario, all userspace applications will work > > >>>>>>>> together to > > >>>>>>>> reach the consensus so it sounds like we're reimplementing the > > policy > > >>>>>>>> that is already existing in userspace. > > >>>>>>>> > > >>>>>>>> Anyway, I'm implementing Joonas' suggestion. Hopefully > > somebody else > > >>>>>>>> than me pick one or the other :) > > >>>>>>> I'll just mention the voting/consensus approach to see if anyone > > else > > >>>>>>> likes it. > > >>>>>>> > > >>>>>>> Each context has a CONTEXT_PARAM_HINT_SSEU { small, > > dontcare, large } > > >>>>>>> (or some other abstract names). > > >>>>>> Yeah, the param name should have the word _HINT_ in it when it's > > not a > > >>>>>> definitive set. > > >>>>>> > > >>>>>> There's no global setter across containers, only a scenario when > > >>>>>> everyone agrees or not. Tallying up the votes and going with a > > majority > > >>>>>> vote might be an option, too. > > >>>>>> > > >>>>>> Regards, Joonas > > >>>>> Trying to test the "everyone agrees" approach here. > > >>>> It's not everyone agrees, but the greater good. > > >>> > > >>> I'm looking forward to the definition of the greater good :) > > >>> Tvrtko wanted to avoid the heuristic territory, it seems like we're just > > >>> stepping into it. > > >> > > >> I am not sure that "small, dontcare, large" models brings much. No one > > >> would probably set "dontcare" since we have to default new contexts to > > >> large to be compatible. > > >> > > >> Don't know, I still prefer the master knob option. Honestly don't yet > > >> see the containers use case as a problem. There is always a master > > >> domain in any system where the knob can be enabled if the customers on > > >> the system are such to warrant it. On mixed systems enable it or not > > >> someone always suffers. And with the knob we are free to add heuristics > > >> later, keep the uapi, and just default the knob to on. > > > > > > Master knob effectively means dead code behind a switch, that's not very > > > upstream friendly. > > > > Hey at least I wasn't proposing a modparam! :))) > > > > Yes it is not the best software practice, upstream or not, however I am > > trying to be pragmatical here and choose the simplest, smallest, good > > enough, and least headache inducing in the future solution. > > > > One way of of looking at the master switch could be like tune your > > system for XYZ - change CPU frequency governor, disable SATA link > > saving, allow i915 media optimized mode. Some live code out, some dead > > code in. > > > > But perhaps discussion is moot since we don't have userspace anyway. > > > > Regards, > > > > Tvrtko > > I'm surprised by the "no deadcode behind knobs comment". We do this all > the time - "display=0" anyone? Or "enable_cmdparser=false"? Display-less machines are in the progress of being added to CI as we speak. And what goes for enable_rc6=0 and enable_execlists=0 options, if you look closely, they have been gotten rid of already. Something existing in the codebase doesn't mean it's fine to add more of the same. > Allowing user space to reduce EU performance for the system as a whole > is not a great idea imho. When all the clients ask for it, there should be no problem in reducing the slice count. > Only sysadmin should have the right to let that > happen, and an admin switch (I WOULD go with a modparam personally) is > a good way to ensure that we can get the optimal media configuration for > dedicated media systems, while for general systems we get the best overall > performance (not just favouring media). There definitely won't be a modparam for it. Regards, Joonas
Gratuitous top posting to re-kick the thread.
For Gen11 we can't have an on/off switch anyway (media simply won't run
with an oncompatible slice config), so let's agree on an api to allow userland
to select the slice configuration for its contexts, for Gen11 only. I'd prefer a
simple {MediaConfig/GeneralConfig} type context param but I really don't
care that much.
For gen9/10 it's arguable whether we need this at all. The effect on media
workloads varies, but I'm guessing normal users (outside a transcode
type environment) will never know they're missing anything. Either way,
we can continue discussing while we progress the gen11 solution.
Jon
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Bloomfield, Jon
> Sent: Wednesday, July 18, 2018 9:44 AM
> To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Chris Wilson <chris@chris-wilson.co.uk>;
> Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; intel-
> gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs entry to let users
> set sseu configs
>
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Tvrtko Ursulin
> > Sent: Thursday, June 14, 2018 1:29 AM
> > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Chris Wilson
> > <chris@chris-wilson.co.uk>; Landwerlin, Lionel G
> > <lionel.g.landwerlin@intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs entry to let
> users
> > set sseu configs
> >
> >
> > On 13/06/2018 13:49, Joonas Lahtinen wrote:
> > > Quoting Tvrtko Ursulin (2018-06-12 15:02:07)
> > >>
> > >> On 12/06/2018 11:52, Lionel Landwerlin wrote:
> > >>> On 12/06/18 11:37, Chris Wilson wrote:
> > >>>> Quoting Lionel Landwerlin (2018-06-12 11:33:34)
> > >>>>> On 12/06/18 10:20, Joonas Lahtinen wrote:
> > >>>>>> Quoting Chris Wilson (2018-06-11 18:02:37)
> > >>>>>>> Quoting Lionel Landwerlin (2018-06-11 14:46:07)
> > >>>>>>>> On 11/06/18 13:10, Tvrtko Ursulin wrote:
> > >>>>>>>>> On 30/05/2018 15:33, Lionel Landwerlin wrote:
> > >>>>>>>>>> There are concerns about denial of service around the per
> > >>>>>>>>>> context sseu
> > >>>>>>>>>> configuration capability. In a previous commit introducing the
> > >>>>>>>>>> capability we allowed it only for capable users. This changes
> > >>>>>>>>>> adds a
> > >>>>>>>>>> new debugfs entry to let any user configure its own context
> > >>>>>>>>>> powergating setup.
> > >>>>>>>>> As far as I understood it, Joonas' concerns here are:
> > >>>>>>>>>
> > >>>>>>>>> 1) That in the containers use case individual containers
> wouldn't
> > be
> > >>>>>>>>> able to turn on the sysfs toggle for them.
> > >>>>>>>>>
> > >>>>>>>>> 2) That also in the containers use case if box admin turns on
> the
> > >>>>>>>>> feature, some containers would potentially start negatively
> > >>>>>>>>> affecting
> > >>>>>>>>> the others (via the accumulated cost of slice re-configuration
> on
> > >>>>>>>>> context switching).
> > >>>>>>>>>
> > >>>>>>>>> I am not familiar with typical container setups to be
> authoritative
> > >>>>>>>>> here, but intuitively I find it reasonable that a low-level
> hardware
> > >>>>>>>>> switch like this would be under the control of a master domain
> > >>>>>>>>> administrator. ("If you are installing our product in the
> container
> > >>>>>>>>> environment, make sure your system administrator enables
> this
> > >>>>>>>>> hardware
> > >>>>>>>>> feature.", "Note to system administrators: Enabling this
> features
> > >>>>>>>>> may
> > >>>>>>>>> negatively affect the performance of other containers.")
> > >>>>>>>>>
> > >>>>>>>>> Alternative proposal is for the i915 to apply an "or" filter on all
> > >>>>>>>>> requested masks and in that way ensure dynamic re-
> > configuration
> > >>>>>>>>> doesn't happen on context switches, but driven from
> userspace
> > via
> > >>>>>>>>> ioctls.
> > >>>>>>>>>
> > >>>>>>>>> In other words, should _all_ userspace agree between
> > themselves that
> > >>>>>>>>> they want to turn off a slice, they would then need to send out
> a
> > >>>>>>>>> concerted ioctl storm, where number of needed ioctls equals
> > the
> > >>>>>>>>> number
> > >>>>>>>>> of currently active contexts. (This may have its own
> performance
> > >>>>>>>>> consequences caused by the barriers needed to modify all
> > context
> > >>>>>>>>> images.)
> > >>>>>>>>>
> > >>>>>>>>> This was deemed acceptable the the media use case, but my
> > concern is
> > >>>>>>>>> the approach is not elegant and will tie us with the "or" policy
> in
> > >>>>>>>>> the ABI. (Performance concerns I haven't evaluated yet, but
> > they
> > >>>>>>>>> also
> > >>>>>>>>> may be significant.)
> > >>>>>>>>>
> > >>>>>>>>> If we go back thinking about the containers use case, then it
> > >>>>>>>>> transpires that even though the "or" policy does prevent one
> > >>>>>>>>> container
> > >>>>>>>>> from affecting the other from one angle, it also prevents one
> > >>>>>>>>> container from exercising the feature unless all containers
> > >>>>>>>>> co-operate.
> > >>>>>>>>>
> > >>>>>>>>> As such, we can view the original problem statement where
> we
> > have an
> > >>>>>>>>> issue if not everyone co-operates, as conceptually the same
> just
> > >>>>>>>>> from
> > >>>>>>>>> an opposite angle. (Rather than one container incurring the
> > >>>>>>>>> increased
> > >>>>>>>>> cost of context switches to the rest, we would have one
> > container
> > >>>>>>>>> preventing the optimized slice configuration to the other.)
> > >>>>>>>>>
> > >>>>>>>>> From this follows that both proposals require complete
> > >>>>>>>>> co-operation
> > >>>>>>>>> from all running userspace to avoid complete control of the
> > feature.
> > >>>>>>>>>
> > >>>>>>>>> Since the balance between the benefit of optimized slice
> > >>>>>>>>> configuration
> > >>>>>>>>> (or penalty of suboptimal one), versus the penalty of increased
> > >>>>>>>>> context switch times, cannot be know by the driver (barring
> > >>>>>>>>> venturing
> > >>>>>>>>> into the heuristics territory), that is another reason why I find
> > >>>>>>>>> the
> > >>>>>>>>> "or" policy in the driver questionable.
> > >>>>>>>>>
> > >>>>>>>>> We can also ask a question of - If we go with the "or" policy,
> why
> > >>>>>>>>> require N per-context ioctls to modify the global GPU
> > configuration
> > >>>>>>>>> and not instead add a global driver ioctl to modify the state?
> > >>>>>>>>>
> > >>>>>>>>> If a future hardware requires, or enables, the per-context
> > behaviour
> > >>>>>>>>> in a more efficient way, we could then revisit the problem
> space.
> > >>>>>>>>>
> > >>>>>>>>> In the mean time I see the "or" policy solution as adding some
> > ABI
> > >>>>>>>>> which doesn't do anything for many use cases without any way
> > for the
> > >>>>>>>>> sysadmin to enable it. At the same time master sysfs knob at
> > least
> > >>>>>>>>> enables the sysadmin to make a decision. Here I am thinking
> > about a
> > >>>>>>>>> random client environment where not all userspace co-
> > operates,
> > >>>>>>>>> but for
> > >>>>>>>>> instance user is running the feature aware media stack, and
> > >>>>>>>>> non-feature aware OpenCL/3d stack.
> > >>>>>>>>>
> > >>>>>>>>> I guess the complete story boils down to - is the master sysfs
> > knob
> > >>>>>>>>> really a problem in container use cases.
> > >>>>>>>>>
> > >>>>>>>>> Regards,
> > >>>>>>>>>
> > >>>>>>>>> Tvrtko
> > >>>>>>>> Hey Tvrtko,
> > >>>>>>>>
> > >>>>>>>> Thanks for summarizing a bunch of discussions.
> > >>>>>>>> Essentially I agree with every you wrote above.
> > >>>>>>>>
> > >>>>>>>> If we have a global setting (determined by the OR policy), what's
> > the
> > >>>>>>>> point of per context settings?
> > >>>>>>>>
> > >>>>>>>> In Dmitry's scenario, all userspace applications will work
> > >>>>>>>> together to
> > >>>>>>>> reach the consensus so it sounds like we're reimplementing the
> > policy
> > >>>>>>>> that is already existing in userspace.
> > >>>>>>>>
> > >>>>>>>> Anyway, I'm implementing Joonas' suggestion. Hopefully
> > somebody else
> > >>>>>>>> than me pick one or the other :)
> > >>>>>>> I'll just mention the voting/consensus approach to see if anyone
> > else
> > >>>>>>> likes it.
> > >>>>>>>
> > >>>>>>> Each context has a CONTEXT_PARAM_HINT_SSEU { small,
> > dontcare, large }
> > >>>>>>> (or some other abstract names).
> > >>>>>> Yeah, the param name should have the word _HINT_ in it when it's
> > not a
> > >>>>>> definitive set.
> > >>>>>>
> > >>>>>> There's no global setter across containers, only a scenario when
> > >>>>>> everyone agrees or not. Tallying up the votes and going with a
> > majority
> > >>>>>> vote might be an option, too.
> > >>>>>>
> > >>>>>> Regards, Joonas
> > >>>>> Trying to test the "everyone agrees" approach here.
> > >>>> It's not everyone agrees, but the greater good.
> > >>>
> > >>> I'm looking forward to the definition of the greater good :)
> > >>> Tvrtko wanted to avoid the heuristic territory, it seems like we're just
> > >>> stepping into it.
> > >>
> > >> I am not sure that "small, dontcare, large" models brings much. No one
> > >> would probably set "dontcare" since we have to default new contexts
> to
> > >> large to be compatible.
> > >>
> > >> Don't know, I still prefer the master knob option. Honestly don't yet
> > >> see the containers use case as a problem. There is always a master
> > >> domain in any system where the knob can be enabled if the customers
> on
> > >> the system are such to warrant it. On mixed systems enable it or not
> > >> someone always suffers. And with the knob we are free to add
> heuristics
> > >> later, keep the uapi, and just default the knob to on.
> > >
> > > Master knob effectively means dead code behind a switch, that's not
> very
> > > upstream friendly.
> >
> > Hey at least I wasn't proposing a modparam! :)))
> >
> > Yes it is not the best software practice, upstream or not, however I am
> > trying to be pragmatical here and choose the simplest, smallest, good
> > enough, and least headache inducing in the future solution.
> >
> > One way of of looking at the master switch could be like tune your
> > system for XYZ - change CPU frequency governor, disable SATA link
> > saving, allow i915 media optimized mode. Some live code out, some dead
> > code in.
> >
> > But perhaps discussion is moot since we don't have userspace anyway.
> >
> > Regards,
> >
> > Tvrtko
>
> I'm surprised by the "no deadcode behind knobs comment". We do this all
> the time - "display=0" anyone? Or "enable_cmdparser=false"?
>
> Allowing user space to reduce EU performance for the system as a whole
> is not a great idea imho. Only sysadmin should have the right to let that
> happen, and an admin switch (I WOULD go with a modparam personally) is
> a good way to ensure that we can get the optimal media configuration for
> dedicated media systems, while for general systems we get the best overall
> performance (not just favouring media).
>
> Why would we want to over engineer this?
>
> Jon
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2018-07-24 at 21:50 +0000, Bloomfield, Jon wrote: > Gratuitous top posting to re-kick the thread. > > For Gen11 we can't have an on/off switch anyway (media simply won't > run > with an oncompatible slice config), so let's agree on an api to allow > userland > to select the slice configuration for its contexts, for Gen11 only. > I'd prefer a > simple {MediaConfig/GeneralConfig} type context param but I really > don't > care that much. Does anyone work on breaking the patch series for Gen11 and Gen8/9/10 with Gen11 preceding since this is required for the functional side? We hope to have userspace media driver ICL patches available pretty soon and would be really nice to have kernel side prepared as much as possible... If no one is working on this, can you, Lionel, help, please? > > For gen9/10 it's arguable whether we need this at all. The effect on > media > workloads varies, but I'm guessing normal users (outside a transcode > type environment) will never know they're missing anything. Either > way, > we can continue discussing while we progress the gen11 solution. > > Jon > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf > > Of > > Bloomfield, Jon > > Sent: Wednesday, July 18, 2018 9:44 AM > > To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Joonas > > Lahtinen > > <joonas.lahtinen@linux.intel.com>; Chris Wilson <chris@chris-wilson > > .co.uk>; > > Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; intel- > > gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs entry > > to let users > > set sseu configs > > > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On > > > Behalf Of > > > Tvrtko Ursulin > > > Sent: Thursday, June 14, 2018 1:29 AM > > > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Chris > > > Wilson > > > <chris@chris-wilson.co.uk>; Landwerlin, Lionel G > > > <lionel.g.landwerlin@intel.com>; intel-gfx@lists.freedesktop.org > > > Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs > > > entry to let > > > > users > > > set sseu configs > > > > > > > > > On 13/06/2018 13:49, Joonas Lahtinen wrote: > > > > Quoting Tvrtko Ursulin (2018-06-12 15:02:07) > > > > > > > > > > On 12/06/2018 11:52, Lionel Landwerlin wrote: > > > > > > On 12/06/18 11:37, Chris Wilson wrote: > > > > > > > Quoting Lionel Landwerlin (2018-06-12 11:33:34) > > > > > > > > On 12/06/18 10:20, Joonas Lahtinen wrote: > > > > > > > > > Quoting Chris Wilson (2018-06-11 18:02:37) > > > > > > > > > > Quoting Lionel Landwerlin (2018-06-11 14:46:07) > > > > > > > > > > > On 11/06/18 13:10, Tvrtko Ursulin wrote: > > > > > > > > > > > > On 30/05/2018 15:33, Lionel Landwerlin wrote: > > > > > > > > > > > > > There are concerns about denial of service > > > > > > > > > > > > > around the per > > > > > > > > > > > > > context sseu > > > > > > > > > > > > > configuration capability. In a previous > > > > > > > > > > > > > commit introducing the > > > > > > > > > > > > > capability we allowed it only for capable > > > > > > > > > > > > > users. This changes > > > > > > > > > > > > > adds a > > > > > > > > > > > > > new debugfs entry to let any user configure > > > > > > > > > > > > > its own context > > > > > > > > > > > > > powergating setup. > > > > > > > > > > > > > > > > > > > > > > > > As far as I understood it, Joonas' concerns > > > > > > > > > > > > here are: > > > > > > > > > > > > > > > > > > > > > > > > 1) That in the containers use case individual > > > > > > > > > > > > containers > > > > wouldn't > > > be > > > > > > > > > > > > able to turn on the sysfs toggle for them. > > > > > > > > > > > > > > > > > > > > > > > > 2) That also in the containers use case if box > > > > > > > > > > > > admin turns on > > > > the > > > > > > > > > > > > feature, some containers would potentially > > > > > > > > > > > > start negatively > > > > > > > > > > > > affecting > > > > > > > > > > > > the others (via the accumulated cost of slice > > > > > > > > > > > > re-configuration > > > > on > > > > > > > > > > > > context switching). > > > > > > > > > > > > > > > > > > > > > > > > I am not familiar with typical container setups > > > > > > > > > > > > to be > > > > authoritative > > > > > > > > > > > > here, but intuitively I find it reasonable that > > > > > > > > > > > > a low-level > > > > hardware > > > > > > > > > > > > switch like this would be under the control of > > > > > > > > > > > > a master domain > > > > > > > > > > > > administrator. ("If you are installing our > > > > > > > > > > > > product in the > > > > container > > > > > > > > > > > > environment, make sure your system > > > > > > > > > > > > administrator enables > > > > this > > > > > > > > > > > > hardware > > > > > > > > > > > > feature.", "Note to system administrators: > > > > > > > > > > > > Enabling this > > > > features > > > > > > > > > > > > may > > > > > > > > > > > > negatively affect the performance of other > > > > > > > > > > > > containers.") > > > > > > > > > > > > > > > > > > > > > > > > Alternative proposal is for the i915 to apply > > > > > > > > > > > > an "or" filter on all > > > > > > > > > > > > requested masks and in that way ensure dynamic > > > > > > > > > > > > re- > > > > > > configuration > > > > > > > > > > > > doesn't happen on context switches, but driven > > > > > > > > > > > > from > > > > userspace > > > via > > > > > > > > > > > > ioctls. > > > > > > > > > > > > > > > > > > > > > > > > In other words, should _all_ userspace agree > > > > > > > > > > > > between > > > > > > themselves that > > > > > > > > > > > > they want to turn off a slice, they would then > > > > > > > > > > > > need to send out > > > > a > > > > > > > > > > > > concerted ioctl storm, where number of needed > > > > > > > > > > > > ioctls equals > > > > > > the > > > > > > > > > > > > number > > > > > > > > > > > > of currently active contexts. (This may have > > > > > > > > > > > > its own > > > > performance > > > > > > > > > > > > consequences caused by the barriers needed to > > > > > > > > > > > > modify all > > > > > > context > > > > > > > > > > > > images.) > > > > > > > > > > > > > > > > > > > > > > > > This was deemed acceptable the the media use > > > > > > > > > > > > case, but my > > > > > > concern is > > > > > > > > > > > > the approach is not elegant and will tie us > > > > > > > > > > > > with the "or" policy > > > > in > > > > > > > > > > > > the ABI. (Performance concerns I haven't > > > > > > > > > > > > evaluated yet, but > > > > > > they > > > > > > > > > > > > also > > > > > > > > > > > > may be significant.) > > > > > > > > > > > > > > > > > > > > > > > > If we go back thinking about the containers use > > > > > > > > > > > > case, then it > > > > > > > > > > > > transpires that even though the "or" policy > > > > > > > > > > > > does prevent one > > > > > > > > > > > > container > > > > > > > > > > > > from affecting the other from one angle, it > > > > > > > > > > > > also prevents one > > > > > > > > > > > > container from exercising the feature unless > > > > > > > > > > > > all containers > > > > > > > > > > > > co-operate. > > > > > > > > > > > > > > > > > > > > > > > > As such, we can view the original problem > > > > > > > > > > > > statement where > > > > we > > > have an > > > > > > > > > > > > issue if not everyone co-operates, as > > > > > > > > > > > > conceptually the same > > > > just > > > > > > > > > > > > from > > > > > > > > > > > > an opposite angle. (Rather than one container > > > > > > > > > > > > incurring the > > > > > > > > > > > > increased > > > > > > > > > > > > cost of context switches to the rest, we would > > > > > > > > > > > > have one > > > > > > container > > > > > > > > > > > > preventing the optimized slice configuration to > > > > > > > > > > > > the other.) > > > > > > > > > > > > > > > > > > > > > > > > From this follows that both proposals > > > > > > > > > > > > require complete > > > > > > > > > > > > co-operation > > > > > > > > > > > > from all running userspace to avoid complete > > > > > > > > > > > > control of the > > > > > > feature. > > > > > > > > > > > > > > > > > > > > > > > > Since the balance between the benefit of > > > > > > > > > > > > optimized slice > > > > > > > > > > > > configuration > > > > > > > > > > > > (or penalty of suboptimal one), versus the > > > > > > > > > > > > penalty of increased > > > > > > > > > > > > context switch times, cannot be know by the > > > > > > > > > > > > driver (barring > > > > > > > > > > > > venturing > > > > > > > > > > > > into the heuristics territory), that is another > > > > > > > > > > > > reason why I find > > > > > > > > > > > > the > > > > > > > > > > > > "or" policy in the driver questionable. > > > > > > > > > > > > > > > > > > > > > > > > We can also ask a question of - If we go with > > > > > > > > > > > > the "or" policy, > > > > why > > > > > > > > > > > > require N per-context ioctls to modify the > > > > > > > > > > > > global GPU > > > > > > configuration > > > > > > > > > > > > and not instead add a global driver ioctl to > > > > > > > > > > > > modify the state? > > > > > > > > > > > > > > > > > > > > > > > > If a future hardware requires, or enables, the > > > > > > > > > > > > per-context > > > > > > behaviour > > > > > > > > > > > > in a more efficient way, we could then revisit > > > > > > > > > > > > the problem > > > > space. > > > > > > > > > > > > > > > > > > > > > > > > In the mean time I see the "or" policy solution > > > > > > > > > > > > as adding some > > > > > > ABI > > > > > > > > > > > > which doesn't do anything for many use cases > > > > > > > > > > > > without any way > > > > > > for the > > > > > > > > > > > > sysadmin to enable it. At the same time master > > > > > > > > > > > > sysfs knob at > > > > > > least > > > > > > > > > > > > enables the sysadmin to make a decision. Here I > > > > > > > > > > > > am thinking > > > > > > about a > > > > > > > > > > > > random client environment where not all > > > > > > > > > > > > userspace co- > > > > > > operates, > > > > > > > > > > > > but for > > > > > > > > > > > > instance user is running the feature aware > > > > > > > > > > > > media stack, and > > > > > > > > > > > > non-feature aware OpenCL/3d stack. > > > > > > > > > > > > > > > > > > > > > > > > I guess the complete story boils down to - is > > > > > > > > > > > > the master sysfs > > > > > > knob > > > > > > > > > > > > really a problem in container use cases. > > > > > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > > > > > > > Tvrtko > > > > > > > > > > > > > > > > > > > > > > Hey Tvrtko, > > > > > > > > > > > > > > > > > > > > > > Thanks for summarizing a bunch of discussions. > > > > > > > > > > > Essentially I agree with every you wrote above. > > > > > > > > > > > > > > > > > > > > > > If we have a global setting (determined by the OR > > > > > > > > > > > policy), what's > > > > > > the > > > > > > > > > > > point of per context settings? > > > > > > > > > > > > > > > > > > > > > > In Dmitry's scenario, all userspace applications > > > > > > > > > > > will work > > > > > > > > > > > together to > > > > > > > > > > > reach the consensus so it sounds like we're > > > > > > > > > > > reimplementing the > > > > > > policy > > > > > > > > > > > that is already existing in userspace. > > > > > > > > > > > > > > > > > > > > > > Anyway, I'm implementing Joonas' suggestion. > > > > > > > > > > > Hopefully > > > > > > somebody else > > > > > > > > > > > than me pick one or the other :) > > > > > > > > > > > > > > > > > > > > I'll just mention the voting/consensus approach to > > > > > > > > > > see if anyone > > > > > > else > > > > > > > > > > likes it. > > > > > > > > > > > > > > > > > > > > Each context has a CONTEXT_PARAM_HINT_SSEU { small, > > > > > > dontcare, large } > > > > > > > > > > (or some other abstract names). > > > > > > > > > > > > > > > > > > Yeah, the param name should have the word _HINT_ in > > > > > > > > > it when it's > > > > > > not a > > > > > > > > > definitive set. > > > > > > > > > > > > > > > > > > There's no global setter across containers, only a > > > > > > > > > scenario when > > > > > > > > > everyone agrees or not. Tallying up the votes and > > > > > > > > > going with a > > > > > > majority > > > > > > > > > vote might be an option, too. > > > > > > > > > > > > > > > > > > Regards, Joonas > > > > > > > > > > > > > > > > Trying to test the "everyone agrees" approach here. > > > > > > > > > > > > > > It's not everyone agrees, but the greater good. > > > > > > > > > > > > I'm looking forward to the definition of the greater good > > > > > > :) > > > > > > Tvrtko wanted to avoid the heuristic territory, it seems > > > > > > like we're just > > > > > > stepping into it. > > > > > > > > > > I am not sure that "small, dontcare, large" models brings > > > > > much. No one > > > > > would probably set "dontcare" since we have to default new > > > > > contexts > > > > to > > > > > large to be compatible. > > > > > > > > > > Don't know, I still prefer the master knob option. Honestly > > > > > don't yet > > > > > see the containers use case as a problem. There is always a > > > > > master > > > > > domain in any system where the knob can be enabled if the > > > > > customers > > > > on > > > > > the system are such to warrant it. On mixed systems enable it > > > > > or not > > > > > someone always suffers. And with the knob we are free to add > > > > heuristics > > > > > later, keep the uapi, and just default the knob to on. > > > > > > > > Master knob effectively means dead code behind a switch, that's > > > > not > > > > very > > > > upstream friendly. > > > > > > Hey at least I wasn't proposing a modparam! :))) > > > > > > Yes it is not the best software practice, upstream or not, > > > however I am > > > trying to be pragmatical here and choose the simplest, smallest, > > > good > > > enough, and least headache inducing in the future solution. > > > > > > One way of of looking at the master switch could be like tune > > > your > > > system for XYZ - change CPU frequency governor, disable SATA link > > > saving, allow i915 media optimized mode. Some live code out, some > > > dead > > > code in. > > > > > > But perhaps discussion is moot since we don't have userspace > > > anyway. > > > > > > Regards, > > > > > > Tvrtko > > > > I'm surprised by the "no deadcode behind knobs comment". We do this > > all > > the time - "display=0" anyone? Or "enable_cmdparser=false"? > > > > Allowing user space to reduce EU performance for the system as a > > whole > > is not a great idea imho. Only sysadmin should have the right to > > let that > > happen, and an admin switch (I WOULD go with a modparam personally) > > is > > a good way to ensure that we can get the optimal media > > configuration for > > dedicated media systems, while for general systems we get the best > > overall > > performance (not just favouring media). > > > > Why would we want to over engineer this? > > > > Jon > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Picking this point in the thread to reply on some points mentioned by folks in the whole thread.] I don't remember if any patches from Lionel's series actually had r-b's, but a few people including myself have certainly been reviewing them. If I had applied the final r-b I wouldn't have made much difference due lack of userspace and disagreement on the DoS mitigation story. So to say effectively put your money where your mouth is and review is not entirely fair. Suggestion to add a master sysfs switch was to alleviate the DoS concerns because dynamic switching has a cost towards all GPU clients, not that it just has potential to slow down media. Suggestion was also that this switch becomes immutable and defaults to "allow" on Gen11 onwards. I preferred sysfs versus a modparam since it makes testing (Both for developers and users (what config works better for my use case?) easier.) I am fine with the suggestion to drive the Gen11 part first, which removes the need for any of this. Patch series is already (AFAIR) nicely split so only the last patch needs to be dropped. If at some point we decide to revisit the Gen8/9 story, we can evaluate/measure whether any master switch is actually warranted. I think Lionel did some micro-benchmarking which showed impact is not so severe, so perhaps for real-world use cases it would be even less. I can re-read the public patches and review them, or perhaps even adopt them if they have been orphaned. Have to check with Francesco first before I commit to the latter. On the uAPI front interface looked fine to me. One recent interesting development are Mesa patches posted to mesa-dev (https://lists.freedesktop.org/archives/mesa-dev/2018-July/200607.html) which add EGL_CONTEXT_load_type extension (low/medium/high). This would need some sort of mapping between low/medium/high to more specific settings but still seems okay to me. This may bring another (open source?) user for the patches. Which Gen's they are interested in is also a question. Regards, Tvrtko On 24/07/2018 22:50, Bloomfield, Jon wrote: > Gratuitous top posting to re-kick the thread. > > For Gen11 we can't have an on/off switch anyway (media simply won't run > with an oncompatible slice config), so let's agree on an api to allow userland > to select the slice configuration for its contexts, for Gen11 only. I'd prefer a > simple {MediaConfig/GeneralConfig} type context param but I really don't > care that much. > > For gen9/10 it's arguable whether we need this at all. The effect on media > workloads varies, but I'm guessing normal users (outside a transcode > type environment) will never know they're missing anything. Either way, > we can continue discussing while we progress the gen11 solution. > > Jon > >> -----Original Message----- >> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of >> Bloomfield, Jon >> Sent: Wednesday, July 18, 2018 9:44 AM >> To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Joonas Lahtinen >> <joonas.lahtinen@linux.intel.com>; Chris Wilson <chris@chris-wilson.co.uk>; >> Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; intel- >> gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs entry to let users >> set sseu configs >> >>> -----Original Message----- >>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of >>> Tvrtko Ursulin >>> Sent: Thursday, June 14, 2018 1:29 AM >>> To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Chris Wilson >>> <chris@chris-wilson.co.uk>; Landwerlin, Lionel G >>> <lionel.g.landwerlin@intel.com>; intel-gfx@lists.freedesktop.org >>> Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs entry to let >> users >>> set sseu configs >>> >>> >>> On 13/06/2018 13:49, Joonas Lahtinen wrote: >>>> Quoting Tvrtko Ursulin (2018-06-12 15:02:07) >>>>> >>>>> On 12/06/2018 11:52, Lionel Landwerlin wrote: >>>>>> On 12/06/18 11:37, Chris Wilson wrote: >>>>>>> Quoting Lionel Landwerlin (2018-06-12 11:33:34) >>>>>>>> On 12/06/18 10:20, Joonas Lahtinen wrote: >>>>>>>>> Quoting Chris Wilson (2018-06-11 18:02:37) >>>>>>>>>> Quoting Lionel Landwerlin (2018-06-11 14:46:07) >>>>>>>>>>> On 11/06/18 13:10, Tvrtko Ursulin wrote: >>>>>>>>>>>> On 30/05/2018 15:33, Lionel Landwerlin wrote: >>>>>>>>>>>>> There are concerns about denial of service around the per >>>>>>>>>>>>> context sseu >>>>>>>>>>>>> configuration capability. In a previous commit introducing the >>>>>>>>>>>>> capability we allowed it only for capable users. This changes >>>>>>>>>>>>> adds a >>>>>>>>>>>>> new debugfs entry to let any user configure its own context >>>>>>>>>>>>> powergating setup. >>>>>>>>>>>> As far as I understood it, Joonas' concerns here are: >>>>>>>>>>>> >>>>>>>>>>>> 1) That in the containers use case individual containers >> wouldn't >>> be >>>>>>>>>>>> able to turn on the sysfs toggle for them. >>>>>>>>>>>> >>>>>>>>>>>> 2) That also in the containers use case if box admin turns on >> the >>>>>>>>>>>> feature, some containers would potentially start negatively >>>>>>>>>>>> affecting >>>>>>>>>>>> the others (via the accumulated cost of slice re-configuration >> on >>>>>>>>>>>> context switching). >>>>>>>>>>>> >>>>>>>>>>>> I am not familiar with typical container setups to be >> authoritative >>>>>>>>>>>> here, but intuitively I find it reasonable that a low-level >> hardware >>>>>>>>>>>> switch like this would be under the control of a master domain >>>>>>>>>>>> administrator. ("If you are installing our product in the >> container >>>>>>>>>>>> environment, make sure your system administrator enables >> this >>>>>>>>>>>> hardware >>>>>>>>>>>> feature.", "Note to system administrators: Enabling this >> features >>>>>>>>>>>> may >>>>>>>>>>>> negatively affect the performance of other containers.") >>>>>>>>>>>> >>>>>>>>>>>> Alternative proposal is for the i915 to apply an "or" filter on all >>>>>>>>>>>> requested masks and in that way ensure dynamic re- >>> configuration >>>>>>>>>>>> doesn't happen on context switches, but driven from >> userspace >>> via >>>>>>>>>>>> ioctls. >>>>>>>>>>>> >>>>>>>>>>>> In other words, should _all_ userspace agree between >>> themselves that >>>>>>>>>>>> they want to turn off a slice, they would then need to send out >> a >>>>>>>>>>>> concerted ioctl storm, where number of needed ioctls equals >>> the >>>>>>>>>>>> number >>>>>>>>>>>> of currently active contexts. (This may have its own >> performance >>>>>>>>>>>> consequences caused by the barriers needed to modify all >>> context >>>>>>>>>>>> images.) >>>>>>>>>>>> >>>>>>>>>>>> This was deemed acceptable the the media use case, but my >>> concern is >>>>>>>>>>>> the approach is not elegant and will tie us with the "or" policy >> in >>>>>>>>>>>> the ABI. (Performance concerns I haven't evaluated yet, but >>> they >>>>>>>>>>>> also >>>>>>>>>>>> may be significant.) >>>>>>>>>>>> >>>>>>>>>>>> If we go back thinking about the containers use case, then it >>>>>>>>>>>> transpires that even though the "or" policy does prevent one >>>>>>>>>>>> container >>>>>>>>>>>> from affecting the other from one angle, it also prevents one >>>>>>>>>>>> container from exercising the feature unless all containers >>>>>>>>>>>> co-operate. >>>>>>>>>>>> >>>>>>>>>>>> As such, we can view the original problem statement where >> we >>> have an >>>>>>>>>>>> issue if not everyone co-operates, as conceptually the same >> just >>>>>>>>>>>> from >>>>>>>>>>>> an opposite angle. (Rather than one container incurring the >>>>>>>>>>>> increased >>>>>>>>>>>> cost of context switches to the rest, we would have one >>> container >>>>>>>>>>>> preventing the optimized slice configuration to the other.) >>>>>>>>>>>> >>>>>>>>>>>> From this follows that both proposals require complete >>>>>>>>>>>> co-operation >>>>>>>>>>>> from all running userspace to avoid complete control of the >>> feature. >>>>>>>>>>>> >>>>>>>>>>>> Since the balance between the benefit of optimized slice >>>>>>>>>>>> configuration >>>>>>>>>>>> (or penalty of suboptimal one), versus the penalty of increased >>>>>>>>>>>> context switch times, cannot be know by the driver (barring >>>>>>>>>>>> venturing >>>>>>>>>>>> into the heuristics territory), that is another reason why I find >>>>>>>>>>>> the >>>>>>>>>>>> "or" policy in the driver questionable. >>>>>>>>>>>> >>>>>>>>>>>> We can also ask a question of - If we go with the "or" policy, >> why >>>>>>>>>>>> require N per-context ioctls to modify the global GPU >>> configuration >>>>>>>>>>>> and not instead add a global driver ioctl to modify the state? >>>>>>>>>>>> >>>>>>>>>>>> If a future hardware requires, or enables, the per-context >>> behaviour >>>>>>>>>>>> in a more efficient way, we could then revisit the problem >> space. >>>>>>>>>>>> >>>>>>>>>>>> In the mean time I see the "or" policy solution as adding some >>> ABI >>>>>>>>>>>> which doesn't do anything for many use cases without any way >>> for the >>>>>>>>>>>> sysadmin to enable it. At the same time master sysfs knob at >>> least >>>>>>>>>>>> enables the sysadmin to make a decision. Here I am thinking >>> about a >>>>>>>>>>>> random client environment where not all userspace co- >>> operates, >>>>>>>>>>>> but for >>>>>>>>>>>> instance user is running the feature aware media stack, and >>>>>>>>>>>> non-feature aware OpenCL/3d stack. >>>>>>>>>>>> >>>>>>>>>>>> I guess the complete story boils down to - is the master sysfs >>> knob >>>>>>>>>>>> really a problem in container use cases. >>>>>>>>>>>> >>>>>>>>>>>> Regards, >>>>>>>>>>>> >>>>>>>>>>>> Tvrtko >>>>>>>>>>> Hey Tvrtko, >>>>>>>>>>> >>>>>>>>>>> Thanks for summarizing a bunch of discussions. >>>>>>>>>>> Essentially I agree with every you wrote above. >>>>>>>>>>> >>>>>>>>>>> If we have a global setting (determined by the OR policy), what's >>> the >>>>>>>>>>> point of per context settings? >>>>>>>>>>> >>>>>>>>>>> In Dmitry's scenario, all userspace applications will work >>>>>>>>>>> together to >>>>>>>>>>> reach the consensus so it sounds like we're reimplementing the >>> policy >>>>>>>>>>> that is already existing in userspace. >>>>>>>>>>> >>>>>>>>>>> Anyway, I'm implementing Joonas' suggestion. Hopefully >>> somebody else >>>>>>>>>>> than me pick one or the other :) >>>>>>>>>> I'll just mention the voting/consensus approach to see if anyone >>> else >>>>>>>>>> likes it. >>>>>>>>>> >>>>>>>>>> Each context has a CONTEXT_PARAM_HINT_SSEU { small, >>> dontcare, large } >>>>>>>>>> (or some other abstract names). >>>>>>>>> Yeah, the param name should have the word _HINT_ in it when it's >>> not a >>>>>>>>> definitive set. >>>>>>>>> >>>>>>>>> There's no global setter across containers, only a scenario when >>>>>>>>> everyone agrees or not. Tallying up the votes and going with a >>> majority >>>>>>>>> vote might be an option, too. >>>>>>>>> >>>>>>>>> Regards, Joonas >>>>>>>> Trying to test the "everyone agrees" approach here. >>>>>>> It's not everyone agrees, but the greater good. >>>>>> >>>>>> I'm looking forward to the definition of the greater good :) >>>>>> Tvrtko wanted to avoid the heuristic territory, it seems like we're just >>>>>> stepping into it. >>>>> >>>>> I am not sure that "small, dontcare, large" models brings much. No one >>>>> would probably set "dontcare" since we have to default new contexts >> to >>>>> large to be compatible. >>>>> >>>>> Don't know, I still prefer the master knob option. Honestly don't yet >>>>> see the containers use case as a problem. There is always a master >>>>> domain in any system where the knob can be enabled if the customers >> on >>>>> the system are such to warrant it. On mixed systems enable it or not >>>>> someone always suffers. And with the knob we are free to add >> heuristics >>>>> later, keep the uapi, and just default the knob to on. >>>> >>>> Master knob effectively means dead code behind a switch, that's not >> very >>>> upstream friendly. >>> >>> Hey at least I wasn't proposing a modparam! :))) >>> >>> Yes it is not the best software practice, upstream or not, however I am >>> trying to be pragmatical here and choose the simplest, smallest, good >>> enough, and least headache inducing in the future solution. >>> >>> One way of of looking at the master switch could be like tune your >>> system for XYZ - change CPU frequency governor, disable SATA link >>> saving, allow i915 media optimized mode. Some live code out, some dead >>> code in. >>> >>> But perhaps discussion is moot since we don't have userspace anyway. >>> >>> Regards, >>> >>> Tvrtko >> >> I'm surprised by the "no deadcode behind knobs comment". We do this all >> the time - "display=0" anyone? Or "enable_cmdparser=false"? >> >> Allowing user space to reduce EU performance for the system as a whole >> is not a great idea imho. Only sysadmin should have the right to let that >> happen, and an admin switch (I WOULD go with a modparam personally) is >> a good way to ensure that we can get the optimal media configuration for >> dedicated media systems, while for general systems we get the best overall >> performance (not just favouring media). >> >> Why would we want to over engineer this? >> >> Jon >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2018-08-02 at 11:00 +0100, Tvrtko Ursulin wrote: > [Picking this point in the thread to reply on some points mentioned > by > folks in the whole thread.] > > I don't remember if any patches from Lionel's series actually had r- > b's, > but a few people including myself have certainly been reviewing them. > If > I had applied the final r-b I wouldn't have made much difference due > lack of userspace and disagreement on the DoS mitigation story. So > to > say effectively put your money where your mouth is and review is not > entirely fair. > > Suggestion to add a master sysfs switch was to alleviate the DoS > concerns because dynamic switching has a cost towards all GPU > clients, > not that it just has potential to slow down media. > > Suggestion was also that this switch becomes immutable and defaults > to > "allow" on Gen11 onwards. > > I preferred sysfs versus a modparam since it makes testing (Both for > developers and users (what config works better for my use case?) > easier.) > > I am fine with the suggestion to drive the Gen11 part first, which > removes the need for any of this. > > Patch series is already (AFAIR) nicely split so only the last patch > needs to be dropped. > > If at some point we decide to revisit the Gen8/9 story, we can > evaluate/measure whether any master switch is actually warranted. I > think Lionel did some micro-benchmarking which showed impact is not > so > severe, so perhaps for real-world use cases it would be even less. > > I can re-read the public patches and review them, or perhaps even > adopt > them if they have been orphaned. Have to check with Francesco first > before I commit to the latter. Thank you for volunteering:). I talked with Lionel about that and he thinks he may be able to do a respin next week. So, please, check with him also to avoid double work. > > On the uAPI front interface looked fine to me. > > One recent interesting development are Mesa patches posted to mesa- > dev > (https://lists.freedesktop.org/archives/mesa-dev/2018-July/200607.htm > l) > which add EGL_CONTEXT_load_type extension (low/medium/high). This > would > need some sort of mapping between low/medium/high to more specific > settings but still seems okay to me. > > This may bring another (open source?) user for the patches. Which > Gen's > they are interested in is also a question. > > Regards, > > Tvrtko > > On 24/07/2018 22:50, Bloomfield, Jon wrote: > > Gratuitous top posting to re-kick the thread. > > > > For Gen11 we can't have an on/off switch anyway (media simply won't > > run > > with an oncompatible slice config), so let's agree on an api to > > allow userland > > to select the slice configuration for its contexts, for Gen11 only. > > I'd prefer a > > simple {MediaConfig/GeneralConfig} type context param but I really > > don't > > care that much. > > > > For gen9/10 it's arguable whether we need this at all. The effect > > on media > > workloads varies, but I'm guessing normal users (outside a > > transcode > > type environment) will never know they're missing anything. Either > > way, > > we can continue discussing while we progress the gen11 solution. > > > > Jon > > > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On > > > Behalf Of > > > Bloomfield, Jon > > > Sent: Wednesday, July 18, 2018 9:44 AM > > > To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Joonas > > > Lahtinen > > > <joonas.lahtinen@linux.intel.com>; Chris Wilson <chris@chris-wils > > > on.co.uk>; > > > Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; intel- > > > gfx@lists.freedesktop.org > > > Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs > > > entry to let users > > > set sseu configs > > > > > > > -----Original Message----- > > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On > > > > Behalf Of > > > > Tvrtko Ursulin > > > > Sent: Thursday, June 14, 2018 1:29 AM > > > > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Chris > > > > Wilson > > > > <chris@chris-wilson.co.uk>; Landwerlin, Lionel G > > > > <lionel.g.landwerlin@intel.com>; intel-gfx@lists.freedesktop.or > > > > g > > > > Subject: Re: [Intel-gfx] [PATCH v9 7/7] drm/i915: add a sysfs > > > > entry to let > > > > > > users > > > > set sseu configs > > > > > > > > > > > > On 13/06/2018 13:49, Joonas Lahtinen wrote: > > > > > Quoting Tvrtko Ursulin (2018-06-12 15:02:07) > > > > > > > > > > > > On 12/06/2018 11:52, Lionel Landwerlin wrote: > > > > > > > On 12/06/18 11:37, Chris Wilson wrote: > > > > > > > > Quoting Lionel Landwerlin (2018-06-12 11:33:34) > > > > > > > > > On 12/06/18 10:20, Joonas Lahtinen wrote: > > > > > > > > > > Quoting Chris Wilson (2018-06-11 18:02:37) > > > > > > > > > > > Quoting Lionel Landwerlin (2018-06-11 14:46:07) > > > > > > > > > > > > On 11/06/18 13:10, Tvrtko Ursulin wrote: > > > > > > > > > > > > > On 30/05/2018 15:33, Lionel Landwerlin wrote: > > > > > > > > > > > > > > There are concerns about denial of service > > > > > > > > > > > > > > around the per > > > > > > > > > > > > > > context sseu > > > > > > > > > > > > > > configuration capability. In a previous > > > > > > > > > > > > > > commit introducing the > > > > > > > > > > > > > > capability we allowed it only for capable > > > > > > > > > > > > > > users. This changes > > > > > > > > > > > > > > adds a > > > > > > > > > > > > > > new debugfs entry to let any user configure > > > > > > > > > > > > > > its own context > > > > > > > > > > > > > > powergating setup. > > > > > > > > > > > > > > > > > > > > > > > > > > As far as I understood it, Joonas' concerns > > > > > > > > > > > > > here are: > > > > > > > > > > > > > > > > > > > > > > > > > > 1) That in the containers use case individual > > > > > > > > > > > > > containers > > > > > > wouldn't > > > > be > > > > > > > > > > > > > able to turn on the sysfs toggle for them. > > > > > > > > > > > > > > > > > > > > > > > > > > 2) That also in the containers use case if > > > > > > > > > > > > > box admin turns on > > > > > > the > > > > > > > > > > > > > feature, some containers would potentially > > > > > > > > > > > > > start negatively > > > > > > > > > > > > > affecting > > > > > > > > > > > > > the others (via the accumulated cost of slice > > > > > > > > > > > > > re-configuration > > > > > > on > > > > > > > > > > > > > context switching). > > > > > > > > > > > > > > > > > > > > > > > > > > I am not familiar with typical container > > > > > > > > > > > > > setups to be > > > > > > authoritative > > > > > > > > > > > > > here, but intuitively I find it reasonable > > > > > > > > > > > > > that a low-level > > > > > > hardware > > > > > > > > > > > > > switch like this would be under the control > > > > > > > > > > > > > of a master domain > > > > > > > > > > > > > administrator. ("If you are installing our > > > > > > > > > > > > > product in the > > > > > > container > > > > > > > > > > > > > environment, make sure your system > > > > > > > > > > > > > administrator enables > > > > > > this > > > > > > > > > > > > > hardware > > > > > > > > > > > > > feature.", "Note to system administrators: > > > > > > > > > > > > > Enabling this > > > > > > features > > > > > > > > > > > > > may > > > > > > > > > > > > > negatively affect the performance of other > > > > > > > > > > > > > containers.") > > > > > > > > > > > > > > > > > > > > > > > > > > Alternative proposal is for the i915 to apply > > > > > > > > > > > > > an "or" filter on all > > > > > > > > > > > > > requested masks and in that way ensure > > > > > > > > > > > > > dynamic re- > > > > > > > > configuration > > > > > > > > > > > > > doesn't happen on context switches, but > > > > > > > > > > > > > driven from > > > > > > userspace > > > > via > > > > > > > > > > > > > ioctls. > > > > > > > > > > > > > > > > > > > > > > > > > > In other words, should _all_ userspace agree > > > > > > > > > > > > > between > > > > > > > > themselves that > > > > > > > > > > > > > they want to turn off a slice, they would > > > > > > > > > > > > > then need to send out > > > > > > a > > > > > > > > > > > > > concerted ioctl storm, where number of needed > > > > > > > > > > > > > ioctls equals > > > > > > > > the > > > > > > > > > > > > > number > > > > > > > > > > > > > of currently active contexts. (This may have > > > > > > > > > > > > > its own > > > > > > performance > > > > > > > > > > > > > consequences caused by the barriers needed to > > > > > > > > > > > > > modify all > > > > > > > > context > > > > > > > > > > > > > images.) > > > > > > > > > > > > > > > > > > > > > > > > > > This was deemed acceptable the the media use > > > > > > > > > > > > > case, but my > > > > > > > > concern is > > > > > > > > > > > > > the approach is not elegant and will tie us > > > > > > > > > > > > > with the "or" policy > > > > > > in > > > > > > > > > > > > > the ABI. (Performance concerns I haven't > > > > > > > > > > > > > evaluated yet, but > > > > > > > > they > > > > > > > > > > > > > also > > > > > > > > > > > > > may be significant.) > > > > > > > > > > > > > > > > > > > > > > > > > > If we go back thinking about the containers > > > > > > > > > > > > > use case, then it > > > > > > > > > > > > > transpires that even though the "or" policy > > > > > > > > > > > > > does prevent one > > > > > > > > > > > > > container > > > > > > > > > > > > > from affecting the other from one angle, it > > > > > > > > > > > > > also prevents one > > > > > > > > > > > > > container from exercising the feature unless > > > > > > > > > > > > > all containers > > > > > > > > > > > > > co-operate. > > > > > > > > > > > > > > > > > > > > > > > > > > As such, we can view the original problem > > > > > > > > > > > > > statement where > > > > > > we > > > > have an > > > > > > > > > > > > > issue if not everyone co-operates, as > > > > > > > > > > > > > conceptually the same > > > > > > just > > > > > > > > > > > > > from > > > > > > > > > > > > > an opposite angle. (Rather than one container > > > > > > > > > > > > > incurring the > > > > > > > > > > > > > increased > > > > > > > > > > > > > cost of context switches to the rest, we > > > > > > > > > > > > > would have one > > > > > > > > container > > > > > > > > > > > > > preventing the optimized slice configuration > > > > > > > > > > > > > to the other.) > > > > > > > > > > > > > > > > > > > > > > > > > > From this follows that both proposals > > > > > > > > > > > > > require complete > > > > > > > > > > > > > co-operation > > > > > > > > > > > > > from all running userspace to avoid complete > > > > > > > > > > > > > control of the > > > > > > > > feature. > > > > > > > > > > > > > > > > > > > > > > > > > > Since the balance between the benefit of > > > > > > > > > > > > > optimized slice > > > > > > > > > > > > > configuration > > > > > > > > > > > > > (or penalty of suboptimal one), versus the > > > > > > > > > > > > > penalty of increased > > > > > > > > > > > > > context switch times, cannot be know by the > > > > > > > > > > > > > driver (barring > > > > > > > > > > > > > venturing > > > > > > > > > > > > > into the heuristics territory), that is > > > > > > > > > > > > > another reason why I find > > > > > > > > > > > > > the > > > > > > > > > > > > > "or" policy in the driver questionable. > > > > > > > > > > > > > > > > > > > > > > > > > > We can also ask a question of - If we go with > > > > > > > > > > > > > the "or" policy, > > > > > > why > > > > > > > > > > > > > require N per-context ioctls to modify the > > > > > > > > > > > > > global GPU > > > > > > > > configuration > > > > > > > > > > > > > and not instead add a global driver ioctl to > > > > > > > > > > > > > modify the state? > > > > > > > > > > > > > > > > > > > > > > > > > > If a future hardware requires, or enables, > > > > > > > > > > > > > the per-context > > > > > > > > behaviour > > > > > > > > > > > > > in a more efficient way, we could then > > > > > > > > > > > > > revisit the problem > > > > > > space. > > > > > > > > > > > > > > > > > > > > > > > > > > In the mean time I see the "or" policy > > > > > > > > > > > > > solution as adding some > > > > > > > > ABI > > > > > > > > > > > > > which doesn't do anything for many use cases > > > > > > > > > > > > > without any way > > > > > > > > for the > > > > > > > > > > > > > sysadmin to enable it. At the same time > > > > > > > > > > > > > master sysfs knob at > > > > > > > > least > > > > > > > > > > > > > enables the sysadmin to make a decision. Here > > > > > > > > > > > > > I am thinking > > > > > > > > about a > > > > > > > > > > > > > random client environment where not all > > > > > > > > > > > > > userspace co- > > > > > > > > operates, > > > > > > > > > > > > > but for > > > > > > > > > > > > > instance user is running the feature aware > > > > > > > > > > > > > media stack, and > > > > > > > > > > > > > non-feature aware OpenCL/3d stack. > > > > > > > > > > > > > > > > > > > > > > > > > > I guess the complete story boils down to - is > > > > > > > > > > > > > the master sysfs > > > > > > > > knob > > > > > > > > > > > > > really a problem in container use cases. > > > > > > > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > > > > > > > > > Tvrtko > > > > > > > > > > > > > > > > > > > > > > > > Hey Tvrtko, > > > > > > > > > > > > > > > > > > > > > > > > Thanks for summarizing a bunch of discussions. > > > > > > > > > > > > Essentially I agree with every you wrote above. > > > > > > > > > > > > > > > > > > > > > > > > If we have a global setting (determined by the > > > > > > > > > > > > OR policy), what's > > > > > > > > the > > > > > > > > > > > > point of per context settings? > > > > > > > > > > > > > > > > > > > > > > > > In Dmitry's scenario, all userspace > > > > > > > > > > > > applications will work > > > > > > > > > > > > together to > > > > > > > > > > > > reach the consensus so it sounds like we're > > > > > > > > > > > > reimplementing the > > > > > > > > policy > > > > > > > > > > > > that is already existing in userspace. > > > > > > > > > > > > > > > > > > > > > > > > Anyway, I'm implementing Joonas' suggestion. > > > > > > > > > > > > Hopefully > > > > > > > > somebody else > > > > > > > > > > > > than me pick one or the other :) > > > > > > > > > > > > > > > > > > > > > > I'll just mention the voting/consensus approach > > > > > > > > > > > to see if anyone > > > > > > > > else > > > > > > > > > > > likes it. > > > > > > > > > > > > > > > > > > > > > > Each context has a CONTEXT_PARAM_HINT_SSEU { > > > > > > > > > > > small, > > > > > > > > dontcare, large } > > > > > > > > > > > (or some other abstract names). > > > > > > > > > > > > > > > > > > > > Yeah, the param name should have the word _HINT_ in > > > > > > > > > > it when it's > > > > > > > > not a > > > > > > > > > > definitive set. > > > > > > > > > > > > > > > > > > > > There's no global setter across containers, only a > > > > > > > > > > scenario when > > > > > > > > > > everyone agrees or not. Tallying up the votes and > > > > > > > > > > going with a > > > > > > > > majority > > > > > > > > > > vote might be an option, too. > > > > > > > > > > > > > > > > > > > > Regards, Joonas > > > > > > > > > > > > > > > > > > Trying to test the "everyone agrees" approach here. > > > > > > > > > > > > > > > > It's not everyone agrees, but the greater good. > > > > > > > > > > > > > > I'm looking forward to the definition of the greater good > > > > > > > :) > > > > > > > Tvrtko wanted to avoid the heuristic territory, it seems > > > > > > > like we're just > > > > > > > stepping into it. > > > > > > > > > > > > I am not sure that "small, dontcare, large" models brings > > > > > > much. No one > > > > > > would probably set "dontcare" since we have to default new > > > > > > contexts > > > > > > to > > > > > > large to be compatible. > > > > > > > > > > > > Don't know, I still prefer the master knob option. Honestly > > > > > > don't yet > > > > > > see the containers use case as a problem. There is always a > > > > > > master > > > > > > domain in any system where the knob can be enabled if the > > > > > > customers > > > > > > on > > > > > > the system are such to warrant it. On mixed systems enable > > > > > > it or not > > > > > > someone always suffers. And with the knob we are free to > > > > > > add > > > > > > heuristics > > > > > > later, keep the uapi, and just default the knob to on. > > > > > > > > > > Master knob effectively means dead code behind a switch, > > > > > that's not > > > > > > very > > > > > upstream friendly. > > > > > > > > Hey at least I wasn't proposing a modparam! :))) > > > > > > > > Yes it is not the best software practice, upstream or not, > > > > however I am > > > > trying to be pragmatical here and choose the simplest, > > > > smallest, good > > > > enough, and least headache inducing in the future solution. > > > > > > > > One way of of looking at the master switch could be like tune > > > > your > > > > system for XYZ - change CPU frequency governor, disable SATA > > > > link > > > > saving, allow i915 media optimized mode. Some live code out, > > > > some dead > > > > code in. > > > > > > > > But perhaps discussion is moot since we don't have userspace > > > > anyway. > > > > > > > > Regards, > > > > > > > > Tvrtko > > > > > > I'm surprised by the "no deadcode behind knobs comment". We do > > > this all > > > the time - "display=0" anyone? Or "enable_cmdparser=false"? > > > > > > Allowing user space to reduce EU performance for the system as a > > > whole > > > is not a great idea imho. Only sysadmin should have the right to > > > let that > > > happen, and an admin switch (I WOULD go with a modparam > > > personally) is > > > a good way to ensure that we can get the optimal media > > > configuration for > > > dedicated media systems, while for general systems we get the > > > best overall > > > performance (not just favouring media). > > > > > > Why would we want to over engineer this? > > > > > > Jon > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b2386c37437d..5aa96a6650b4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1842,6 +1842,8 @@ struct drm_i915_private { struct ida hw_ida; #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */ #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */ + + bool dynamic_sseu; } contexts; u32 fdi_rx_config; @@ -3259,6 +3261,10 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id) return ctx; } +int i915_gem_contexts_set_dynamic_sseu(struct drm_i915_private *i915, + bool allowed); +bool i915_gem_contexts_get_dynamic_sseu(struct drm_i915_private *i915); + int i915_perf_open_ioctl(struct drm_device *dev, void *data, struct drm_file *file); int i915_perf_add_config_ioctl(struct drm_device *dev, void *data, @@ -3859,11 +3865,13 @@ intel_engine_prepare_sseu(struct intel_engine_cs *engine, struct drm_i915_private *i915 = engine->i915; /* - * If i915/perf is active, we want a stable powergating configuration - * on the system. The most natural configuration to take in that case - * is the default (i.e maximum the hardware can do). + * If i915/perf is active or dynamic sseu configuration is not allowed + * (through sysfs), we want a stable powergating configuration on the + * system. The most natural configuration to take in that case is the + * default (i.e maximum the hardware can do). */ - return i915->perf.oa.exclusive_stream ? + return (i915->perf.oa.exclusive_stream || + !i915->contexts.dynamic_sseu) ? intel_device_default_sseu(i915) : sseu; } diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 79029815c21f..453bdc976be3 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -1118,6 +1118,44 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, return ret; } +int i915_gem_contexts_set_dynamic_sseu(struct drm_i915_private *i915, + bool allowed) +{ + struct intel_engine_cs *engine = i915->engine[RCS]; + struct i915_gem_context *ctx; + int ret = 0; + + lockdep_assert_held(&i915->drm.struct_mutex); + + if (!engine->emit_rpcs_config) + return -ENODEV; + + if (allowed == i915->contexts.dynamic_sseu) + return 0; + + i915->contexts.dynamic_sseu = allowed; + + list_for_each_entry(ctx, &i915->contexts.list, link) { + struct intel_context *ce = to_intel_context(ctx, engine); + + ret = i915_gem_context_reconfigure_sseu(ctx, engine, ce->sseu); + if (ret) + break; + } + + return ret; +} + +bool i915_gem_contexts_get_dynamic_sseu(struct drm_i915_private *i915) +{ + struct intel_engine_cs *engine = i915->engine[RCS]; + + if (!engine->emit_rpcs_config) + return false; + + return i915->contexts.dynamic_sseu; +} + #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftests/mock_context.c" #include "selftests/i915_gem_context.c" diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index e5e6f6bb2b05..d895054d245e 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -483,6 +483,44 @@ static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr return snprintf(buf, PAGE_SIZE, "%d\n", val); } +static ssize_t allow_dynamic_sseu_show(struct device *kdev, + struct device_attribute *attr, + char *buf) +{ + struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); + bool value = i915_gem_contexts_get_dynamic_sseu(i915); + + return snprintf(buf, PAGE_SIZE, "%d\n", value); +} + +static ssize_t allow_dynamic_sseu_store(struct device *kdev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct drm_i915_private *i915 = kdev_minor_to_i915(kdev); + ssize_t ret; + u32 val; + + ret = kstrtou32(buf, 0, &val); + if (ret) + return ret; + + if (val != 0 && val != 1) + return -EINVAL; + + ret = mutex_lock_interruptible(&i915->drm.struct_mutex); + if (ret) + return ret; + + ret = i915_gem_contexts_set_dynamic_sseu(i915, val != 0); + + mutex_unlock(&i915->drm.struct_mutex); + + return ret ?: count; +} + +static DEVICE_ATTR_RW(allow_dynamic_sseu); + static const struct attribute *gen6_attrs[] = { &dev_attr_gt_act_freq_mhz.attr, &dev_attr_gt_cur_freq_mhz.attr, @@ -492,6 +530,7 @@ static const struct attribute *gen6_attrs[] = { &dev_attr_gt_RP0_freq_mhz.attr, &dev_attr_gt_RP1_freq_mhz.attr, &dev_attr_gt_RPn_freq_mhz.attr, + &dev_attr_allow_dynamic_sseu.attr, NULL, }; @@ -505,6 +544,7 @@ static const struct attribute *vlv_attrs[] = { &dev_attr_gt_RP1_freq_mhz.attr, &dev_attr_gt_RPn_freq_mhz.attr, &dev_attr_vlv_rpe_freq_mhz.attr, + &dev_attr_allow_dynamic_sseu.attr, NULL, };
There are concerns about denial of service around the per context sseu configuration capability. In a previous commit introducing the capability we allowed it only for capable users. This changes adds a new debugfs entry to let any user configure its own context powergating setup. v2: Rename sysfs entry (Tvrtko) Lock interruptible the device in sysfs (Tvrtko) Fix dropped error code in getting dynamic sseu value (Tvrtko) s/dev_priv/i915/ (Tvrtko) v3: Drop locked function suffix (Tvrtko) Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 16 +++++++--- drivers/gpu/drm/i915/i915_gem_context.c | 38 +++++++++++++++++++++++ drivers/gpu/drm/i915/i915_sysfs.c | 40 +++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 4 deletions(-)