Re: [PATCH 2/2] drm/i915: Implement I915_PERF_ADD/REMOVE_CONFIG interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 13/07/17 15:42, Matthew Auld wrote:
On 13 July 2017 at 12:22, Lionel Landwerlin
<lionel.g.landwerlin@xxxxxxxxx> wrote:
From: Matthew Auld <matthew.auld@xxxxxxxxx>

The motivation behind this new interface is expose at runtime the
creation of new OA configs which can be used as part of the i915 perf
open interface. This will enable the kernel to learn new configs which
may be experimental, or otherwise not part of the core set currently
available through the i915 perf interface.

This will relieve the kernel from holding all the possible configs, so
we can leave only the test configs here.

Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
Signed-off-by: Andrzej Datczuk <andrzej.datczuk@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_drv.c  |   2 +
  drivers/gpu/drm/i915/i915_drv.h  |  25 +++
  drivers/gpu/drm/i915/i915_perf.c | 351 ++++++++++++++++++++++++++++++++++++++-
  drivers/gpu/drm/i915/i915_reg.h  |   2 +
  include/uapi/drm/i915_drm.h      |  24 +++
  5 files changed, 403 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d310d8245dca..a3d339670ec1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2730,6 +2730,8 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
         DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
         DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
         DRM_IOCTL_DEF_DRV(I915_PERF_OPEN, i915_perf_open_ioctl, DRM_RENDER_ALLOW),
+       DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
+       DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
  };

  static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0c45a89d165e..3d62276d9fad 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2037,6 +2037,25 @@ struct i915_perf_stream {
   */
  struct i915_oa_ops {
         /**
+        * @is_valid_b_counter_reg: Validates register's address for
+        * programming boolean counters for a particular platform.
+        */
+       bool (*is_valid_b_counter_reg)(struct drm_i915_private *dev_priv,
+                                      u32 addr);
+
+       /**
+        * @is_valid_mux_reg: Validates register's address for programming mux
+        * for a particular platform.
+        */
+       bool (*is_valid_mux_reg)(struct drm_i915_private *dev_priv, u32 addr);
+
+       /**
+        * @is_valid_flex_reg: Validates register's address for programming
+        * flex EU filtering for a particular platform.
+        */
+       bool (*is_valid_flex_reg)(struct drm_i915_private *dev_priv, u32 addr);
+
+       /**
          * @init_oa_buffer: Resets the head and tail pointers of the
          * circular buffer for periodic OA reports.
          *
@@ -2427,6 +2446,8 @@ struct drm_i915_private {
                 struct mutex lock;
                 struct list_head streams;

+               struct idr metrics_idr;
+
                 struct {
                         struct i915_perf_stream *exclusive_stream;

@@ -3595,6 +3616,10 @@ i915_gem_context_lookup_timeline(struct i915_gem_context *ctx,

  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,
+                              struct drm_file *file);
+int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
+                                 struct drm_file *file);
  void i915_oa_init_reg_state(struct intel_engine_cs *engine,
                             struct i915_gem_context *ctx,
                             uint32_t *reg_state);
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index d4647b3d8119..5f3d544a93cf 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2652,7 +2652,7 @@ static struct i915_oa_config *get_oa_config(struct drm_i915_private *dev_priv,
         if (metrics_set == 1)
                 return &dev_priv->perf.oa.test_config;

-       return NULL;
+       return idr_find(&dev_priv->perf.metrics_idr, metrics_set);
  }

  /**
@@ -2913,6 +2913,7 @@ void i915_perf_register(struct drm_i915_private *dev_priv)
                                  &dev_priv->perf.oa.test_config.sysfs_metric);
         if (ret)
                 goto sysfs_error;
+
         goto exit;

  sysfs_error:
@@ -2944,6 +2945,333 @@ void i915_perf_unregister(struct drm_i915_private *dev_priv)
         dev_priv->perf.metrics_kobj = NULL;
  }

+static bool gen8_is_valid_flex_addr(struct drm_i915_private *dev_priv, u32 addr)
+{
+       static const i915_reg_t flex_eu_regs[] = {
+               EU_PERF_CNTL0,
+               EU_PERF_CNTL1,
+               EU_PERF_CNTL2,
+               EU_PERF_CNTL3,
+               EU_PERF_CNTL4,
+               EU_PERF_CNTL5,
+               EU_PERF_CNTL6,
+       };
+       int i;
+
+       for (i = 0; i < ARRAY_SIZE(flex_eu_regs); i++) {
+               if (flex_eu_regs[i].reg == addr)
+                       return true;
+       }
+       return false;
+}
+
+static bool gen7_is_valid_b_counter_addr(struct drm_i915_private *dev_priv, u32 addr)
+{
+       return (addr >= 0x2380 && addr <= 0x27ac);
+}
+
+static bool gen7_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
+{
+       return addr == NOA_WRITE.reg ||
+               (addr >= 0xd0c && addr <= 0xd3c) ||
+               (addr >= 0x25100 && addr <= 0x2FB9C);
+}
+
+static bool hsw_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
+{
+       return (addr >= 0x25100 && addr <= 0x2FF90) ||
+               gen7_is_valid_mux_addr(dev_priv, addr);
+}
+
+static bool chv_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
+{
+       return (addr >= 0x182300 && addr <= 0x1823A4) ||
+               gen7_is_valid_mux_addr(dev_priv, addr);
+}
+
+static struct i915_oa_reg *alloc_oa_regs(struct drm_i915_private *dev_priv,
+                                        bool (*is_valid)(struct drm_i915_private *dev_priv, u32 addr),
+                                        u32 __user *regs,
+                                        u32 n_regs)
+{
+       struct i915_oa_reg *oa_regs;
+       int err, i;
+
+       if (!n_regs)
+               return NULL;
+
+       /* No is_valid function means we're not allowing any register to be programmed. */
+       GEM_BUG_ON(!is_valid);
+       if (!is_valid)
+               return ERR_PTR(-EINVAL);
+
+       oa_regs = kmalloc(sizeof(*oa_regs) * n_regs, GFP_KERNEL);
+       if (!oa_regs)
+               return ERR_PTR(-ENOMEM);
+
+       for (i = 0; i < n_regs; i++) {
I think in a bunch of places we mix u32/int for n_regs, make it
consistent now that user-space can set this?

Done.


+               u32 addr, value;
+
+               err = get_user(addr, regs);
+               if (err)
+                       goto addr_err;
+
+               if (!is_valid(dev_priv, addr)) {
+                       DRM_DEBUG("Invalid oa_reg address: %X\n", addr);
+                       err = -EINVAL;
+                       goto addr_err;
+               }
+
+               err = get_user(value, regs + 1);
+               if (err)
+                       goto addr_err;
+
+               oa_regs[i].addr = _MMIO(addr);
+               oa_regs[i].value = value;
+
+               regs += 2;
+       }
+
+       return oa_regs;
+
+addr_err:
+       kfree(oa_regs);
+       return ERR_PTR(err);
+}
+
+static ssize_t show_dynamic_id(struct device *dev,
+                              struct device_attribute *attr,
+                              char *buf)
+{
+       struct i915_oa_config *oa_config =
+               container_of(attr, typeof(*oa_config), sysfs_metric_id);
+
+       return sprintf(buf, "%d\n", oa_config->id);
+}
+
+static int create_dynamic_oa_sysfs_entry(struct drm_i915_private *dev_priv,
+                                        struct i915_oa_config *oa_config)
+{
+       oa_config->sysfs_metric_id.attr.name = "id";
+       oa_config->sysfs_metric_id.attr.mode = S_IRUGO;
+       oa_config->sysfs_metric_id.show = show_dynamic_id;
+       oa_config->sysfs_metric_id.store = NULL;
+
+       oa_config->attrs[0] = &oa_config->sysfs_metric_id.attr;
+       oa_config->attrs[1] = NULL;
+
+       oa_config->sysfs_metric.name = oa_config->uuid;
+       oa_config->sysfs_metric.attrs = oa_config->attrs;
+
+       return sysfs_create_group(dev_priv->perf.metrics_kobj,
+                                 &oa_config->sysfs_metric);
+}
+
+static int destroy_config(int id, void *p, void *data)
+{
+       struct drm_i915_private *dev_priv = data;
+       struct i915_oa_config *oa_config = p;
+
+       sysfs_remove_group(dev_priv->perf.metrics_kobj,
+                          &oa_config->sysfs_metric);
+       kfree(oa_config->flex_regs);
+       kfree(oa_config->b_counter_regs);
+       kfree(oa_config->mux_regs);
+       kfree(oa_config);
+
+       return 0;
+}
+
+int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
+                              struct drm_file *file)
+{
+       struct drm_i915_private *dev_priv = dev->dev_private;
+       struct drm_i915_perf_oa_config *args = data;
+       struct i915_oa_config *oa_config, *tmp;
+       unsigned int x1, x2, x3, x4, x5;
+       int err, id;
+
+       if (!dev_priv->perf.initialized) {
+               DRM_DEBUG("i915 perf interface not available for this system\n");
+               return -ENOTSUPP;
+       }
+
+       if (!dev_priv->perf.metrics_kobj) {
+               DRM_DEBUG("OA metrics weren't advertised via sysfs\n");
+               return -EINVAL;
+       }
+
+       if (!capable(CAP_SYS_ADMIN)) {
+               DRM_DEBUG("Insufficient privileges to add i915 OA config\n");
+               return -EACCES;
+       }
+
+       if ((!args->mux_regs || !args->n_mux_regs) &&
+           (!args->boolean_regs || !args->n_boolean_regs) &&
+           (!args->flex_regs || !args->n_flex_regs)) {
+               DRM_DEBUG("No OA registers given\n");
+               return -EINVAL;
+       }
+
+       oa_config = kzalloc(sizeof(*oa_config), GFP_KERNEL);
+       if (!oa_config) {
+               DRM_DEBUG("Failed to allocate memory for the OA config\n");
+               return -ENOMEM;
+       }
+
+       err = strncpy_from_user(oa_config->uuid, u64_to_user_ptr(args->uuid),
+                               sizeof(oa_config->uuid));
I think we'll need to do:

struct i915_oa_config {
     char uuid[UUID_STRING_LEN+1];
};

strncpy_from_user(oa_config->uuid, u64_to_user_ptr(args->uuid),
UUID_STRING_LEN);

Done.

Hmm, alternatively we might want to consider using uuid_t, maybe just
for the user-space portion, once that lands in tip, not sure how
worthwhile that would be though...

No uuid_t for now.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux