Re: [PATCH v9 7/7] drm/i915: add a sysfs entry to let users set sseu configs

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>   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,
>   };
>   
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx