<div dir="ltr">On Thu, Nov 10, 2016 at 11:03 PM, Matthew Auld <span dir="ltr"><<a href="mailto:matthew.william.auld@gmail.com" target="_blank">matthew.william.auld@gmail.<wbr>com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_-7076742584929518405HOEnZb"><div class="m_-7076742584929518405h5">On 11/09, Robert Bragg wrote:<br>
> +<br>
> +igt_main<br>
> +{<br>
> +        igt_skip_on_simulation();<br>
> +<br>
> +        igt_fixture {<br>
> +                struct stat sb;<br>
> +                int ret;<br>
> +<br>
> +                drm_fd = drm_open_driver_render(DRIVER_<wbr>INTEL);<br>
> +                devid = intel_get_drm_devid(drm_fd);<br>
> +                device = drm_get_card();<br>
> +<br>
> +                igt_require(IS_HASWELL(devid))<wbr>;<br>
> +                igt_require(lookup_hsw_render_<wbr>basic_id());<br>
> +<br>
> +                ret = stat("/proc/sys/dev/i915/perf_<wbr>stream_paranoid", &sb);<br>
> +                igt_require(ret == 0);<br>
> +                ret = stat("/proc/sys/dev/i915/oa_ma<wbr>x_sample_rate", &sb);<br>
> +                igt_require(ret == 0);<br>
</div></div>The absence of the above files would indicate a failure in the kernel,<br>
so would it not be more apt to assert, rather than skip ?<br></blockquote><div><br></div><div>The test could be running against an older kernel which won't have these files and we should skip all the tests in that case.<br><br></div><div>E.g. Chris asked me to maintain compatibility within the gem_exec_parse test for older versions of the command parser so I suppose i-g-t tries to maintain backwards compatibility.<br><br></div><div>We could potentially require one and assert the other.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> +<br>
> +                gt_frequency_range_save();<br>
> +<br>
> +                write_u64_file("/proc/sys/dev/<wbr>i915/perf_stream_paranoid", 1);<br>
</span>Don't we also want to ensure that the oa_max_sample_rate is also in a<br>
"good" starting state before we begin, especially since we ensure that<br>
we leave in its default state when cleaning up ?<br></blockquote><div><br></div><div>Explicitly setting the max rate interferes with being able to assert what the default is, but that's already a problem with the cleanup fixture explicitly setting the rate.<br><br></div><div>What I'm doing now is initializing oa_max_sample_rate before tests, as suggested here, and I've also added a test_sysctl_defaults() test that's run very early, just after the i915-ref-count test.<br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Anyway, I think it all looks pretty reasonable to me and it looks like<br>
we have a good amount of coverage, so you can have my r-b with Chris'<br>
comment addressed.<br></blockquote><div><br></div><div>Thanks, I've moved the ref counting test in front of the first fixture as suggested by chris (with just the requirements check that the i915-perf interface exists in another fixture before).<br></div><div> <br><br></div></div></div></div>