Re: [Intel-gfx] [PATCH] drm/i915: Asynchronously perform the set-base for a simple modeset

2013-08-12 Thread Ville Syrjälä
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Chris Wilson
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Ville Syrjälä
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

2013-08-12 Thread Chris Wilson
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Chris Wilson
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Damien Lespiau
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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Kees Cook
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

2013-08-12 Thread Stéphane Marchesin
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

2013-08-12 Thread Paulo Zanoni
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

2013-08-12 Thread Paulo Zanoni
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

2013-08-12 Thread Zanoni, Paulo R
 -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

2013-08-12 Thread Daniel Vetter
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

2013-08-12 Thread Ben Widawsky
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

2013-08-12 Thread Chris Wilson
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-08-12 Thread Paulo Zanoni
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

2013-08-12 Thread Ben Widawsky
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

2013-08-12 Thread Ben Widawsky
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

2013-08-12 Thread Ben Widawsky
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

2013-08-12 Thread Ben Widawsky
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

2013-08-12 Thread Ben Widawsky
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

2013-08-12 Thread Ben Widawsky
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