[igt-dev] [PATCH i-g-t] tools/i915-perf-recorder: fix topology alignment issue

[Lionel Landwerlin]

On 01/04/2020 14:53, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2020-04-01 12:19:37)
>> On 01/04/2020 14:15, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2020-04-01 11:36:06)
>>>> The additional alignment added when writing into the output was not
>>>> accounted in the header. This is preventing reading the recorded data.
>>>>
>>>> Instead of adding the alignment when writing, just account for it when
>>>> querying the topology.
>>>>
>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>>> Fixes: f08865e58cd3 ("tools: add i915 perf recorder tool")
>>>> ---
>>>>    tools/i915-perf/i915_perf_recorder.c | 10 ++--------
>>>>    1 file changed, 2 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tools/i915-perf/i915_perf_recorder.c b/tools/i915-perf/i915_perf_recorder.c
>>>> index 4d729b0e..104a425b 100644
>>>> --- a/tools/i915-perf/i915_perf_recorder.c
>>>> +++ b/tools/i915-perf/i915_perf_recorder.c
>>>> @@ -485,8 +485,8 @@ get_topology(int drm_fd, uint32_t *topology_size)
>>>>                   return NULL;
>>>>    
>>>>           assert(item.length > 0);
>>>> -       *topology_size = item.length;
>>>> -       topo_info = malloc(item.length);
>>>> +       *topology_size = ALIGN(item.length, 8);
>>>> +       topo_info = malloc(*topology_size);
>>>>           item.data_ptr = (uintptr_t) topo_info;
>>> As I understand the problem statement, when we used the length the
>>> kernel told us to use, the kernel rejected the query?
>>>
>>> Could you clarify as that sounds like a kernel bug.
>>> -Chris
>> We get the result of the query from the kernel and write it to the file.
>>
>> To keep structures in the file aligned to 8 bytes (so that we can mmap
>> stuff and it doesn't fall on weird alignments), the topology data size
>> has to be aligned as well.
>>
>>
>> The problem is that we align the data when writing it to the file, but
>> fail to update the header.size field properly written just before the
>> topology data.
> Gotcha, you're talking about the returned *topology_size not matching
> what was fwritten to the pipe.
>
> Ok, one problem I see here is that you are passing around uninitialised
> bytes (for some value of problem).
> -Chris

Heh, yeah, fixed in a v2 locally as well as an unused variable (pad) 
dropped.

-Lionel