Re: [Intel-gfx] [REGRESSION BISECTED] backlight control stops workin with 3.14 and later
Hi, On 07/22/2014 08:52 AM, Daniel Vetter wrote: On Tue, Jul 22, 2014 at 8:42 AM, Hans de Goede hdego...@redhat.com wrote: Hi Jani et al, A friend of mine Bertrik Sikken (in the Cc) his backlight control stopped working for him on his Samsung N150Plus netbook. I took a quick look, and the raw intel_backlight backlight interface works under 3.14, but the firmware samsung_laptop backlight interface, which is what most userspace apps will use by default, stops working in 3.14 . I've asked him to bisect this and the bisect points out this commit as the culprit: b35684b8fa94e04f55fd38bf672b737741d2f9e2 is the first bad commit commit b35684b8fa94e04f55fd38bf672b737741d2f9e2 Author: Jani Nikula jani.nik...@intel.com Date: Thu Nov 14 12:13:41 2013 +0200 drm/i915: do full backlight setup at enable time We should now have all the information we need to do a full initialization of the backlight registers. v2: Keep QUIRK_NO_PCH_PWM_ENABLE for now (Imre). Signed-off-by: Jani Nikula jani.nik...@intel.com Reviewed-by: Imre Deak imre.d...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Note that this laptop has an acpi_video backlight interface too, but that has been broken from the start and gets disabled by samsung-laptop based on dmi matching. How does the intel backlight fare? That works fine with 3.14 . Please test both 3.14 and 3.15 and We've tested with 3.14, please let us know if you also want Bertrik to test with 3.15. also test what happens when you blacklist the samsung-laptop driver (if that's possible without wreaking the machine). Then the vendor interface won't get promoted, acpi-video will load, and things likely will not work. Bertrik, can you try blacklisting the samsung-laptop module, then check /sys/class/backlight, the samsung_laptop dir should be gone replaced by an acpi_video0 (or some such) dir. Please try if that works. If that does not work, try booting with both the module blacklisted and acpi_backlight=vendor on the kernel commandline, then you should see only the intel-backlight under /sys/class/backlight and things should work. Also please grab latest intel-gpu-tools and record a register dump with intel_reg_dump, again for both broken and working kernels. Bertrik, can you do this please (without the blacklisting or special kernel commandline options). Regards, Hans ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 6/8] tests: convert simple tests to use igt_simple_init_parse_opts
Convert simple tests to use igt_simple_init_parse_opts if they require
extra options.
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.h | 1 +
tests/gem_ctx_basic.c | 20 +++--
tests/gem_render_copy.c | 25 +---
tests/gem_seqno_wrap.c | 82 +++--
tests/gem_stress.c | 105 ++--
5 files changed, 98 insertions(+), 135 deletions(-)
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 408cf3a..b19a897 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -36,6 +36,7 @@
#include string.h
#include sys/types.h
#include stdarg.h
+#include getopt.h
/**
* IGT_EXIT_TIMEOUT:
diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c
index a2464fd..a0bec60 100644
--- a/tests/gem_ctx_basic.c
+++ b/tests/gem_ctx_basic.c
@@ -39,7 +39,6 @@
#include errno.h
#include sys/stat.h
#include sys/time.h
-#include getopt.h
#include drm.h
#include ioctl_wrappers.h
#include drmtest.h
@@ -119,11 +118,9 @@ static void *work(void *arg)
pthread_exit(NULL);
}
-static void parse(int argc, char *argv[])
+static int opt_handler(int opt, int opt_index)
{
- int opt;
- while ((opt = getopt(argc, argv, i:c:n:muh?)) != -1) {
- switch (opt) {
+ switch (opt) {
case 'i':
iter = atoi(optarg);
break;
@@ -136,20 +133,17 @@ static void parse(int argc, char *argv[])
case 'u':
uncontexted = 1;
break;
- case 'h':
- case '?':
- default:
- igt_success();
- break;
- }
}
+
+ return 0;
}
int main(int argc, char *argv[])
{
int i;
- igt_simple_init(argc, argv);
+ igt_simple_init_parse_opts(argc, argv, i:c:n:mu, NULL, NULL,
+ opt_handler);
fd = drm_open_any_render();
devid = intel_get_drm_devid(fd);
@@ -159,8 +153,6 @@ int main(int argc, char *argv[])
iter = 4;
}
- parse(argc, argv);
-
threads = calloc(num_contexts, sizeof(*threads));
for (i = 0; i num_contexts; i++)
diff --git a/tests/gem_render_copy.c b/tests/gem_render_copy.c
index 76ba40e..6ff0c77 100644
--- a/tests/gem_render_copy.c
+++ b/tests/gem_render_copy.c
@@ -41,7 +41,6 @@
#include errno.h
#include sys/stat.h
#include sys/time.h
-#include getopt.h
#include drm.h
@@ -67,6 +66,7 @@ typedef struct {
drm_intel_bufmgr *bufmgr;
uint32_t linear[WIDTH * HEIGHT];
} data_t;
+static int opt_dump_png = false;
static void scratch_buf_write_to_png(struct igt_buf *buf, const char *filename)
{
@@ -117,27 +117,24 @@ scratch_buf_check(data_t *data, struct igt_buf *buf, int
x, int y,
color, val, x, y);
}
+static int opt_handler(int opt, int opt_index)
+{
+ if (opt == 'd') {
+ opt_dump_png = true;
+ }
+
+ return 0;
+}
+
int main(int argc, char **argv)
{
data_t data = {0, };
struct intel_batchbuffer *batch = NULL;
struct igt_buf src, dst;
igt_render_copyfunc_t render_copy = NULL;
- int opt;
- int opt_dump_png = false;
int opt_dump_aub = igt_aub_dump_enabled();
- igt_simple_init(argc, argv);
-
- while ((opt = getopt(argc, argv, d)) != -1) {
- switch (opt) {
- case 'd':
- opt_dump_png = true;
- break;
- default:
- break;
- }
- }
+ igt_simple_init_parse_opts(argc, argv, d, NULL, NULL, opt_handler);
igt_fixture {
data.drm_fd = drm_open_any_render();
diff --git a/tests/gem_seqno_wrap.c b/tests/gem_seqno_wrap.c
index 0fa722d..3a40860 100644
--- a/tests/gem_seqno_wrap.c
+++ b/tests/gem_seqno_wrap.c
@@ -38,7 +38,6 @@
#include sys/types.h
#include sys/wait.h
#include limits.h
-#include getopt.h
#include signal.h
#include errno.h
@@ -452,45 +451,9 @@ static void background_run_once(void)
sleep(3);
}
-static void print_usage(const char *s)
+static int parse_options(int opt, int opt_index)
{
- igt_info(%s: [OPTION]...\n, s);
- igt_info(where options are:\n);
- igt_info(-b --background run in background inducing
wraps\n);
- igt_info(-n --rounds=num run num times across wrap boundary,
0 == forever\n);
- igt_info(-t --timeout=sec set timeout to wait for testrun to
sec seconds\n);
- igt_info(-d --dontwrap don't wrap just run the test\n);
- igt_info(-p --prewrap=nset seqno to WRAP - n for each
testrun\n);
- igt_info(-r --norandom dont randomize prewrap space\n);
- igt_info(-i --buffers number of buffers to copy\n);
-
[Intel-gfx] [PATCH i-g-t 4/8] lib: add igt_simple_init_parse_opts
This function allows simple tests to register additional command line
options.
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.c | 22 ++
lib/igt_core.h | 5 +
2 files changed, 27 insertions(+)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index ee6f90c..72c77e6 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -500,6 +500,28 @@ void igt_simple_init(int argc, char **argv)
optind = 1;
}
+/**
+ * igt_simple_init_parse_opts:
+ * @argc: argc from the test's main()
+ * @argv: argv from the test's main()
+ * @extra_short_opts: getopt_long() compliant list with additional short
options
+ * @extra_long_opts: getopt_long() compliant list with additional long options
+ * @help_str: help string for the additional options
+ * @extra_opt_handler: handler for the additional options
+ *
+ * This initializes a simple test without any support for subtests and allows
+ * an arbitrary set of additional options.
+ */
+void igt_simple_init_parse_opts(int argc, char **argv,
+ const char *extra_short_opts,
+ struct option *extra_long_opts,
+ const char *help_str,
+ igt_opt_handler_t extra_opt_handler)
+{
+ common_init(argc, argv, extra_short_opts, extra_long_opts, help_str,
+ extra_opt_handler);
+}
+
/*
* Note: Testcases which use these helpers MUST NOT output anything to stdout
* outside of places protected by igt_run_subtest checks - the piglit
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 6138487..408cf3a 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -162,6 +162,11 @@ bool igt_only_list_subtests(void);
static void igt_tokencat(__real_main, __LINE__)(void) \
void igt_simple_init(int argc, char **argv);
+void igt_simple_init_parse_opts(int argc, char **argv,
+ const char *extra_short_opts,
+ struct option *extra_long_opts,
+ const char *help_str,
+ igt_opt_handler_t extra_opt_handler);
/**
* igt_simple_main:
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 0/8] command line parsing
The following series attempts to standardise command line parsing and options across the various types of tests. This allows all tests to respond to common options such as --help and --debug, and also warn when unknown or invalid options are used. Thomas Wood (8): lib: warn when attempting to run an unknown subtest tests: remove unused getopt header includes lib: move option parsing into common_init lib: add igt_simple_init_parse_opts lib: don't ignore unknown options in multi-tests tests: convert simple tests to use igt_simple_init_parse_opts lib: always warn about unknown options lib: add a command line option to enable debug output in tests lib/drmtest.c | 1 - lib/igt_aux.c | 1 - lib/igt_core.c | 184 lib/igt_core.h | 6 ++ lib/rendercopy_gen6.c | 1 - lib/rendercopy_gen7.c | 1 - lib/rendercopy_gen8.c | 1 - lib/rendercopy_i830.c | 1 - lib/rendercopy_i915.c | 1 - tests/gem_ctx_basic.c | 20 ++--- tests/gem_media_fill.c | 1 - tests/gem_render_copy.c | 25 +++--- tests/gem_render_copy_redux.c | 1 - tests/gem_render_linear_blits.c | 1 - tests/gem_render_tiled_blits.c | 1 - tests/gem_seqno_wrap.c | 82 +++--- tests/gem_stress.c | 105 +++ tests/gem_wait_render_timeout.c | 1 - tests/kms_setmode.c | 1 - tests/pm_rps.c | 1 - 20 files changed, 215 insertions(+), 221 deletions(-) -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 1/8] lib: warn when attempting to run an unknown subtest
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index b197932..5c20581 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -204,6 +204,7 @@ static unsigned int exit_handler_count;
/* subtests helpers */
static bool list_subtests = false;
static char *run_single_subtest = NULL;
+static bool run_single_subtest_found = false;
static const char *in_subtest = NULL;
static bool in_fixture = false;
static bool test_with_subtests = false;
@@ -484,9 +485,14 @@ bool __igt_run_subtest(const char *subtest_name)
return false;
}
- if (run_single_subtest
- strcmp(subtest_name, run_single_subtest) != 0)
- return false;
+ if (run_single_subtest) {
+ if (strcmp(subtest_name, run_single_subtest) != 0)
+ return false;
+ else
+ run_single_subtest_found = true;
+ }
+
+
if (skip_subtests_henceforth) {
printf(Subtest %s: %s\n, subtest_name,
@@ -722,6 +728,12 @@ void igt_exit(void)
{
igt_exit_called = true;
+ if (run_single_subtest !run_single_subtest_found) {
+ igt_warn(Unknown subtest: %s\n, run_single_subtest);
+ exit(-1);
+ }
+
+
if (igt_only_list_subtests())
exit(IGT_EXIT_SUCCESS);
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 5/8] lib: don't ignore unknown options in multi-tests
None of the current tests have additional options that might make use of
any unknown options and igt_subtest_init_parse_opts is available that
integrates additional option parsing.
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 72c77e6..0867c27 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -459,18 +459,7 @@ enum igt_log_level igt_log_level = IGT_LOG_INFO;
*/
void igt_subtest_init(int argc, char **argv)
{
- int ret;
-
- /* supress getopt errors about unknown options */
- opterr = 0;
-
- ret = igt_subtest_init_parse_opts(argc, argv, NULL, NULL, NULL, NULL);
- if (ret 0)
- /* exit with no error for -h/--help */
- exit(ret == -1 ? 0 : ret);
-
- /* reset opt parsing */
- optind = 1;
+ igt_subtest_init_parse_opts(argc, argv, NULL, NULL, NULL, NULL);
}
/**
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 3/8] lib: move option parsing into common_init
Move option parsing into common_init so it can be shared between simple
tests and tests with subtests. This allows for more common command line
behaviour across all tests.
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.c | 118 -
1 file changed, 74 insertions(+), 44 deletions(-)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 5c20581..ee6f90c 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -291,30 +291,11 @@ static void oom_adjust_for_doom(void)
igt_assert(write(fd, always_kill, sizeof(always_kill)) ==
sizeof(always_kill));
}
-/**
- * igt_subtest_init_parse_opts:
- * @argc: argc from the test's main()
- * @argv: argv from the test's main()
- * @extra_short_opts: getopt_long() compliant list with additional short
options
- * @extra_long_opts: getopt_long() compliant list with additional long options
- * @help_str: help string for the additional options
- * @extra_opt_handler: handler for the additional options
- *
- * This function handles the subtest related cmdline options and allows an
- * arbitrary set of additional options. This is useful for tests which have
- * additional knobs to tune when run manually like the number of rounds execute
- * or the size of the allocated buffer objects.
- *
- * Tests without special needs should just use igt_subtest_init() or use
- * #igt_main directly instead of their own main() function.
- *
- * Returns: Forwards any option parsing errors from getopt_long.
- */
-int igt_subtest_init_parse_opts(int argc, char **argv,
- const char *extra_short_opts,
- struct option *extra_long_opts,
- const char *help_str,
- igt_opt_handler_t extra_opt_handler)
+static int common_init(int argc, char **argv,
+ const char *extra_short_opts,
+ struct option *extra_long_opts,
+ const char *help_str,
+ igt_opt_handler_t extra_opt_handler)
{
int c, option_index = 0;
static struct option long_options[] = {
@@ -328,8 +309,18 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
int extra_opt_count;
int all_opt_count;
int ret = 0;
+ char *env = getenv(IGT_LOG_LEVEL);
- test_with_subtests = true;
+ if (env) {
+ if (strcmp(env, debug) == 0)
+ igt_log_level = IGT_LOG_DEBUG;
+ else if (strcmp(env, info) == 0)
+ igt_log_level = IGT_LOG_INFO;
+ else if (strcmp(env, warn) == 0)
+ igt_log_level = IGT_LOG_WARN;
+ else if (strcmp(env, none) == 0)
+ igt_log_level = IGT_LOG_NONE;
+ }
command_str = argv[0];
if (strrchr(command_str, '/'))
@@ -389,36 +380,70 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
}
}
- igt_install_exit_handler(check_igt_exit);
oom_adjust_for_doom();
out:
free(short_opts);
free(combined_opts);
+
+ /* exit immediately if this test has no subtests and a subtest or the
+* list of subtests has been requested */
+ if (!test_with_subtests) {
+ if (run_single_subtest) {
+ igt_warn(Unknown subtest: %s\n, run_single_subtest);
+ exit(-1);
+ }
+ if (list_subtests)
+ exit(-1);
+ }
+
+ if (ret 0)
+ /* exit with no error for -h/--help */
+ exit(ret == -1 ? 0 : ret);
+
print_version();
return ret;
}
-enum igt_log_level igt_log_level = IGT_LOG_INFO;
-static void common_init(void)
+/**
+ * igt_subtest_init_parse_opts:
+ * @argc: argc from the test's main()
+ * @argv: argv from the test's main()
+ * @extra_short_opts: getopt_long() compliant list with additional short
options
+ * @extra_long_opts: getopt_long() compliant list with additional long options
+ * @help_str: help string for the additional options
+ * @extra_opt_handler: handler for the additional options
+ *
+ * This function handles the subtest related cmdline options and allows an
+ * arbitrary set of additional options. This is useful for tests which have
+ * additional knobs to tune when run manually like the number of rounds execute
+ * or the size of the allocated buffer objects.
+ *
+ * Tests without special needs should just use igt_subtest_init() or use
+ * #igt_main directly instead of their own main() function.
+ *
+ * Returns: Forwards any option parsing errors from getopt_long.
+ */
+int igt_subtest_init_parse_opts(int argc, char **argv,
+ const char *extra_short_opts,
+ struct option *extra_long_opts,
+ const char *help_str,
+
[Intel-gfx] [PATCH i-g-t 8/8] lib: add a command line option to enable debug output in tests
Add --debug as a common command line option for all tests to enable
debug output.
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 42b22fc..d90e6bb 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -276,7 +276,9 @@ static void print_usage(const char *command_str, const char
*help_str,
fprintf(f, Usage: %s [OPTIONS]\n
--list-subtests\n
---run-subtest pattern\n, command_str);
+--run-subtest pattern\n
+--debug\n
+--help\n, command_str);
if (help_str)
fprintf(f, %s\n, help_str);
}
@@ -301,6 +303,7 @@ static int common_init(int argc, char **argv,
static struct option long_options[] = {
{list-subtests, 0, 0, 'l'},
{run-subtest, 1, 0, 'r'},
+ {debug, 0, 0, 'd'},
{help, 0, 0, 'h'},
};
const char *command_str;
@@ -349,6 +352,9 @@ static int common_init(int argc, char **argv,
while ((c = getopt_long(argc, argv, short_opts, combined_opts,
option_index)) != -1) {
switch(c) {
+ case 'd':
+ igt_log_level = IGT_LOG_DEBUG;
+ break;
case 'l':
if (!run_single_subtest)
list_subtests = true;
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 7/8] lib: always warn about unknown options
All tests can now register extra options, so there should not be any
unknown options.
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.c | 27 ---
1 file changed, 4 insertions(+), 23 deletions(-)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 0867c27..42b22fc 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -362,17 +362,9 @@ static int common_init(int argc, char **argv,
ret = -1;
goto out;
case '?':
- if (opterr) {
- print_usage(command_str, help_str, true);
- ret = -2;
- goto out;
- }
- /*
-* Just ignore the error, since the unknown argument
-* can be something the caller understands and will
-* parse by doing a second getopt scanning.
-*/
- break;
+ print_usage(command_str, help_str, true);
+ ret = -2;
+ goto out;
default:
ret = extra_opt_handler(c, option_index);
if (ret)
@@ -475,18 +467,7 @@ void igt_subtest_init(int argc, char **argv)
*/
void igt_simple_init(int argc, char **argv)
{
- int ret;
-
- /* supress getopt errors about unknown options */
- opterr = 0;
-
- ret = common_init(argc, argv, NULL, NULL, NULL, NULL);
- if (ret 0)
- /* exit with no error for -h/--help */
- exit(ret == -1 ? 0 : ret);
-
- /* reset opt parsing */
- optind = 1;
+ common_init(argc, argv, NULL, NULL, NULL, NULL);
}
/**
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 2/8] tests: remove unused getopt header includes
Signed-off-by: Thomas Wood thomas.w...@intel.com --- lib/drmtest.c | 1 - lib/igt_aux.c | 1 - lib/rendercopy_gen6.c | 1 - lib/rendercopy_gen7.c | 1 - lib/rendercopy_gen8.c | 1 - lib/rendercopy_i830.c | 1 - lib/rendercopy_i915.c | 1 - tests/gem_media_fill.c | 1 - tests/gem_render_copy_redux.c | 1 - tests/gem_render_linear_blits.c | 1 - tests/gem_render_tiled_blits.c | 1 - tests/gem_wait_render_timeout.c | 1 - tests/kms_setmode.c | 1 - tests/pm_rps.c | 1 - 14 files changed, 14 deletions(-) diff --git a/lib/drmtest.c b/lib/drmtest.c index 7be2e40..f921f67 100644 --- a/lib/drmtest.c +++ b/lib/drmtest.c @@ -39,7 +39,6 @@ #include sys/mman.h #include signal.h #include pciaccess.h -#include getopt.h #include stdlib.h #include unistd.h #include sys/wait.h diff --git a/lib/igt_aux.c b/lib/igt_aux.c index 7b277be..0bcaa3b 100644 --- a/lib/igt_aux.c +++ b/lib/igt_aux.c @@ -39,7 +39,6 @@ #include sys/mman.h #include signal.h #include pciaccess.h -#include getopt.h #include stdlib.h #include unistd.h #include sys/wait.h diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c index 7b3104c..afe7562 100644 --- a/lib/rendercopy_gen6.c +++ b/lib/rendercopy_gen6.c @@ -9,7 +9,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h #include i915_drm.h #include drmtest.h diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c index 5131d8f..2932f1e 100644 --- a/lib/rendercopy_gen7.c +++ b/lib/rendercopy_gen7.c @@ -9,7 +9,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h #include i915_drm.h #include drmtest.h diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c index a32871d..ff0b0c8 100644 --- a/lib/rendercopy_gen8.c +++ b/lib/rendercopy_gen8.c @@ -9,7 +9,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h #include i915_drm.h diff --git a/lib/rendercopy_i830.c b/lib/rendercopy_i830.c index f0235a5..04215b1 100644 --- a/lib/rendercopy_i830.c +++ b/lib/rendercopy_i830.c @@ -8,7 +8,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h #include i915_drm.h #include drmtest.h diff --git a/lib/rendercopy_i915.c b/lib/rendercopy_i915.c index 1acf9da..fc9583c 100644 --- a/lib/rendercopy_i915.c +++ b/lib/rendercopy_i915.c @@ -8,7 +8,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h #include i915_drm.h #include drmtest.h diff --git a/tests/gem_media_fill.c b/tests/gem_media_fill.c index db2380b..b06a556 100644 --- a/tests/gem_media_fill.c +++ b/tests/gem_media_fill.c @@ -41,7 +41,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h #include ioctl_wrappers.h #include drmtest.h diff --git a/tests/gem_render_copy_redux.c b/tests/gem_render_copy_redux.c index f711fdb..cb48aa7 100644 --- a/tests/gem_render_copy_redux.c +++ b/tests/gem_render_copy_redux.c @@ -43,7 +43,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h diff --git a/tests/gem_render_linear_blits.c b/tests/gem_render_linear_blits.c index ee99dea..28fd8c8 100644 --- a/tests/gem_render_linear_blits.c +++ b/tests/gem_render_linear_blits.c @@ -46,7 +46,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h diff --git a/tests/gem_render_tiled_blits.c b/tests/gem_render_tiled_blits.c index 3d83f7c..ea1d59d 100644 --- a/tests/gem_render_tiled_blits.c +++ b/tests/gem_render_tiled_blits.c @@ -42,7 +42,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h diff --git a/tests/gem_wait_render_timeout.c b/tests/gem_wait_render_timeout.c index a34c006..3afab9c 100644 --- a/tests/gem_wait_render_timeout.c +++ b/tests/gem_wait_render_timeout.c @@ -36,7 +36,6 @@ #include errno.h #include sys/stat.h #include sys/time.h -#include getopt.h #include drm.h diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c index 0b765a9..8762255 100644 --- a/tests/kms_setmode.c +++ b/tests/kms_setmode.c @@ -29,7 +29,6 @@ #include stdint.h #include unistd.h #include string.h -#include getopt.h #include sys/time.h #include drmtest.h diff --git a/tests/pm_rps.c b/tests/pm_rps.c index c1156a5..96fec99 100644 --- a/tests/pm_rps.c +++ b/tests/pm_rps.c @@ -31,7 +31,6 @@ #include stdlib.h #include string.h #include unistd.h -#include getopt.h #include fcntl.h #include signal.h #include errno.h -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 3/8] lib: move option parsing into common_init
On Wed, Jul 23, 2014 at 11:57:50AM +0100, Thomas Wood wrote:
Move option parsing into common_init so it can be shared between simple
tests and tests with subtests. This allows for more common command line
behaviour across all tests.
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.c | 118
-
1 file changed, 74 insertions(+), 44 deletions(-)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 5c20581..ee6f90c 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -291,30 +291,11 @@ static void oom_adjust_for_doom(void)
igt_assert(write(fd, always_kill, sizeof(always_kill)) ==
sizeof(always_kill));
}
-/**
- * igt_subtest_init_parse_opts:
- * @argc: argc from the test's main()
- * @argv: argv from the test's main()
- * @extra_short_opts: getopt_long() compliant list with additional short
options
- * @extra_long_opts: getopt_long() compliant list with additional long
options
- * @help_str: help string for the additional options
- * @extra_opt_handler: handler for the additional options
- *
- * This function handles the subtest related cmdline options and allows an
- * arbitrary set of additional options. This is useful for tests which have
- * additional knobs to tune when run manually like the number of rounds
execute
- * or the size of the allocated buffer objects.
- *
- * Tests without special needs should just use igt_subtest_init() or use
- * #igt_main directly instead of their own main() function.
- *
- * Returns: Forwards any option parsing errors from getopt_long.
- */
-int igt_subtest_init_parse_opts(int argc, char **argv,
- const char *extra_short_opts,
- struct option *extra_long_opts,
- const char *help_str,
- igt_opt_handler_t extra_opt_handler)
+static int common_init(int argc, char **argv,
+const char *extra_short_opts,
+struct option *extra_long_opts,
+const char *help_str,
+igt_opt_handler_t extra_opt_handler)
{
int c, option_index = 0;
static struct option long_options[] = {
@@ -328,8 +309,18 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
int extra_opt_count;
int all_opt_count;
int ret = 0;
+ char *env = getenv(IGT_LOG_LEVEL);
- test_with_subtests = true;
+ if (env) {
+ if (strcmp(env, debug) == 0)
+ igt_log_level = IGT_LOG_DEBUG;
+ else if (strcmp(env, info) == 0)
+ igt_log_level = IGT_LOG_INFO;
+ else if (strcmp(env, warn) == 0)
+ igt_log_level = IGT_LOG_WARN;
+ else if (strcmp(env, none) == 0)
+ igt_log_level = IGT_LOG_NONE;
+ }
command_str = argv[0];
if (strrchr(command_str, '/'))
@@ -389,36 +380,70 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
}
}
- igt_install_exit_handler(check_igt_exit);
oom_adjust_for_doom();
out:
free(short_opts);
free(combined_opts);
+
+ /* exit immediately if this test has no subtests and a subtest or the
+ * list of subtests has been requested */
+ if (!test_with_subtests) {
+ if (run_single_subtest) {
+ igt_warn(Unknown subtest: %s\n, run_single_subtest);
+ exit(-1);
+ }
+ if (list_subtests)
+ exit(-1);
Instead of -1 I think we should have a common IGT_EXIT_INVALID code like
79 similar to skip and timeout and use this one here and in the code
you've added to igt_exit in a previous patch.
Probably easiest if you do this as a fixup on top.
Otherwise I think this goes into the right direction, so ack on the entire
series. I haven't done an in-depth review, but didn't spot anything else
really.
-Daniel
+ }
+
+ if (ret 0)
+ /* exit with no error for -h/--help */
+ exit(ret == -1 ? 0 : ret);
+
print_version();
return ret;
}
-enum igt_log_level igt_log_level = IGT_LOG_INFO;
-static void common_init(void)
+/**
+ * igt_subtest_init_parse_opts:
+ * @argc: argc from the test's main()
+ * @argv: argv from the test's main()
+ * @extra_short_opts: getopt_long() compliant list with additional short
options
+ * @extra_long_opts: getopt_long() compliant list with additional long
options
+ * @help_str: help string for the additional options
+ * @extra_opt_handler: handler for the additional options
+ *
+ * This function handles the subtest related cmdline options and allows an
+ * arbitrary set of additional options. This is useful for tests which have
+ * additional knobs to tune when run manually like the number of rounds
execute
+ * or the size of the
Re: [Intel-gfx] [PATCH i-g-t 3/8] lib: move option parsing into common_init
On Wed, Jul 23, 2014 at 02:13:14PM +0200, Daniel Vetter wrote:
On Wed, Jul 23, 2014 at 11:57:50AM +0100, Thomas Wood wrote:
Move option parsing into common_init so it can be shared between simple
tests and tests with subtests. This allows for more common command line
behaviour across all tests.
Signed-off-by: Thomas Wood thomas.w...@intel.com
---
lib/igt_core.c | 118
-
1 file changed, 74 insertions(+), 44 deletions(-)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 5c20581..ee6f90c 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -291,30 +291,11 @@ static void oom_adjust_for_doom(void)
igt_assert(write(fd, always_kill, sizeof(always_kill)) ==
sizeof(always_kill));
}
-/**
- * igt_subtest_init_parse_opts:
- * @argc: argc from the test's main()
- * @argv: argv from the test's main()
- * @extra_short_opts: getopt_long() compliant list with additional short
options
- * @extra_long_opts: getopt_long() compliant list with additional long
options
- * @help_str: help string for the additional options
- * @extra_opt_handler: handler for the additional options
- *
- * This function handles the subtest related cmdline options and allows an
- * arbitrary set of additional options. This is useful for tests which have
- * additional knobs to tune when run manually like the number of rounds
execute
- * or the size of the allocated buffer objects.
- *
- * Tests without special needs should just use igt_subtest_init() or use
- * #igt_main directly instead of their own main() function.
- *
- * Returns: Forwards any option parsing errors from getopt_long.
- */
-int igt_subtest_init_parse_opts(int argc, char **argv,
- const char *extra_short_opts,
- struct option *extra_long_opts,
- const char *help_str,
- igt_opt_handler_t extra_opt_handler)
+static int common_init(int argc, char **argv,
+ const char *extra_short_opts,
+ struct option *extra_long_opts,
+ const char *help_str,
+ igt_opt_handler_t extra_opt_handler)
{
int c, option_index = 0;
static struct option long_options[] = {
@@ -328,8 +309,18 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
int extra_opt_count;
int all_opt_count;
int ret = 0;
+ char *env = getenv(IGT_LOG_LEVEL);
- test_with_subtests = true;
+ if (env) {
+ if (strcmp(env, debug) == 0)
+ igt_log_level = IGT_LOG_DEBUG;
+ else if (strcmp(env, info) == 0)
+ igt_log_level = IGT_LOG_INFO;
+ else if (strcmp(env, warn) == 0)
+ igt_log_level = IGT_LOG_WARN;
+ else if (strcmp(env, none) == 0)
+ igt_log_level = IGT_LOG_NONE;
+ }
command_str = argv[0];
if (strrchr(command_str, '/'))
@@ -389,36 +380,70 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
}
}
- igt_install_exit_handler(check_igt_exit);
oom_adjust_for_doom();
out:
free(short_opts);
free(combined_opts);
+
+ /* exit immediately if this test has no subtests and a subtest or the
+* list of subtests has been requested */
+ if (!test_with_subtests) {
+ if (run_single_subtest) {
+ igt_warn(Unknown subtest: %s\n, run_single_subtest);
+ exit(-1);
+ }
+ if (list_subtests)
+ exit(-1);
Instead of -1 I think we should have a common IGT_EXIT_INVALID code like
79 similar to skip and timeout and use this one here and in the code
you've added to igt_exit in a previous patch.
Probably easiest if you do this as a fixup on top.
Otherwise I think this goes into the right direction, so ack on the entire
series. I haven't done an in-depth review, but didn't spot anything else
really.
Aside: If we have this special exit code which no other tests uses
otherwise we can catch it in piglit. That will be extremely useful to
catch all those offenders who accidentally left a printf or similar
unguarded by igt_fixture or igt_subtest somewhere in their code.
-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] igt/gem_userptr_blits: Fix forked access test
On Tue, Jul 22, 2014 at 12:33:49PM +0100, Tvrtko Ursulin wrote: copy() blit helper assumes a certain object size much larger than a page size. Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Cc: Chris Wilson ch...@chris-wilson.co.uk Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk -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
[Intel-gfx] [PATCH] igt/gem_userptr_benchmark: Fix for upstream ioctl number
Hardcoding has upsides and downsides.
Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com
---
benchmarks/gem_userptr_benchmark.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/benchmarks/gem_userptr_benchmark.c
b/benchmarks/gem_userptr_benchmark.c
index bdfce12..4d7442b 100644
--- a/benchmarks/gem_userptr_benchmark.c
+++ b/benchmarks/gem_userptr_benchmark.c
@@ -58,7 +58,7 @@
#define PAGE_SIZE 4096
#endif
-#define LOCAL_I915_GEM_USERPTR 0x34
+#define LOCAL_I915_GEM_USERPTR 0x33
#define LOCAL_IOCTL_I915_GEM_USERPTR DRM_IOWR (DRM_COMMAND_BASE +
LOCAL_I915_GEM_USERPTR, struct local_i915_gem_userptr)
struct local_i915_gem_userptr {
uint64_t user_ptr;
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] lib/drm_lib.sh: Bare-bones long option parsing
Just enough to stay compatible with simple subtests.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
tests/drm_lib.sh | 13 +
1 file changed, 13 insertions(+)
diff --git a/tests/drm_lib.sh b/tests/drm_lib.sh
index 38befa88acb8..d71e6ae2223e 100755
--- a/tests/drm_lib.sh
+++ b/tests/drm_lib.sh
@@ -1,4 +1,17 @@
#!/bin/sh
+
+# hacked-up long option parsing
+for arg in $@ ; do
+ case $arg in
+ --list-subtests)
+ exit 79
+ ;;
+ --run-subtest)
+ exit 79
+ ;;
+ esac
+done
+
die() {
echo $@
exit 1
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] tests: Move root check to lib/drm_lib.sh
All tests want that anyway.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
tests/drm_lib.sh | 2 ++
tests/test_rte_check | 2 --
tests/tools_test | 2 --
3 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/tests/drm_lib.sh b/tests/drm_lib.sh
index 97f6f9253072..38befa88acb8 100755
--- a/tests/drm_lib.sh
+++ b/tests/drm_lib.sh
@@ -34,6 +34,8 @@ if [ `cat $i915_dfs_path/clients | wc -l` -gt 2 ] ; then
die ERROR: other drm clients running
fi
+whoami | grep -q root || ( echo ERROR: not running as root; exit 1 )
+
i915_sfs_path=
if [ -d /sys/class/drm ] ; then
sysfs_path=/sys/class/drm
diff --git a/tests/test_rte_check b/tests/test_rte_check
index 6389592dba9e..eb12416af6be 100755
--- a/tests/test_rte_check
+++ b/tests/test_rte_check
@@ -1,7 +1,5 @@
#!/bin/bash
-whoami | grep root || ( echo ERROR: not running as root; exit 1 )
-
SOURCE_DIR=$( dirname ${BASH_SOURCE[0]} )
. $SOURCE_DIR/drm_lib.sh
diff --git a/tests/tools_test b/tests/tools_test
index 4c5577a787a9..8bda2638564f 100755
--- a/tests/tools_test
+++ b/tests/tools_test
@@ -2,8 +2,6 @@
# Test some of the most critical tools we have accidentally broken before.
# TODO: Possibly make tests parse output
-whoami | grep -q root || ( echo ERROR: not running as root; exit 1 )
-
SOURCE_DIR=$( dirname ${BASH_SOURCE[0]} )
. $SOURCE_DIR/drm_lib.sh
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] igt/gem_userptr_benchmark: Fix for upstream ioctl number
On Wed, Jul 23, 2014 at 01:33:18PM +0100, Tvrtko Ursulin wrote:
Hardcoding has upsides and downsides.
Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com
Ooops, merged.
-Daniel
---
benchmarks/gem_userptr_benchmark.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/benchmarks/gem_userptr_benchmark.c
b/benchmarks/gem_userptr_benchmark.c
index bdfce12..4d7442b 100644
--- a/benchmarks/gem_userptr_benchmark.c
+++ b/benchmarks/gem_userptr_benchmark.c
@@ -58,7 +58,7 @@
#define PAGE_SIZE 4096
#endif
-#define LOCAL_I915_GEM_USERPTR 0x34
+#define LOCAL_I915_GEM_USERPTR 0x33
#define LOCAL_IOCTL_I915_GEM_USERPTR DRM_IOWR (DRM_COMMAND_BASE +
LOCAL_I915_GEM_USERPTR, struct local_i915_gem_userptr)
struct local_i915_gem_userptr {
uint64_t user_ptr;
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] igt/gem_userptr_benchmark: Testing of overlapping synchronized objects
Larger performance impact is expected with first draft of the kernel
implementation for overlapping objects so this is just to confirm that.
Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com
Cc: Chris Wilson ch...@chris-wilson.co.uk
---
benchmarks/gem_userptr_benchmark.c | 97 --
1 file changed, 72 insertions(+), 25 deletions(-)
diff --git a/benchmarks/gem_userptr_benchmark.c
b/benchmarks/gem_userptr_benchmark.c
index 4d7442b..04af219 100644
--- a/benchmarks/gem_userptr_benchmark.c
+++ b/benchmarks/gem_userptr_benchmark.c
@@ -213,7 +213,8 @@ static void exchange_ptr(void *array, unsigned i, unsigned
j)
static void test_malloc_free(int random)
{
unsigned long iter = 0;
- unsigned int i, tot = 1000;
+ unsigned int i;
+ const unsigned int tot = 1000;
void *ptr[tot];
start_test(test_duration_sec);
@@ -230,13 +231,14 @@ static void test_malloc_free(int random)
iter++;
}
- printf(%8lu iter/s\n, iter / test_duration_sec);
+ printf(%10lu op/s\n, 2 * tot * iter / test_duration_sec);
}
static void test_malloc_realloc_free(int random)
{
unsigned long iter = 0;
- unsigned int i, tot = 1000;
+ unsigned int i;
+ const unsigned int tot = 1000;
void *ptr[tot];
start_test(test_duration_sec);
@@ -259,13 +261,14 @@ static void test_malloc_realloc_free(int random)
iter++;
}
- printf(%8lu iter/s\n, iter / test_duration_sec);
+ printf(%10lu op/s\n, 3 * tot * iter / test_duration_sec);
}
static void test_mmap_unmap(int random)
{
unsigned long iter = 0;
- unsigned int i, tot = 1000;
+ unsigned int i;
+ const unsigned int tot = 1000;
void *ptr[tot];
start_test(test_duration_sec);
@@ -283,7 +286,7 @@ static void test_mmap_unmap(int random)
iter++;
}
- printf(%8lu iter/s\n, iter / test_duration_sec);
+ printf(%10lu op/s\n, 2 * tot * iter / test_duration_sec);
}
static void test_ptr_read(void *ptr)
@@ -307,7 +310,7 @@ static void test_ptr_read(void *ptr)
iter++;
}
- printf(%8lu MB/s\n, iter / test_duration_sec * BO_SIZE / 100);
+ printf(%10lu MB/s\n, iter / test_duration_sec * BO_SIZE / 100);
}
static void test_ptr_write(void *ptr)
@@ -331,7 +334,31 @@ static void test_ptr_write(void *ptr)
iter++;
}
- printf(%8lu MB/s\n, iter / test_duration_sec * BO_SIZE / 100);
+ printf(%10lu MB/s\n, iter / test_duration_sec * BO_SIZE / 100);
+}
+
+static void test_impact_common(void *ptr, unsigned int nr)
+{
+ printf(ptr-read, %5u bos = , nr);
+ test_ptr_read(ptr);
+
+ printf(ptr-write, %5u bos = , nr);
+ test_ptr_write(ptr);
+
+ printf(malloc-free,%5u bos = , nr);
+ test_malloc_free(0);
+ printf(malloc-free-random, %5u bos = , nr);
+ test_malloc_free(1);
+
+ printf(malloc-realloc-free,%5u bos = , nr);
+ test_malloc_realloc_free(0);
+ printf(malloc-realloc-free-random, %5u bos = , nr);
+ test_malloc_realloc_free(1);
+
+ printf(mmap-unmap, %5u bos = , nr);
+ test_mmap_unmap(0);
+ printf(mmap-unmap-random, %5u bos = , nr);
+ test_mmap_unmap(1);
}
static void test_impact(int fd)
@@ -351,29 +378,46 @@ static void test_impact(int fd)
else
ptr = buffer;
- printf(ptr-read, %5u bos = ,
nr_bos[subtest]);
- test_ptr_read(ptr);
+ test_impact_common(ptr, nr_bos[subtest]);
- printf(ptr-write %5u bos = ,
nr_bos[subtest]);
- test_ptr_write(ptr);
+ for (i = 0; i nr_bos[subtest]; i++)
+ free_userptr_bo(fd, handles[i]);
+ }
+}
- printf(malloc-free,%5u bos = ,
nr_bos[subtest]);
- test_malloc_free(0);
- printf(malloc-free-random %5u bos = ,
nr_bos[subtest]);
- test_malloc_free(1);
+static void test_impact_overlap(int fd)
+{
+ unsigned int total = sizeof(nr_bos) / sizeof(nr_bos[0]);
+ unsigned int subtest, i;
+ uint32_t handles[nr_bos[total-1]];
+ void *ptr;
+ char buffer[BO_SIZE];
+ void *block;
+ uint32_t handle;
+ int ret;
- printf(malloc-realloc-free,%5u bos = ,
nr_bos[subtest]);
- test_malloc_realloc_free(0);
- printf(malloc-realloc-free-random, %5u bos = ,
nr_bos[subtest]);
- test_malloc_realloc_free(1);
+ for (subtest = 0; subtest total; subtest++) {
+ if (nr_bos[subtest] 0) {
+ ret = posix_memalign(block, PAGE_SIZE, BO_SIZE *
Re: [Intel-gfx] [REGRESSION BISECTED] backlight control stops workin with 3.14 and later
On Tue, Jul 22, 2014 at 8:42 AM, Hans de Goede hdego...@redhat.com Bertrik, can you try blacklisting the samsung-laptop module, then check /sys/class/backlight, the samsung_laptop dir should be gone replaced by an acpi_video0 (or some such) dir. Please try if that works. If that does not work, try booting with both the module blacklisted and acpi_backlight=vendor on the kernel commandline, then you should see only the intel-backlight under /sys/class/backlight and things should work. With linux kernel v3.14 and the samsung_laptop module blacklisted, I indeed see a /sys/class/backlight/acpi_video0, but changing brightness by writing (as root) to the brightness file does not have an effect. Additionally, with the acpi_backlight=vendor kernel command line option, there is only an intel_backlight directory in /sys/class/backlight. Brightness does work using brightness up and down function keys, with 'udevadm monitor' I can see it uses the intel_backlight: bertrik@netbook:~$ udevadm monitor monitor will print the received events for: UDEV - the event which udev sends out after rule processing KERNEL - the kernel uevent KERNEL[40.375921] change /devices/pci:00/:00:02.0/drm/card0/card0-LVDS-1/intel_backlight (backlight) UDEV [40.503704] change /devices/pci:00/:00:02.0/drm/card0/card0-LVDS-1/intel_backlight (backlight) With kind regards, Bertrik Sikken ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Allow overlapping userptr objects
On 07/21/2014 01:21 PM, Chris Wilson wrote:
Whilst I strongly advise against doing so for the implicit coherency
issues between the multiple buffer objects accessing the same backing
store, it nevertheless is a valid use case, akin to mmaping the same
file multiple times.
The reason why we forbade it earlier was that our use of the interval
tree for fast invalidation upon vma changes excluded overlapping
objects. So in the case where the user wishes to create such pairs of
overlapping objects, we degrade the range invalidation to walkin the
linear list of objects associated with the mm.
A situation where overlapping objects could arise is the lax implementation
of MIT-SHM Pixmaps in the xserver. A second situation is where the user
wishes to have different access modes to a region of memory (e.g. access
through a read-only userptr buffer and through a normal userptr buffer).
v2: Compile for mmu-notifiers after tweaking
v3: Rename is_linear/has_linear
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Li, Victor Y victor.y...@intel.com
Cc: Kelley, Sean V sean.v.kel...@intel.com
Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com
Cc: Gong, Zhipeng zhipeng.g...@intel.com
Cc: Akash Goel akash.g...@intel.com
Cc: Volkin, Bradley D bradley.d.vol...@intel.com
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 142
1 file changed, 106 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
b/drivers/gpu/drm/i915/i915_gem_userptr.c
index b41614d..74c45da 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -40,19 +40,87 @@ struct i915_mmu_notifier {
struct hlist_node node;
struct mmu_notifier mn;
struct rb_root objects;
+ struct list_head linear;
struct drm_device *dev;
struct mm_struct *mm;
struct work_struct work;
unsigned long count;
unsigned long serial;
+ bool has_linear;
};
struct i915_mmu_object {
struct i915_mmu_notifier *mmu;
struct interval_tree_node it;
+ struct list_head link;
struct drm_i915_gem_object *obj;
+ bool is_linear;
};
+static unsigned long cancel_userptr(struct drm_i915_gem_object *obj)
+{
+ struct drm_device *dev = obj-base.dev;
+ unsigned long end;
+
+ mutex_lock(dev-struct_mutex);
+ /* Cancel any active worker and force us to re-evaluate gup */
+ obj-userptr.work = NULL;
+
+ if (obj-pages != NULL) {
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct i915_vma *vma, *tmp;
+ bool was_interruptible;
+
+ was_interruptible = dev_priv-mm.interruptible;
+ dev_priv-mm.interruptible = false;
+
+ list_for_each_entry_safe(vma, tmp, obj-vma_list, vma_link) {
+ int ret = i915_vma_unbind(vma);
+ WARN_ON(ret ret != -EIO);
+ }
+ WARN_ON(i915_gem_object_put_pages(obj));
+
+ dev_priv-mm.interruptible = was_interruptible;
+ }
+
+ end = obj-userptr.ptr + obj-base.size;
+
+ drm_gem_object_unreference(obj-base);
+ mutex_unlock(dev-struct_mutex);
+
+ return end;
+}
+
+static void invalidate_range__linear(struct i915_mmu_notifier *mn,
+struct mm_struct *mm,
+unsigned long start,
+unsigned long end)
+{
+ struct i915_mmu_object *mmu;
+ unsigned long serial;
+
+restart:
+ serial = mn-serial;
+ list_for_each_entry(mmu, mn-linear, link) {
+ struct drm_i915_gem_object *obj;
+
+ if (mmu-it.last start || mmu-it.start end)
+ continue;
+
+ obj = mmu-obj;
+ drm_gem_object_reference(obj-base);
+ spin_unlock(mn-lock);
+
+ cancel_userptr(obj);
+
+ spin_lock(mn-lock);
+ if (serial != mn-serial)
+ goto restart;
+ }
+
+ spin_unlock(mn-lock);
+}
+
static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier
*_mn,
struct mm_struct *mm,
unsigned long start,
@@ -60,16 +128,19 @@ static void
i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
{
struct i915_mmu_notifier *mn = container_of(_mn, struct
i915_mmu_notifier, mn);
struct interval_tree_node *it = NULL;
+ unsigned long next = start;
unsigned long serial = 0;
end--; /* interval ranges are inclusive, but invalidate range is
exclusive */
- while (start end) {
+ while (next end) {
struct drm_i915_gem_object *obj;
obj = NULL;
spin_lock(mn-lock);
+
[Intel-gfx] [ANNOUNCE] xf86-video-intel 2.99.913
Snapshot 2.99.913 (2014-07-23) == This should be it... A few fixes from testing the new code, we should be ready for the final release. However, we do have one standout feature in this snapshot, we now officially recognise HD Graphics 5300/5500/5600, Iris Graphics 6100 and Iris Pro Graphics 6200/P6300 (formerly known as Broadwell). * Check the window actually covers the CRTC before doing a single CRTC flip, and then restore the right framebuffer after completing CRTC flips. Otherwise we would detect an error and disable an output under TearFree Regression in 2.99.912 https://bugs.freedesktop.org/show_bug.cgi?id=80191 * Fix framebuffer creation on kernels older than 3.11 Regression in 2.99.912 * Check that the damage still exists after implicit reduction Regression in 2.99.912 https://bugs.freedesktop.org/show_bug.cgi?id=77436 * Fix direction flags for fallback composited CopyAreas which caused scrolling corruption in a few configurations Regression from 2.20.0 https://bugs.freedesktop.org/show_bug.cgi?id=79843 * Do not throw away damage if there is no redundant copy https://bugs.freedesktop.org/show_bug.cgi?id=79992 * Check clipping on PolyRect before discarding the clipped damage Regression from 2.99.903 https://bugs.freedesktop.org/show_bug.cgi?id=79992 * Fix hints for GLXPixmapa, as these are never swapped and so miss invalidating the hints on SwapBuffers with the result that they are often presumed blank Regression in 2.99.912 https://bugs.freedesktop.org/show_bug.cgi?id=7 * Fix incoherent choice of source bo when constructing 8x8 tiles, incorrect pattern origin when extracting, and then fix the alignment of colour patterns for BLT operations https://bugs.freedesktop.org/show_bug.cgi?id=80033 * Disable blending with the render engine on snoopable buffers https://bugs.freedesktop.org/show_bug.cgi?id=80253 * Restore throttling to prevent client lag under heavy GPU load Regression from 2.21.10 https://bugs.freedesktop.org/show_bug.cgi?id=77436 * Use ClientGone for notifications on shared DRI2 windows to prevent rare crashes due to use-after-free of the swap requests https://bugs.freedesktop.org/show_bug.cgi?id=80157 * Ensure the mmaped CPU bo is idle before migrating damage https://bugs.freedesktop.org/show_bug.cgi?id=80560 * Fix incorrect clipping by the render engine for large DRI2 windows * Ensure that the aperture tiling fallbacks are bounded * Validate parameter to xf86-video-intel-backlight-helper more carefully (CVE-2014-4910) * Fix slaved scanouts for reverse optimus, though rotated slaves will require further patches to Xorg. https://bugs.freedesktop.org/show_bug.cgi?id=81383 * Fix build without Composite extension. * Fix build without gettline(). * UXA: Allocate and resize frontbuffer consistently to pass sanity checks https://bugs.freedesktop.org/show_bug.cgi?id=80088 * UXA: Report cached backlight value when the output is off (like sna) https://bugzilla.redhat.com/show_bug.cgi?id=1032978 * UXA: Mark outputs as off before the kernel does (like sna) This will prevent the internal panel from starting up blank in some multi-monitor configurations https://bugzilla.redhat.com/show_bug.cgi?id=1103806 Note that the DRI2 exchange mechanism introduced in 2.99.912 exposes bugs in some compositors, at least kwin and comptom, which discard DRI2 buffer invalidates rather than resourcing their texture. For example, https://bugs.kde.org/show_bug.cgi?id=336589 Note that the improved triple buffering introduced in DRI2 requires a patch to Xorg (now released upstream) to prevent crashes with DRI_PRIME. https://bugs.freedesktop.org/show_bug.cgi?id=80001 Note that DRI3/Present require tracking the relevant development trees for mesa and the xserver as they are very much still under early testing. Also be aware that Mesa provides no support for explicit fencing so Damage tracking between compositors and clients is unserialised. Complete list of changes since 2.99.912 --- Chris Wilson (254): sna: Handle the user passing Backlight sna: Tidy a few asserts on the state of crtc-flip_bo sna: Fix arguments when flipping transformed TearFree outputs sna: Expand debugging to cover gen8 BLT variations sna: Cast away compiler warning sna/dri2: Check that the window covers the whole CRTC before xchg sna: Mark the GPU as all damaged when discarding CPU bo during uploads sna: Squelch log messages for fb/pixmap tiling in the default case sna: Silence compiler warnings for discarding const Region points sna: Discard TearFree damage before checking for an overwriting upload sna: Prefer to operate inplace on the GPU rather than stall on the CPU sna: Ignore setting read-only for temporary userptr maps sna: Create a stable output name based
Re: [Intel-gfx] [PATCH] igt/gem_userptr_benchmark: Testing of overlapping synchronized objects
On Wed, Jul 23, 2014 at 02:14:19PM +0100, Tvrtko Ursulin wrote: Larger performance impact is expected with first draft of the kernel implementation for overlapping objects so this is just to confirm that. Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Cc: Chris Wilson ch...@chris-wilson.co.uk Looks fine. I can't comment on likely overlapping arrangements, so your overlap with adjacent neighbours seems a sensible first choice. -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
[Intel-gfx] [PATCH] drm/i915: Ditch UMS config option
Let's march ahead with the deprecation plan laid out in commit b30324adaf8d2e5950a602bde63030d15a61826f Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Wed Nov 13 22:11:25 2013 +0100 drm/i915: Deprecated UMS support Thus far no regression report yet, so the transparent fallback plan seems to pan out. Cc: Dave Airlie airl...@gmail.com Cc: David Herrmann dh.herrm...@gmail.com Suggested-by: David Herrmann dh.herrm...@gmail.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/Kconfig | 12 1 file changed, 12 deletions(-) diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index 437e1824d0bf..4e39ab34eb1c 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -69,15 +69,3 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT option changes the default for that module option. If in doubt, say N. - -config DRM_I915_UMS - bool Enable userspace modesetting on Intel hardware (DEPRECATED) - depends on DRM_I915 BROKEN - default n - help - Choose this option if you still need userspace modesetting. - - Userspace modesetting is deprecated for quite some time now, so - enable this only if you have ancient versions of the DDX drivers. - - If in doubt, say N. -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Allow overlapping userptr objects
On Wed, Jul 23, 2014 at 02:23:53PM +0100, Tvrtko Ursulin wrote: Looks fine. Performance impact is potentially big as we discussed but I suppose we can leave that for later if an issue. So: Reviewed-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Merged, thanks for patch and review. I think it would be good to add some more tests to cover the tracking handover between the interval tree and linear list to ensure invalidation still works correctly in non-trivial cases. Code looks correct in that respect but just in case. It is not a top priority so not sure when I'll find time to actually do it. We don't yet have some tests with overlapping allocations? I think at least some basic smoke test should be there ... -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 1/2] drm/i915: Allow overlapping userptr objects
On 07/23/2014 03:34 PM, Daniel Vetter wrote: On Wed, Jul 23, 2014 at 02:23:53PM +0100, Tvrtko Ursulin wrote: I think it would be good to add some more tests to cover the tracking handover between the interval tree and linear list to ensure invalidation still works correctly in non-trivial cases. Code looks correct in that respect but just in case. It is not a top priority so not sure when I'll find time to actually do it. We don't yet have some tests with overlapping allocations? I think at least some basic smoke test should be there ... We do have basic overlapping tests. I was talking about adding some more to exercise specific kernel paths which this patch adds. It switches between using a linear list and an interval tree to track ranges so just to make sure state remains correct under such transitions. But you also reminded me now, IGT needs to be told overalap is now allowed... Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 13/44] drm/i915: Added scheduler hook when closing DRM file handles
On 02/07/2014 19:20, Jesse Barnes wrote:
On Thu, 26 Jun 2014 18:24:04 +0100
john.c.harri...@intel.com wrote:
From: John Harrison john.c.harri...@intel.com
The scheduler decouples the submission of batch buffers to the driver with
submission of batch buffers to the hardware. Thus it is possible for an
application to submit work, then close the DRM handle and free up all the
resources that piece of work wishes to use before the work has even been
submitted to the hardware. To prevent this, the scheduler needs to be informed
of the DRM close event so that it can force through any outstanding work
attributed to that file handle.
---
drivers/gpu/drm/i915/i915_dma.c |3 +++
drivers/gpu/drm/i915/i915_scheduler.c | 18 ++
drivers/gpu/drm/i915/i915_scheduler.h |2 ++
3 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 494b156..6c9ce82 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -42,6 +42,7 @@
#include linux/vga_switcheroo.h
#include linux/slab.h
#include acpi/video.h
+#include i915_scheduler.h
#include linux/pm.h
#include linux/pm_runtime.h
#include linux/oom.h
@@ -1930,6 +1931,8 @@ void i915_driver_lastclose(struct drm_device * dev)
void i915_driver_preclose(struct drm_device *dev, struct drm_file *file)
{
+ i915_scheduler_closefile(dev, file);
+
mutex_lock(dev-struct_mutex);
i915_gem_context_close(dev, file);
i915_gem_release(dev, file);
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c
b/drivers/gpu/drm/i915/i915_scheduler.c
index d9c1879..66a6568 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -78,6 +78,19 @@ bool i915_scheduler_is_seqno_in_flight(struct
intel_engine_cs *ring,
return found;
}
+int i915_scheduler_closefile(struct drm_device *dev, struct drm_file *file)
+{
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_scheduler *scheduler = dev_priv-scheduler;
+
+ if (!scheduler)
+ return 0;
+
+ /* Do stuff... */
+
+ return 0;
+}
+
#else /* CONFIG_DRM_I915_SCHEDULER */
int i915_scheduler_init(struct drm_device *dev)
@@ -85,4 +98,9 @@ int i915_scheduler_init(struct drm_device *dev)
return 0;
}
+int i915_scheduler_closefile(struct drm_device *dev, struct drm_file *file)
+{
+ return 0;
+}
+
#endif /* CONFIG_DRM_I915_SCHEDULER */
diff --git a/drivers/gpu/drm/i915/i915_scheduler.h
b/drivers/gpu/drm/i915/i915_scheduler.h
index 4044b6e..95641f6 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.h
+++ b/drivers/gpu/drm/i915/i915_scheduler.h
@@ -27,6 +27,8 @@
booli915_scheduler_is_enabled(struct drm_device *dev);
int i915_scheduler_init(struct drm_device *dev);
+int i915_scheduler_closefile(struct drm_device *dev,
+struct drm_file *file);
#ifdef CONFIG_DRM_I915_SCHEDULER
Yeah I guess the client could have passed a ref to some other process
for tracking the outstanding work, so we need to complete it.
But shouldn't that happen as part of the clearing of the outstanding
requests in i915_gem_suspend() which is called from lastclose()? We do
a gpu_idle() and retire_requests() in there already...
Note that this is per file close not the global close. Individual DRM
file handles are closed whenever a user land app stops using DRM. When
that happens, the scheduler needs to clean up all references to that
handle. It is not just to ensure all work belonging to that handle has
completed but also to ensure the scheduler does not attempt to deference
dodgy file pointers later on.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 16/44] drm/i915: Alloc early seqno
On 02/07/2014 19:29, Jesse Barnes wrote:
On Thu, 26 Jun 2014 18:24:07 +0100
john.c.harri...@intel.com wrote:
From: John Harrison john.c.harri...@intel.com
The scheduler needs to explicitly allocate a seqno to track each submitted batch
buffer. This must happen a long time before any commands are actually written to
the ring.
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |5 +
drivers/gpu/drm/i915/intel_ringbuffer.c|2 +-
drivers/gpu/drm/i915/intel_ringbuffer.h|1 +
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ee836a6..ec274ef 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1317,6 +1317,11 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
*data,
vma-bind_vma(vma, batch_obj-cache_level, GLOBAL_BIND);
}
+ /* Allocate a seqno for this batch buffer nice and early. */
+ ret = intel_ring_alloc_seqno(ring);
+ if (ret)
+ goto err;
+
if (flags I915_DISPATCH_SECURE)
exec_start += i915_gem_obj_ggtt_offset(batch_obj);
else
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 34d6d6e..737c41b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1662,7 +1662,7 @@ int intel_ring_idle(struct intel_engine_cs *ring)
return i915_wait_seqno(ring, seqno);
}
-static int
+int
intel_ring_alloc_seqno(struct intel_engine_cs *ring)
{
if (ring-outstanding_lazy_seqno)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 30841ea..cc92de2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -347,6 +347,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs
*ring);
int __must_check intel_ring_begin(struct intel_engine_cs *ring, int n);
int __must_check intel_ring_cacheline_align(struct intel_engine_cs *ring);
+int __must_check intel_ring_alloc_seqno(struct intel_engine_cs *ring);
static inline void intel_ring_emit(struct intel_engine_cs *ring,
u32 data)
{
This ought to be ok even w/o the scheduler, we'll just pick up the
lazy_seqno later on rather than allocating a new one at ring_begin
right?
Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org
Yes. The early allocation is completely benign.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] igt/gem_userptr_blits: Overlapping objects are now allowed
Just tell the test to expect them to work then. Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Cc: Chris Wilson ch...@chris-wilson.co.uk --- tests/gem_userptr_blits.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c index 2a52856..cc6d31f 100644 --- a/tests/gem_userptr_blits.c +++ b/tests/gem_userptr_blits.c @@ -1340,7 +1340,7 @@ int main(int argc, char **argv) test_create_destroy(fd); igt_subtest(sync-overlap) - test_overlap(fd, EINVAL); + test_overlap(fd, 0); igt_subtest(sync-unmap) test_unmap(fd, EFAULT); -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] igt/gem_userptr_blits: Overlapping objects are now allowed
On Wed, Jul 23, 2014 at 04:12:55PM +0100, Tvrtko Ursulin wrote: Just tell the test to expect them to work then. Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Cc: Chris Wilson ch...@chris-wilson.co.uk For funsies we should also make sure that the results written to one are visible in the other. -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] [RFC 15/44] drm/i915: Added deferred work handler for scheduler
On 07/07/2014 20:14, Daniel Vetter wrote:
On Thu, Jun 26, 2014 at 06:24:06PM +0100, john.c.harri...@intel.com wrote:
From: John Harrison john.c.harri...@intel.com
The scheduler needs to do interrupt triggered work that is too complex to do in
the interrupt handler. Thus it requires a deferred work handler to process this
work asynchronously.
---
drivers/gpu/drm/i915/i915_dma.c |3 +++
drivers/gpu/drm/i915/i915_drv.h | 10 ++
drivers/gpu/drm/i915/i915_gem.c | 27 +++
drivers/gpu/drm/i915/i915_scheduler.c |7 +++
drivers/gpu/drm/i915/i915_scheduler.h |1 +
5 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 1668316..d1356f3 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1813,6 +1813,9 @@ int i915_driver_unload(struct drm_device *dev)
WARN_ON(unregister_oom_notifier(dev_priv-mm.oom_notifier));
unregister_shrinker(dev_priv-mm.shrinker);
+ /* Cancel the scheduler work handler, which should be idle now. */
+ cancel_work_sync(dev_priv-mm.scheduler_work);
+
io_mapping_free(dev_priv-gtt.mappable);
arch_phys_wc_del(dev_priv-gtt.mtrr);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0977653..fbafa68 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1075,6 +1075,16 @@ struct i915_gem_mm {
struct delayed_work idle_work;
/**
+* New scheme is to get an interrupt after every work packet
+* in order to allow the low latency scheduling of pending
+* packets. The idea behind adding new packets to a pending
+* queue rather than directly into the hardware ring buffer
+* is to allow high priority packets to over take low priority
+* ones.
+*/
+ struct work_struct scheduler_work;
Latency for work items isn't too awesome, and e.g. Oscar's execlist code
latches the next context right away from the irq handler. Why can't we do
something similar for the scheduler? Fishing the next item out of a
priority queue shouldn't be expensive ...
-Daniel
The problem is that taking batch buffers from the scheduler's queue and
submitting them to the hardware requires lots of processing that is not
IRQ compatible. It isn't just a simple register write. Half of the code
in 'i915_gem_do_execbuffer()' must be executed. Probably/possibly it
could be made IRQ friendly but that would place a lot of restrictions on
a lot of code that currently doesn't expect to be restricted. Instead,
the submission is done via a work handler that acquires the driver mutex
lock.
In order to cover the extra latency, the scheduler operates in a
multi-buffered mode and aims to keep eight batch buffers in flight at
all times. That number being obtained empirically by running lots of
benchmarks on Android with lots of different settings and seeing where
the buffer size stopped making a difference.
John.
+
+ /**
* Are we in a non-interruptible section of code like
* modesetting?
*/
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fece5e7..57b24f0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2712,6 +2712,29 @@ i915_gem_idle_work_handler(struct work_struct *work)
intel_mark_idle(dev_priv-dev);
}
+#ifdef CONFIG_DRM_I915_SCHEDULER
+static void
+i915_gem_scheduler_work_handler(struct work_struct *work)
+{
+ struct intel_engine_cs *ring;
+ struct drm_i915_private *dev_priv;
+ struct drm_device *dev;
+ int i;
+
+ dev_priv = container_of(work, struct drm_i915_private,
mm.scheduler_work);
+ dev = dev_priv-dev;
+
+ mutex_lock(dev-struct_mutex);
+
+ /* Do stuff: */
+ for_each_ring(ring, dev_priv, i) {
+ i915_scheduler_remove(ring);
+ }
+
+ mutex_unlock(dev-struct_mutex);
+}
+#endif
+
/**
* Ensures that an object will eventually get non-busy by flushing any
required
* write domains, emitting any outstanding lazy request and retiring and
@@ -4916,6 +4939,10 @@ i915_gem_load(struct drm_device *dev)
i915_gem_retire_work_handler);
INIT_DELAYED_WORK(dev_priv-mm.idle_work,
i915_gem_idle_work_handler);
+#ifdef CONFIG_DRM_I915_SCHEDULER
+ INIT_WORK(dev_priv-mm.scheduler_work,
+ i915_gem_scheduler_work_handler);
+#endif
init_waitqueue_head(dev_priv-gpu_error.reset_queue);
/* On GEN3 we really need to make sure the ARB C3 LP bit is set */
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c
b/drivers/gpu/drm/i915/i915_scheduler.c
index 66a6568..37f8a98 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@
Re: [Intel-gfx] [RFC 13/44] drm/i915: Added scheduler hook when closing DRM file handles
On Wed, 23 Jul 2014 16:10:32 +0100
John Harrison john.c.harri...@intel.com wrote:
On 02/07/2014 19:20, Jesse Barnes wrote:
On Thu, 26 Jun 2014 18:24:04 +0100
john.c.harri...@intel.com wrote:
From: John Harrison john.c.harri...@intel.com
The scheduler decouples the submission of batch buffers to the driver with
submission of batch buffers to the hardware. Thus it is possible for an
application to submit work, then close the DRM handle and free up all the
resources that piece of work wishes to use before the work has even been
submitted to the hardware. To prevent this, the scheduler needs to be
informed
of the DRM close event so that it can force through any outstanding work
attributed to that file handle.
---
drivers/gpu/drm/i915/i915_dma.c |3 +++
drivers/gpu/drm/i915/i915_scheduler.c | 18 ++
drivers/gpu/drm/i915/i915_scheduler.h |2 ++
3 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 494b156..6c9ce82 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -42,6 +42,7 @@
#include linux/vga_switcheroo.h
#include linux/slab.h
#include acpi/video.h
+#include i915_scheduler.h
#include linux/pm.h
#include linux/pm_runtime.h
#include linux/oom.h
@@ -1930,6 +1931,8 @@ void i915_driver_lastclose(struct drm_device * dev)
void i915_driver_preclose(struct drm_device *dev, struct drm_file *file)
{
+ i915_scheduler_closefile(dev, file);
+
mutex_lock(dev-struct_mutex);
i915_gem_context_close(dev, file);
i915_gem_release(dev, file);
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c
b/drivers/gpu/drm/i915/i915_scheduler.c
index d9c1879..66a6568 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -78,6 +78,19 @@ bool i915_scheduler_is_seqno_in_flight(struct
intel_engine_cs *ring,
return found;
}
+int i915_scheduler_closefile(struct drm_device *dev, struct drm_file
*file)
+{
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_scheduler *scheduler = dev_priv-scheduler;
+
+ if (!scheduler)
+ return 0;
+
+ /* Do stuff... */
+
+ return 0;
+}
+
#else /* CONFIG_DRM_I915_SCHEDULER */
int i915_scheduler_init(struct drm_device *dev)
@@ -85,4 +98,9 @@ int i915_scheduler_init(struct drm_device *dev)
return 0;
}
+int i915_scheduler_closefile(struct drm_device *dev, struct drm_file
*file)
+{
+ return 0;
+}
+
#endif /* CONFIG_DRM_I915_SCHEDULER */
diff --git a/drivers/gpu/drm/i915/i915_scheduler.h
b/drivers/gpu/drm/i915/i915_scheduler.h
index 4044b6e..95641f6 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.h
+++ b/drivers/gpu/drm/i915/i915_scheduler.h
@@ -27,6 +27,8 @@
booli915_scheduler_is_enabled(struct drm_device *dev);
int i915_scheduler_init(struct drm_device *dev);
+int i915_scheduler_closefile(struct drm_device *dev,
+ struct drm_file *file);
#ifdef CONFIG_DRM_I915_SCHEDULER
Yeah I guess the client could have passed a ref to some other process
for tracking the outstanding work, so we need to complete it.
But shouldn't that happen as part of the clearing of the outstanding
requests in i915_gem_suspend() which is called from lastclose()? We do
a gpu_idle() and retire_requests() in there already...
Note that this is per file close not the global close. Individual DRM
file handles are closed whenever a user land app stops using DRM. When
that happens, the scheduler needs to clean up all references to that
handle. It is not just to ensure all work belonging to that handle has
completed but also to ensure the scheduler does not attempt to deference
dodgy file pointers later on.
Ah yeah sorry, mixed it up with lastclose. Looks fine for per-client
close.
--
Jesse Barnes, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [ANNOUNCE] xf86-video-intel 2.99.914
Snapshot 2.99.914 (2014-07-23) == And a brown paper bag to hide the rebuilding from the tarball with 'autoreconf -fi' error that arose from not distributing the libobj/ directory. Complete list of changes since 2.99.913 --- Chris Wilson (4): Fix compile failure on old Xorg with XF86_ALLOCATE_GPU_SCREEN Add automake magic required for libobj/ sna/dri2: Compile fix for old xorg/dri2 2.99.914 snapshot git tag: 2.99.914 http://xorg.freedesktop.org/archive/individual/driver/xf86-video-intel-2.99.914.tar.bz2 MD5: b53d96cb42fb1d71f0b671dd2064cba4 xf86-video-intel-2.99.914.tar.bz2 SHA1: a2d8415dd4be514720b8470399d5d22d9fb4caa8 xf86-video-intel-2.99.914.tar.bz2 SHA256: 78a22e5efd460b790c634caaf1afbb756046dd890482e204bb0d179baad27e46 xf86-video-intel-2.99.914.tar.bz2 http://xorg.freedesktop.org/archive/individual/driver/xf86-video-intel-2.99.914.tar.gz MD5: 1298bfa698fe60583a2b410473d24df9 xf86-video-intel-2.99.914.tar.gz SHA1: e74591cad6968026e741e558fc51504bf61f62b8 xf86-video-intel-2.99.914.tar.gz SHA256: 6c26e2d5bf440ef0028aae4e00a5432f9c88f4eb61eeadb7a169cbe909b9e81b xf86-video-intel-2.99.914.tar.gz -- Chris Wilson, Intel Open Source Technology Centre signature.asc Description: Digital signature ___ 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: Prevent recursive deadlock on releasing a busy userptr
On 07/21/2014 01:21 PM, Chris Wilson wrote:
During release of the GEM object we hold the struct_mutex. As the
object may be holding onto the last reference for the task-mm,
calling mmput() may trigger exit_mmap() which close the vma
which will call drm_gem_vm_close() and attempt to reacquire
the struct_mutex. In order to avoid that recursion, we have
to defer the mmput() until after we drop the struct_mutex,
i.e. we need to schedule a worker to do the clean up. A further issue
spotted by Tvrtko was caused when we took a GTT mmapping of a userptr
buffer object. In that case, we would never call mmput as the object
would be cyclically referenced by the GTT mmapping and not freed upon
process exit - keeping the entire process mm alive after the process
task was reaped. The fix employed is to replace the mm_users/mmput()
reference handling to mm_count/mmdrop() for the shared i915_mm_struct.
INFO: task test_surfaces:1632 blocked for more than 120 seconds.
Tainted: GF O 3.14.5+ #1
echo 0 /proc/sys/kernel/hung_task_timeout_secs disables this message.
test_surfaces D 0 1632 1590 0x0082
88014914baa8 0046 88014914a010
00012c40 00012c40 8800a0058210 88014784b010
88014914a010 880037b1c820 8800a0058210 880037b1c824
Call Trace:
[81582499] schedule+0x29/0x70
[815825fe] schedule_preempt_disabled+0xe/0x10
[81583b93] __mutex_lock_slowpath+0x183/0x220
[81583c53] mutex_lock+0x23/0x40
[a005c2a3] drm_gem_vm_close+0x33/0x70 [drm]
[8115a483] remove_vma+0x33/0x70
[8115a5dc] exit_mmap+0x11c/0x170
[8104d6eb] mmput+0x6b/0x100
[a00f44b9] i915_gem_userptr_release+0x89/0xc0 [i915]
[a00e6706] i915_gem_free_object+0x126/0x250 [i915]
[a005c06a] drm_gem_object_free+0x2a/0x40 [drm]
[a005cc32] drm_gem_object_handle_unreference_unlocked+0xe2/0x120
[drm]
[a005ccd4] drm_gem_object_release_handle+0x64/0x90 [drm]
[8127ffeb] idr_for_each+0xab/0x100
[a005cc70] ?
drm_gem_object_handle_unreference_unlocked+0x120/0x120 [drm]
[81583c46] ? mutex_lock+0x16/0x40
[a005c354] drm_gem_release+0x24/0x40 [drm]
[a005b82b] drm_release+0x3fb/0x480 [drm]
[8118d482] __fput+0xb2/0x260
[8118d6de] fput+0xe/0x10
[8106f27f] task_work_run+0x8f/0xf0
[81052228] do_exit+0x1a8/0x480
[81052551] do_group_exit+0x51/0xc0
[810525d7] SyS_exit_group+0x17/0x20
[8158e092] system_call_fastpath+0x16/0x1b
Reported-by: Jacek Danecki jacek.dane...@intel.com
Test-case: igt/gem_userptr_blits/process-exit*
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Tested-by: Gong, Zhipeng zhipeng.g...@intel.com
Cc: Jacek Danecki jacek.dane...@intel.com
Cc: Ursulin, Tvrtko tvrtko.ursu...@intel.com
I could not fail the new locking/mm handling scheme. It looks neat,
actually nicer than before, but it is quite complex. So I don't
guarantee I haven't overlooked something.
Some minor comments are in line.
And on the topic of IGT, I did not come up with any fool proof
reliable way of improving what you added. We could spin some leak tests
in a loop a bit, to increase chances of exhausting all resources but
that is again non-deterministic.
drivers/gpu/drm/i915/i915_drv.h | 10 +-
drivers/gpu/drm/i915/i915_gem_userptr.c | 413 ++--
2 files changed, 239 insertions(+), 184 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8eb9e05..d426aac7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -185,6 +185,7 @@ enum hpd_pin {
if ((1 (domain)) (mask))
struct drm_i915_private;
+struct i915_mm_struct;
struct i915_mmu_object;
enum intel_dpll_id {
@@ -1518,9 +1519,8 @@ struct drm_i915_private {
struct i915_gtt gtt; /* VM representing the global address space */
struct i915_gem_mm mm;
-#if defined(CONFIG_MMU_NOTIFIER)
- DECLARE_HASHTABLE(mmu_notifiers, 7);
-#endif
+ DECLARE_HASHTABLE(mm_structs, 7);
+ struct mutex mm_lock;
/* Kernel Modesetting */
@@ -1818,8 +1818,8 @@ struct drm_i915_gem_object {
unsigned workers :4;
#define I915_GEM_USERPTR_MAX_WORKERS 15
- struct mm_struct *mm;
- struct i915_mmu_object *mn;
+ struct i915_mm_struct *mm;
+ struct i915_mmu_object *mmu_object;
struct work_struct *work;
} userptr;
};
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 74c45da..12358fd 100644
---
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Prevent recursive deadlock on releasing a busy userptr
On Wed, Jul 23, 2014 at 05:39:49PM +0100, Tvrtko Ursulin wrote:
On 07/21/2014 01:21 PM, Chris Wilson wrote:
+mn = i915_mmu_notifier_get(obj-userptr.mm);
+if (IS_ERR(mn))
+return PTR_ERR(mn);
Very minor, but I would perhaps consider renaming this to _find
since _get in my mind strongly associates with reference counting
and this does not do that. Especially if the reviewer looks at the
bail out below and sees no matching put. But minor as I said, you
can judge what you prefer.
The same. It was _get because it did used to a reference counter, now
that counting has been removed from the i915_mmu_notifier.
+static int
+i915_gem_userptr_init__mm_struct(struct drm_i915_gem_object *obj)
+{
+struct drm_i915_private *dev_priv = to_i915(obj-base.dev);
+struct i915_mm_struct *mm;
+struct mm_struct *real;
+int ret = 0;
+
+real = get_task_mm(current);
+if (real == NULL)
+return -EINVAL;
Do you think we need get_task_mm()/mmput() here, given it is all
inside a single system call?
No. I kept using get_task_mm() simply because it looked neater than
current-mm, but current-mm looks like it gives simpler code.
+/* During release of the GEM object we hold the struct_mutex. As the
+ * object may be holding onto the last reference for the task-mm,
+ * calling mmput() may trigger exit_mmap() which close the vma
+ * which will call drm_gem_vm_close() and attempt to reacquire
+ * the struct_mutex. In order to avoid that recursion, we have
+ * to defer the mmput() until after we drop the struct_mutex,
+ * i.e. we need to schedule a worker to do the clean up.
+ */
This comment reads like a strange mixture and past and present eg.
what used to be the case and what is the fix. We don't hold a
reference to the process mm as the address space (terminology OK?).
We do hold a reference to the mm struct itself - which is enough to
unregister the notifiers, correct?
True. I was more or less trying to explain the bug and that comment
ended up being the changelog entry. It doesn't work well as a comment.
+ /* During release of the GEM object we hold the struct_mutex. This
+* precludes us from calling mmput() at that time as that may be
+* the last reference and so call exit_mmap(). exit_mmap() will
+* attempt to reap the vma, and if we were holding a GTT mmap
+* would then call drm_gem_vm_close() and attempt to reacquire
+* the struct mutex. So in order to avoid that recursion, we have
+* to defer releasing the mm reference until after we drop the
+* struct_mutex, i.e. we need to schedule a worker to do the clean
+* up.
*/
-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
[Intel-gfx] [PATCH 00/11]: Color manager framework for I915 driver
From: Shashank Sharma shashank.sha...@intel.com This patchset adds color-manager, a new framework in I915 driver which adds color correction and tweak capabilities in the driver. Color manager creates a DRM propery based interface for each color correction, and based on the property type, registers it with each CRTC/plane available. The current implementation is for valleyview family. Valleyview supports following color correction properties: 1. CSC correction (wide gamut): This is a pipe level correction. There are total 9 correction coefficients in form of a 3x3 matrix, which are to be programmed on 6 correction registers. CSC correction 2. Gamma correction: This is also pipe level correction There are total 256 palette registers, which can be programmed with 128 correction values, in 10.6 (10bit) format. The expected color Correction can be applied using 129, 64 bit correction values. First 128 correction values are to program palette, 129th value is for GCMAX register value. correction format in a 64 bit value is: | 16 higher bits| 16bit R value|16 bit G value|16 bit B value| 3. Contrast: This is sprite plane level correction Expected correction value is 9 bit value Driver expects values in this format: |bits 64:32 | bits 31:9 | 8:0 contrast correction value| 4. Brightness: This is also a sprite level correction Expected correction value is 8 bit value Driver expects values in this format: |bits 64:32 | bits 31:8 | 7:0 9 bit brightness correction value| 5. Hue and saturation: This is also a sprite level correction Expected correction value is 32 bit value Driver expects values in this format: |bits 64:32| bits 31:0 hs correction value| Patches: 1. First three patches create the basic framework. 2. Next 4 add functions to do color correction per property. 3. Next 2 add interface to set property. 4. last 2 patches plug-in init and exit in modeset sequences. Shashank Sharma (11): drm/i915: Color manager framework for valleyview drm/i915: Register pipe level color properties drm/i915: Register plane level color properties drm/i915: Add color manager CSC correction drm/i915: Add color manager gamma correction drm/i915: Add contrast and brightness correction drm/i915: Add hue and saturation correction drm/i915: Add CRTC set property functions drm/i915: Add set plane property functions drm/i915: Plug-in color manager init drm/i915: Plug-in color manager exit drivers/gpu/drm/i915/Makefile| 3 +- drivers/gpu/drm/i915/i915_reg.h | 22 + drivers/gpu/drm/i915/intel_clrmgr.c | 795 +++ drivers/gpu/drm/i915/intel_clrmgr.h | 282 + drivers/gpu/drm/i915/intel_display.c | 50 +++ drivers/gpu/drm/i915/intel_drv.h | 6 + drivers/gpu/drm/i915/intel_sprite.c | 45 ++ 7 files changed, 1202 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/11] drm/i915: Color manager framework for valleyview
From: Shashank Sharma shashank.sha...@intel.com
Color manager is a framework which adds color correction
and tuning capabilities in I915 driver. This framework creates
DRM properties for each color correction property, and allows
userspace to tune the display appearance.
This is the first patch of the series, what this patch does is:
1. Create 2 new files
intel_clrmgr.c
intel_clrmgr.h
2. Add color manager init function, this functions allocates
memory to save an array of color properties. This is called
during the CRTC init and Plane init time.
3. Add color manager exit function. This function free's the
memory allocated by init function, and registered color
properties.
4. Some data structure definitions, in header.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/Makefile | 3 +-
drivers/gpu/drm/i915/intel_clrmgr.c | 79 +++
drivers/gpu/drm/i915/intel_clrmgr.h | 83 +
3 files changed, 164 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 91bd167..7fa9c58 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -45,7 +45,8 @@ i915-y += intel_bios.o \
intel_modes.o \
intel_overlay.o \
intel_sideband.o \
- intel_sprite.o
+ intel_sprite.o \
+ intel_clrmgr.o
i915-$(CONFIG_ACPI)+= intel_acpi.o intel_opregion.o
i915-$(CONFIG_DRM_I915_FBDEV) += intel_fbdev.o
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
new file mode 100644
index 000..09a168d
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -0,0 +1,79 @@
+/*
+ * Copyright © 2014 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 the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * ==
+ * Shashank Sharma shashank.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Shobhit Kumar shobhit.ku...@intel.com
+ * Sonika Jindal sonika.jin...@intel.com
+ */
+
+#include i915_drm.h
+#include i915_drv.h
+#include i915_reg.h
+#include intel_clrmgr.h
+
+struct clrmgr_status *intel_clrmgr_init(struct drm_device *dev)
+{
+ struct clrmgr_status *status;
+
+ /* Todo: extend this framework for other gen devices also */
+ if (!IS_VALLEYVIEW(dev)) {
+ DRM_ERROR(Color manager is supported for VLV for now\n);
+ return NULL;
+ }
+
+ /* Allocate and attach color status tracker */
+ status = kzalloc(sizeof(struct clrmgr_status), GFP_KERNEL);
+ if (!status) {
+ DRM_ERROR(Out of memory, cant init color manager\n);
+ return NULL;
+ }
+ DRM_DEBUG_DRIVER(\n);
+ return status;
+}
+
+void intel_clrmgr_exit(struct drm_device *dev, struct clrmgr_status *status)
+{
+ u32 count = 0;
+ struct clrmgr_regd_prop *cp;
+
+ if (!status)
+ return;
+
+ /* First free the DRM property, then status */
+ while (count status-no_of_properties) {
+ cp = status-cp[count++];
+
+ /* Destroy DRM property */
+ drm_property_destroy(dev, cp-property);
+
+ /* Release the color property */
+ kfree(status-cp[count]);
+ status-cp[count] = NULL;
+ }
+
+ /* Now free the status itself */
+ kfree(status);
+ DRM_DEBUG_DRIVER(\n);
+}
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h
b/drivers/gpu/drm/i915/intel_clrmgr.h
new file mode 100644
index 000..6a36c8d
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -0,0 +1,83 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ *
[Intel-gfx] [PATCH 02/11] drm/i915: Register pipe level color properties
From: Shashank Sharma shashank.sha...@intel.com
In valleyview we have two pipe level color correction
properties:
1. CSC correction (wide gamut)
2. Gamma correction
What this patch does:
1. This patch adds software infrastructure to register pipe level
color correction properties per CRTC. Adding a new function,
intel_attach_pipe_color_correction to register the pipe level
color correction properties with the given CRTC.
2. Adding a pointer in intel_crtc structure to store this property.
3. Adding structure gen6_pipe_color_corrections, which contains different
pipe level correction values for VLV.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/intel_clrmgr.c | 148
drivers/gpu/drm/i915/intel_clrmgr.h | 64
drivers/gpu/drm/i915/intel_drv.h| 3 +
3 files changed, 215 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index 09a168d..8d02a62 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -33,6 +33,154 @@
#include i915_reg.h
#include intel_clrmgr.h
+/*
+ * Gen 6 SOC allows following color correction values:
+ *- CSC(wide gamut) with 3x3 matrix = 9 csc correction values.
+ *- Gamma correction with 128 gamma values.
+ */
+struct clrmgr_property gen6_pipe_color_corrections[] = {
+ {
+ .tweak_id = csc,
+ .type = DRM_MODE_PROP_BLOB,
+ .len = VLV_CSC_MATRIX_MAX_VALS,
+ .name = csc-correction,
+ },
+ {
+ .tweak_id = gamma,
+ .type = DRM_MODE_PROP_BLOB,
+ .len = VLV_10BIT_GAMMA_MAX_VALS,
+ .name = gamma-correction,
+ },
+};
+
+struct drm_property *intel_clrmgr_register(struct drm_device *dev,
+ struct drm_mode_object *obj, struct clrmgr_property *cp)
+{
+ struct drm_property *property;
+
+ /* Create drm property */
+ switch (cp-type) {
+ case DRM_MODE_PROP_BLOB:
+ property = drm_property_create(dev, DRM_MODE_PROP_BLOB,
+ cp-name, cp-len);
+ if (!property) {
+ DRM_ERROR(Failed to create property %s\n, cp-name);
+ goto error;
+ }
+ break;
+
+ case DRM_MODE_PROP_RANGE:
+ property = drm_property_create_range(dev, DRM_MODE_PROP_RANGE,
+ cp-name, cp-min, cp-max);
+ if (!property) {
+ DRM_ERROR(Failed to create property %s\n, cp-name);
+ goto error;
+ }
+ break;
+
+ default:
+ DRM_ERROR(Unsupported type for property %s\n, cp-name);
+ goto error;
+ }
+ /* Attach property to object */
+ drm_object_attach_property(obj, property, 0);
+ DRM_DEBUG_DRIVER(Registered property %s\n, property-name);
+ return property;
+
+error:
+ DRM_ERROR(Failed to create property %s\n, cp-name);
+ return NULL;
+}
+
+bool intel_clrmgr_register_pipe_property(struct intel_crtc *intel_crtc,
+ struct clrmgr_reg_request *features)
+
+{
+ u32 count = 0;
+ struct clrmgr_property *cp;
+ struct clrmgr_regd_prop *regd_property;
+ struct drm_property *property;
+ struct drm_device *dev = intel_crtc-base.dev;
+ struct drm_mode_object *obj = intel_crtc-base.base;
+ struct clrmgr_status *status = intel_crtc-color_status;
+
+ /* Color manager initialized? */
+ if (!status) {
+ DRM_ERROR(Register request without pipe init ?\n);
+ return false;
+ }
+
+ /* Validate input */
+ if (!features || !features-no_of_properties) {
+ DRM_ERROR(Invalid input to color manager register\n);
+ return false;
+ }
+
+ /* Create drm property */
+ while (count features-no_of_properties) {
+ cp = features-cp[count++];
+ property = intel_clrmgr_register(dev, obj, cp);
+ if (!property) {
+ DRM_ERROR(Failed to register property %s\n,
+ property-name);
+ goto error;
+ }
+
+ /* Add the property in global pipe status */
+ regd_property = kzalloc(sizeof(struct clrmgr_regd_prop),
+ GFP_KERNEL);
+ regd_property-property = property;
+ regd_property-enabled = false;
+ regd_property-set_property = cp-set_property;
+ status-cp[status-no_of_properties++] = regd_property;
+ }
+ /* Successfully registered all */
+ DRM_DEBUG_DRIVER(Registered color properties on pipe %c\n,
+ pipe_name(intel_crtc-pipe));
+ return true;
+
+error:
+ if
[Intel-gfx] [PATCH 03/11] drm/i915: Register plane level color properties
From: Shashank Sharma shashank.sha...@intel.com
In valleyview we have three possible sprite plane level
color correction:
1. Contrast
2. Brightness
What this patch does:
1. This patch adds software infrastructure to register plane level
color correction properties per plane. Adding a new function,
intel_attach_plane_color_correction to register the plane level
color correction properties.
2. Adding a pointer in intel_plane structure to store this property.
3. Adding structure gen6_plane_color_corrections, which contains different
plane level correction values for VLV.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/intel_clrmgr.c | 125
drivers/gpu/drm/i915/intel_clrmgr.h | 8 +++
drivers/gpu/drm/i915/intel_drv.h| 3 +
3 files changed, 136 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index 8d02a62..0aa3734 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -53,6 +53,41 @@ struct clrmgr_property gen6_pipe_color_corrections[] = {
},
};
+/*
+* Gen6 plane level properties:
+* - Contrast adjustment for all sprite planes.
+* - Brightness adjustment for all sprite planes.
+* - Hue and Saturation adjustment for all sprite planes.
+*/
+struct clrmgr_property gen6_plane_color_corrections[] = {
+
+ {
+ .tweak_id = contrast,
+ .type = DRM_MODE_PROP_RANGE,
+ .max = CLRMGR_PROP_RANGE_MAX,
+ .min = 0,
+ .len = VLV_CB_MAX_VALS,
+ .name = contrast,
+ },
+ {
+ .tweak_id = brightness,
+ .type = DRM_MODE_PROP_RANGE,
+ .max = CLRMGR_PROP_RANGE_MAX,
+ .min = 0,
+ .len = VLV_CB_MAX_VALS,
+ .name = brightness,
+ },
+ {
+ .tweak_id = hue_saturation,
+ .type = DRM_MODE_PROP_RANGE,
+ .max = CLRMGR_PROP_RANGE_MAX,
+ .min = 0,
+ .len = VLV_HS_MAX_VALS,
+ .name = hue-saturation,
+ }
+};
+
+
struct drm_property *intel_clrmgr_register(struct drm_device *dev,
struct drm_mode_object *obj, struct clrmgr_property *cp)
{
@@ -92,6 +127,96 @@ error:
return NULL;
}
+bool intel_clrmgr_register_plane_property(struct intel_plane *intel_plane,
+ struct clrmgr_reg_request *features)
+{
+ u32 count = 0;
+ struct clrmgr_property *cp;
+ struct clrmgr_regd_prop *regd_property;
+ struct drm_property *property;
+ struct drm_device *dev = intel_plane-base.dev;
+ struct drm_mode_object *obj = intel_plane-base.base;
+ struct clrmgr_status *status = intel_plane-color_status;
+
+ /* Color manager initialized? */
+ if (!status) {
+ DRM_ERROR(Register request without plane init ?\n);
+ return false;
+ }
+
+ /* Validate input */
+ if (!features || !features-no_of_properties) {
+ DRM_ERROR(Invalid input to register plane property\n);
+ return false;
+ }
+
+ /* Create drm property */
+ while (count features-no_of_properties) {
+ cp = features-cp[count++];
+ property = intel_clrmgr_register(dev, obj, cp);
+ if (!property) {
+ DRM_ERROR(Failed to register property %s\n,
+ property-name);
+ goto error;
+ }
+
+ /* Add the property in global pipe status */
+ regd_property = kzalloc(sizeof(struct clrmgr_regd_prop),
+ GFP_KERNEL);
+ regd_property-property = property;
+ regd_property-enabled = false;
+ regd_property-set_property = cp-set_property;
+ status-cp[status-no_of_properties++] = regd_property;
+ }
+
+ /* Successfully registered all */
+ DRM_DEBUG_DRIVER(Registered color properties on plane %d\n,
+ intel_plane-plane);
+ return true;
+
+error:
+ if (--count) {
+ DRM_ERROR(Can only register following properties:\n);
+ while (count--)
+ DRM_ERROR(%s, status-cp[count]-property-name);
+ } else
+ DRM_ERROR(Can not register any property\n);
+ return false;
+}
+
+void
+intel_attach_plane_color_correction(struct intel_plane *intel_plane)
+{
+ struct clrmgr_reg_request *features;
+
+ /* Color manager initialized? */
+ if (!intel_plane-color_status) {
+ DRM_ERROR(Color manager not initialized for plane %d\n,
+ intel_plane-plane);
+ return;
+ }
+
+ DRM_DEBUG_DRIVER(\n);
+ features = kzalloc(sizeof(struct clrmgr_reg_request), GFP_KERNEL);
+
[Intel-gfx] [PATCH 08/11] drm/i915: Add CRTC set property functions
From: Shashank Sharma shashank.sha...@intel.com
Color manager's pipe level correction properties are
registered as CRTC property. So its required to have a
.set_crtc function in CRTC functions.
This patch adds:
1. A .set_property function for intel_crtc, intel_crtc_set_property
which checks if a CRTC property is of type color property, it
calls color manager's pipe level set_property handler function.
2. A intel_clrmgr_set_pipe_property, which will extract the data
to be set, and then pass it to appropriate set_property function.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/intel_clrmgr.c | 47
drivers/gpu/drm/i915/intel_clrmgr.h | 11 +
drivers/gpu/drm/i915/intel_display.c | 45 ++
3 files changed, 103 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index a4c8f0f..eb18ee2 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -654,6 +654,53 @@ intel_attach_pipe_color_correction(struct intel_crtc
*intel_crtc)
kfree(features);
}
+bool intel_clrmgr_set_pipe_property(struct intel_crtc *intel_crtc,
+ struct clrmgr_regd_prop *cp, uint64_t value)
+{
+ bool ret = false;
+ uint64_t *data;
+ struct drm_property *property;
+
+ /* Sanity */
+ if (!cp-property) {
+ DRM_ERROR(NULL input to set_property\n);
+ return false;
+ }
+
+ property = cp-property;
+ DRM_DEBUG_DRIVER(Property %s len:%d\n,
+ cp-property-name, cp-property-num_values);
+ data = kmalloc(sizeof(uint64_t) * (property-num_values), GFP_KERNEL);
+ if (!data) {
+ DRM_ERROR(Out of memory\n);
+ return false;
+ }
+
+ if (copy_from_user((void *)data, (const void __user *)value,
+ property-num_values * sizeof(uint64_t))) {
+ DRM_ERROR(Failed to copy all data\n);
+ ret = false;
+ goto free_and_return;
+ }
+
+ /* Now do the actual work */
+ if (cp-set_property) {
+ if (!cp-set_property((void *)intel_crtc, cp, data)) {
+ DRM_ERROR(Set property for %s failed\n,
+ cp-property-name);
+ ret = false;
+ } else {
+ ret = true;
+ cp-enabled = true;
+ DRM_DEBUG_DRIVER(Set property %s successful\n,
+ cp-property-name);
+ }
+ }
+free_and_return:
+ kfree(data);
+ return ret;
+}
+
struct clrmgr_status *intel_clrmgr_init(struct drm_device *dev)
{
struct clrmgr_status *status;
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h
b/drivers/gpu/drm/i915/intel_clrmgr.h
index 6d316d2..d962585 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.h
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -212,6 +212,17 @@ bool intel_clrmgr_set_csc(void *crtc,
struct clrmgr_regd_prop *csc, u64 *data);
/*
+* intel_clrmgr_set_pipe_property
+* Set value of a registered CRTC property
+* input:
+* - intel_crtc: the CRTC with which the property is attached
+* - cp: registered color property
+* - value: value to be set
+*/
+bool intel_clrmgr_set_pipe_property(struct intel_crtc *intel_crtc,
+ struct clrmgr_regd_prop *cp, uint64_t value);
+
+/*
* intel_clrmgr_register_pipe_property
* register set of properties with a CRTC
* input:
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 99eb7ca..a6181b5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -42,6 +42,7 @@
#include drm/drm_plane_helper.h
#include drm/drm_rect.h
#include linux/dma_remapping.h
+#include intel_clrmgr.h
/* Primary plane formats supported by all gen */
#define COMMON_PRIMARY_FORMATS \
@@ -8438,6 +8439,49 @@ mode_fits_in_fbdev(struct drm_device *dev,
#endif
}
+/*
+* intel_crtc_set_property
+* Set a CRTC property, like color tweaks
+*/
+static int intel_crtc_set_property(struct drm_crtc *crtc,
+ struct drm_property *property, uint64_t val)
+{
+ int ret = 0;
+ int count = 0;
+ struct clrmgr_regd_prop *cp;
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct clrmgr_status *status = intel_crtc-color_status;
+
+ DRM_DEBUG_DRIVER(\n);
+
+ /* Is this color property ?*/
+ if (!status) {
+ DRM_DEBUG_DRIVER(Color manager not initialized\n);
+ ret = -1;
+ goto skip_color;
+ }
+
+ /* Color correction property */
+ while (count status-no_of_properties) {
+ cp = status-cp[count++];
+ if (property == cp-property) {
+ /* Found it, now set
[Intel-gfx] [PATCH 04/11] drm/i915: Add color manager CSC correction
From: Shashank Sharma shashank.sha...@intel.com
This patch adds support for pipe CSC correction color property
for intel color manager framework. It adds two functions:
1. intel_clrmgr_set_csc: This is a wrapper function
which checks the platform type, and calls the valleyview
specific set_csc function. As different platforms have different
methods of setting CSC, this function is required.The support for
other platfroms can be plugged-in here in the wrapper function.
Adding this function as .set_property CSC color property.
2. vlv_set_csc: core function to program CSC coefficients as per
vlv specs, and then enable CSC.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 11 +
drivers/gpu/drm/i915/intel_clrmgr.c | 82 +
drivers/gpu/drm/i915/intel_clrmgr.h | 16
3 files changed, 109 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fe5c276..3199f96 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6084,6 +6084,17 @@ enum punit_power_well {
#define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME,
_PIPE_B_CSC_POSTOFF_ME)
#define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO,
_PIPE_B_CSC_POSTOFF_LO)
+/* VLV color correction registers */
+/* CSC */
+#define PIPECONF_CSC_ENABLE(1 15)
+#define _PIPEACSC (dev_priv-info.display_mmio_offset + \
+ 0x600b0)
+#define _PIPEBCSC (dev_priv-info.display_mmio_offset + \
+ 0x610b0)
+#define PIPECSC(pipe) (_PIPEACSC + (pipe * CSC_OFFSET))
+#define CSC_OFFSET (_PIPEBCSC - _PIPEACSC)
+#define PIPECSC(pipe) (_PIPEACSC + (pipe * CSC_OFFSET))
+
/* VLV MIPI registers */
#define _MIPIA_PORT_CTRL (VLV_DISPLAY_BASE + 0x61190)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index 0aa3734..601076b 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -44,6 +44,7 @@ struct clrmgr_property gen6_pipe_color_corrections[] = {
.type = DRM_MODE_PROP_BLOB,
.len = VLV_CSC_MATRIX_MAX_VALS,
.name = csc-correction,
+ .set_property = intel_clrmgr_set_csc,
},
{
.tweak_id = gamma,
@@ -87,6 +88,87 @@ struct clrmgr_property gen6_plane_color_corrections[] = {
}
};
+/*
+* vlv_set_csc
+* Valleyview specific csc correction method.
+* Programs the 6 csc registers with 3x3 correction matrix
+* values.
+* inputs:
+* - intel_crtc*
+* - color manager registered property for csc correction
+* - data: pointer to correction values to be applied
+*/
+/* Enable color space conversion on PIPE */
+bool vlv_set_csc(struct intel_crtc *intel_crtc,
+ struct clrmgr_regd_prop *csc, u64 *data)
+{
+ u32 count = 0;
+ u32 pipeconf, csc_reg, data_size;
+ struct drm_device *dev = intel_crtc-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_property *property;
+ u32 c0, c1, c2;
+
+ property = csc-property;
+ data_size = property-num_values;
+
+ /* Validate input */
+ if (data_size != VLV_CSC_MATRIX_MAX_VALS) {
+ DRM_ERROR(Unexpected value count for GAMMA LUT\n);
+ return false;
+ }
+
+ DRM_DEBUG_DRIVER(Setting CSC on pipe = %d\n, intel_crtc-pipe);
+ csc_reg = PIPECSC(intel_crtc-pipe);
+
+ /* Read CSC matrix, one row at a time */
+ while (count VLV_CSC_MATRIX_MAX_VALS) {
+ c0 = data[count] VLV_CSC_VALUE_MASK;
+ property-values[count++] = c0;
+ c1 = data[count] VLV_CSC_VALUE_MASK;
+ property-values[count++] = c1;
+ c2 = data[count] VLV_CSC_VALUE_MASK;
+ property-values[count++] = c2;
+
+ /* C0 is LSB 12bits, C1 is MSB 16-27 */
+ I915_WRITE(csc_reg, (c1 VLV_CSC_COEFF_SHIFT) | c0);
+ csc_reg += 4;
+
+ /* C2 is LSB 12 bits */
+ I915_WRITE(csc_reg, c2);
+ csc_reg += 4;
+ }
+
+ /* Enable csc correction */
+ pipeconf = I915_READ(PIPECONF(intel_crtc-pipe)) | PIPECONF_CSC_ENABLE;
+ I915_WRITE(PIPECONF(intel_crtc-pipe), pipeconf);
+ POSTING_READ(PIPECONF(intel_crtc-pipe));
+
+ DRM_DEBUG_DRIVER(CSC successfully set on pipe = %d\n,
+ intel_crtc-pipe);
+ return true;
+}
+
+bool intel_clrmgr_set_csc(void *crtc,
+ struct clrmgr_regd_prop *csc, u64 *data)
+{
+ struct intel_crtc *intel_crtc = crtc;
+ struct drm_device *dev = intel_crtc-base.dev;
+
+ /* Validate input */
+ if (!data || !csc || !csc-property)
[Intel-gfx] [PATCH 05/11] drm/i915: Add color manager gamma correction
From: Shashank Sharma shashank.sha...@intel.com
This patch adds support for pipe gamma correction color property
for intel color manager framework. It adds two functions:
1. intel_clrmgr_set_gamma: This is a wrapper function
which checks the platform type, and calls the valleyview
specific set_gamma function. As different platforms have different
methods of setting pipe gamma, this function is required.The support for
other platfroms can be plugged-in here in the wrapper function.
Adding this function as .set_property of gamma-correction color property.
2. vlv_set_10_bit_gamma: Core function to program gamma coefficients as per
vlv specs, and then enable gamma on pipe. This function uses 10-bit gamma
programming method of VLV (10.6 method), which is more accurate, and can
support fraction values also. Userspace encodes 16 bit per channel gamma
correction value in 64bit value in NoneR16G16B16 format, and sends
128 such gamma correction values. The 129th value is GCMAX register value.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 5 ++
drivers/gpu/drm/i915/intel_clrmgr.c | 125
drivers/gpu/drm/i915/intel_clrmgr.h | 29 +
3 files changed, 159 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3199f96..9501ad8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6095,6 +6095,11 @@ enum punit_power_well {
#define CSC_OFFSET (_PIPEBCSC - _PIPEACSC)
#define PIPECSC(pipe) (_PIPEACSC + (pipe * CSC_OFFSET))
+/* Gamma GCMAX */
+#define VLV_PIPEA_GCMAX(dev_priv-info.display_mmio_offset + 0x70010)
+#define VLV_PIPEB_GCMAX(dev_priv-info.display_mmio_offset + 0x71010)
+#define VLV_PIPE_GCMAX(pipe) _PIPE(pipe, VLV_PIPEA_GCMAX, VLV_PIPEB_GCMAX)
+
/* VLV MIPI registers */
#define _MIPIA_PORT_CTRL (VLV_DISPLAY_BASE + 0x61190)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index 601076b..38ba878 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -51,6 +51,7 @@ struct clrmgr_property gen6_pipe_color_corrections[] = {
.type = DRM_MODE_PROP_BLOB,
.len = VLV_10BIT_GAMMA_MAX_VALS,
.name = gamma-correction,
+ .set_property = intel_clrmgr_set_gamma,
},
};
@@ -89,6 +90,130 @@ struct clrmgr_property gen6_plane_color_corrections[] = {
};
/*
+* vlv_set_10bit_gamma
+* Valleyview specific gamma correction method.
+* Programs the palette registers in 10bit method
+* with 128 correction values, sampled across 1024
+* gamma correction values at sampling rate of 8.
+* inputs:
+* - intel_crtc*
+* - color manager registered property for gamma correction
+* - data: pointer to correction values to be applied
+*/
+static bool vlv_set_10bit_gamma(struct intel_crtc *intel_crtc,
+ struct clrmgr_regd_prop *gamma, u64 *data)
+{
+ u16 red, green, blue;
+ u64 correct_rgb;
+ u32 val, even, odd;
+ u32 count = 0;
+ u32 reg = 0;
+ struct drm_property *property = gamma-property;
+ struct drm_device *dev = intel_crtc-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ u32 palette = PALETTE(intel_crtc-pipe);
+
+ /* Validate input */
+ if (property-num_values != VLV_10BIT_GAMMA_MAX_VALS) {
+ DRM_ERROR(Unexpected value count for GAMMA LUT);
+ return false;
+ }
+
+ /* 128, 64 bit values, coming in 0R16G16B16 format containing
+ only 10 integer and 6fraction correction values */
+ while (count (property-num_values - CLRMGR_GAMMA_GCMAX_VAL)) {
+ correct_rgb = data[count];
+ property-values[count] = correct_rgb;
+
+ blue = correct_rgb CLRMGR_GAMMA_PARSER_SHIFT_BLUE;
+ green = correct_rgb CLRMGR_GAMMA_PARSER_SHIFT_GREEN;
+ red = correct_rgb CLRMGR_GAMMA_PARSER_SHIFT_RED;
+
+ /* Prepare even and odd regs. Even register contains 6
+ fractional and 2 integer base bits, so lower 8 bits */
+ even = ((blue VLV_GAMMA_EVEN_MASK)
+ VLV_GAMMA_SHIFT_BLUE_REG) |
+ ((green VLV_GAMMA_EVEN_MASK)
+ VLV_GAMMA_SHIFT_GREEN_REG) |
+ ((red VLV_GAMMA_EVEN_MASK)
+ VLV_GAMMA_SHIFT_RED_REG);
+
+ /* Odd register contains upper 8 (integer) bits */
+ odd = ((blue VLV_GAMMA_ODD_SHIFT)
+ VLV_GAMMA_SHIFT_BLUE_REG) |
+ ((green VLV_GAMMA_ODD_SHIFT)
+ VLV_GAMMA_SHIFT_GREEN_REG) |
+ ((red VLV_GAMMA_ODD_SHIFT)
+
[Intel-gfx] [PATCH 07/11] drm/i915: Add hue and saturation correction
From: Shashank Sharma shashank.sha...@intel.com
This patch adds support for color property to set sprite plane
hue and saturation values, for intel color manager framework.
It adds two functions:
1. intel_clrmgr_set_hue_sat: This is a wrapper function
which checks the platform type, and calls the valleyview
specific set_hue_saturation function. As different platforms have different
methods of setting hue/saturation, this function is required.The support
for
other platfroms can be plugged-in here in the wrapper function.
Adding this function as .set_property for hue and saturation color
properties.
2. vlv_set_hs: Core function to program hue/saturation values as per
vlv specs. This function expects one 32bit value as input, encoded in exact
register format, and applies it.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 3 ++
drivers/gpu/drm/i915/intel_clrmgr.c | 57 +
drivers/gpu/drm/i915/intel_clrmgr.h | 17 +++
3 files changed, 77 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 414a113..7614a0f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6103,6 +6103,9 @@ enum punit_power_well {
/* Contrast and brightness */
#define VLV_SPRITE_CB_BASE (dev_priv-info.display_mmio_offset + 0x721d0)
+/* Hue and saturation */
+#define VLV_SPRITE_HS_BASE (dev_priv-info.display_mmio_offset + 0x721d4)
+
/* VLV MIPI registers */
#define _MIPIA_PORT_CTRL (VLV_DISPLAY_BASE + 0x61190)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index 781df59..a4c8f0f 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -88,10 +88,67 @@ struct clrmgr_property gen6_plane_color_corrections[] = {
.min = 0,
.len = VLV_HS_MAX_VALS,
.name = hue-saturation,
+ .set_property = intel_clrmgr_set_hue_sat,
}
};
/*
+* vlv_set_hs
+* Valleyview specific hue/saturation setting function.
+* Valleyview supports hue/saturation correction only on
+* sprite planes.
+* inputs:
+* - intel_crtc *
+* - hs: registered property for hue/sat. This encapsulates a drm_property
+* which is common for all sprite planes. The property has entries equal
+* to no of sprite planes in valleyview = 2
+* -data: 64 bit encoded values, low 32 bits contain the new contrast/
+ brightness values, whereas the upper 32 bits contain the sprite no.
+*/
+bool vlv_set_hs(struct intel_plane *intel_plane, struct clrmgr_regd_prop *hs,
+ uint64_t *data)
+{
+
+ u32 new_val, reg, sprite;
+ struct drm_device *dev = intel_plane-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_property *property = hs-property;
+
+ sprite = intel_plane-plane;
+
+ /* If plane enabled */
+ if (!(SPCNTR(intel_plane-pipe, sprite) SP_ENABLE)) {
+ DRM_ERROR(Sprite plane %d not enabled\n, sprite);
+ return false;
+ }
+
+ /* Apply correction */
+ DRM_DEBUG_DRIVER(Applying hs correction on Sprite\n);
+ reg = SPRITE_HS(intel_plane-pipe, sprite);
+ new_val = *data;
+
+ /* Contrast and brightness are single value properties */
+ I915_WRITE(reg, new_val);
+ property-values[property-num_values - 1] = *data;
+ DRM_DEBUG_DRIVER(Set Hue/Saturation to 0x%x successful\n, new_val);
+ return true;
+}
+
+bool intel_clrmgr_set_hue_sat(void *plane,
+ struct clrmgr_regd_prop *hs, u64 *data)
+{
+ struct intel_plane *intel_plane = plane;
+ struct drm_device *dev = intel_plane-base.dev;
+
+ if (IS_VALLEYVIEW(dev))
+ return vlv_set_hs(intel_plane, hs, data);
+
+ /* Todo: Support other gen devices */
+ DRM_ERROR(Color correction is supported only on VLV for now\n);
+ return false;
+}
+
+/*
* vlv_set_cb
* Valleyview specific common functtion for contsrast/brightness
* setting. The method and registes are same for both.
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h
b/drivers/gpu/drm/i915/intel_clrmgr.h
index d1fc787..6d316d2 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.h
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -80,6 +80,9 @@
/* Sprite Hue and Saturation Registers */
#define VLV_HS_MAX_VALS1
+#define SPRITE_HS(p, s)(VLV_SPRITE_HS_BASE + \
+ ((p * 2 + s) * SPRITE_COLOR_OFFSET))
+
/* Color manager features */
enum clrmgr_tweaks {
@@ -169,6 +172,20 @@ bool intel_clrmgr_set_brightness(void *plane,
struct clrmgr_regd_prop *bright, u64 *data);
/*
+* intel_clrmgr_set_hue-sat
+* Set contrast level.
+* Different gen devices have different methods for
+* contrast
[Intel-gfx] [PATCH 10/11] drm/i915: Plug-in color manager init
From: Shashank Sharma shashank.sha...@intel.com
Call color manager init and attach color properties
during the pipe and plane init time. This will register
all pipe level properties with each intel_crtc and all plane
level properties with each intel_plane objects.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/intel_clrmgr.c | 67 ++--
drivers/gpu/drm/i915/intel_clrmgr.h | 11 +-
drivers/gpu/drm/i915/intel_display.c | 4 +++
drivers/gpu/drm/i915/intel_sprite.c | 3 ++
4 files changed, 51 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index d0d952b..ab38fab 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -532,39 +532,6 @@ error:
return false;
}
-void
-intel_attach_plane_color_correction(struct intel_plane *intel_plane)
-{
- struct clrmgr_reg_request *features;
-
- /* Color manager initialized? */
- if (!intel_plane-color_status) {
- DRM_ERROR(Color manager not initialized for plane %d\n,
- intel_plane-plane);
- return;
- }
-
- DRM_DEBUG_DRIVER(\n);
- features = kzalloc(sizeof(struct clrmgr_reg_request), GFP_KERNEL);
- if (!features) {
- DRM_ERROR(No memory for plane color features\n);
- return;
- }
-
- features-no_of_properties = ARRAY_SIZE(gen6_plane_color_corrections);
- memcpy(features-cp, gen6_plane_color_corrections,
- features-no_of_properties *
- sizeof(struct clrmgr_property));
-
- /* Register plane level color properties */
- if (!intel_clrmgr_register_plane_property(intel_plane, features))
- DRM_ERROR(Register plane color property failed\n);
- else
- DRM_DEBUG_DRIVER(Attached colot corrections for plane %d\n,
- intel_plane-plane);
- kfree(features);
-}
-
bool intel_clrmgr_register_pipe_property(struct intel_crtc *intel_crtc,
struct clrmgr_reg_request *features)
@@ -623,6 +590,40 @@ error:
}
void
+intel_attach_plane_color_correction(struct intel_plane *intel_plane)
+{
+ struct clrmgr_reg_request *features;
+
+ /* Color manager initialized? */
+ if (!intel_plane-color_status) {
+ DRM_ERROR(Color manager not initialized for plane %d\n,
+ intel_plane-plane);
+ return;
+ }
+
+ DRM_DEBUG_DRIVER(\n);
+ features = kzalloc(sizeof(struct clrmgr_reg_request), GFP_KERNEL);
+ if (!features) {
+ DRM_ERROR(No memory for plane color features\n);
+ return;
+ }
+
+ features-no_of_properties = ARRAY_SIZE(gen6_plane_color_corrections);
+ memcpy(features-cp, gen6_plane_color_corrections,
+ features-no_of_properties *
+ sizeof(struct clrmgr_property));
+
+ /* Register plane level color properties */
+ if (!intel_clrmgr_register_plane_property(intel_plane, features))
+ DRM_ERROR(Register plane color property failed\n);
+ else
+ DRM_DEBUG_DRIVER(Attached colot corrections for plane %d\n,
+ intel_plane-plane);
+ kfree(features);
+}
+
+
+void
intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
{
struct clrmgr_reg_request *features;
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h
b/drivers/gpu/drm/i915/intel_clrmgr.h
index c1de823..c78d1db 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.h
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -244,8 +244,17 @@ bool intel_clrmgr_register_pipe_property(struct intel_crtc
*intel_crtc,
struct clrmgr_reg_request *features);
/*
+* intel_attach_plane_color_correction:
+* Register color correction properties as plane properties
+* input:
+* - intel_plane : plane to attach color correcection with
+*/
+void
+intel_attach_plane_color_correction(struct intel_plane *intel_plane);
+
+/*
* intel_attach_pipe_color_correction:
-* Register color correction properties as DRM CRTC properties
+* Register color correction properties as CRTC properties
* input:
* - intel_crtc : CRTC to attach color correcection with
*/
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a6181b5..81d9002 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11835,6 +11835,10 @@ static void intel_crtc_init(struct drm_device *dev,
int pipe)
intel_crtc-plane = !pipe;
}
+ /* Register color properties */
+ intel_crtc-color_status = intel_clrmgr_init(dev);
+ intel_attach_pipe_color_correction(intel_crtc);
+
intel_crtc-cursor_base = ~0;
intel_crtc-cursor_cntl =
[Intel-gfx] [PATCH 09/11] drm/i915: Add set plane property functions
From: Shashank Sharma shashank.sha...@intel.com
Color manager's plane level correction properties are
registered as plane property. So its required to have a
.set_property function in plane functions.
This patch adds:
1. A .set_property function for intel_plane, intel_plane_set_property
which checks if a plane property is of type color property, it
calls color manager's plane level set_property handler function.
2. A intel_clrmgr_set_plane_property, which will extract the data
to be set, and then pass it to appropriate set_property function
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/intel_clrmgr.c | 47 +
drivers/gpu/drm/i915/intel_clrmgr.h | 11 +
drivers/gpu/drm/i915/intel_sprite.c | 41
3 files changed, 99 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index eb18ee2..d0d952b 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -654,6 +654,53 @@ intel_attach_pipe_color_correction(struct intel_crtc
*intel_crtc)
kfree(features);
}
+bool intel_clrmgr_set_plane_property(struct intel_plane *intel_plane,
+ struct clrmgr_regd_prop *cp, uint64_t value)
+{
+ bool ret = false;
+ uint64_t *data;
+ struct drm_property *property;
+
+ /* Sanity */
+ if (!cp-property) {
+ DRM_ERROR(NULL input to set_property\n);
+ return false;
+ }
+
+ property = cp-property;
+ DRM_DEBUG_DRIVER(Property %s len:%d\n,
+ cp-property-name, cp-property-num_values);
+ data = kmalloc(sizeof(uint64_t) * (property-num_values), GFP_KERNEL);
+ if (!data) {
+ DRM_ERROR(Out of memory\n);
+ return false;
+ }
+
+ if (copy_from_user((void *)data, (const void __user *)value,
+ property-num_values * sizeof(uint64_t))) {
+ DRM_ERROR(Failed to copy all data\n);
+ ret = false;
+ goto free_and_return;
+ }
+
+ /* Now do the actual work */
+ if (cp-set_property) {
+ if (!cp-set_property((void *)intel_plane, cp, data)) {
+ DRM_ERROR(Set property for %s failed\n,
+ cp-property-name);
+ ret = false;
+ } else {
+ ret = true;
+ cp-enabled = true;
+ DRM_DEBUG_DRIVER(Set property %s successful\n,
+ cp-property-name);
+ }
+ }
+free_and_return:
+ kfree(data);
+ return ret;
+}
+
bool intel_clrmgr_set_pipe_property(struct intel_crtc *intel_crtc,
struct clrmgr_regd_prop *cp, uint64_t value)
{
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h
b/drivers/gpu/drm/i915/intel_clrmgr.h
index d962585..c1de823 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.h
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -212,6 +212,17 @@ bool intel_clrmgr_set_csc(void *crtc,
struct clrmgr_regd_prop *csc, u64 *data);
/*
+* intel_clrmgr_set_plane_property
+* Set value of a registered plane property
+* input:
+* - intel_plane: the plane with which the property is attached
+* - cp: registered color property
+* - value: value to be set
+*/
+bool intel_clrmgr_set_plane_property(struct intel_plane *intel_plane,
+ struct clrmgr_regd_prop *cp, uint64_t value);
+
+/*
* intel_clrmgr_set_pipe_property
* Set value of a registered CRTC property
* input:
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 168c665..affb429 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -36,6 +36,7 @@
#include intel_drv.h
#include drm/i915_drm.h
#include i915_drv.h
+#include intel_clrmgr.h
static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
{
@@ -1180,6 +1181,45 @@ out_unlock:
return ret;
}
+int intel_plane_set_property(struct drm_plane *plane,
+ struct drm_property *property, uint64_t val)
+{
+ int ret = 0;
+ int count = 0;
+ struct clrmgr_regd_prop *cp;
+ struct intel_plane *intel_plane = to_intel_plane(plane);
+ struct clrmgr_status *status = intel_plane-color_status;
+
+ DRM_DEBUG_DRIVER(\n);
+
+ /* Is this color property ?*/
+ if (!status) {
+ DRM_DEBUG_DRIVER(Color manager not initialized\n);
+ ret = -1;
+ goto skip_color;
+ }
+
+ /* Color manager property */
+ while (count status-no_of_properties) {
+ cp = status-cp[count++];
+ if (property == cp-property) {
+ /* Found it, now set it */
+ if (intel_clrmgr_set_plane_property(intel_plane,
+ cp,
[Intel-gfx] [PATCH 06/11] drm/i915: Add contrast and brightness correction
From: Shashank Sharma shashank.sha...@intel.com
This patch adds support for color property to set sprite plane
contrast and brightness for intel color manager framework.
As, in valleyview the register for contrast and brightess
adjustment is same, one common function has been added to serve
both. It adds three functions:
1. intel_clrmgr_set_contrast: This is a wrapper function
which checks the platform type, and calls the valleyview
specific set_contrast function. As different platforms have different
methods of setting contrast, this function is required.The support for
other platfroms can be plugged-in here in the wrapper function.
Adding this function as .set_property for contrast and brightness
color properties.
2. intel_clrmgr_set_brightness: This is again a wrapper function for
brightness setting.
3. vlv_set_cb: Core function to program brightness/contrast as per
vlv specs. This function takes one 64bit value as input, and extracts
contrast/brightness values, and applies.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 3 ++
drivers/gpu/drm/i915/intel_clrmgr.c | 84 +
drivers/gpu/drm/i915/intel_clrmgr.h | 38 -
3 files changed, 123 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9501ad8..414a113 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6100,6 +6100,9 @@ enum punit_power_well {
#define VLV_PIPEB_GCMAX(dev_priv-info.display_mmio_offset + 0x71010)
#define VLV_PIPE_GCMAX(pipe) _PIPE(pipe, VLV_PIPEA_GCMAX, VLV_PIPEB_GCMAX)
+/* Contrast and brightness */
+#define VLV_SPRITE_CB_BASE (dev_priv-info.display_mmio_offset + 0x721d0)
+
/* VLV MIPI registers */
#define _MIPIA_PORT_CTRL (VLV_DISPLAY_BASE + 0x61190)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index 38ba878..781df59 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -70,6 +70,7 @@ struct clrmgr_property gen6_plane_color_corrections[] = {
.min = 0,
.len = VLV_CB_MAX_VALS,
.name = contrast,
+ .set_property = intel_clrmgr_set_contrast,
},
{
.tweak_id = brightness,
@@ -78,6 +79,7 @@ struct clrmgr_property gen6_plane_color_corrections[] = {
.min = 0,
.len = VLV_CB_MAX_VALS,
.name = brightness,
+ .set_property = intel_clrmgr_set_brightness,
},
{
.tweak_id = hue_saturation,
@@ -90,6 +92,88 @@ struct clrmgr_property gen6_plane_color_corrections[] = {
};
/*
+* vlv_set_cb
+* Valleyview specific common functtion for contsrast/brightness
+* setting. The method and registes are same for both.
+* Valleyview supports contrast/brightness correction only on
+* sprite planes.
+* inputs:
+* - intel_crtc *
+* - cb: registered property for contrast.
+* - data: value to be applied
+*/
+bool vlv_set_cb(struct intel_plane *intel_plane, struct clrmgr_regd_prop *cb,
+ u64 *data, enum clrmgr_tweaks tweak)
+{
+
+ u32 val, new_val, reg, sprite;
+ struct drm_device *dev = intel_plane-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_property *property = cb-property;
+
+ sprite = intel_plane-plane;
+ if (!(SPCNTR(intel_plane-pipe, sprite) SP_ENABLE)) {
+ DRM_ERROR(Sprite plane %d not enabled\n, sprite);
+ return false;
+ }
+
+ /* Apply correction only if sprite is enabled */
+ DRM_DEBUG_DRIVER(Applying cb correction on Sprite\n);
+ reg = SPRITE_CB(intel_plane-pipe, sprite);
+
+ if (tweak == contrast) {
+ /* Contrast value is lower 9 bit value */
+ new_val = *data VLV_CONTRAST_MASK;
+ DRM_DEBUG_DRIVER(Setting Contrast to 0x%x, new_val);
+
+ /* Contrast correction position is bit [26:18] */
+ val = I915_READ(reg)
+ ~(VLV_CONTRAST_MASK VLV_CONTRAST_SHIFT);
+ val |= (new_val VLV_CONTRAST_SHIFT);
+ } else {
+ new_val = *data VLV_BRIGHTNESS_MASK;
+ DRM_DEBUG_DRIVER(Setting Brightness to 0x%x, new_val);
+
+ /* Brightness correction is lower 8 [7:0] register bits */
+ val = I915_READ(reg) | new_val;
+ }
+
+ /* Contrast and brightness are single value properties */
+ I915_WRITE(reg, val);
+ property-values[property-num_values - 1] = *data;
+ DRM_DEBUG_DRIVER(Set Contrast/Brightness correction successful);
+ return true;
+}
+
+bool intel_clrmgr_set_brightness(void *plane,
+ struct clrmgr_regd_prop *bright, u64 *data)
+{
+ struct intel_plane *intel_plane = plane;
[Intel-gfx] [PATCH 11/11] drm/i915: Plug-in color manager exit
From: Shashank Sharma shashank.sha...@intel.com
Call color manager exit from the CRTC destroy/ plane destroy
function to free all the allocated memory, and cleanup all
the registered DRM properties.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 1 +
drivers/gpu/drm/i915/intel_sprite.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 81d9002..6babcc5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9144,6 +9144,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
kfree(work);
}
+ intel_clrmgr_exit(crtc-dev, intel_crtc-color_status);
drm_crtc_cleanup(crtc);
kfree(intel_crtc);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 32aa6e9..44504b1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1119,6 +1119,7 @@ static void intel_destroy_plane(struct drm_plane *plane)
{
struct intel_plane *intel_plane = to_intel_plane(plane);
intel_disable_plane(plane);
+ intel_clrmgr_exit(plane-dev, intel_plane-color_status);
drm_plane_cleanup(plane);
kfree(intel_plane);
}
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 17/44] drm/i915: Prelude to splitting i915_gem_do_execbuffer in two
On Wed, Jul 23, 2014 at 05:33:42PM +0100, John Harrison wrote: On 07/07/2014 20:21, Daniel Vetter wrote: On Wed, Jul 02, 2014 at 11:34:23AM -0700, Jesse Barnes wrote: On Thu, 26 Jun 2014 18:24:08 +0100 john.c.harri...@intel.com wrote: From: John Harrison john.c.harri...@intel.com The scheduler decouples the submission of batch buffers to the driver with their submission to the hardware. This basically means splitting the execbuffer() function in half. This change rearranges some code ready for the split to occur. --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 23 --- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index ec274ef..fda9187 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -32,6 +32,7 @@ #include i915_trace.h #include intel_drv.h #include linux/dma_remapping.h +#include i915_scheduler.h #define __EXEC_OBJECT_HAS_PIN (131) #define __EXEC_OBJECT_HAS_FENCE (130) @@ -874,10 +875,7 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring, if (flush_domains I915_GEM_DOMAIN_GTT) wmb(); - /* Unconditionally invalidate gpu caches and ensure that we do flush - * any residual writes from the previous batch. - */ - return intel_ring_invalidate_all_caches(ring); + return 0; } static bool @@ -1219,8 +1217,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, } } - intel_runtime_pm_get(dev_priv); - ret = i915_mutex_lock_interruptible(dev); if (ret) goto pre_mutex_err; @@ -1331,6 +1327,20 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (ret) goto err; + i915_gem_execbuffer_move_to_active(eb-vmas, ring); + + /* To be split into two functions here... */ + + intel_runtime_pm_get(dev_priv); + + /* Unconditionally invalidate gpu caches and ensure that we do flush + * any residual writes from the previous batch. + */ + ret = intel_ring_invalidate_all_caches(ring); + if (ret) + goto err; + + /* Switch to the correct context for the batch */ ret = i915_switch_context(ring, ctx); if (ret) goto err; @@ -1381,7 +1391,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags); - i915_gem_execbuffer_move_to_active(eb-vmas, ring); i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj); err: I'd like Chris to take a look too, but it looks safe afaict. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org switch_context can fail with EINTR so we really can't move stuff to the active list before that point. Or we need to make sure that all the stuff between the old and new move_to_active callsite can't fail. Or we need to track this and tell userspace with an EIO and adjusted reset stats that something between our point of no return where the kernel committed to executing the batch failed. Or we need to unrol move_to_active (which is currently not really possible). -Daniel switch_context can fail with quite a lot of different error codes. Is there anything particularly special about EINTR? I can't spot that particular code path at the moment. The context switch is done at the point of submission to the hardware. As batch buffers can be re-ordered between submission to driver and submission to hardware, there is no point choosing a context any earlier. Whereas the move to active needs to be done at the point of submission to the driver. The object needs to be marked as in use even though the batch buffer that actually uses it might not be executed for some time. From the software viewpoint, the object is in use and all the syncrhonisation code needs to know that. The scheduler makes the batch buffer execution asynchronous to its submission to the driver. There is no way to communicate back a return code to user land. Instead, it is up to the scheduler to check the return codes from all the execution paths and to retry later if something fails for a temporary reason. Or to discard the buffer if it is truly toast. EINTR is simply really easy to testhit since you can provoke it with signals. And X uses signals excessively. One point where EINTR might happen is in intel_ring_begin, the other when we try to pin the context into ggtt. The other error codes are true exceptions and will much harder to hit. -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 00/11]: Color manager framework for I915 driver
On Wed, Jul 23, 2014 at 11:34:54PM +0530, shashank.sha...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com This patchset adds color-manager, a new framework in I915 driver which adds color correction and tweak capabilities in the driver. Color manager creates a DRM propery based interface for each color correction, and based on the property type, registers it with each CRTC/plane available. The current implementation is for valleyview family. Valleyview supports following color correction properties: 1. CSC correction (wide gamut): This is a pipe level correction. There are total 9 correction coefficients in form of a 3x3 matrix, which are to be programmed on 6 correction registers. CSC correction 2. Gamma correction: This is also pipe level correction There are total 256 palette registers, which can be programmed with 128 correction values, in 10.6 (10bit) format. The expected color Correction can be applied using 129, 64 bit correction values. First 128 correction values are to program palette, 129th value is for GCMAX register value. correction format in a 64 bit value is: | 16 higher bits| 16bit R value|16 bit G value|16 bit B value| 3. Contrast: This is sprite plane level correction Expected correction value is 9 bit value Driver expects values in this format: |bits 64:32 | bits 31:9 | 8:0 contrast correction value| 4. Brightness: This is also a sprite level correction Expected correction value is 8 bit value Driver expects values in this format: |bits 64:32 | bits 31:8 | 7:0 9 bit brightness correction value| 5. Hue and saturation: This is also a sprite level correction Expected correction value is 32 bit value Driver expects values in this format: |bits 64:32| bits 31:0 hs correction value| Patches: 1. First three patches create the basic framework. 2. Next 4 add functions to do color correction per property. 3. Next 2 add interface to set property. 4. last 2 patches plug-in init and exit in modeset sequences. Shashank Sharma (11): drm/i915: Color manager framework for valleyview drm/i915: Register pipe level color properties drm/i915: Register plane level color properties So you're now using drm properties instead of a private ioctl, which is good. But there's still this entire indirection through the color manager and I don't understand it at all and what it serves us. From what I can see it only makes the code more complex. The basic drm approach for properties is - Create the property once per device. If we standardize it, then it should be created in drm_device-mode_config, otherwise at a suitable place in our driver private structure. - Link up that property with the right kms objects in the relevant init functions. That needs a pointer on each object to store that association. - Match for these properties in the set_prop callback. Please explain why is not sufficient with this basicstraightforward approach and what the color manager adds in value to this. Thus far I think we can just rip out the color manager without loss of functionality. drm/i915: Add color manager CSC correction drm/i915: Add color manager gamma correction We already have gamma support in core drm, so we shouldn't duplicate things. There's a few things we need to do though: - Convert gamma to be a property from the special-purpose ioctl it is currently. - Add an enum property with all the gamma table formats support. Default would be the 256 entry 8 bit table. - Add new table layout for vlv. - Implement the new layout and switch between high-res and legacy gamma table appropriately. Note that the gamma table should be flexible enough to also support the high-res gamma tables on big cores. Or any other gpu fwiw. drm/i915: Add contrast and brightness correction drm/i915: Add hue and saturation correction These 4 properties should imo all be drm core properties registered in drm_device-mode_config, with a setup function for all four. Also the commit message still talk about the old design where you split up a 64 bit value. That's highly confusing. And all the indirection needs to be ripped out (see my above comment about the color manager). Finally we already have a few sprite properties exposed as ioctls on vlv. I'd like those to be converted to real properties as part of this work if possible. For a suitable merge stragety I think it would be good to split up this work into different parts: - Convert existing ioctl properties. - Gamma table improvements. - Color adjustements (brightness, contrast, hue, saturation). - CSC. Cheers, Daniel drm/i915: Add CRTC set property functions drm/i915: Add set plane property functions drm/i915: Plug-in color manager init drm/i915: Plug-in color manager exit drivers/gpu/drm/i915/Makefile| 3 +- drivers/gpu/drm/i915/i915_reg.h | 22 + drivers/gpu/drm/i915/intel_clrmgr.c | 795 +++
Re: [Intel-gfx] [RFC 15/44] drm/i915: Added deferred work handler for scheduler
On Wed, Jul 23, 2014 at 5:37 PM, John Harrison
john.c.harri...@intel.com wrote:
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 0977653..fbafa68 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1075,6 +1075,16 @@ struct i915_gem_mm {
struct delayed_work idle_work;
/**
+* New scheme is to get an interrupt after every work packet
+* in order to allow the low latency scheduling of pending
+* packets. The idea behind adding new packets to a pending
+* queue rather than directly into the hardware ring buffer
+* is to allow high priority packets to over take low priority
+* ones.
+*/
+ struct work_struct scheduler_work;
Latency for work items isn't too awesome, and e.g. Oscar's execlist code
latches the next context right away from the irq handler. Why can't we do
something similar for the scheduler? Fishing the next item out of a
priority queue shouldn't be expensive ...
-Daniel
The problem is that taking batch buffers from the scheduler's queue and
submitting them to the hardware requires lots of processing that is not IRQ
compatible. It isn't just a simple register write. Half of the code in
'i915_gem_do_execbuffer()' must be executed. Probably/possibly it could be
made IRQ friendly but that would place a lot of restrictions on a lot of
code that currently doesn't expect to be restricted. Instead, the submission
is done via a work handler that acquires the driver mutex lock.
In order to cover the extra latency, the scheduler operates in a
multi-buffered mode and aims to keep eight batch buffers in flight at all
times. That number being obtained empirically by running lots of benchmarks
on Android with lots of different settings and seeing where the buffer size
stopped making a difference.
So I've tried to stitch together that part of the scheduler from the
patch series. Afaics you do the actual scheduling under the protection
of irqsave spinlocks (well you also hold the dev-struct_mutex). That
means you disable local interrupts. Up to the actual submit point I
spotted two such critcial sections encompassing pretty much all the
code.
If we'd run the same code from the interrupt handler then only our own
interrupt handler is blocked, all other interrupt processing can
continue. So that's actually a lot nicer than what you have. In any
case you can't do expensive operations under an irqsave spinlock
anyway.
So either I've missed something big here, or this justification doesn't hold up.
-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] tests/pm_rpm: Convert to new optparsing
Thomas missed the add-hoc parameter parsing in here, which did break
things for Paulo since the core now fails for unknown options.
Convert them over to the new infrastructure, which has the nice upside
that we can remove the comments and put them into the usage available
with --help.
Cc: Thomas Wood thomas.w...@intel.com
Cc: Paulo Zanoni paulo.r.zan...@intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
tests/pm_rpm.c | 42 --
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 1e63bc8f99e4..869e6f3d3bdb 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1407,24 +1407,38 @@ static void dpms_mode_unset_subtest(enum screen_type
type)
igt_assert(wait_for_suspended());
}
-int main(int argc, char *argv[])
+int rounds = 50;
+bool stay = false;
+
+static int opt_handler(int opt, int opt_index)
{
- int rounds = 50;
- bool stay = false;
+ switch (opt) {
+ case 'q':
+ rounds = 10;
+ break;
+ case 's':
+ stay = true;
+ break;
+ default:
+ igt_assert(0);
+ }
- igt_subtest_init(argc, argv);
+ return 0;
+}
- /* The --quick option makes the stress tests not so stressful. Useful
-* when you're developing and just want to make a quick test to make
-* sure you didn't break everything. */
- if (argc 1 strcmp(argv[1], --quick) == 0)
- rounds = 10;
+int main(int argc, char *argv[])
+{
+ const char *help_str =
+--quick\t\tMake the stress-tests not stressful, for quick
regression testing.\n
+--stay\t\tDisable all screen and try to go into runtime pm.
Useful for debugging.;
+ static struct option long_options[] = {
+ {quick, 0, 0, 'q'},
+ {stay, 0, 0, 's'},
+ { 0, 0, 0, 0 }
+ };
- /* The --stay option enables a mode where we disable all the screens,
-* then stay like that, runtime suspended. This mode is useful for
-* running manual tests while debugging. */
- if (argc 1 strcmp(argv[1], --stay) == 0)
- stay = true;
+ igt_subtest_init_parse_opts(argc, argv, , long_options,
+ help_str, opt_handler);
/* Skip instead of failing in case the machine is not prepared to reach
* PC8+. We don't want bug reports from cases where the machine is just
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] tests/pm_rpm: Convert to new optparsing
2014-07-23 16:16 GMT-03:00 Daniel Vetter daniel.vet...@ffwll.ch:
Thomas missed the add-hoc parameter parsing in here, which did break
things for Paulo since the core now fails for unknown options.
Convert them over to the new infrastructure, which has the nice upside
that we can remove the comments and put them into the usage available
with --help.
Cc: Thomas Wood thomas.w...@intel.com
Cc: Paulo Zanoni paulo.r.zan...@intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
Thanks for the quick fix!
Tested-by: Paulo Zanoni paulo.r.zan...@intel.com
---
tests/pm_rpm.c | 42 --
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 1e63bc8f99e4..869e6f3d3bdb 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1407,24 +1407,38 @@ static void dpms_mode_unset_subtest(enum screen_type
type)
igt_assert(wait_for_suspended());
}
-int main(int argc, char *argv[])
+int rounds = 50;
+bool stay = false;
+
+static int opt_handler(int opt, int opt_index)
{
- int rounds = 50;
- bool stay = false;
+ switch (opt) {
+ case 'q':
+ rounds = 10;
+ break;
+ case 's':
+ stay = true;
+ break;
+ default:
+ igt_assert(0);
+ }
- igt_subtest_init(argc, argv);
+ return 0;
+}
- /* The --quick option makes the stress tests not so stressful. Useful
-* when you're developing and just want to make a quick test to make
-* sure you didn't break everything. */
- if (argc 1 strcmp(argv[1], --quick) == 0)
- rounds = 10;
+int main(int argc, char *argv[])
+{
+ const char *help_str =
+--quick\t\tMake the stress-tests not stressful, for quick
regression testing.\n
+--stay\t\tDisable all screen and try to go into runtime pm.
Useful for debugging.;
+ static struct option long_options[] = {
+ {quick, 0, 0, 'q'},
+ {stay, 0, 0, 's'},
+ { 0, 0, 0, 0 }
+ };
- /* The --stay option enables a mode where we disable all the screens,
-* then stay like that, runtime suspended. This mode is useful for
-* running manual tests while debugging. */
- if (argc 1 strcmp(argv[1], --stay) == 0)
- stay = true;
+ igt_subtest_init_parse_opts(argc, argv, , long_options,
+ help_str, opt_handler);
/* Skip instead of failing in case the machine is not prepared to
reach
* PC8+. We don't want bug reports from cases where the machine is
just
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] NEWS: Updates
--- NEWS | 6 ++ 1 file changed, 6 insertions(+) diff --git a/NEWS b/NEWS index 1b5ee83ec849..4866d59b5619 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,12 @@ Release 1.8 (-xx-xx) - As usual piles of new tests. +- Improved plane/pipe handling in the igt_kms library (Damien). + +- Unified option parsing between simple tests and tests with subtests (Thomas). + This will allow us to merge the different Makefile targets once test runners + are converted. + Release 1.7 (2014-06-09) -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On Sat, Jul 19, 2014 at 12:27:21AM +, Kay, Allen M wrote: For the MCH PCI registers that do need to be read - can you tell us which ones those are? In qemu/hw/xen_pt_igd.c/igd_pci_read(), following MCH PCI config register reads are passthrough to the host HW. Some of the registers are needed by Ironlake GFX driver which we probably can remove. I did a trace recently on Broadwell, the number of register accessed are even smaller (0, 2, 2c, 2e, 50, 52, a0, a4). Given that we now have integrated MCH and GPU in the same socket, looks like driver can easily remove reads for offsets 0 - 0x2e. case 0x00:/* vendor id */ case 0x02:/* device id */ case 0x08:/* revision id */ case 0x2c:/* sybsystem vendor id */ case 0x2e:/* sybsystem id */ Right. We can fix the i915 to use the mechanism that Michael mentioned. (see attached RFC patches) case 0x50:/* SNB: processor graphics control register */ case 0x52:/* processor graphics control register */ case 0xa0:/* top of memory */ case 0xb0:/* ILK: BSM: should read from dev 2 offset 0x5c */ case 0x58:/* SNB: PAVPC Offset */ case 0xa4:/* SNB: graphics base of stolen memory */ case 0xa8:/* SNB: base of GTT stolen memory */ I dug in the intel-gtt.c (see ironlake_gtt_driver) to figure out this a bit more. As in, I speculated, what if we returned 0 (and not implement any support for reading from these registers). What would happen? 0x52 for Ironlake (g5): -- It looks like intel_gmch_probe is called when i915_gem_gtt_init starts (there is a lot of code that looks to be used between intel-gtt.c and i915.c). Anyhow the interesting parts are that i9xx_setup ends up calling ioremap the i915 BAR to setup some of these registers for older generations. Then i965_gtt_total_entries gets which reads 0x52, but it is only needed for v5 generation. For other (v4 and G33) it reads it from the GPU's 0x2020 register. If there is a mismatch, it writes to the GPU at 0x2020 to update the the size based on the bridge. And then it reads from 0x2020 and that is returned and stuck in intel_private.gtt_total_entries. So 0x52 in the emulated bridge could be populated with what the GPU has at 0x2020. And the writes go in the emulated area. 0x52 for v6 - v8: - We seem to go to gen6_gmch_probe which just figures out the the GTT based on the GPU's BAR sizes. The stolen values are read from 0x50 from the GPU. So no need to touch the bridge (see gen6_gmch_probe) OK, so no need to have 0x52 or 0x50 in the bridge. 0xA0: - Could not find any reference in the Linux code. Why would Windows driver need this? If we returned the _real_ TOM would it matter? Is it used to figure out the device should use 32-bit DMA operations or 40-bit? 0xb0 or 0x5c: - No mention of them in the Linux code. 0x58, 0xa4, 0xa8: - No usage of them in the Linux code. We seem to be using the 0x52 from the bridge and 0x2020 from the GPU for this functionality. So in theory*, if using Ironlake we need to have a proper value in 0x52 in the bridge. But for the later chipsets we do not need these values (I am assuming that intel_setup_mchbar can safely return as it does that for Ironlake and could very well for later generations). Can this be reflected in the Windows driver as well? P.S. *theory: That is assuming we modify the Linux i915_drv.c:intel_detect_pch to pick up the id as suggested earlier. See the RFC patches attached. (Not compile tested at all!) Allen -Original Message- From: Konrad Rzeszutek Wilk [mailto:konrad.w...@oracle.com] Sent: Friday, July 18, 2014 6:45 AM To: Kay, Allen M Cc: Michael S. Tsirkin; Jesse Barnes; peter.mayd...@linaro.org; xen-de...@lists.xensource.com; Ross Philipson; airl...@linux.ie; daniel.vet...@ffwll.ch; intel-gfx@lists.freedesktop.org; kelly.zyta...@amd.com; qemu-de...@nongnu.org; Anthony Perard; Stefano Stabellini; anth...@codemonkey.ws; Paolo Bonzini; Zhang, Yang Z; Chen, Tiejun Subject: Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support On Thu, Jul 17, 2014 at 05:37:12PM +, Kay, Allen M wrote: That sounds great. Tiejun could you confirm that with windows driver guys too? I believe windows driver can also assume specific CPU/PCH combos. I will discuss this with native Windows driver guys. Preferably, the same code path can be used for both native and virtualization cases to avoid frequent breakage as most developers and QA do not test new code changes in virtualization environment. I have verified that Windows driver do not need to write to any MCH PCI registers on HSW/BDW so the PCI write function can be
[Intel-gfx] [PATCH] drm/i915: fix cursor handling when runtime suspended
From: Paulo Zanoni paulo.r.zan...@intel.com
If we're runtime suspended and try to use the cursor interfaces, we
will get a lot of WARNs saying we did the wrong thing.
For intel_crtc_update_cursor(), all we need to do is return if the
CRTC is not active, since writing the registers won't really have any
effect if the screen is not visible, and we will write the registers
later when enabling the screen.
For intel_crtc_cursor_set_obj(), we just get the proper power domain
reference, since this function does a lot of stuff.
Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Cc: sta...@vger.kernel.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index d1e9570..c8f36b0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8151,6 +8151,9 @@ static void intel_crtc_update_cursor(struct drm_crtc
*crtc,
if (base == 0 intel_crtc-cursor_base == 0)
return;
+ if (!intel_crtc-active)
+ return;
+
I915_WRITE(CURPOS(pipe), pos);
if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
@@ -8181,6 +8184,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc
*crtc,
uint32_t addr;
int ret;
+ intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE(pipe));
+
/* if we want to turn off the cursor ignore width and height */
if (!obj) {
DRM_DEBUG_KMS(cursor off\n);
@@ -8195,13 +8200,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc
*crtc,
(width == 128 height == 128 !IS_GEN2(dev)) ||
(width == 256 height == 256 !IS_GEN2(dev {
DRM_DEBUG(Cursor dimension not supported\n);
- return -EINVAL;
+ ret = -EINVAL;
+ goto fail;
}
if (obj-base.size width * height * 4) {
DRM_DEBUG_KMS(buffer is too small\n);
ret = -ENOMEM;
- goto fail;
+ goto fail_unref;
}
/* we only need to pin inside GTT if cursor is non-phy */
@@ -8275,13 +8281,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc
*crtc,
intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+ intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE(pipe));
return 0;
fail_unpin:
i915_gem_object_unpin_from_display_plane(obj);
fail_locked:
mutex_unlock(dev-struct_mutex);
-fail:
+fail_unref:
drm_gem_object_unreference_unlocked(obj-base);
+fail:
+ intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE(pipe));
return ret;
}
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: fix cursor handling when runtime suspended
On Wed, Jul 23, 2014 at 06:30:59PM -0300, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zan...@intel.com
If we're runtime suspended and try to use the cursor interfaces, we
will get a lot of WARNs saying we did the wrong thing.
For intel_crtc_update_cursor(), all we need to do is return if the
CRTC is not active, since writing the registers won't really have any
effect if the screen is not visible, and we will write the registers
later when enabling the screen.
For intel_crtc_cursor_set_obj(), we just get the proper power domain
reference, since this function does a lot of stuff.
Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Cc: sta...@vger.kernel.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index d1e9570..c8f36b0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8151,6 +8151,9 @@ static void intel_crtc_update_cursor(struct drm_crtc
*crtc,
if (base == 0 intel_crtc-cursor_base == 0)
return;
+ if (!intel_crtc-active)
+ return;
Don't we need the same trick in intel_crtc_cursor_set_obj? This gets
called if the cursor object changes (instead of just moving it around).
+
I915_WRITE(CURPOS(pipe), pos);
if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
@@ -8181,6 +8184,8 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc
*crtc,
uint32_t addr;
int ret;
+ intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE(pipe));
This shouldn't be needed - if intel_crtc-active is set, we do hold a
reference to the pipe power domain already. There's no way the pipe would
be able to run otherwise ;-)
-Daniel
+
/* if we want to turn off the cursor ignore width and height */
if (!obj) {
DRM_DEBUG_KMS(cursor off\n);
@@ -8195,13 +8200,14 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc
*crtc,
(width == 128 height == 128 !IS_GEN2(dev)) ||
(width == 256 height == 256 !IS_GEN2(dev {
DRM_DEBUG(Cursor dimension not supported\n);
- return -EINVAL;
+ ret = -EINVAL;
+ goto fail;
}
if (obj-base.size width * height * 4) {
DRM_DEBUG_KMS(buffer is too small\n);
ret = -ENOMEM;
- goto fail;
+ goto fail_unref;
}
/* we only need to pin inside GTT if cursor is non-phy */
@@ -8275,13 +8281,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc
*crtc,
intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
+ intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE(pipe));
return 0;
fail_unpin:
i915_gem_object_unpin_from_display_plane(obj);
fail_locked:
mutex_unlock(dev-struct_mutex);
-fail:
+fail_unref:
drm_gem_object_unreference_unlocked(obj-base);
+fail:
+ intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE(pipe));
return ret;
}
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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: fix cursor handling when runtime suspended
2014-07-23 19:41 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Thu, Jul 24, 2014 at 12:35:25AM +0200, Daniel Vetter wrote: On Wed, Jul 23, 2014 at 06:30:59PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com If we're runtime suspended and try to use the cursor interfaces, we will get a lot of WARNs saying we did the wrong thing. For intel_crtc_update_cursor(), all we need to do is return if the CRTC is not active, since writing the registers won't really have any effect if the screen is not visible, and we will write the registers later when enabling the screen. For intel_crtc_cursor_set_obj(), we just get the proper power domain reference, since this function does a lot of stuff. Testcase: igt/pm_rpm/cursor Testcase: igt/pm_rpm/cursor-dpms Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645 Cc: sta...@vger.kernel.org Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/intel_display.c | 15 --- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d1e9570..c8f36b0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8151,6 +8151,9 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc, if (base == 0 intel_crtc-cursor_base == 0) return; + if (!intel_crtc-active) + return; Don't we need the same trick in intel_crtc_cursor_set_obj? This gets called if the cursor object changes (instead of just moving it around). Rechecked and realized the only I915_WRITE in there is for gen2. I guess we don't care ;-) Nope. You need to look at the subfunctions and their subsubfunctions and their subsubsubfunctions and so on. This is what happens when I remove just the display_power_get/put calls: [ 35.762635] [drm:intel_crtc_cursor_set_obj] cursor off [ 35.762665] [drm:add_framebuffer_internal] [FB:63] [ 35.762685] [ cut here ] [ 35.762714] WARNING: CPU: 0 PID: 3169 at drivers/gpu/drm/i915/i915_gem_gtt.c:1480 gen6_ggtt_insert_entries+0x116/0x120 [i915]() [ 35.762716] Modules linked in: fuse intel_rapl x86_pkg_temp_thermal intel_powerclamp serio_raw i915 i2c_algo_bit drm_kms_helper drm i2c_i801 mei_me mei i2c_designware_platform i2c_designware_core sg sd_mod ehci_pci ehci_hcd ahci libahci e1000e xhci_hcd sdhci_acpi sdhci [ 35.762748] CPU: 0 PID: 3169 Comm: pm_rpm Not tainted 3.16.0-rc6.1407232028+ #695 [ 35.762751] Hardware name: Intel Corporation Shark Bay Client platform/WhiteTip Mountain 1, BIOS HSWLPTU1.86C.0137.R00.1403031632 03/03/2014 [ 35.762754] 0009 88009ce139c8 816b6a61 [ 35.762760] 88009ce13a00 81075d88 92946000 [ 35.762765] c90010d04d74 88009d938bb8 c90010d04d68 88009ce13a10 [ 35.762770] Call Trace: [ 35.762782] [816b6a61] dump_stack+0x4d/0x66 [ 35.762789] [81075d88] warn_slowpath_common+0x78/0xa0 [ 35.762793] [81075e65] warn_slowpath_null+0x15/0x20 [ 35.762814] [a0169716] gen6_ggtt_insert_entries+0x116/0x120 [i915] [ 35.762831] [a0168ce9] ggtt_bind_vma+0xd9/0x100 [i915] [ 35.762850] [a0172583] i915_gem_object_pin+0x683/0x750 [i915] [ 35.762869] [a0173cd7] i915_gem_object_pin_to_display_plane+0x97/0x1d0 [i915] [ 35.762894] [a01a891c] intel_crtc_cursor_set_obj+0x16c/0x520 [i915] [ 35.762916] [a01a8df5] intel_cursor_plane_update+0xe5/0x120 [i915] [ 35.762937] [a00d83a4] setplane_internal+0x264/0x2b0 [drm] [ 35.762952] [a00d850e] drm_mode_cursor_common+0x11e/0x320 [drm] [ 35.762968] [a00dbb0c] drm_mode_cursor_ioctl+0x3c/0x40 [drm] [ 35.762978] [a00cb87f] drm_ioctl+0x1df/0x6a0 [drm] [ 35.762983] [816be9b9] ? mutex_unlock+0x9/0x10 [ 35.762988] [811eaae6] ? seq_read+0xb6/0x3e0 [ 35.762994] [811d97e0] do_vfs_ioctl+0x2e0/0x4e0 [ 35.762998] [816c0bf7] ? sysret_check+0x1b/0x56 [ 35.763004] [810c47fd] ? trace_hardirqs_on_caller+0x15d/0x200 [ 35.763008] [811d9a61] SyS_ioctl+0x81/0xa0 [ 35.763013] [816c0bd2] system_call_fastpath+0x16/0x1b [ 35.763015] ---[ end trace 189706dc7c79e8d7 ]--- [ 35.763018] [ cut here ] [ 35.763039] WARNING: CPU: 0 PID: 3169 at drivers/gpu/drm/i915/intel_uncore.c:47 assert_device_not_suspended.isra.8+0x43/0x50 [i915]() [ 35.763041] Device suspended [ 35.763043] Modules linked in: fuse intel_rapl x86_pkg_temp_thermal intel_powerclamp serio_raw i915 i2c_algo_bit drm_kms_helper drm i2c_i801 mei_me mei i2c_designware_platform i2c_designware_core sg sd_mod ehci_pci ehci_hcd ahci libahci e1000e xhci_hcd sdhci_acpi sdhci [ 35.763070] CPU: 0 PID: 3169 Comm: pm_rpm Tainted: GW
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On 2014/7/24 4:54, Konrad Rzeszutek Wilk wrote: On Sat, Jul 19, 2014 at 12:27:21AM +, Kay, Allen M wrote: For the MCH PCI registers that do need to be read - can you tell us which ones those are? In qemu/hw/xen_pt_igd.c/igd_pci_read(), following MCH PCI config register reads are passthrough to the host HW. Some of the registers are needed by Ironlake GFX driver which we probably can remove. I did a trace recently on Broadwell, the number of register accessed are even smaller (0, 2, 2c, 2e, 50, 52, a0, a4). Given that we now have integrated MCH and GPU in the same socket, looks like driver can easily remove reads for offsets 0 - 0x2e. case 0x00:/* vendor id */ case 0x02:/* device id */ case 0x08:/* revision id */ case 0x2c:/* sybsystem vendor id */ case 0x2e:/* sybsystem id */ Right. We can fix the i915 to use the mechanism that Michael mentioned. (see attached RFC patches) case 0x50:/* SNB: processor graphics control register */ case 0x52:/* processor graphics control register */ case 0xa0:/* top of memory */ case 0xb0:/* ILK: BSM: should read from dev 2 offset 0x5c */ case 0x58:/* SNB: PAVPC Offset */ case 0xa4:/* SNB: graphics base of stolen memory */ case 0xa8:/* SNB: base of GTT stolen memory */ I dug in the intel-gtt.c (see ironlake_gtt_driver) to figure out this a bit more. As in, I speculated, what if we returned 0 (and not implement any support for reading from these registers). What would happen? 0x52 for Ironlake (g5): -- It looks like intel_gmch_probe is called when i915_gem_gtt_init starts (there is a lot of code that looks to be used between intel-gtt.c and i915.c). Anyhow the interesting parts are that i9xx_setup ends up calling ioremap the i915 BAR to setup some of these registers for older generations. Then i965_gtt_total_entries gets which reads 0x52, but it is only needed for v5 generation. For other (v4 and G33) it reads it from the GPU's 0x2020 register. If there is a mismatch, it writes to the GPU at 0x2020 to update the the size based on the bridge. And then it reads from 0x2020 and that is returned and stuck in intel_private.gtt_total_entries. So 0x52 in the emulated bridge could be populated with what the GPU has at 0x2020. And the writes go in the emulated area. 0x52 for v6 - v8: - We seem to go to gen6_gmch_probe which just figures out the the GTT based on the GPU's BAR sizes. The stolen values are read from 0x50 from the GPU. So no need to touch the bridge (see gen6_gmch_probe) OK, so no need to have 0x52 or 0x50 in the bridge. 0xA0: - Could not find any reference in the Linux code. Why would Windows driver need this? If we returned the _real_ TOM would it matter? Is it used to figure out the device should use 32-bit DMA operations or 40-bit? 0xb0 or 0x5c: - No mention of them in the Linux code. 0x58, 0xa4, 0xa8: - No usage of them in the Linux code. We seem to be using the 0x52 from the bridge and 0x2020 from the GPU for this functionality. So in theory*, if using Ironlake we need to have a proper value in 0x52 in the bridge. But for the later chipsets we do not need these values (I am assuming that intel_setup_mchbar can safely return as it does that for Ironlake and could very well for later generations). Can this be reflected in the Windows driver as well? P.S. *theory: That is assuming we modify the Linux i915_drv.c:intel_detect_pch to pick up the id as suggested earlier. See the RFC patches attached. (Not compile tested at all!) I take a look these patches, looks we still scan all PCI Bridge to walk all PCHs. So this means we still need to fake a PCI bridge, right? Or maybe you don't cover this problem this time. I prefer we should check dev slot to get that PCH like my previous patch, gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type. Because Windows always use this way, so I think this point should be same between Linux and Windows. Or we need anther better way to unify all OSs. Thanks Tiejun Allen -Original Message- From: Konrad Rzeszutek Wilk [mailto:konrad.w...@oracle.com] Sent: Friday, July 18, 2014 6:45 AM To: Kay, Allen M Cc: Michael S. Tsirkin; Jesse Barnes; peter.mayd...@linaro.org; xen-de...@lists.xensource.com; Ross Philipson; airl...@linux.ie; daniel.vet...@ffwll.ch; intel-gfx@lists.freedesktop.org; kelly.zyta...@amd.com; qemu-de...@nongnu.org; Anthony Perard; Stefano Stabellini; anth...@codemonkey.ws; Paolo Bonzini; Zhang, Yang Z; Chen, Tiejun Subject: Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support On Thu, Jul 17, 2014 at 05:37:12PM +, Kay, Allen M wrote:
Re: [Intel-gfx] [PATCH 00/11]: Color manager framework for I915 driver
Hi Daniel, Thanks for your time and comments. The current design is exactly same as we discussed previously in the mail chain, color manager is just the framework which does this job: 1. Create a DRM property for requesting object. 2. Attach the DRM property with the object. There is no other job done here in the framework, no parsing and nothing else. The color manager data structures also just add and array of DRM properties for an object (CRTC/PIPE) and total no of DRM properties. So there is nothing which is not required. Typical sequence of how it works: 1. intel CRTC init calls color-manager init for that CRTC, gets a color pointer, which has space to save DRM properties. 2. intel CRTC init calls attach color properties, which will register the DRM property, add into the color pointer, and return. 3. A CRTC set property checks if this is color property, calls color-manager-set-property. 4. Color manager set-property calls core set property function, which takes care of calling platform specific set_propety function. 5. Color manager exit gets call from CRTC/plane destroy, and frees the DRM property per CRTC/plane, plus color pointer. Can you please point out, which of the above steps is not falling in line for you? The current gamma implementation is in 8bit gamma format, which can be only used to initialize the lut, it doesn't do the gamma correction. The actual correction is done in 10bit gamma correction mode, which is more accurate and can support fraction level gamma correction. Please find the detail explanation per comment, inline. Regards Shashank -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Thursday, July 24, 2014 12:04 AM To: Sharma, Shashank Cc: intel-gfx@lists.freedesktop.org; ville.syrj...@linux.intel.com; Lespiau, Damien; Vetter, Daniel; Kumar, Shobhit; M, Satheeshakrishna; =indranil.mukher...@intel.com Subject: Re: [Intel-gfx] [PATCH 00/11]: Color manager framework for I915 driver On Wed, Jul 23, 2014 at 11:34:54PM +0530, shashank.sha...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com This patchset adds color-manager, a new framework in I915 driver which adds color correction and tweak capabilities in the driver. Color manager creates a DRM propery based interface for each color correction, and based on the property type, registers it with each CRTC/plane available. The current implementation is for valleyview family. Valleyview supports following color correction properties: 1. CSC correction (wide gamut): This is a pipe level correction. There are total 9 correction coefficients in form of a 3x3 matrix, which are to be programmed on 6 correction registers. CSC correction 2. Gamma correction: This is also pipe level correction There are total 256 palette registers, which can be programmed with 128 correction values, in 10.6 (10bit) format. The expected color Correction can be applied using 129, 64 bit correction values. First 128 correction values are to program palette, 129th value is for GCMAX register value. correction format in a 64 bit value is: | 16 higher bits| 16bit R value|16 bit G value|16 bit B value| 3. Contrast: This is sprite plane level correction Expected correction value is 9 bit value Driver expects values in this format: |bits 64:32 | bits 31:9 | 8:0 contrast correction value| 4. Brightness: This is also a sprite level correction Expected correction value is 8 bit value Driver expects values in this format: |bits 64:32 | bits 31:8 | 7:0 9 bit brightness correction value| 5. Hue and saturation: This is also a sprite level correction Expected correction value is 32 bit value Driver expects values in this format: |bits 64:32| bits 31:0 hs correction value| Patches: 1. First three patches create the basic framework. 2. Next 4 add functions to do color correction per property. 3. Next 2 add interface to set property. 4. last 2 patches plug-in init and exit in modeset sequences. Shashank Sharma (11): drm/i915: Color manager framework for valleyview drm/i915: Register pipe level color properties drm/i915: Register plane level color properties So you're now using drm properties instead of a private ioctl, which is good. But there's still this entire indirection through the color manager and I don't understand it at all and what it serves us. From what I can see it only makes the code more complex. The basic drm approach for properties is - Create the property once per device. If we standardize it, then it should be created in drm_device-mode_config, otherwise at a suitable place in our driver private structure. - Link up that property with the right kms objects in the relevant init functions. That needs a pointer on each object to store that association. - Match for these properties in the set_prop callback. Please explain
