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

[Emil Velikov]

On 29 August 2017 at 14:51, Lionel Landwerlin
<lionel.g.landwerlin at intel.com> wrote:
> On 29/08/17 14:39, Emil Velikov wrote:
>>
>> 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.
>
>
> This is the number of pair of (register, value).
>
>>
>> Can I suggest adding a comment [in the kernel i915_drm.h], or dropping
>> the sentinel - whichever seems applicable.
>
>
> I guess I'll add a comment in the kernel.
>
>>
>>> +         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.
>
>
> Hmm... What's not compat safe in there? :
>
> struct drm_i915_perf_oa_config {
>         /** String formatted like "%08x-%04x-%04x-%04x-%012x" */
>         char uuid[36];
>
>         __u32 n_mux_regs;
>         __u32 n_boolean_regs;
>         __u32 n_flex_regs;
>
>         __u64 mux_regs_ptr;
>         __u64 boolean_regs_ptr;
>         __u64 flex_regs_ptr;
> };
>
Scratch that - I misread 36 as 32, which would have left a 4 byte
whole in 64bit builds.
Pardon for the noise.

-Emil