Re: [PATCH igt v3 01/11] igt/perf: add i915 perf stream tests for Haswell

[Robert Bragg <robert@sixbynine.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2426 bytes --]

On Thu, Nov 10, 2016 at 11:03 PM, Matthew Auld <matthew.william.auld@gmail.
com> wrote:

> On 11/09, Robert Bragg wrote:
> > +
> > +igt_main
> > +{
> > +        igt_skip_on_simulation();
> > +
> > +        igt_fixture {
> > +                struct stat sb;
> > +                int ret;
> > +
> > +                drm_fd = drm_open_driver_render(DRIVER_INTEL);
> > +                devid = intel_get_drm_devid(drm_fd);
> > +                device = drm_get_card();
> > +
> > +                igt_require(IS_HASWELL(devid));
> > +                igt_require(lookup_hsw_render_basic_id());
> > +
> > +                ret = stat("/proc/sys/dev/i915/perf_stream_paranoid",
> &sb);
> > +                igt_require(ret == 0);
> > +                ret = stat("/proc/sys/dev/i915/oa_max_sample_rate",
> &sb);
> > +                igt_require(ret == 0);
> The absence of the above files would indicate a failure in the kernel,
> so would it not be more apt to assert, rather than skip ?
>

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.

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.

We could potentially require one and assert the other.


> > +
> > +                gt_frequency_range_save();
> > +
> > +                write_u64_file("/proc/sys/dev/i915/perf_stream_paranoid",
> 1);
> Don't we also want to ensure that the oa_max_sample_rate is also in a
> "good" starting state before we begin, especially since we ensure that
> we leave in its default state when cleaning up ?
>

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.

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.



>
> Anyway, I think it all looks pretty reasonable to me and it looks like
> we have a good amount of coverage, so you can have my r-b with Chris'
> comment addressed.
>

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).

[-- Attachment #1.2: Type: text/html, Size: 3647 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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