Re: [Intel-gfx] [PATCH] drm/i915: Asynchronously perform the set-base for a simple modeset
On Fri, Aug 09, 2013 at 09:06:36PM +0100, Chris Wilson wrote: On Fri, Aug 09, 2013 at 09:17:11PM +0200, Daniel Vetter wrote: On Fri, Aug 09, 2013 at 03:13:22PM +0100, Chris Wilson wrote: A simple modeset, where we only wish to switch over to a new framebuffer such as the transition from fbcon to X, takes around 30-60ms. This is due to three factors: 1. We need to make sure the fb-obj is in the display domain, which incurs a cache flush to ensure no dirt is left on the scanout. 2. We need to flush any pending rendering before performing the mmio so that the frame is complete before it is shown. 3. We currently wait for the vblank after the mmio to be sure that the old fb is no longer being shown before releasing it. (1) can only be eliminated by userspace preparing the fb-obj in advance to already be in the display domain. This can be done through use of the create2 ioctl, or by reusing an existing fb-obj. However, (2) and (3) are already solved by the existing page flip mechanism, and it is surprisingly trivial to wire them up for use in the set-base fast path. Though it can be argued that this represents a subtle ABI break in that the set_config ioctl now returns before the old framebuffer is unpinned. The danger is that userspace will start to modify it before it is no longer being shown, however we should be able to prevent that through proper domain tracking. Hm, right now we don't prevent anyone from rendering into a to-be-flipped out buffer. There was once code in it, using MI_WAIT_EVENT but we've ripped it out. I guess we could just throw in a synchronous stall on the flip queue though, that should work always. I'm glad we did. I'd rather put that into userspace rather than have the kernel impose that policy on everybody, as for X that is exactly the behaviour we want (i.e. not blocking rendering on the next scanout). Testing would be easy if we have the crtc CRC stuff, but that's atm stuck due to lack of volunteers ... Overall I really like the idea and I think doing most of the plane enabling (including psr, fbc, ips, and all that stuff which potentially blows through a wblank wait) should be done in async work queues. That should then also help resume time a lot. I'd also like to hear Ville's opinion since with his atomic modesetting I hope we will be able to achieve something very similar. Async is nice. Like everyone else I suppose, my only concern has to do with writes to the old scanout buffer. One option would be to add an event also to setcrtc, and maybe a new flag to request the event+nonblocking operation. That's what I have in my atomic page flip code (actually I think I had separate flags for each). Unfortunately we don't seem to have flags in setcrtc, unless we commandeer some bits from mode_valid. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/7] drmtest framework support for retval handling
Hi all, So I've grown fed-up with our add-hoc (and pretty much always buggy) return value tracking for testcases with subtests. Furthermore sprinkling testcases with tons of if (kernel_has_some_optional_feature) return 77; isn't really helping test case readability. So I've gone ahead and implement some magic stuff using setjmp/longjmp and a few macros. On top of that a few exemplary conversions of existing testcases. Comments highly welcome. Cheers, Daniel Daniel Vetter (7): tests: s/cacheing/caching lib/drmtest: Add drmtest_subtest_block macro lib/drmtest: skip/fail handling tests/gem_caching: convert to use drmtest retval infrastructure lib/drmtest: make signal process cleanup more robust tests: use drmtest_skip() in caching ioctl helpers tests: use drmtest_skip to check for rings demos/intel_sprite_on.c| 2 - lib/drmtest.c | 134 --- lib/drmtest.h | 46 - tests/.gitignore | 2 +- tests/Makefile.am | 6 +- tests/gem_bad_length.c | 2 - tests/gem_basic.c | 6 +- tests/gem_cacheing.c | 303 tests/gem_caching.c| 305 + tests/gem_cpu_concurrent_blit.c| 12 +- tests/gem_cs_tlb.c | 20 +-- tests/gem_ctx_bad_exec.c | 1 - tests/gem_dummy_reloc_loop.c | 18 +- tests/gem_exec_bad_domains.c | 10 +- tests/gem_exec_big.c | 1 - tests/gem_exec_faulting_reloc.c| 4 +- tests/gem_exec_lut_handle.c| 1 - tests/gem_exec_nop.c | 19 +- tests/gem_fence_thrash.c | 14 +- tests/gem_flink.c | 10 +- tests/gem_flink_race.c | 4 +- tests/gem_gtt_concurrent_blit.c| 12 +- tests/gem_linear_blits.c | 4 +- tests/gem_lut_handle.c | 1 - tests/gem_mmap_gtt.c | 14 +- tests/gem_partial_pwrite_pread.c | 21 +-- tests/gem_pread.c | 50 +++--- tests/gem_pread_after_blit.c | 25 +-- tests/gem_prw_concurrent_blit.c| 12 +- tests/gem_pwrite.c | 49 +++--- tests/gem_pwrite_pread.c | 194 ++--- tests/gem_ringfill.c | 8 +- tests/gem_set_tiling_vs_blt.c | 6 +- tests/gem_suspend.c| 4 +- tests/gem_tiled_blits.c| 4 +- tests/gem_tiled_partial_pwrite_pread.c | 6 +- tests/gem_write_read_ring_switch.c | 15 +- tests/kms_flip.c | 4 +- tests/kms_render.c | 2 +- tests/prime_nv_api.c | 2 +- tests/prime_nv_pcopy.c | 17 +- tests/prime_nv_test.c | 2 +- tests/prime_self_import.c | 4 +- tests/testdisplay.c| 2 - 44 files changed, 749 insertions(+), 629 deletions(-) delete mode 100644 tests/gem_cacheing.c create mode 100644 tests/gem_caching.c -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/7] tests: s/cacheing/caching
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- lib/drmtest.c| 28 ++-- lib/drmtest.h| 6 +- tests/.gitignore | 2 +- tests/Makefile.am| 2 +- tests/gem_cacheing.c | 303 --- tests/gem_caching.c | 303 +++ tests/gem_partial_pwrite_pread.c | 2 +- tests/gem_pread.c| 2 +- tests/gem_pread_after_blit.c | 4 +- tests/gem_pwrite.c | 2 +- tests/gem_pwrite_pread.c | 12 +- 11 files changed, 333 insertions(+), 333 deletions(-) delete mode 100644 tests/gem_cacheing.c create mode 100644 tests/gem_caching.c diff --git a/lib/drmtest.c b/lib/drmtest.c index f546784..cae07a2 100644 --- a/lib/drmtest.c +++ b/lib/drmtest.c @@ -380,56 +380,56 @@ skip: return num_rings; } -struct local_drm_i915_gem_cacheing { +struct local_drm_i915_gem_caching { uint32_t handle; - uint32_t cacheing; + uint32_t caching; }; #define LOCAL_DRM_I915_GEM_SET_CACHEING0x2f #define LOCAL_DRM_I915_GEM_GET_CACHEING0x30 #define LOCAL_DRM_IOCTL_I915_GEM_SET_CACHEING \ - DRM_IOW(DRM_COMMAND_BASE + LOCAL_DRM_I915_GEM_SET_CACHEING, struct local_drm_i915_gem_cacheing) + DRM_IOW(DRM_COMMAND_BASE + LOCAL_DRM_I915_GEM_SET_CACHEING, struct local_drm_i915_gem_caching) #define LOCAL_DRM_IOCTL_I915_GEM_GET_CACHEING \ - DRM_IOWR(DRM_COMMAND_BASE + LOCAL_DRM_I915_GEM_GET_CACHEING, struct local_drm_i915_gem_cacheing) + DRM_IOWR(DRM_COMMAND_BASE + LOCAL_DRM_I915_GEM_GET_CACHEING, struct local_drm_i915_gem_caching) -int gem_has_cacheing(int fd) +int gem_has_caching(int fd) { - struct local_drm_i915_gem_cacheing arg; + struct local_drm_i915_gem_caching arg; int ret; arg.handle = gem_create(fd, 4096); if (arg.handle == 0) return 0; - arg.cacheing = 0; + arg.caching = 0; ret = ioctl(fd, LOCAL_DRM_IOCTL_I915_GEM_SET_CACHEING, arg); gem_close(fd, arg.handle); return ret == 0; } -int gem_set_cacheing(int fd, uint32_t handle, int cacheing) +int gem_set_caching(int fd, uint32_t handle, int caching) { - struct local_drm_i915_gem_cacheing arg; + struct local_drm_i915_gem_caching arg; int ret; arg.handle = handle; - arg.cacheing = cacheing; + arg.caching = caching; ret = ioctl(fd, LOCAL_DRM_IOCTL_I915_GEM_SET_CACHEING, arg); return ret == 0 ? 0 : -errno; } -int gem_get_cacheing(int fd, uint32_t handle) +uint32_t gem_get_caching(int fd, uint32_t handle) { - struct local_drm_i915_gem_cacheing arg; + struct local_drm_i915_gem_caching arg; int ret; arg.handle = handle; - arg.cacheing = 0; + arg.caching = 0; ret = ioctl(fd, LOCAL_DRM_IOCTL_I915_GEM_GET_CACHEING, arg); assert(ret == 0); - return arg.cacheing; + return arg.caching; } uint32_t gem_open(int fd, uint32_t name) diff --git a/lib/drmtest.h b/lib/drmtest.h index 8320dbe..773beaa 100644 --- a/lib/drmtest.h +++ b/lib/drmtest.h @@ -53,9 +53,9 @@ bool gem_has_bsd(int fd); bool gem_has_blt(int fd); bool gem_has_vebox(int fd); int gem_get_num_rings(int fd); -int gem_has_cacheing(int fd); -int gem_set_cacheing(int fd, uint32_t handle, int cacheing); -int gem_get_cacheing(int fd, uint32_t handle); +int gem_has_caching(int fd); +int gem_set_caching(int fd, uint32_t handle, int caching); +uint32_t gem_get_caching(int fd, uint32_t handle); uint32_t gem_flink(int fd, uint32_t handle); uint32_t gem_open(int fd, uint32_t name); void gem_close(int fd, uint32_t handle); diff --git a/tests/.gitignore b/tests/.gitignore index 8d0b6e5..6395c73 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -9,7 +9,7 @@ gem_bad_batch gem_bad_blit gem_bad_length gem_basic -gem_cacheing +gem_caching gem_cpu_concurrent_blit gem_cpu_reloc gem_cs_prefetch diff --git a/tests/Makefile.am b/tests/Makefile.am index 5d0ed3e..8fff22c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -17,7 +17,7 @@ endif TESTS_progs_M = \ gem_basic \ - gem_cacheing \ + gem_caching \ gem_cpu_concurrent_blit \ gem_cs_tlb \ gem_dummy_reloc_loop \ diff --git a/tests/gem_cacheing.c b/tests/gem_cacheing.c deleted file mode 100644 index 8a169f1..000 --- a/tests/gem_cacheing.c +++ /dev/null @@ -1,303 +0,0 @@ -/* - * Copyright © 2012 Intel Corporation - * - * Permission is hereby granted, free of charge, to any person obtaining a - * copy of this software and associated documentation files (the Software), - * to deal in the Software without restriction, including without limitation - * the rights to use, copy, modify, merge, publish, distribute, sublicense, - * and/or sell copies of the Software, and to permit persons to whom the - * Software is furnished to do so, subject to
[Intel-gfx] [PATCH 2/7] lib/drmtest: Add drmtest_subtest_block macro
Doesn't do more than an if (drmtest_run_test(name)) right now, but as soon as we get a bit of infrastructure to handle test failures and skipping, this will get more interesting. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- lib/drmtest.h | 1 + tests/gem_basic.c | 6 +++--- tests/gem_cpu_concurrent_blit.c| 12 ++-- tests/gem_cs_tlb.c | 8 tests/gem_dummy_reloc_loop.c | 10 +- tests/gem_exec_bad_domains.c | 10 +- tests/gem_exec_faulting_reloc.c| 4 ++-- tests/gem_exec_nop.c | 8 tests/gem_fence_thrash.c | 14 +++--- tests/gem_flink.c | 10 +- tests/gem_flink_race.c | 4 ++-- tests/gem_gtt_concurrent_blit.c| 12 ++-- tests/gem_linear_blits.c | 4 ++-- tests/gem_mmap_gtt.c | 14 +++--- tests/gem_partial_pwrite_pread.c | 6 +++--- tests/gem_pread_after_blit.c | 16 tests/gem_prw_concurrent_blit.c| 12 ++-- tests/gem_pwrite_pread.c | 24 tests/gem_ringfill.c | 8 +--- tests/gem_set_tiling_vs_blt.c | 6 +++--- tests/gem_suspend.c| 4 ++-- tests/gem_tiled_blits.c| 4 ++-- tests/gem_tiled_partial_pwrite_pread.c | 6 +++--- tests/gem_write_read_ring_switch.c | 9 + tests/kms_flip.c | 4 ++-- tests/kms_render.c | 2 +- tests/prime_nv_api.c | 2 +- tests/prime_nv_pcopy.c | 17 + tests/prime_nv_test.c | 2 +- tests/prime_self_import.c | 2 +- 30 files changed, 123 insertions(+), 118 deletions(-) diff --git a/lib/drmtest.h b/lib/drmtest.h index 773beaa..ada8e81 100644 --- a/lib/drmtest.h +++ b/lib/drmtest.h @@ -95,6 +95,7 @@ void drmtest_progress(const char *header, uint64_t i, uint64_t total); /* subtest infrastructure */ void drmtest_subtest_init(int argc, char **argv); bool drmtest_run_subtest(const char *subtest_name); +#define drmtest_subtest_block(name) if (drmtest_run_subtest((name))) bool drmtest_only_list_subtests(void); /* helpers to automatically reduce test runtime in simulation */ diff --git a/tests/gem_basic.c b/tests/gem_basic.c index 5cc8684..900194d 100644 --- a/tests/gem_basic.c +++ b/tests/gem_basic.c @@ -84,11 +84,11 @@ int main(int argc, char **argv) fd = drm_open_any(); - if (drmtest_run_subtest(bad-close)) + drmtest_subtest_block(bad-close) test_bad_close(fd); - if (drmtest_run_subtest(create-close)) + drmtest_subtest_block(create-close) test_create_close(fd); - if (drmtest_run_subtest(create-fd-close)) + drmtest_subtest_block(create-fd-close) test_create_fd_close(fd); return 0; diff --git a/tests/gem_cpu_concurrent_blit.c b/tests/gem_cpu_concurrent_blit.c index e6cc50b..7bcfcd8 100644 --- a/tests/gem_cpu_concurrent_blit.c +++ b/tests/gem_cpu_concurrent_blit.c @@ -117,7 +117,7 @@ main(int argc, char **argv) } /* try to overwrite the source values */ - if (drmtest_run_subtest(overwrite-source)) { + drmtest_subtest_block(overwrite-source) { for (i = 0; i num_buffers; i++) { set_bo(src[i], i, width, height); set_bo(dst[i], i, width, height); @@ -131,7 +131,7 @@ main(int argc, char **argv) } /* try to read the results before the copy completes */ - if (drmtest_run_subtest(early-read)) { + drmtest_subtest_block(early-read) { for (i = num_buffers; i--; ) set_bo(src[i], 0xdeadbeef, width, height); for (i = 0; i num_buffers; i++) @@ -141,7 +141,7 @@ main(int argc, char **argv) } /* and finally try to trick the kernel into loosing the pending write */ - if (drmtest_run_subtest(gpu-read-after-write)) { + drmtest_subtest_block(gpu-read-after-write) { for (i = num_buffers; i--; ) set_bo(src[i], 0xabcdabcd, width, height); for (i = 0; i num_buffers; i++) @@ -155,7 +155,7 @@ main(int argc, char **argv) drmtest_fork_signal_helper(); /* try to overwrite the source values */ - if (drmtest_run_subtest(overwrite-source-interruptible)) { + drmtest_subtest_block(overwrite-source-interruptible) { for (loop = 0; loop 10; loop++) { gem_quiescent_gpu(fd); for (i = 0; i num_buffers; i++) { @@ -172,7 +172,7 @@ main(int argc, char **argv) } /* try to read the results before the copy completes */ - if
[Intel-gfx] [PATCH 3/7] lib/drmtest: skip/fail handling
Exitcode handling in igt testcases has too ugly facets in combination with subtests: - When individual subtest needed to be skipped for different reasons we need to painstakingly thread this information through the test to make sure that we still succeed if just one testcase worked (for running it with the simple automake test runner in igt). But it also needs to correctly report the skip exit value if only this one skipped testcase has been run (e.g. when run with piglit or in QA's test infrastructure). - We'd prefer to easily skip (and also fail) subtests without hampering other subtests. Again threading the return value of each subtest through the code is cumbersome. To simplify test case writing use longjmps to be able to get out of subcases easily. But since longjmps are funny things thug it all away into the newly added drmtest_subtest_block macro. Note that if drmtest_skip is called outside of a subtest, but in a testcase with subtests all subsequent subtests will be automatically skipped. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- lib/drmtest.c | 77 +-- lib/drmtest.h | 10 +++- 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/lib/drmtest.c b/lib/drmtest.c index cae07a2..767c8dc 100644 --- a/lib/drmtest.c +++ b/lib/drmtest.c @@ -657,6 +657,9 @@ void drmtest_stop_signal_helper(void) /* subtests helpers */ static bool list_subtests = false; static char *run_single_subtest = NULL; +static bool in_subtest = false; +static bool test_with_subtests = false; +static bool skip_subtests_henceforth = false; void drmtest_subtest_init(int argc, char **argv) { @@ -667,6 +670,8 @@ void drmtest_subtest_init(int argc, char **argv) {NULL, 0, 0, 0,} }; + test_with_subtests = true; + /* supress getopt errors about unknown options */ opterr = 0; /* restrict the option parsing to long option names to avoid collisions @@ -695,16 +700,21 @@ out: */ bool drmtest_run_subtest(const char *subtest_name) { + assert(in_subtest == false); + if (list_subtests) { printf(%s\n, subtest_name); return false; } + if (skip_subtests_henceforth) + return false; + if (!run_single_subtest) { - return true; + return in_subtest = true; } else { if (strcmp(subtest_name, run_single_subtest) == 0) - return true; + return in_subtest = true; return false; } @@ -715,6 +725,69 @@ bool drmtest_only_list_subtests(void) return list_subtests; } +static bool skipped_one = false; +static bool succeeded_one = false; +static bool failed_one = false; +static int drmtest_exitcode; + +static void exit_subtest(void) __attribute__((noreturn)); +static void exit_subtest(void) +{ + in_subtest = false; + longjmp(drmtest_subtest_jmpbuf, 1); +} + +void drmtest_skip(void) +{ + skipped_one = true; + if (in_subtest) + exit_subtest(); + else if (test_with_subtests) + skip_subtests_henceforth = true; + else + exit(77); +} + +void drmtest_success(void) +{ + succeeded_one = true; + if (in_subtest) + exit_subtest(); +} + +void drmtest_fail(int exitcode) +{ + assert(exitcode != 0 exitcode != 77); + + if (!failed_one) + drmtest_exitcode = exitcode; + + failed_one = true; + + if (in_subtest) + exit_subtest(); + else { + assert(!test_with_subtests); + exit(exitcode); + } +} + +int drmtest_retval(void) +{ + if (drmtest_only_list_subtests()) + return 0; + + /* Calling this without calling one of the above is a failure */ + assert(skipped_one || succeeded_one || failed_one); + + if (failed_one) + return drmtest_exitcode; + else if (succeeded_one) + return 0; + else + return 77; +} + static bool env_set(const char *env_var, bool default_value) { char *val; diff --git a/lib/drmtest.h b/lib/drmtest.h index ada8e81..13e25bb 100644 --- a/lib/drmtest.h +++ b/lib/drmtest.h @@ -32,6 +32,7 @@ #include errno.h #include stdbool.h #include cairo.h +#include setjmp.h #include xf86drm.h #include xf86drmMode.h @@ -93,10 +94,17 @@ void drmtest_permute_array(void *array, unsigned size, void drmtest_progress(const char *header, uint64_t i, uint64_t total); /* subtest infrastructure */ +jmp_buf drmtest_subtest_jmpbuf; void drmtest_subtest_init(int argc, char **argv); bool drmtest_run_subtest(const char *subtest_name); -#define drmtest_subtest_block(name) if (drmtest_run_subtest((name))) +#define drmtest_subtest_block(name) for (; drmtest_run_subtest((name)) \ +
[Intel-gfx] [PATCH 5/7] lib/drmtest: make signal process cleanup more robust
If we skip a test and and fail somewhere the parent might die before the child. So add some cleanup to handle this case. Also make sure that the parent indeed waits for the child to die. This is required to make the latest version of the piglit runner happy, it tends to wait for all zombies to disappear. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- lib/drmtest.c | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/drmtest.c b/lib/drmtest.c index 767c8dc..77e8002 100644 --- a/lib/drmtest.c +++ b/lib/drmtest.c @@ -625,11 +625,18 @@ static void sig_handler(int i) sig_stat++; } +static void signal_helper_exit_handler(int sig) +{ + drmtest_stop_signal_helper(); +} + void drmtest_fork_signal_helper(void) { pid_t pid; sighandler_t oldsig; + drmtest_install_exit_handler(signal_helper_exit_handler); + signal(SIGUSR1, sig_handler); oldsig = signal(SIGQUIT, SIG_DFL); pid = fork(); @@ -644,8 +651,12 @@ void drmtest_fork_signal_helper(void) void drmtest_stop_signal_helper(void) { - if (signal_helper != -1) + int exitcode; + + if (signal_helper != -1) { kill(signal_helper, SIGQUIT); + wait(exitcode); + } if (sig_stat) fprintf(stdout, signal handler called %llu times\n, sig_stat); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/7] tests: use drmtest_skip() in caching ioctl helpers
This way we can rip out all the skip handling from the test control flow, and additionally (by using drmtest_retval()) even get correct exit codes. The only tricky part is that when we only want ot skip parts of a test (like for gem_pread and gem_pwrite) we need to split out those parts as subtests. But no addition of control-flow is required, the set/longjmp magic in the helpers all makes it happen. Also we make extensive use of the behaviour of drmtest_skip to skip all subsequent subtests if it is called outside of a subtest. This allows us to re-flatten the control flow a lot. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- lib/drmtest.c| 20 ++-- lib/drmtest.h| 4 +- tests/Makefile.am| 4 +- tests/gem_caching.c | 5 +- tests/gem_partial_pwrite_pread.c | 15 +-- tests/gem_pread.c| 50 +- tests/gem_pread_after_blit.c | 9 +- tests/gem_pwrite.c | 48 +- tests/gem_pwrite_pread.c | 194 +++ 9 files changed, 178 insertions(+), 171 deletions(-) diff --git a/lib/drmtest.c b/lib/drmtest.c index 77e8002..7e3c1d8 100644 --- a/lib/drmtest.c +++ b/lib/drmtest.c @@ -40,6 +40,7 @@ #include stdlib.h #include linux/kd.h #include unistd.h +#include sys/wait.h #include drm_fourcc.h #include drmtest.h @@ -392,23 +393,26 @@ struct local_drm_i915_gem_caching { #define LOCAL_DRM_IOCTL_I915_GEM_GET_CACHEING \ DRM_IOWR(DRM_COMMAND_BASE + LOCAL_DRM_I915_GEM_GET_CACHEING, struct local_drm_i915_gem_caching) -int gem_has_caching(int fd) +void gem_check_caching(int fd) { struct local_drm_i915_gem_caching arg; int ret; arg.handle = gem_create(fd, 4096); - if (arg.handle == 0) - return 0; + assert(arg.handle != 0); arg.caching = 0; ret = ioctl(fd, LOCAL_DRM_IOCTL_I915_GEM_SET_CACHEING, arg); gem_close(fd, arg.handle); - return ret == 0; + if (ret != 0) { + if (!drmtest_only_list_subtests()) + printf(no set_caching support detected\n); + drmtest_skip(); + } } -int gem_set_caching(int fd, uint32_t handle, int caching) +void gem_set_caching(int fd, uint32_t handle, int caching) { struct local_drm_i915_gem_caching arg; int ret; @@ -416,7 +420,11 @@ int gem_set_caching(int fd, uint32_t handle, int caching) arg.handle = handle; arg.caching = caching; ret = ioctl(fd, LOCAL_DRM_IOCTL_I915_GEM_SET_CACHEING, arg); - return ret == 0 ? 0 : -errno; + + if (ret != 0 (errno == ENOTTY || errno == EINVAL)) + drmtest_skip(); + else + assert(ret == 0); } uint32_t gem_get_caching(int fd, uint32_t handle) diff --git a/lib/drmtest.h b/lib/drmtest.h index 13e25bb..624abb2 100644 --- a/lib/drmtest.h +++ b/lib/drmtest.h @@ -54,8 +54,8 @@ bool gem_has_bsd(int fd); bool gem_has_blt(int fd); bool gem_has_vebox(int fd); int gem_get_num_rings(int fd); -int gem_has_caching(int fd); -int gem_set_caching(int fd, uint32_t handle, int caching); +void gem_check_caching(int fd); +void gem_set_caching(int fd, uint32_t handle, int caching); uint32_t gem_get_caching(int fd, uint32_t handle); uint32_t gem_flink(int fd, uint32_t handle); uint32_t gem_open(int fd, uint32_t name); diff --git a/tests/Makefile.am b/tests/Makefile.am index 8fff22c..f3475ad 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -30,8 +30,10 @@ TESTS_progs_M = \ gem_linear_blits \ gem_mmap_gtt \ gem_partial_pwrite_pread \ + gem_pread \ gem_pread_after_blit \ gem_prw_concurrent_blit \ + gem_pwrite \ gem_pwrite_pread \ gem_ringfill \ gem_set_tiling_vs_blt \ @@ -73,8 +75,6 @@ TESTS_progs = \ gem_mmap_offset_exhaustion \ gem_pin \ gem_pipe_control_store_loop \ - gem_pread \ - gem_pwrite \ gem_readwrite \ gem_reg_read \ gem_reloc_overflow \ diff --git a/tests/gem_caching.c b/tests/gem_caching.c index 259662f..ed58c28 100644 --- a/tests/gem_caching.c +++ b/tests/gem_caching.c @@ -118,10 +118,7 @@ int main(int argc, char **argv) fd = drm_open_any(); - if (!gem_has_caching(fd)) { - printf(no set_caching support detected\n); - return 77; - } + gem_check_caching(fd); devid = intel_get_drm_devid(fd); if (IS_GEN2(devid)) /* chipset only handles cached - uncached */ diff --git a/tests/gem_partial_pwrite_pread.c b/tests/gem_partial_pwrite_pread.c index a27e584..f6e6e32 100644 --- a/tests/gem_partial_pwrite_pread.c +++ b/tests/gem_partial_pwrite_pread.c @@ -257,17 +257,8 @@ static void do_tests(int cache_level, const char *suffix) { char name[80]; - if (cache_level != -1) { - switch (gem_set_caching(fd, scratch_bo-handle,
[Intel-gfx] [PATCH 7/7] tests: use drmtest_skip to check for rings
To simplify things add a set of gem_check_ring functions which take care of this. Since I've opted for static inlines drmtest.h grew a few more header includes which was a neat opportunity to dump a few redundant #defines. This kills all the skipped_all hand-rolled logic we have. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- demos/intel_sprite_on.c| 2 -- lib/drmtest.h | 33 - tests/gem_bad_length.c | 2 -- tests/gem_cs_tlb.c | 12 tests/gem_ctx_bad_exec.c | 1 - tests/gem_dummy_reloc_loop.c | 8 tests/gem_exec_big.c | 1 - tests/gem_exec_lut_handle.c| 1 - tests/gem_exec_nop.c | 11 --- tests/gem_lut_handle.c | 1 - tests/gem_pwrite.c | 1 - tests/gem_write_read_ring_switch.c | 6 +++--- tests/prime_self_import.c | 2 -- tests/testdisplay.c| 2 -- 14 files changed, 47 insertions(+), 36 deletions(-) diff --git a/demos/intel_sprite_on.c b/demos/intel_sprite_on.c index 783f9af..5c380c1 100644 --- a/demos/intel_sprite_on.c +++ b/demos/intel_sprite_on.c @@ -51,8 +51,6 @@ #include drm_fourcc.h #endif -#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) - /* * Mode setting with the kernel interfaces is a bit of a chore. * First you have to find the connector in question and make sure the diff --git a/lib/drmtest.h b/lib/drmtest.h index 624abb2..b687411 100644 --- a/lib/drmtest.h +++ b/lib/drmtest.h @@ -37,6 +37,8 @@ #include xf86drm.h #include xf86drmMode.h #include intel_batchbuffer.h +#include intel_chipset.h +#include intel_gpu_tools.h drm_intel_bo * gem_handle_to_libdrm_bo(drm_intel_bufmgr *bufmgr, int fd, const char *name, uint32_t handle); @@ -54,7 +56,7 @@ bool gem_has_bsd(int fd); bool gem_has_blt(int fd); bool gem_has_vebox(int fd); int gem_get_num_rings(int fd); -void gem_check_caching(int fd); + void gem_set_caching(int fd, uint32_t handle, int caching); uint32_t gem_get_caching(int fd, uint32_t handle); uint32_t gem_flink(int fd, uint32_t handle); @@ -106,6 +108,35 @@ void drmtest_success(void); void drmtest_fail(int exitcode) __attribute__((noreturn)); int drmtest_retval(void); +/* check functions which auto-skip tests by calling drmtest_skip() */ +void gem_check_caching(int fd); +static inline bool gem_check_vebox(int fd) +{ + if (gem_has_vebox(fd)) + return true; + + drmtest_skip(); + return false; +} + +static inline bool gem_check_bsd(int fd) +{ + if (HAS_BSD_RING(intel_get_drm_devid(fd))) + return true; + + drmtest_skip(); + return false; +} + +static inline bool gem_check_blt(int fd) +{ + if (HAS_BLT_RING(intel_get_drm_devid(fd))) + return true; + + drmtest_skip(); + return false; +} + /* helpers to automatically reduce test runtime in simulation */ bool drmtest_run_in_simulation(void); #define SLOW_QUICK(slow,quick) (drmtest_run_in_simulation() ? (quick) : (slow)) diff --git a/tests/gem_bad_length.c b/tests/gem_bad_length.c index bb8b6b8..2d5f977 100644 --- a/tests/gem_bad_length.c +++ b/tests/gem_bad_length.c @@ -40,8 +40,6 @@ #include i915_drm.h #include drmtest.h -#define MI_BATCH_BUFFER_END(0xA23) - /* * Testcase: Minmal bo_create and batchbuffer exec * diff --git a/tests/gem_cs_tlb.c b/tests/gem_cs_tlb.c index 081b6f2..0378fdb 100644 --- a/tests/gem_cs_tlb.c +++ b/tests/gem_cs_tlb.c @@ -55,7 +55,6 @@ #define LOCAL_I915_EXEC_VEBOX (40) #define BATCH_SIZE (1024*1024) -bool skipped_all = true; static int exec(int fd, uint32_t handle, int split, uint64_t *gtt_ofs, unsigned ring_id) @@ -104,7 +103,6 @@ static void run_on_ring(int fd, unsigned ring_id, const char *ring_name) int i; sprintf(buf, testing %s cs tlb coherency: , ring_name); - skipped_all = false; /* Shut up gcc, too stupid. */ batch_ptr_old = NULL; @@ -149,13 +147,11 @@ static void run_on_ring(int fd, unsigned ring_id, const char *ring_name) int main(int argc, char **argv) { int fd; - uint32_t devid; drmtest_subtest_init(argc, argv); drmtest_skip_on_simulation(); fd = drm_open_any(); - devid = intel_get_drm_devid(fd); if (!drmtest_only_list_subtests()) { /* This test is very sensitive to residual gtt_mm noise from previous @@ -168,18 +164,18 @@ int main(int argc, char **argv) run_on_ring(fd, I915_EXEC_RENDER, render); drmtest_subtest_block(bsd) - if (HAS_BSD_RING(devid)) + if (gem_check_bsd(fd)) run_on_ring(fd, I915_EXEC_BSD, bsd); drmtest_subtest_block(blt) - if (HAS_BLT_RING(devid)) + if (gem_check_blt(fd)) run_on_ring(fd,
Re: [Intel-gfx] [PATCH 0/7] drmtest framework support for retval handling
On Mon, Aug 12, 2013 at 11:09:54AM +0200, Daniel Vetter wrote: Hi all, So I've grown fed-up with our add-hoc (and pretty much always buggy) return value tracking for testcases with subtests. Furthermore sprinkling testcases with tons of if (kernel_has_some_optional_feature) return 77; isn't really helping test case readability. So I've gone ahead and implement some magic stuff using setjmp/longjmp and a few macros. On top of that a few exemplary conversions of existing testcases. Comments highly welcome. Cheers, Daniel Just noticed that this already doesn't apply, so I've pushed a rebased version to http://cgit.freedesktop.org/~danvet/intel-gpu-tools/log/?h=retval-infrastructure That one also contains one hunk I've failed to git add. Cheers, Daniel Daniel Vetter (7): tests: s/cacheing/caching lib/drmtest: Add drmtest_subtest_block macro lib/drmtest: skip/fail handling tests/gem_caching: convert to use drmtest retval infrastructure lib/drmtest: make signal process cleanup more robust tests: use drmtest_skip() in caching ioctl helpers tests: use drmtest_skip to check for rings demos/intel_sprite_on.c| 2 - lib/drmtest.c | 134 --- lib/drmtest.h | 46 - tests/.gitignore | 2 +- tests/Makefile.am | 6 +- tests/gem_bad_length.c | 2 - tests/gem_basic.c | 6 +- tests/gem_cacheing.c | 303 tests/gem_caching.c| 305 + tests/gem_cpu_concurrent_blit.c| 12 +- tests/gem_cs_tlb.c | 20 +-- tests/gem_ctx_bad_exec.c | 1 - tests/gem_dummy_reloc_loop.c | 18 +- tests/gem_exec_bad_domains.c | 10 +- tests/gem_exec_big.c | 1 - tests/gem_exec_faulting_reloc.c| 4 +- tests/gem_exec_lut_handle.c| 1 - tests/gem_exec_nop.c | 19 +- tests/gem_fence_thrash.c | 14 +- tests/gem_flink.c | 10 +- tests/gem_flink_race.c | 4 +- tests/gem_gtt_concurrent_blit.c| 12 +- tests/gem_linear_blits.c | 4 +- tests/gem_lut_handle.c | 1 - tests/gem_mmap_gtt.c | 14 +- tests/gem_partial_pwrite_pread.c | 21 +-- tests/gem_pread.c | 50 +++--- tests/gem_pread_after_blit.c | 25 +-- tests/gem_prw_concurrent_blit.c| 12 +- tests/gem_pwrite.c | 49 +++--- tests/gem_pwrite_pread.c | 194 ++--- tests/gem_ringfill.c | 8 +- tests/gem_set_tiling_vs_blt.c | 6 +- tests/gem_suspend.c| 4 +- tests/gem_tiled_blits.c| 4 +- tests/gem_tiled_partial_pwrite_pread.c | 6 +- tests/gem_write_read_ring_switch.c | 15 +- tests/kms_flip.c | 4 +- tests/kms_render.c | 2 +- tests/prime_nv_api.c | 2 +- tests/prime_nv_pcopy.c | 17 +- tests/prime_nv_test.c | 2 +- tests/prime_self_import.c | 4 +- tests/testdisplay.c| 2 - 44 files changed, 749 insertions(+), 629 deletions(-) delete mode 100644 tests/gem_cacheing.c create mode 100644 tests/gem_caching.c -- 1.8.3.2 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] lib/drmtest: Add drmtest_subtest_block macro
On Mon, Aug 12, 2013 at 11:09:56AM +0200, Daniel Vetter wrote: Doesn't do more than an if (drmtest_run_test(name)) right now, but as soon as we get a bit of infrastructure to handle test failures and skipping, this will get more interesting. Just use drm_subtest(name) { } drm_subtest_block() reads like a function and so one expects block to be a verb. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] lib/drmtest: Add drmtest_subtest_block macro
On Mon, Aug 12, 2013 at 10:15:06AM +0100, Chris Wilson wrote: On Mon, Aug 12, 2013 at 11:09:56AM +0200, Daniel Vetter wrote: Doesn't do more than an if (drmtest_run_test(name)) right now, but as soon as we get a bit of infrastructure to handle test failures and skipping, this will get more interesting. Just use drm_subtest(name) { } drm_subtest_block() reads like a function and so one expects block to be a verb. The intention was to make clear that the macro expects a C code block afterwards. See the later patches for some of the fun I add ... I see that the _block suffix can be misread, but tbh I don't have any other ideas that might properly convey what this thing is. The 2nd best I could think of is do_subtest(name) similar to do {} while (). But that's not great either. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/7] drmtest framework support for retval handling
On Mon, Aug 12, 2013 at 11:15:53AM +0200, Daniel Vetter wrote: On Mon, Aug 12, 2013 at 11:09:54AM +0200, Daniel Vetter wrote: Hi all, So I've grown fed-up with our add-hoc (and pretty much always buggy) return value tracking for testcases with subtests. Furthermore sprinkling testcases with tons of if (kernel_has_some_optional_feature) return 77; isn't really helping test case readability. So I've gone ahead and implement some magic stuff using setjmp/longjmp and a few macros. On top of that a few exemplary conversions of existing testcases. Comments highly welcome. Cheers, Daniel Just noticed that this already doesn't apply, so I've pushed a rebased version to http://cgit.freedesktop.org/~danvet/intel-gpu-tools/log/?h=retval-infrastructure That one also contains one hunk I've failed to git add. One more thing I've forgotten to mention is that I want to add a drmtest_assert macro on top of this which would replace allmost all usage of assert in lib/drmtest and tests/*. With a few useful printks sprinkled over drmtest_run_subtest and the drmtest_(skip|success|fail) functions we could dump per-subtest results even when running with the legacy igt automake testrunner. And with that we could also make sure that we run all subtests and don't just fall over after the first one completed. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Align tiled scanouts from stolen memory to 256k in the GTT
On Sun, Aug 11, 2013 at 11:17:28PM +0100, Chris Wilson wrote: For unfathomable reasons this alignment appears to be required for tiled scanouts being read from stolen memory. I can find no reference in the w/a db to support this requirement, but the evidence of my own eyes says this prevents many headaches. Note that I have not tricked anything older than Sandybridge into using stolen tiled scanouts, so the extra alignment may be required there as well. Strange. I can't find anything except async flips and vt-d which would require 256k alignment. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_display.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6b7ce06..a7573f2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1848,6 +1848,8 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev, case I915_TILING_X: /* pin() will align the object as required by fence */ alignment = 0; + if (obj-stolen INTEL_INFO(dev)-gen = 6) + alignment = 256 * 1024; break; case I915_TILING_Y: /* Despite that we check this in framebuffer_init userspace can -- 1.8.4.rc2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Align tiled scanouts from stolen memory to 256k in the GTT
On Mon, Aug 12, 2013 at 12:38:03PM +0300, Ville Syrjälä wrote: On Sun, Aug 11, 2013 at 11:17:28PM +0100, Chris Wilson wrote: For unfathomable reasons this alignment appears to be required for tiled scanouts being read from stolen memory. I can find no reference in the w/a db to support this requirement, but the evidence of my own eyes says this prevents many headaches. Note that I have not tricked anything older than Sandybridge into using stolen tiled scanouts, so the extra alignment may be required there as well. Strange. I can't find anything except async flips and vt-d which would require 256k alignment. I can empathically state it is required though. It might not be 256k, that's just the first value that stopped giving me headaches. :) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] lib/drmtest: Add drmtest_subtest_block macro
On Mon, Aug 12, 2013 at 11:26:49AM +0200, Daniel Vetter wrote: On Mon, Aug 12, 2013 at 10:15:06AM +0100, Chris Wilson wrote: On Mon, Aug 12, 2013 at 11:09:56AM +0200, Daniel Vetter wrote: Doesn't do more than an if (drmtest_run_test(name)) right now, but as soon as we get a bit of infrastructure to handle test failures and skipping, this will get more interesting. Just use drm_subtest(name) { } drm_subtest_block() reads like a function and so one expects block to be a verb. The intention was to make clear that the macro expects a C code block afterwards. See the later patches for some of the fun I add ... I see that the _block suffix can be misread, but tbh I don't have any other ideas that might properly convey what this thing is. The 2nd best I could think of is do_subtest(name) similar to do {} while (). But that's not great either. Ok, I've applied the following three bikesheds onto my tree: - s/drmtest_run_subtest/__drmtest_run_subtest/ since that fuction is now internal to drmtest.c (but needs to be in the header due to the fancy macro). - s/drmtest_subtest_block/drmtest/subtest/ ... see above. - s/drmtest_/igt_/ just because (suggested by Chris on irc). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] lib/drmtest: Add drmtest_subtest_block macro
On Mon, Aug 12, 2013 at 12:20:24PM +0200, Daniel Vetter wrote: On Mon, Aug 12, 2013 at 11:26:49AM +0200, Daniel Vetter wrote: On Mon, Aug 12, 2013 at 10:15:06AM +0100, Chris Wilson wrote: On Mon, Aug 12, 2013 at 11:09:56AM +0200, Daniel Vetter wrote: Doesn't do more than an if (drmtest_run_test(name)) right now, but as soon as we get a bit of infrastructure to handle test failures and skipping, this will get more interesting. Just use drm_subtest(name) { } drm_subtest_block() reads like a function and so one expects block to be a verb. The intention was to make clear that the macro expects a C code block afterwards. See the later patches for some of the fun I add ... I see that the _block suffix can be misread, but tbh I don't have any other ideas that might properly convey what this thing is. The 2nd best I could think of is do_subtest(name) similar to do {} while (). But that's not great either. Ok, I've applied the following three bikesheds onto my tree: - s/drmtest_run_subtest/__drmtest_run_subtest/ since that fuction is now internal to drmtest.c (but needs to be in the header due to the fancy macro). - s/drmtest_subtest_block/drmtest/subtest/ ... see above. - s/drmtest_/igt_/ just because (suggested by Chris on irc). One more on top: - s/return igt_retval()/igt_exit()/ I'll push this one the US has woken up an had a chance to comment. There's too much tree-wide sed and sed-like stuff in this series now. Latest patches pushed to my private repo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Drop the overzealous warning from i915_gem_set_cache_level
By our earlier reckoning, move from a snooped/llc setting to an uncached setting, leaves the CPU cache in a consistent state irrespective of our domain tracking - so we can forgo the warning about the lack of invalidation. Similarly for any writes posted to the snooped CPU domain, we know will be safely clflushed to the uncached PTEs after forcing the domain change. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_gem.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 925c77d..1d3e57e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3520,7 +3520,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, * Just set it to the CPU cache for now. */ WARN_ON(obj-base.write_domain ~I915_GEM_DOMAIN_CPU); - WARN_ON(obj-base.read_domains ~I915_GEM_DOMAIN_CPU); old_read_domains = obj-base.read_domains; old_write_domain = obj-base.write_domain; -- 1.8.4.rc2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] tests: scrap old automake based kernel test runner
Upstream broke our dynamic creation of the testlist, and I think adding stupid .tests suffixes everywhere just to appease upstream autohell tools isn't that great. So scrap it, we can use piglit instead. References: https://lists.gnu.org/archive/html/help-debbugs/2013-06/msg0.html Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- Makefile.am | 3 --- README| 25 ++--- tests/Makefile.am | 5 - 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/Makefile.am b/Makefile.am index 67b6563..c4c8de1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -35,9 +35,6 @@ if BUILD_TESTS SUBDIRS += tests endif -test: - ${MAKE} -C tests test - MAINTAINERCLEANFILES = ChangeLog INSTALL .PHONY: ChangeLog INSTALL diff --git a/README b/README index c484a93..d4325be 100644 --- a/README +++ b/README @@ -24,17 +24,10 @@ tests/ changes. Hopefully this can cover the relevant cases we need to worry about, including backwards compatibility. - Run this tests with make test as root from this directory. Note that - no other drm clients (X server) may run. - - make test only runs a default set of tests and is useful for - regression testing. Other tests not run are: - - tests that might hang the gpu, see HANG in Makefile.am - - gem_stress, a stress test suite. Look at the source for all the - various options. - - testdisplay is only run in the default mode. testdisplay has tons of - options to test different kms functionality, again read the source for - the details. + Note: The old automake based testrunner had to be scraped due to + upstream changes which broke dynamic creation of the test list. Of + course it is still possible to directly run tests, even when not always + limiting tests to specific subtests (like piglit does). The more comfortable way to run tests is with piglit. First grab piglit from: @@ -60,6 +53,16 @@ tests/ for some useful options. + Piglit only runs a default set of tests and is useful for regression + testing. Other tests not run are: + - tests that might hang the gpu, see HANG in Makefile.am + - gem_stress, a stress test suite. Look at the source for all the + various options. + - testdisplay is only run in the default mode. testdisplay has tons of + options to test different kms functionality, again read the source for + the details. + + lib/ Common helper functions and headers used by the other tools. diff --git a/tests/Makefile.am b/tests/Makefile.am index f3475ad..ebd7c57 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -150,11 +150,6 @@ kernel_tests = \ TESTS = \ $(NULL) -test: - @whoami | grep -q root || ( echo ERROR: not running as root; exit 1 ) - @./check_drm_clients - @make TESTS=${kernel_tests} check - list-single-tests: @echo TESTLIST @echo ${single_kernel_tests} -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] tests: scrap old automake based kernel test runner
On Mon, Aug 12, 2013 at 02:03:40PM +0200, Daniel Vetter wrote: Upstream broke our dynamic creation of the testlist, and I think adding stupid .tests suffixes everywhere just to appease upstream autohell tools isn't that great. So scrap it, we can use piglit instead. References: https://lists.gnu.org/archive/html/help-debbugs/2013-06/msg0.html Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch If you need one of those: Acked-by: Damien Lespiau damien.lesp...@intel.com --- Makefile.am | 3 --- README| 25 ++--- tests/Makefile.am | 5 - 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/Makefile.am b/Makefile.am index 67b6563..c4c8de1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -35,9 +35,6 @@ if BUILD_TESTS SUBDIRS += tests endif -test: - ${MAKE} -C tests test - MAINTAINERCLEANFILES = ChangeLog INSTALL .PHONY: ChangeLog INSTALL diff --git a/README b/README index c484a93..d4325be 100644 --- a/README +++ b/README @@ -24,17 +24,10 @@ tests/ changes. Hopefully this can cover the relevant cases we need to worry about, including backwards compatibility. - Run this tests with make test as root from this directory. Note that - no other drm clients (X server) may run. - - make test only runs a default set of tests and is useful for - regression testing. Other tests not run are: - - tests that might hang the gpu, see HANG in Makefile.am - - gem_stress, a stress test suite. Look at the source for all the - various options. - - testdisplay is only run in the default mode. testdisplay has tons of - options to test different kms functionality, again read the source for - the details. + Note: The old automake based testrunner had to be scraped due to + upstream changes which broke dynamic creation of the test list. Of + course it is still possible to directly run tests, even when not always + limiting tests to specific subtests (like piglit does). The more comfortable way to run tests is with piglit. First grab piglit from: @@ -60,6 +53,16 @@ tests/ for some useful options. + Piglit only runs a default set of tests and is useful for regression + testing. Other tests not run are: + - tests that might hang the gpu, see HANG in Makefile.am + - gem_stress, a stress test suite. Look at the source for all the + various options. + - testdisplay is only run in the default mode. testdisplay has tons of + options to test different kms functionality, again read the source for + the details. + + lib/ Common helper functions and headers used by the other tools. diff --git a/tests/Makefile.am b/tests/Makefile.am index f3475ad..ebd7c57 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -150,11 +150,6 @@ kernel_tests = \ TESTS = \ $(NULL) -test: - @whoami | grep -q root || ( echo ERROR: not running as root; exit 1 ) - @./check_drm_clients - @make TESTS=${kernel_tests} check - list-single-tests: @echo TESTLIST @echo ${single_kernel_tests} -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 9/9] drm/i915: Allow the user to set bo into the DISPLAY cache domain
On Sat, Aug 10, 2013 at 12:09:26PM +0200, Daniel Vetter wrote: On Thu, Aug 08, 2013 at 02:41:11PM +0100, Chris Wilson wrote: This is primarily for the benefit of the create2 ioctl so that the caller can avoid the later step of rebinding the bo with new PTE bits. After introducing WT (and possibly GFDT) cacheing for display targets, not everything in the display is earmarked as UC, and more importantly what is is controlled by the kernel. Note that set_cache_level/get_cache_level for DISPLAY is not necessarily idempotent; get_cache_level may return UC for architectures that have no special cache domain for the display engine. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk You know the drill: A bit of igt testcoverage would be neat, I think simply adding the display domain everywhere we test uncached/snooped already should be more than good enough. So I'll punt on this one here for now, all other patches (with the exception of the hw context from stolen one) are merged to dinq. Ok, merged this one and the previous one (which I've thought I've merged, but apparently didn't). Thanks for the patches and writing the igts. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: drop unnecessary local variable to suppress build warning
On Sun, Aug 11, 2013 at 3:15 AM, Daniel Vetter dan...@ffwll.ch wrote: On Sun, Aug 11, 2013 at 10:50:17AM +0100, Chris Wilson wrote: On Sun, Aug 11, 2013 at 12:44:02PM +0300, Jani Nikula wrote: Although I could not reproduce this (different compiler version, perhaps), reportedly we get: drivers/gpu/drm/i915/i915_irq.c:1943:27: warning: ‘score’ may be used uninitialized in this function [-Wuninitialized] Drop the 'score' variable altogether as it's not really needed. Right, it was only there because I thought the switch was more readable with the temporary. Reported-by: Kees Cook keesc...@chromium.org Go fix your compiler! Indeed, Kees seems to have an especially dense bread of gcc ;-) 4.6.3! :P What're you all using? (Regardless, I'm looking forward to 4.9.) Signed-off-by: Jani Nikula jani.nik...@intel.com Both patches Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Both merged to dinq, thanks for the patches and review. Cool. My stderr during compilation thanks you. :) -Kees -- Kees Cook Chrome OS Security ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't load context at driver init time on SNB
On Fri, Aug 9, 2013 at 9:55 PM, Ben Widawsky b...@bwidawsk.net wrote: On Fri, Aug 09, 2013 at 08:32:54PM -0700, Stéphane Marchesin wrote: This is a partial revert of b4ae3f22d238617ca11610b29fde16cf8c0bc6e0 (drm/i915: load boot context at driver init time) This bit breaks hardware video decode for me after resume. Signed-off-by: Stéphane Marchesin marc...@chromium.org --- drivers/gpu/drm/i915/intel_pm.c | 4 1 file changed, 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f895d15..ffa4ab2 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4614,10 +4614,6 @@ static void gen6_init_clock_gating(struct drm_device *dev) ILK_DPARBUNIT_CLOCK_GATE_ENABLE | ILK_DPFDUNIT_CLOCK_GATE_ENABLE); - /* WaMbcDriverBootEnable:snb */ - I915_WRITE(GEN6_MBCTL, I915_READ(GEN6_MBCTL) | -GEN6_MBCTL_ENABLE_BOOT_FETCH); - g4x_disable_trickle_feed(dev); /* The default value should be 0x200 according to docs, but the two I was looking at this a bit with Stéphane. One thing we both see is that the bit isn't sticking as it should. Clearly doing the write is having an effect, but something is seriously wrong. Writing the bit manually with IGT does make the bit stick. Stéphane, could you try to write the bit with IGT before and after resume, and see if it helps? It doesn't seem to stick, and so seems to have no effect. The confusing thing is that video decode works fine before suspend, even though that reg is 0. And after resume, it is broken, and that reg is still 0. So something else is going on. Maybe this reg is write-once-ever? Stéphane ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: print a message when we detect an early Haswell SDV
From: Paulo Zanoni paulo.r.zan...@intel.com The machines that fall in this category are the SDVs that have a PCI ID starting with 0x0C. These are very early pre-production machines and may not fully work. Other Haswell SDVs have PCI IDs that match the real Haswell machines and we expect them to work better. Even though they have problems, they still mostly work so I don't see a reason to refuse loading our driver. But I do see a reason to reject bug reports from these machines, so the message should help the bug triagers. As far as I know, we don't implement some workarounds that are specific to these machines and suspend/resume may not work on most of them, but besides this, they may work. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61508 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_dma.c | 8 drivers/gpu/drm/i915/i915_drv.h | 2 ++ 2 files changed, 10 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 0adfe40..908442e 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1485,6 +1485,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) i915_dump_device_info(dev_priv); + /* Not all pre-production machines fall into this category, only the +* very first ones. Almost everything should work, except for maybe +* suspend/resume. And we don't implement workarounds that affect only +* pre-production machines. */ + if (IS_HSW_EARLY_SDV(dev)) + DRM_INFO(This is an early pre-production Haswell machine. +It may not be fully functional.\n); + if (i915_get_bridge_dev(dev)) { ret = -EIO; goto free_priv; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index cccd608..3f1f067 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1510,6 +1510,8 @@ struct drm_i915_file_private { #define IS_VALLEYVIEW(dev) (INTEL_INFO(dev)-is_valleyview) #define IS_HASWELL(dev)(INTEL_INFO(dev)-is_haswell) #define IS_MOBILE(dev) (INTEL_INFO(dev)-is_mobile) +#define IS_HSW_EARLY_SDV(dev) (IS_HASWELL(dev) \ +((dev)-pci_device 0xFF00) == 0x0C00) #define IS_ULT(dev)(IS_HASWELL(dev) \ ((dev)-pci_device 0xFF00) == 0x0A00) -- 1.8.1.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: remove set but unused variables
From: Paulo Zanoni paulo.r.zan...@intel.com Caught by make W=1 drivers/gpu/drm/i915/. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_display.c | 12 ++-- drivers/gpu/drm/i915/intel_dp.c | 3 --- drivers/gpu/drm/i915/intel_hdmi.c| 2 -- 3 files changed, 2 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index af1b2f0..cec9189 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -690,7 +690,7 @@ vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc, { u32 p1, p2, m1, m2, vco, bestn, bestm1, bestm2, bestp1, bestp2; u32 m, n, fastclk; - u32 updrate, minupdate, fracbits, p; + u32 updrate, minupdate, p; unsigned long bestppm, ppm, absppm; int dotclk, flag; @@ -701,7 +701,6 @@ vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc, fastclk = dotclk / (2*100); updrate = 0; minupdate = 19200; - fracbits = 1; n = p = p1 = p2 = m = m1 = m2 = vco = bestn = 0; bestm1 = bestm2 = bestp1 = bestp2 = 0; @@ -4423,13 +4422,10 @@ static void vlv_update_pll(struct intel_crtc *crtc) int pipe = crtc-pipe; u32 dpll, mdiv; u32 bestn, bestm1, bestm2, bestp1, bestp2; - bool is_hdmi; u32 coreclk, reg_val, dpll_md; mutex_lock(dev_priv-dpio_lock); - is_hdmi = intel_pipe_has_type(crtc-base, INTEL_OUTPUT_HDMI); - bestn = crtc-config.dpll.n; bestm1 = crtc-config.dpll.m1; bestm2 = crtc-config.dpll.m2; @@ -8896,14 +8892,13 @@ intel_modeset_stage_output_state(struct drm_device *dev, struct drm_crtc *new_crtc; struct intel_connector *connector; struct intel_encoder *encoder; - int count, ro; + int ro; /* The upper layers ensure that we either disable a crtc or have a list * of connectors. For paranoia, double-check this. */ WARN_ON(!set-fb (set-num_connectors != 0)); WARN_ON(set-fb (set-num_connectors == 0)); - count = 0; list_for_each_entry(connector, dev-mode_config.connector_list, base.head) { /* Otherwise traverse passed in connector list and get encoders @@ -8937,7 +8932,6 @@ intel_modeset_stage_output_state(struct drm_device *dev, /* connector-new_encoder is now updated for all connectors. */ /* Update crtc of enabled connectors. */ - count = 0; list_for_each_entry(connector, dev-mode_config.connector_list, base.head) { if (!connector-new_encoder) @@ -10297,7 +10291,6 @@ void intel_modeset_cleanup(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; struct drm_crtc *crtc; - struct intel_crtc *intel_crtc; /* * Interrupts and polling as the first thing to avoid creating havoc. @@ -10321,7 +10314,6 @@ void intel_modeset_cleanup(struct drm_device *dev) if (!crtc-fb) continue; - intel_crtc = to_intel_crtc(crtc); intel_increase_pllclock(crtc); } diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 63b6722d..2726d4d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2326,7 +2326,6 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) struct drm_device *dev = encoder-dev; int i; uint8_t voltage; - bool clock_recovery = false; int voltage_tries, loop_tries; uint32_t DP = intel_dp-DP; @@ -2344,7 +2343,6 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) voltage = 0xff; voltage_tries = 0; loop_tries = 0; - clock_recovery = false; for (;;) { /* Use intel_dp-train_set[0] to set the voltage and pre emphasis values */ uint8_t link_status[DP_LINK_STATUS_SIZE]; @@ -2365,7 +2363,6 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) if (drm_dp_clock_recovery_ok(link_status, intel_dp-lane_count)) { DRM_DEBUG_KMS(clock recovery OK\n); - clock_recovery = true; break; } diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index dd4fa35..a619d94 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1245,7 +1245,6 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port) { struct intel_digital_port *intel_dig_port; struct intel_encoder *intel_encoder; - struct drm_encoder *encoder; struct intel_connector *intel_connector; intel_dig_port = kzalloc(sizeof(struct intel_digital_port), GFP_KERNEL); @@ -1259,7
Re: [Intel-gfx] [PATCH] drm/i915: VGA also requires the power well
-Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Sunday, August 04, 2013 4:50 PM To: Paulo Zanoni Cc: Ville Syrjälä; intel-gfx@lists.freedesktop.org; Zanoni, Paulo R Subject: Re: [Intel-gfx] [PATCH] drm/i915: VGA also requires the power well On Fri, Aug 02, 2013 at 02:17:53PM -0300, Paulo Zanoni wrote: 2013/6/6 Ville Syrjälä ville.syrj...@linux.intel.com: On Wed, Jun 05, 2013 at 06:05:51PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com So add a power domain and check for it before we try to read VGA_CONTROL. This fixes unclaimed register messages that happen on suspend/resume. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_display.c | 3 +++ drivers/gpu/drm/i915/intel_pm.c | 1 + 3 files changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 46b1f70..d51ce13 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -89,6 +89,7 @@ enum port { #define port_name(p) ((p) + 'A') enum intel_display_power_domain { + POWER_DOMAIN_VGA, POWER_DOMAIN_PIPE_A, POWER_DOMAIN_PIPE_B, POWER_DOMAIN_PIPE_C, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4c8fcec..3719d99 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9950,6 +9950,9 @@ void i915_redisable_vga(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; u32 vga_reg = i915_vgacntrl_reg(dev); + if (!intel_display_power_enabled(dev, POWER_DOMAIN_VGA)) + return; + So it looks like you're essentially making intel_redisable_vga() a nop for HSW. Shouldn't we instead enable the power well during resume? We enable the power well during resume, but it's only after this function... Hm, so better move the (temporary) power well enabling in the resume code up a bit to cover this? I don't think so. If you look at i915_redisable_vga and commit 0fde901f1ddd2ce0e380a6444f1fb7ca555859e9, you will start thinking that just closing/opening the lid could make the BIOS somehow enable the power well and then reenable VGA, so moving the check to earlier in the resume sequence won't solve any problems, as VGA could be reenabled later. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: remove set but unused variables
On Mon, Aug 12, 2013 at 02:56:53PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Caught by make W=1 drivers/gpu/drm/i915/. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Drop the overzealous warning from i915_gem_set_cache_level
On Mon, Aug 12, 2013 at 11:46:17AM +0100, Chris Wilson wrote: By our earlier reckoning, move from a snooped/llc setting to an uncached setting, leaves the CPU cache in a consistent state irrespective of our domain tracking - so we can forgo the warning about the lack of invalidation. Similarly for any writes posted to the snooped CPU domain, we know will be safely clflushed to the uncached PTEs after forcing the domain change. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com I ran into this several times while doing the PPGTT development, and was always scared to just remove it. Does it make sense to keep the write_domain assertion with this gone? --- drivers/gpu/drm/i915/i915_gem.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 925c77d..1d3e57e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3520,7 +3520,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, * Just set it to the CPU cache for now. */ WARN_ON(obj-base.write_domain ~I915_GEM_DOMAIN_CPU); - WARN_ON(obj-base.read_domains ~I915_GEM_DOMAIN_CPU); old_read_domains = obj-base.read_domains; old_write_domain = obj-base.write_domain; -- 1.8.4.rc2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Drop the overzealous warning from i915_gem_set_cache_level
On Mon, Aug 12, 2013 at 02:02:09PM -0700, Ben Widawsky wrote: On Mon, Aug 12, 2013 at 11:46:17AM +0100, Chris Wilson wrote: By our earlier reckoning, move from a snooped/llc setting to an uncached setting, leaves the CPU cache in a consistent state irrespective of our domain tracking - so we can forgo the warning about the lack of invalidation. Similarly for any writes posted to the snooped CPU domain, we know will be safely clflushed to the uncached PTEs after forcing the domain change. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com I ran into this several times while doing the PPGTT development, and was always scared to just remove it. Does it make sense to keep the write_domain assertion with this gone? I think we've justified in the earlier series why we can drop the WARN_ON(write) with impunity. As we don't need to do so immediately, I'd like to sleep on it for a while. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled)
2013/8/10 Chris Wilson ch...@chris-wilson.co.uk: On Sat, Aug 10, 2013 at 09:55:14AM +0200, Daniel Vetter wrote: On Fri, Aug 9, 2013 at 11:34 PM, Paulo Zanoni przan...@gmail.com wrote: 2013/8/9 Chris Wilson ch...@chris-wilson.co.uk: Quick note... On Fri, Aug 09, 2013 at 05:10:05PM -0300, Paulo Zanoni wrote: + WARN_ON(!mutex_is_locked(dev_priv-pc8.lock)); Preferred form is now lockdep_assert_held(dev_priv-pc8.lock); Should I also convert all our other usages of WARN_ON(!mutex_is_locked()) and BUG_ON(!mutex_is_locked()) too? On a separate patch, of course. We have currently no usage of lockdep_assert_held, and I like consistency, so fully switching to the preferred form is good IMHO. Tbh I don't understand really why lockdep_assert_held is better ... it's right that it also checks that indeed the current task is holding the lock (and not some random other imposter). But the downside is that it's a noop without CONFIG_PROVE_LOCKING. And due to the massive perf impact of that option not many people actually run with it. At least I tend to only enable it when doing tricky locking work on my dev machines and not in general ... It doesn't shout so much and people are starting to complain about all the sanity checks existing outside of the grand lockdep. Who doesn't have a few machines running with lockdep all the time? ;-) Considering we still don't have a consensus, I'll keep the WARNs so our driver stays consistent. If I see patches converting everything on our driver to lockdep_assert_held, then I'll update this patch. IMHO we should either convert everything or nothing. -Chris -- Chris Wilson, Intel Open Source Technology Centre -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Initialize seqno for VECS too
Cc: Mika Kuoppala mika.kuopp...@intel.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=65387 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67198 Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 10c2aaa..665602f 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1592,6 +1592,8 @@ void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno) if (INTEL_INFO(ring-dev)-gen = 6) { I915_WRITE(RING_SYNC_0(ring-mmio_base), 0); I915_WRITE(RING_SYNC_1(ring-mmio_base), 0); + if (HAS_VEBOX(ring-dev)) + I915_WRITE(RING_SYNC_2(ring-mmio_base), 0); } ring-set_seqno(ring, seqno); -- 1.8.3.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Get VECS semaphore info on error
Ideally we could use for_each_ring with the ring flags as I've done a couple times (http://lists.freedesktop.org/archives/intel-gfx/2013-June/029450.html). Until Daniel merges that patch though, we can just use this. Cc: Mika Kuoppala mika.kuopp...@intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gpu_error.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 60393cb..558e568 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -243,6 +243,11 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m, err_printf(m, SYNC_1: 0x%08x [last synced 0x%08x]\n, error-semaphore_mboxes[ring][1], error-semaphore_seqno[ring][1]); + if (HAS_VEBOX(dev)) { + err_printf(m, SYNC_2: 0x%08x [last synced 0x%08x]\n, + error-semaphore_mboxes[ring][2], + error-semaphore_seqno[ring][2]); + } } err_printf(m, seqno: 0x%08x\n, error-seqno[ring]); err_printf(m, waiting: %s\n, yesno(error-waiting[ring])); @@ -682,6 +687,12 @@ static void i915_record_ring_state(struct drm_device *dev, error-semaphore_seqno[ring-id][1] = ring-sync_seqno[1]; } + if (HAS_VEBOX(dev)) { + error-semaphore_mboxes[ring-id][2] = + I915_READ(RING_SYNC_2(ring-mmio_base)); + error-semaphore_seqno[ring-id][2] = ring-sync_seqno[2]; + } + if (INTEL_INFO(dev)-gen = 4) { error-faddr[ring-id] = I915_READ(RING_DMA_FADD(ring-mmio_base)); error-ipeir[ring-id] = I915_READ(RING_IPEIR(ring-mmio_base)); -- 1.8.3.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/i915: bind mf cleanup
On Sat, Aug 10, 2013 at 09:41:16AM +0100, Chris Wilson wrote: On Fri, Aug 09, 2013 at 10:12:13PM -0700, Ben Widawsky wrote: Cleanup the map and fenceable setting during bind to make more sense, and not check i915_is_ggtt() 2 unnecessary times Recommended-by: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Ben Widawsky b...@bwidawsk.net There were a couple of bools that only exist to make this computation more readable that could be moved to this block. -Chris I saw these. The cleanup is somewhat involved, so I punted on it. I'll implement it now, but I expect you to review it. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/5] [v2] drm/i915: cleanup mapfence in bind
Cleanup the map and fenceable setting during bind to make more sense, and not check i915_is_ggtt() 2 unnecessary times v2: Move the bools into the if block (Chris) - There are ways to tidy this function (fence calculations for instance) even further, but they are quite invasive, so I am punting on those unless specifically asked. Recommended-by: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c | 18 -- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index b91a7f0..8dfe6d5 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3088,7 +3088,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, struct drm_device *dev = obj-base.dev; drm_i915_private_t *dev_priv = dev-dev_private; u32 size, fence_size, fence_alignment, unfenced_alignment; - bool mappable, fenceable; size_t gtt_max = map_and_fenceable ? dev_priv-gtt.mappable_end : vm-total; struct i915_vma *vma; @@ -3173,18 +3172,17 @@ search_free: list_move_tail(obj-global_list, dev_priv-mm.bound_list); list_add_tail(vma-mm_list, vm-inactive_list); - fenceable = - i915_is_ggtt(vm) - i915_gem_obj_ggtt_size(obj) == fence_size - (i915_gem_obj_ggtt_offset(obj) (fence_alignment - 1)) == 0; + if (i915_is_ggtt(vm)) { + bool mappable, fenceable; + fenceable = + i915_gem_obj_ggtt_size(obj) == fence_size + (i915_gem_obj_ggtt_offset(obj) (fence_alignment - 1)) == 0; - mappable = - i915_is_ggtt(vm) - vma-node.start + obj-base.size = dev_priv-gtt.mappable_end; + mappable = + vma-node.start + obj-base.size = dev_priv-gtt.mappable_end; - /* Map and fenceable only changes if the VM is the global GGTT */ - if (i915_is_ggtt(vm)) obj-map_and_fenceable = mappable fenceable; + } WARN_ON(map_and_fenceable !obj-map_and_fenceable); -- 1.8.3.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915: Remove node only when allocated
On Sat, Aug 10, 2013 at 09:45:05AM +0100, Chris Wilson wrote: On Fri, Aug 09, 2013 at 10:12:14PM -0700, Ben Widawsky wrote: In upcoming code, it will be possible for a vma to have been created, but no space reserved for it in the address space. The drm_mm semantics are such that trying to remove an unallocated node is not allowed. But not allocated during unbind, i.e. calling unbind() before bind()? That seems scary enough. -Chris The condition can occur if we create a vma, fail to bind it, and then free up the object (which tries to unbind). As I alluded to, this cannot happen until we do the execbufer vma creation. The example for which is can happen is if we have some object created, with a vma AFAICT, this condition cannot occur until we actually are using multiple VMs, but once we are the following can occur: object X created for context Y VMA-Y created for object X VMA-Z created for object X, but fails before or during bind VMA-Z persists object X destroyed free calls unbind on all VMAs. VMA-Z has no node allocated for it. One way to may this better is to not call unbind() if it isn't allocated, and simply call vma_destroy(). I don't really care. You tell me what you want. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Convert execbuf code to use vmas
On Sat, Aug 10, 2013 at 10:13:20AM +0100, Chris Wilson wrote: On Fri, Aug 09, 2013 at 10:12:16PM -0700, Ben Widawsky wrote: static int i915_gem_execbuffer_move_to_gpu(struct intel_ring_buffer *ring, - struct list_head *objects) + struct list_head *vmas) { - struct drm_i915_gem_object *obj; + struct i915_vma *vma; uint32_t flush_domains = 0; int ret; - list_for_each_entry(obj, objects, exec_list) { - ret = i915_gem_object_sync(obj, ring); + list_for_each_entry(vma, vmas, exec_list) { + ret = i915_gem_object_sync(vma-obj, ring); if (ret) return ret; - if (obj-base.write_domain I915_GEM_DOMAIN_CPU) - i915_gem_clflush_object(obj); + if (vma-obj-base.write_domain I915_GEM_DOMAIN_CPU) + i915_gem_clflush_object(vma-obj); - flush_domains |= obj-base.write_domain; + flush_domains |= vma-obj-base.write_domain; This, and a few more places, will be neater with a local struct drm_i915_gem_object *obj = vma-obj; -Chris I got this and the other fixed you requested for needs_reloc_mappable. I only found one other place that had more than one instance of vma-obj though. i915_gem_execbuffer_unreserve_vma i915_gem_execbuffer_move_to_gpu Please let me know if I missed one. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx