On 23/05/2018 18:33, Lionel Landwerlin wrote:
On 23/05/18 16:30, Tvrtko Ursulin wrote:
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(-)
[snip]
} 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.
Okay, let's get Dmitry input on this.
To expand, my thinking is to let media stack configure its contexts for
fewer slices, but let the user/sysadmin decide whether 3d/compute
performance is more important, or media.
Downside is that with this approach you would have to re-configure all
contexts when sysfs toggle is changed from zero to one.
[snip]
+
+ /*
+ * 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.
Yeah, I'm not sure whether I can read a boolean or whether it should be
an atomic...
You can just read a boolean.
+ 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?
Duh! Well spotted.
I think I confused set and get while reviewing.
It is probably fine to read back zero when the feature is not supported
and just fail the write. But could ENODEV here as well, my opinion is
not strong.
+}
+
+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.)
Which one is it? :) i915 or dev_priv?
i915 unless I915_READ/WRITE are used in the function. :(
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
