Re: [PATCH 2/3] igt/gem_pread: Support to verify pread/pwrite for non-shmem backed obj

[Dave Gordon <david.s.gordon@intel.com>]

On 03/07/15 10:37, Tvrtko Ursulin wrote:
>
>
> On 07/01/2015 10:26 AM, ankitprasad.r.sharma@intel.com wrote:
>> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>>
>> This patch adds support to verify pread/pwrite for non-shmem backed
>> objects. It also shows the pread/pwrite speed.
>> It also tests speeds for pread with and without user side page faults
>>
>> v2: Rebased to the latest (Ankit)
>>
>> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
>> ---
>>   tests/gem_pread.c  | 100
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/gem_pwrite.c |  45 ++++++++++++++++++++++++
>>   2 files changed, 145 insertions(+)

[snip]

>> @@ -142,9 +154,97 @@ int main(int argc, char **argv)
>>           }
>>       }
>>
>> +    igt_subtest("stolen-normal") {
>> +        for (count = 1; count <= 1<<17; count <<= 1) {
>> +            struct timeval start, end;
>> +
>> +            gettimeofday(&start, NULL);
>> +            do_gem_read(fd, src_stolen, dst_user, object_size, count);
>> +            gettimeofday(&end, NULL);
>> +            igt_info("Time to pread %d bytes x %6d:    %7.3fµs, %s\n",
>> +                 object_size, count,
>> +                 elapsed(&start, &end, count),
>> +                 bytes_per_sec((char *)buf,
>> +                 object_size/elapsed(&start, &end, count)*1e6));
>
> There is no checking that bytes_per_sec won't overflow buf. Which is
> also declared as unit32_t just so we can cast here. :) Suggest fixing if
> you feel like it, won't mandate it since it is existing code.

That printf format appears to have 4 %-fields but 5 parameters?
Ah, no it hasn't, but the indentation is misleading -- how about 
indenting the last line (the one beginning object_size ...) some more so 
that it's more obviously a parameter to bytes_per_sec() and not igt_info().

Also perhaps nicer to precalculate elapsed(&start, &end, count) and save 
it rather than inlining twice in the parameter list? Maybe even do the 
same with the bps value (even though it's only used once) just so the 
long printf lines don't have complicated expressions embedded?

Thus:

+        for (count = 1; count <= 1<<17; count <<= 1) {
+            struct timeval start, end;
*            double usecs;
*            char *bps;
+
+            gettimeofday(&start, NULL);
+            do_gem_read(fd, src_stolen, dst_user, object_size, count);
+            gettimeofday(&end, NULL);
*            usecs = elapsed(&start, &end, count);
*            bps = bytes_per_sec((char *)buf, 1e6*object_size/usecs);
*            igt_info("Time to pread %d bytes x %6d:    %7.3fµs, %s\n",
*                 object_size, count, time, bps);

... and obviously for all other similar code below.

.Dave.

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