[Mesa-dev] [PATCH v2 4/5] i965: perf: add support for userspace configurations

[Emil Velikov]

Hi Lionel,

On 29 August 2017 at 14:05, Lionel Landwerlin
<lionel.g.landwerlin at intel.com> wrote:
> This allows us to deploy new configurations without touching the
> kernel.
>
> v2: Detect loadable configs without creating one (Chris)
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_performance_query.c | 98 ++++++++++++++++++++++-
>  1 file changed, 95 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_performance_query.c b/src/mesa/drivers/dri/i965/brw_performance_query.c
> index 4b585c95b7d..6ec6b417e52 100644
> --- a/src/mesa/drivers/dri/i965/brw_performance_query.c
> +++ b/src/mesa/drivers/dri/i965/brw_performance_query.c
> @@ -1717,11 +1717,11 @@ read_file_uint64(const char *file, uint64_t *val)
>
>      fd = open(file, 0);
>      if (fd < 0)
> -       return false;
> +        return false;
>      n = read(fd, buf, sizeof (buf) - 1);
>      close(fd);
>      if (n < 0)
> -       return false;
> +        return false;
>
>      buf[n] = '\0';
>      *val = strtoull(buf, NULL, 0);
> @@ -1807,6 +1807,95 @@ read_sysfs_drm_device_file_uint64(struct brw_context *brw,
>     return read_file_uint64(buf, value);
>  }
>
> +static bool
> +kernel_has_dynamic_config_support(struct brw_context *brw,
> +                                  const char *sysfs_dev_dir)
> +{
> +   __DRIscreen *screen = brw->screen->driScrnPriv;
> +   struct hash_entry *entry;
> +
> +   hash_table_foreach(brw->perfquery.oa_metrics_table, entry) {
> +      struct brw_perf_query_info *query = entry->data;
> +      char config_path[256];
> +      uint64_t config_id;
> +
> +      snprintf(config_path, sizeof(config_path),
> +               "%s/metrics/%s/id", sysfs_dev_dir, query->guid);
> +
> +      /* Look for the test config, which we know we can't replace. */
> +      if (read_file_uint64(config_path, &config_id) && config_id == 1) {
> +         uint32_t mux_regs[] = { 0x9888 /* NOA_WRITE */, 0x0 };
The array seems to have a sentinel, yet one provides explicit array
size (of 1) further down.
Having a look through i915_drm.h the expectation is not documented.

Can I suggest adding a comment [in the kernel i915_drm.h], or dropping
the sentinel - whichever seems applicable.

> +         struct drm_i915_perf_oa_config config;
> +
Unless I failed at math - the struct does not seem compat safe.
That is sizeof(struct drm_i915_perf_oa_config) varies across 32/64bit builds.

Some padding should fix it.

HTH
Emil