On 22/05/2018 19:00, 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.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 5 +++
drivers/gpu/drm/i915/i915_gem_context.c | 52 ++++++++++++++++++++++++-
drivers/gpu/drm/i915/i915_sysfs.c | 30 ++++++++++++++
3 files changed, 86 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 09cfcfe1c339..0fccec29fdda 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1843,6 +1843,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 allow_sseu;
I think spelling out "dynamic" in patch titles, commit messages and even
in this variable name would be better and clearer.
} contexts;
u32 fdi_rx_config;
@@ -3274,6 +3276,9 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id)
return ctx;
}
+int i915_gem_contexts_set_allow_sseu(struct drm_i915_private *dev_priv, bool allowed);
+bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private *dev_priv);
+
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,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5c5a12f1c265..815a9d1c29f3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -981,7 +981,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
break;
}
- if (!capable(CAP_SYS_ADMIN)) {
+ if (!dev_priv->contexts.allow_sseu &&
+ !capable(CAP_SYS_ADMIN)) {
So the thing I mentioned in the previous patch. My thinking was not to
fail it if sysfs toggle is disabled, but just don't do dynamic
switching. That way the software stack works in all cases but user has
the option of tuning his system.
I mean it can still be done but in userspace. Set param fails, userspace
goes for a fall back.
Only difference is I guess whether it is useful to allow switching at
runtime. I am imagining running some 3d and media, and then toggling the
knob to see what happens to power and performance on the fly. But maybe
that is not so interesting.
Along the same lines I was thinking CAP_SYS_ADMIN limitation could be
dropped.
Both points are for a wider discussion I guess.
ret = -EPERM;
break;
}
@@ -1058,6 +1059,55 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
return ret;
}
+int i915_gem_contexts_set_allow_sseu(struct drm_i915_private *dev_priv,
+ bool allowed)
+{
+ struct intel_engine_cs *engine = dev_priv->engine[RCS];
+ int ret = 0;
+
+ if (!engine->emit_rpcs_config)
+ return -ENODEV;
+
+ mutex_lock(&dev_priv->drm.struct_mutex);
This should be interruptible so user can Ctrl-C if it is stuck.
+
+ /*
+ * When we allow each context to configure its powergating
+ * configuration, there is no need to put the configurations back to
+ * the default, it should already be the case.
+ */
+ if (!allowed) {
+ union intel_sseu default_sseu =
+ intel_sseu_from_device_sseu(&INTEL_INFO(dev_priv)->sseu);
+ struct i915_gem_context *ctx;
+
+ list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
+ ret = i915_gem_context_reconfigure_sseu(ctx, engine,
+ default_sseu);
+ if (ret)
+ break;
+ }
+ }
+
+ dev_priv->contexts.allow_sseu = allowed;
+
+ mutex_unlock(&dev_priv->drm.struct_mutex);
+ return ret;
+}
+
+bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private *dev_priv)
+{
+ struct intel_engine_cs *engine = dev_priv->engine[RCS];
+ bool ret;
+
+ if (!engine->emit_rpcs_config)
+ return false;
+
+ mutex_lock(&dev_priv->drm.struct_mutex);
+ ret = dev_priv->contexts.allow_sseu;
+ mutex_unlock(&dev_priv->drm.struct_mutex);
I guess this mutex does nothing in this case.
+ return ret;
+}
+
#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..9fd15b138ac9 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -483,6 +483,34 @@ 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 gem_allow_sseu_show(struct device *kdev,
+ struct device_attribute *attr, char *buf)
+{
+ struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
+ int ret = i915_gem_contexts_get_allow_sseu(dev_priv);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", ret);
Propagate ENODEV all the way by making i915_gem_contexts_get_allow_sseu
return an int?
+}
+
+static ssize_t gem_allow_sseu_store(struct device *kdev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev);
+ u32 val;
+ ssize_t ret;
Chris will complain about Christmas trees. :)
(And possible about new use of dev_priv throughout the series but that
battle is in stale mate.)
+
+ ret = kstrtou32(buf, 0, &val);
+ if (ret)
+ return ret;
+
+ i915_gem_contexts_set_allow_sseu(dev_priv, val != 0);
I would enforce 0 or 1 to allow (hypothetical) future expansion with
more values.
+
+ return ret ?: count;
+}
+
+static DEVICE_ATTR_RW(gem_allow_sseu);
I would like to open up a round of bike-shedding of the knob name. :)
How about allow_dynamic_sseu? gem_ or not, don't know.
+
static const struct attribute *gen6_attrs[] = {
&dev_attr_gt_act_freq_mhz.attr,
&dev_attr_gt_cur_freq_mhz.attr,
@@ -492,6 +520,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_gem_allow_sseu.attr,
NULL,
};
@@ -505,6 +534,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_gem_allow_sseu.attr,
NULL,
};
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
