Re: [Intel-gfx] [PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register
On Sat, Mar 22, 2014 at 04:25:31AM +, Gupta, Sourab wrote: On Fri, 2014-03-21 at 16:52 +, Chris Wilson wrote: On Fri, Mar 21, 2014 at 08:58:08PM +0530, sourab.gu...@intel.com wrote: From: Akash Goel akash.g...@intel.com This patch Enables the bit for TLB invalidate in GFX Mode register. According to bspec, When enabled this bit limits the invalidation of the TLB only to batch buffer boundaries, to pipe_control commands which have the TLB invalidation bit set and sync flushes. If disabled, the TLB caches are flushed for every full flush of the pipeline. v2: Explicitly enabling TLB invalidate bit instead of assuming default 1 (Chris Wilson) Right, but there is nothing special about this code for vlv, all of gen7 share the same TLB invalidation code, and there is no documented reason not to do the switch. So do a cursory test on ivb/hsw and send a patch that doesn't say FIXME. -Chris Hi Chris, I agree, this could be applicable to ivb also. But we are constrained by the availability of ivb/hsw machines and we're not able to test it on those. We didn't want to cause any regression on those platforms. Thats why we were limiting our patch to vlv only with a FIXME comment. We've done this for other WAs also. If required we can put this patch for full GEN7 (not tested on ivb/hsw) and keep our fingers crossed! :) How about for the generalised patch, Tested-by: Chris Wilson ch...@chris-wilson.co.uk # ivb, hsw Happy now? :) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Rename GFX_TLB_INVALIDATE_ALWAYS
On Sat, Mar 22, 2014 at 03:29:11AM +, Gupta, Sourab wrote: On Fri, 2014-03-21 at 17:18 +, Chris Wilson wrote: The documentation calls this GFX_MODE bit Flush TLB invalidate Mode. However, that is not a good name for an enable bit as it doesn't make it clear what is enabled. An even worse name is GFX_TLB_INVALIDATE_ALWAYS as enabling that bit actually prevents the TLB from being invalidated at every flush. This leads to great confusion when reading code and proposed patches. To get around this try to bake in what is enabled by setting the bit and call it GFX_TLB_INVALIDATE_EXPLICIT. This looks fine. It clears the confusion around TLB INVALIDATE Bit. We'll be developing our patch on top of this and sending the full series of WA patches for VLV. btw if you think the patch is useful and you've reviewed it according to the Reviewer's Statement of Oversight [1] then please always supply your reviewed-by tag. Otherwise it's just the informal acked-by. Patch merged into dinq. Thanks, Daniel [1] https://www.kernel.org/doc/Documentation/SubmittingPatches -- 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/12] Broadwell 3.14 backports
On Fri, Mar 21, 2014 at 05:17:34PM -0700, Ben Widawsky wrote: On Fri, Mar 21, 2014 at 05:06:06PM -0700, Ben Widawsky wrote: On Fri, Mar 21, 2014 at 04:47:05PM -0700, Greg KH wrote: I have no idea what is going on here, what this original email was from / about, or what I am supposed to do here... The stable patch process is pretty well defined, and documented, is that lacking somehow, and if so, in what? greg k-h My apologies, I didn't understand what Daniel had originally wanted from me, and I think the plan changed a bit in flight. I'm sorry you got dragged into it. The stable process documentation is perfectly adequate. And if it wasn't clear, like Daniel said, please ignore these 12 patches for now. Sorry again. For clarification: BDW support was enabled for the first time in 3.14, but in the -rc phase suddenly lots of workaround patches and little fixes start to pile in. Since pretty much no one has the hardware already I decided to withold all bdw fixes and queued them for -next. Once it all stabilized we could then reevaluate whether bdw support in 3.14 makes sense or not, i.e. whether to backport a pile of fixes or just disable it again. bdw seems to have calmed down now and it doesn't look too bad (it's a bit more than these 12 patches here, but all fairly isolated), so I've asked Ben to assemble the required patches, backport and test them and then submit it all to stable (once drm-next has landed, ofc). Ben was a bit overeager and submitted them to stable a bit too early ;-) My apologies for the fuzz and my unclear communication. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Broadwell 3.14 backports
On Fri, Mar 21, 2014 at 05:51:01PM -0700, Ben Widawsky wrote: Let's try this again. I've pushed a branch here: http://cgit.freedesktop.org/~bwidawsk/drm-intel/log/?h=bdw-backports I need to re-review some of the merge conflicts for 4g GGTT, which I will try to do before Monday. Daniel: please make sure this is what you had in mind. I don't know where you want Cc: stable tags. We don't need cc: stable, we just need to submit it (since it has the upstream sha1s already, which is the requirement for stable patches). Cc: stable is only for when you want it to get backport anyway. Otherwise looks good. I dunno whether git cherry-pick can be told to use the sha1 reference layout Greg prefers or not, he uses This is sha1 in upstream. between the commit header and the actual commit message. But ime his scripts reformat your commit messages anyway. James: please test this as soon as possible. Once this is tested and we conclude it's sufficient to get bdw going on 3.14 without hilarity I think we should do a quick review on intel-gfx to check that the impact outside of bdw is indeed minimal. Then once drm-next has landed with all the referenced commits we can submit it to Greg with a small cover letter why we want this and that plan B would be to kill bdw in 3.14. Thanks for doing this, Daniel Thanks. On Fri, Mar 21, 2014 at 11:48:09AM -0700, Ben Widawsky wrote: The following patches are the backported simple fixes for 3.14. Some of these already had Cc: stable on them, but required conflict resolution which I've provided (presumably they canbe dropped if it's easier for upstream). There will be another series of backports which has fixes that require more than a single patch. I will not have a machine to test these on until Monday, but I am mailing them out now in case our QA can get it tested sooner. Ben Widawsky (2): drm/i915/bdw: Use scratch page table for GEN8 PPGTT drm/i915/bdw: Restore PPAT on thaw Damien Lespiau (1): drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM Jani Nikula (1): drm/i915: don't flood the logs about bdw semaphores Kenneth Graunke (2): drm/i915: Add a partial instruction shootdown workaround on Broadwell. drm/i915: Add thread stall DOP clock gating workaround on Broadwell. Mika Kuoppala (2): drm/i915: Fix forcewake counts for gen8 drm/i915: Do forcewake reset on gen8 Ville Syrjälä (4): drm/i915: Disable semaphore wait event idle message on BDW drm/i915: Implement WaDisableSDEUnitClockGating:bdw drm/i915: We implement WaDisableAsyncFlipPerfMode:bdw drm/i915: Don't clobber CHICKEN_PIPESL_1 on BDW drivers/gpu/drm/i915/i915_drv.c | 5 - drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++ drivers/gpu/drm/i915/i915_reg.h | 10 ++ drivers/gpu/drm/i915/intel_pm.c | 18 -- drivers/gpu/drm/i915/intel_ringbuffer.c | 12 +--- drivers/gpu/drm/i915/intel_uncore.c | 29 +++-- 6 files changed, 61 insertions(+), 20 deletions(-) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center -- 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] lib: allow igt_skip_on_simulation outside of fixtures.
Thomas noticed that in simulation mode a lot of the tests fall over
instead of skipping properly. This is due to recently added
self-checks which ensure that any call to igt_skip happens either
within a fixture or subtest block (or it's a simple test without
subtests). This is to catch bugs since pretty much always not wrapping
up hardware setup and checks into these blocks is a bug.
Bug simulation skipping is a bit different, so allow that exception.
Otherwise we'd need to fix up piles of tests (and likely need to play
a game of whack-a-mole).
Also add a library testcase for all the different variants to make
sure it really works.
Cc: Thomas Wood thomas.w...@intel.com
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
lib/igt_core.c | 12 -
tests/.gitignore | 1 +
tests/Makefile.sources | 1 +
tests/igt_simulation.c | 139 +
4 files changed, 152 insertions(+), 1 deletion(-)
create mode 100644 tests/igt_simulation.c
diff --git a/lib/igt_core.c b/lib/igt_core.c
index bdace83ab382..dbfcd74f9160 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1117,13 +1117,23 @@ bool igt_run_in_simulation(void)
*
* Skip tests when INTEL_SIMULATION environment variable is set. It uses
* igt_skip() internally and hence is fully subtest aware.
+ *
+ * Note that in contrast to all other functions which use igt_skip() internally
+ * it is allowed to use this outside of an #igt_fixture block in a test with
+ * subtests. This is because in contrast to most other test requirements,
+ * checking for simulation mode doesn't depend upon the present hardware and it
+ * so makes a lot of sense to have this check in the outermost #igt_main block.
*/
void igt_skip_on_simulation(void)
{
if (igt_only_list_subtests())
return;
- igt_require(!igt_run_in_simulation());
+ if (!in_fixture) {
+ igt_fixture
+ igt_require(!igt_run_in_simulation());
+ } else
+ igt_require(!igt_run_in_simulation());
}
/* structured logging */
diff --git a/tests/.gitignore b/tests/.gitignore
index 623a621ccace..60aa3b497f88 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -107,6 +107,7 @@ igt_list_only
igt_no_exit
igt_no_exit_list_only
igt_no_subtest
+igt_simulation
kms_addfb
kms_cursor_crc
kms_fbc_crc
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 88866ac7ea75..8aeaac0ed15f 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -179,6 +179,7 @@ TESTS_testsuite = \
igt_fork_helper \
igt_list_only \
igt_no_subtest \
+ igt_simulation \
$(NULL)
TESTS = \
diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c
new file mode 100644
index ..b9c6241d12e4
--- /dev/null
+++ b/tests/igt_simulation.c
@@ -0,0 +1,139 @@
+/*
+ * 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:
+ *Daniel Vetter daniel.vet...@ffwll.ch
+ *
+ */
+
+#include stdlib.h
+#include sys/wait.h
+#include sys/types.h
+
+#include drmtest.h
+#include igt_core.h
+
+bool simple;
+bool list_subtests;
+bool in_fixture;
+
+char test[] = test;
+char list[] = --list-subtests;
+char *argv_list[] = { test, list };
+char *argv_run[] = { test };
+
+static int do_fork(void)
+{
+ int pid, status;
+
+ switch (pid = fork()) {
+ case -1:
+ assert(0);
+ case 0:
+ if (simple) {
+ igt_simple_init();
+
+ igt_skip_on_simulation();
+
+ exit(0);
+ } else {
+ if (list_subtests)
+ igt_subtest_init(2, argv_list);
+ else
+ igt_subtest_init(1, argv_run);
+
+
[Intel-gfx] [PATCH] tests/kms_cursor_crc: Fix up breakage
I've accidentally broken the new cursor size extensions, but it
wasn't quite correct before already: Variables which are set in
igt_fixtures _must_ be outside of the stackframe of the igt_fixture
block.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
tests/kms_cursor_crc.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index 7423f51409fb..f95448f47356 100644
--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -341,11 +341,12 @@ static void run_test_generic(data_t *data, int
cursor_max_size)
}
+uint64_t cursor_width, cursor_height;
+
igt_main
{
data_t data = {};
- int cursor_max_size, ret;
- uint64_t cursor_width, cursor_height;
+ int ret;
igt_skip_on_simulation();
@@ -371,7 +372,7 @@ igt_main
}
- run_test_generic(data, cursor_max_size);
+ run_test_generic(data, cursor_width);
igt_fixture {
free(data.pipe_crc);
--
1.8.5.2
___
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: Drop WARN_ON(flags) from ppgtt_bind_vma()
On Wed, Mar 12, 2014 at 07:32:26PM +0200, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
We will call ppgtt_bind_vma() with flags != 0, so the WARN_ON(flags)
is bogus. Kill it.
This is not an appropriate commit message to change an invariant. The
case was true, and it apparently no longer holds. At the very least the
commit should have the SHA which changed the invariant, and preferably
an explanation as to why the invariant no longer holds (is bogus). I
The reason you have given to remove this WARN_ON can be used for any
assertion we ever hit and simply reiterates what the patch does.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7727103..0dce6fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1243,8 +1243,6 @@ ppgtt_bind_vma(struct i915_vma *vma,
enum i915_cache_level cache_level,
u32 flags)
{
- WARN_ON(flags);
-
vma-vm-insert_entries(vma-vm, vma-obj-pages, vma-node.start,
cache_level);
}
--
1.8.3.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] lib: allow igt_skip_on_simulation outside of fixtures.
On Sat, Mar 22, 2014 at 01:27:07PM +0100, Daniel Vetter wrote:
Thomas noticed that in simulation mode a lot of the tests fall over
instead of skipping properly. This is due to recently added
self-checks which ensure that any call to igt_skip happens either
within a fixture or subtest block (or it's a simple test without
subtests). This is to catch bugs since pretty much always not wrapping
up hardware setup and checks into these blocks is a bug.
Bug simulation skipping is a bit different, so allow that exception.
But
Otherwise we'd need to fix up piles of tests (and likely need to play
a game of whack-a-mole).
Also add a library testcase for all the different variants to make
sure it really works.
Cc: Thomas Wood thomas.w...@intel.com
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
You do realize you've just written a test for your test suite? Yo dawg,
I heard you like tests
I honestly don't know enough about the internals here to do a proper
review in a minute, but sounds fine from the commit msg. Just one
comment below and it's:
Acked-by: Ben Widawsky b...@bwidawsk.net
---
lib/igt_core.c | 12 -
tests/.gitignore | 1 +
tests/Makefile.sources | 1 +
tests/igt_simulation.c | 139
+
4 files changed, 152 insertions(+), 1 deletion(-)
create mode 100644 tests/igt_simulation.c
diff --git a/lib/igt_core.c b/lib/igt_core.c
index bdace83ab382..dbfcd74f9160 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1117,13 +1117,23 @@ bool igt_run_in_simulation(void)
*
* Skip tests when INTEL_SIMULATION environment variable is set. It uses
* igt_skip() internally and hence is fully subtest aware.
+ *
+ * Note that in contrast to all other functions which use igt_skip()
internally
+ * it is allowed to use this outside of an #igt_fixture block in a test with
+ * subtests. This is because in contrast to most other test requirements,
+ * checking for simulation mode doesn't depend upon the present hardware and
it
+ * so makes a lot of sense to have this check in the outermost #igt_main
block.
*/
void igt_skip_on_simulation(void)
{
if (igt_only_list_subtests())
return;
- igt_require(!igt_run_in_simulation());
+ if (!in_fixture) {
+ igt_fixture
+ igt_require(!igt_run_in_simulation());
+ } else
+ igt_require(!igt_run_in_simulation());
}
/* structured logging */
diff --git a/tests/.gitignore b/tests/.gitignore
index 623a621ccace..60aa3b497f88 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -107,6 +107,7 @@ igt_list_only
igt_no_exit
igt_no_exit_list_only
igt_no_subtest
+igt_simulation
kms_addfb
kms_cursor_crc
kms_fbc_crc
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 88866ac7ea75..8aeaac0ed15f 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -179,6 +179,7 @@ TESTS_testsuite = \
igt_fork_helper \
igt_list_only \
igt_no_subtest \
+ igt_simulation \
$(NULL)
TESTS = \
diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c
new file mode 100644
index ..b9c6241d12e4
--- /dev/null
+++ b/tests/igt_simulation.c
@@ -0,0 +1,139 @@
+/*
+ * 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:
+ *Daniel Vetter daniel.vet...@ffwll.ch
+ *
+ */
+
+#include stdlib.h
+#include sys/wait.h
+#include sys/types.h
+
+#include drmtest.h
+#include igt_core.h
+
+bool simple;
+bool list_subtests;
+bool in_fixture;
+
+char test[] = test;
+char list[] = --list-subtests;
+char *argv_list[] = { test, list };
+char *argv_run[] = { test };
+
+static int do_fork(void)
+{
+ int pid, status;
+
Re: [Intel-gfx] [PATCH 1/2] drm/i915: fix up semaphore_waits_for
On Fri, Mar 21, 2014 at 07:33:59PM +0200, Mika Kuoppala wrote: Hi, Daniel Vetter daniel.vet...@ffwll.ch writes: There's an entire pile of issues in here: - Use the main RING_HEAD register, not ACTHD. ACTHD points at the gtt offset of the batch buffer when a batch is executed. Semaphores are always emitted to the main ring, so we always want to look at that. The ipehr check should make sure that we are at the ring and acthd should not be at batch. Regardless I agree that RING_HEAD is much more safer. When I have tried to get bottom of the snb reset hang, I have noticed that after reset the acthd is sometimes 0x0c even tho head is 0x00, on snb. Hm, should we maybe mask out the lower bits, too? If you can confirm this, can you please add a follow-up patch? - Mask the obtained HEAD pointer with the actual ring size, which is much smaller. Together with the above issue this resulted us in trying to dereference a pointer way outside of the ring mmio mapping. The resulting invalid access in interrupt context (hangcheck is executed from timers) lead to a full blown kernel panic. The fbcon panic handler then tried to frob our driver harder, resulting in a full machine hang at least on my snb here where I've stumbled over this. - Handle ring wrapping correctly and be a bit more explicit about how many dwords we're scanning. We probably should also scan more than just 4 ... ipehr dont update on MI_NOOPS and the head should point to the extra MI_NOOP after semaphore sequence. So 4 should be enough. Perhaps revisit this when bdw gains semaphores. Yeah, Chris also mentioned that the HEAD should point at one dword past the end of the ring, so should even work when there are no MI_NOOPs. In any case this code is more robust in case hw engineers suddenly change the behaviour. - Space out some of teh computations for readability. This prevents hard-hangs on my snb here. Verdict from QA is still pending, but the symptoms match. Cc: Mika Kuoppala mika.kuopp...@intel.com Cc: Ben Widawsky b...@bwidawsk.net Cc: Chris Wilson ch...@chris-wilson.co.uk Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74100 The hard hangs are not so frequent with this patch but they are still there. This patch should take care of the oops we have been seeing, but there is still something else to be found until #74100 can be marked as fixed. Very often after reset, when igt has pushed the quietence batches into rings, blitter and video ring heads gets moved properly but all updates to hardware status page and to the sync registers are missing. And result is hang by hangcheck. Repeat enough times and it is a hard hang. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Please remove the blowup comment and also update the bugzilla tag. Yeah, QA also says that it doesn't fix the hard hangs, only seems to reduce them a bit on certain setups. I've updated the commit message. btw are you testing with FBDEV=n? The lack of a fbcon panic handler should greatly increase the chances that the last few message get correctly transmitted through other means like netconsole. Patches 1-2 and the followup one are Reviewed-by: Mika Kuoppala mika.kuopp...@intel.com Thanks for the review, patches merged. -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: Drop WARN_ON(flags) from ppgtt_bind_vma()
On Sat, Mar 22, 2014 at 09:59:50AM -0700, Ben Widawsky wrote: On Wed, Mar 12, 2014 at 07:32:26PM +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com We will call ppgtt_bind_vma() with flags != 0, so the WARN_ON(flags) is bogus. Kill it. This is not an appropriate commit message to change an invariant. The case was true, and it apparently no longer holds. At the very least the commit should have the SHA which changed the invariant, and preferably an explanation as to why the invariant no longer holds (is bogus). I The reason you have given to remove this WARN_ON can be used for any assertion we ever hit and simply reiterates what the patch does. I agree that the commit message is a bit too thin. Unfortunately I've already baked in dinq so I can't go back and rectify history :( Since this patch removes something we also can't fix that by adding a comment around the new code and supplying the missing commit messages bits 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] lib: allow igt_skip_on_simulation outside of fixtures.
On Sat, Mar 22, 2014 at 10:14:41AM -0700, Ben Widawsky wrote:
On Sat, Mar 22, 2014 at 01:27:07PM +0100, Daniel Vetter wrote:
Thomas noticed that in simulation mode a lot of the tests fall over
instead of skipping properly. This is due to recently added
self-checks which ensure that any call to igt_skip happens either
within a fixture or subtest block (or it's a simple test without
subtests). This is to catch bugs since pretty much always not wrapping
up hardware setup and checks into these blocks is a bug.
Bug simulation skipping is a bit different, so allow that exception.
But
Otherwise we'd need to fix up piles of tests (and likely need to play
a game of whack-a-mole).
Also add a library testcase for all the different variants to make
sure it really works.
Cc: Thomas Wood thomas.w...@intel.com
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
You do realize you've just written a test for your test suite? Yo dawg,
I heard you like tests
You do realize that this is about number 6 in my set of tests for my test
suite?
I think the proper conclusion is simply that at least occasionally I
should hand in my C programmer lincense due to accute incompetence ;-)
~/xorg/intel-gpu-tools$ ls tests/igt_*c
tests/igt_fork_helper.c tests/igt_no_exit.ctests/igt_no_subtest.c
tests/igt_list_only.ctests/igt_no_exit_list_only.c tests/igt_simulation.c
Fyi you can run the with
$ make check
which is always recommended when you touch some of the library code.
I honestly don't know enough about the internals here to do a proper
review in a minute, but sounds fine from the commit msg. Just one
comment below and it's:
Acked-by: Ben Widawsky b...@bwidawsk.net
Already pushed the patch ...
+int main(int argc, char **argv)
+{
+ /* simple tests */
+ simple = true;
+ assert(setenv(INTEL_SIMULATION, 1, 1) == 0);
+ assert(do_fork() == 77);
+
+ assert(setenv(INTEL_SIMULATION, 0, 1) == 0);
+ assert(do_fork() == 0);
+
+ /* subtests, list mode */
+ simple = false;
+ list_subtests = true;
+
+ in_fixture = false;
+ assert(setenv(INTEL_SIMULATION, 1, 1) == 0);
+ assert(do_fork() == 0);
+
+ in_fixture = false;
+ assert(setenv(INTEL_SIMULATION, 0, 1) == 0);
+ assert(do_fork() == 0);
+
+ in_fixture = true;
+ assert(setenv(INTEL_SIMULATION, 1, 1) == 0);
+ assert(do_fork() == 0);
+
+ in_fixture = true;
+ assert(setenv(INTEL_SIMULATION, 0, 1) == 0);
+ assert(do_fork() == 0);
+
+ /* subtest, run mode */
+ simple = false;
+ list_subtests = false;
+
+ in_fixture = false;
+ assert(setenv(INTEL_SIMULATION, 1, 1) == 0);
+ assert(do_fork() == 77);
+
+ in_fixture = false;
+ assert(setenv(INTEL_SIMULATION, 0, 1) == 0);
+ assert(do_fork() == 0);
+
+ in_fixture = true;
+ assert(setenv(INTEL_SIMULATION, 1, 1) == 0);
+ assert(do_fork() == 77);
+
+ in_fixture = true;
+ assert(setenv(INTEL_SIMULATION, 0, 1) == 0);
+ assert(do_fork() == 0);
+
You could have saved some setenvs here and above by doing all the trues,
then falses. If you want to keep the same pattern though, it probably
makes sense to dump this into a helper
test(bool fixture, char *sim_env, int retval);
OR
in_fixture = false;
assert(setenv(INTEL_SIMULATION, 1, 1) == 0);
assert(do_fork() == 77);
in_fixture = true;
assert(do_fork() == 77);
in_fixture = false;
assert(setenv(INTEL_SIMULATION, 0, 1) == 0);
assert(do_fork() == 0);
in_fixture = true;
assert(do_fork() == 0);
you can even get down to just 2 setenvs by permuting in_fixture, simple, and
list_subtests.
Yeah, it's not pretty. If I'd go totally ocd I'd do a for_boolean macro
for simple, list_subtests and in_fixture and for the simulation flag. But
my thinking here was that I want to explicitly list all the case, since
it's no totally obvious which should return 77 and which 0. So I've left
it at the very verbose but explicit version above.
After all this is a testcase for a now fairly established (and documented)
piece of igt infrastructure, so I don't think anyone needs to look at this
any time soon (i.e. hopefully not in the next few years).
Also, alread pushed ;-)
Thanks for the comments anyway.
-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 01/26] drm/i915: Split out verbose PPGTT dumping
On Thu, Mar 20, 2014 at 12:08:00PM +, Chris Wilson wrote:
On Thu, Mar 20, 2014 at 11:57:42AM +, Chris Wilson wrote:
static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev,
bool verbose)
@@ -1838,14 +1841,11 @@ static void gen6_ppgtt_info(struct seq_file *m,
struct drm_device *dev, bool ver
list_for_each_entry_reverse(file, dev-filelist, lhead) {
struct drm_i915_file_private *file_priv = file-driver_priv;
- struct i915_hw_ppgtt *pvt_ppgtt;
- pvt_ppgtt = ctx_to_ppgtt(file_priv-private_default_ctx);
seq_printf(m, proc: %s\n,
get_pid_task(file-pid, PIDTYPE_PID)-comm);
And
seq_printf(m, \nproc: %s\n,
for good measure
- print_ppgtt(m, pvt_ppgtt, Default context);
- if (verbose)
- idr_for_each(file_priv-context_idr, per_file_ctx, m);
+ idr_for_each(file_priv-context_idr, per_file_ctx,
+(void *)((unsigned long)m | verbose));
}
}
--
Thanks, I like it. I'm assuming you didn't want the count_pt_pages stuck
in at this point (your diff was based on the end result)? I can do that
if you prefer it. It seems pointless to me though.
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/12] drm/i915/bdw: Set initial rps freq to RP0
On Thu, Mar 20, 2014 at 07:24:38AM +, Chris Wilson wrote: On Wed, Mar 19, 2014 at 06:31:14PM -0700, Ben Widawsky wrote: Programming it outside of the rp0-rp1 range is considered a programming error. Since we do not know that the previous value would actually be in the range, program something we've read from the hardware, and therefore know will work. This is potentially an issue for platforms whose ranges are outside the norms given in the programming guide (ie. early silicon) v2: Use RP1 instead of RPn Signed-off-by: Ben Widawsky b...@bwidawsk.net Do you have a reference for GEN6_RC_VIDEO_FREQ? I still have no idea what that controls, nor its valid range. -Chris I have no reference for the video freq other than the brief mention in the programming guide, and know nothing more than you do about it. It's there because the original spec I had said to program it to 600MHz. The reason for /this/ patch was that I noticed the default values happened to be a *really* close to our max freq. and figured someone, somewhere might get a part whose lower, or upper bound precludes setting the constant provided in the programming guide. Interestingly, the programming guide has been updated since I originally wrote this patch to clearly indicate both of these registers need to be programmed between Rp1-Rp0. So I guess that means that Rp1-Rp0 is the valid range. Therefore, I think we should either proceed with this patch, or create a new patch to avoid writing it at all. The current code seems like the worst solution of all. If you want to argue we can drop the write to GEN6_RPNSWREQ since we do the correct thing after step 6: gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) 0xff00) 8); I wouldn't be too opposed. I was just trying to follow the spec as closely as possible, and it says to write the register value in this sequence, so I did. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/i915/bdw: Extract rp_state_caps logic
On Thu, Mar 20, 2014 at 07:28:29AM +, Chris Wilson wrote: On Wed, Mar 19, 2014 at 06:31:15PM -0700, Ben Widawsky wrote: We have a need for duplicated parsing of the RP_STATE_CAPS register (and the setting of the associated fields). To reuse some code, we can extract the function into a simple helper. This patch also addresses the fact that we missed doing this for gen8, something we should have done anyway. This could be two patches, one to extract, and one to add gen8, but it's trivial enough that I think one is fine. I will accept a request to split it. Please notice the fix addressed by v2 below. Valleyview is left untouched because it is different. v2: Logically rebased on top of commit dd0a1aa19bd3d7203e58157b84cea78bbac605ac Author: Jeff McGee jeff.mc...@intel.com Date: Tue Feb 4 11:32:31 2014 -0600 drm/i915: Restore rps/rc6 on reset Note with the above change the fix for gen8 is also handled (which was not the case in Jeff's original patch). Signed-off-by: Ben Widawsky b...@bwidawsk.net Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk By setting max_freq_soft before querying overclocking frequencies, we force the user to have to manually raise the max freq through sysfs, right? Hasn't the user already explicitly asked for overclocking through the BIOS setting in the first place, so isn't that a needless burden upon the user? -Chris It's debatable, and if my memory serves, we've debated it before. Overclocking has a range. BIOS enables the user to select a value within that range. Selecting the highest possible value for the user is a policy decision (IMO). If BIOS/punit wanted to control this, it should set rp0 equal to the max overclock frequency, and not even bother letting the driver deal with it. By not selecting anything, we're not making any decision. Daniel, please notice he did put the r-b tag on it. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/26] drm/i915: clean up PPGTT init error path
On Tue, Mar 18, 2014 at 08:44:28AM +, Chris Wilson wrote:
On Mon, Mar 17, 2014 at 10:48:39PM -0700, Ben Widawsky wrote:
The old code (I'm having trouble finding the commit) had a reason for
doing things when there was an error, and would continue on, thus the
!ret. For the newer code however, this looks completely silly.
Follow the normal idiom of if (ret) return ret.
Also, put the pde wiring in the gen specific init, now that GEN8 exists.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +-
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 1620211..5f73284 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1202,6 +1202,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt
*ppgtt)
ppgtt-pd_offset =
ppgtt-node.start / PAGE_SIZE * sizeof(gen6_gtt_pte_t);
+ gen6_write_pdes(ppgtt);
+
ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true);
DRM_DEBUG_DRIVER(Allocated pde space (%ldM) at GTT entry: %lx\n,
@@ -1226,20 +1228,14 @@ int i915_gem_init_ppgtt(struct drm_device *dev,
struct i915_hw_ppgtt *ppgtt)
else
BUG();
- if (!ret) {
- struct drm_i915_private *dev_priv = dev-dev_private;
- kref_init(ppgtt-ref);
- drm_mm_init(ppgtt-base.mm, ppgtt-base.start,
- ppgtt-base.total);
- i915_init_vm(dev_priv, ppgtt-base);
- if (INTEL_INFO(dev)-gen 8) {
- gen6_write_pdes(ppgtt);
- DRM_DEBUG(Adding PPGTT at offset %x\n,
- ppgtt-pd_offset 10);
- }
- }
+ if (ret)
+ return ret;
- return ret;
+ kref_init(ppgtt-ref);
+ drm_mm_init(ppgtt-base.mm, ppgtt-base.start, ppgtt-base.total);
+ i915_init_vm(dev_priv, ppgtt-base);
Didn't we just delete the dev_priv local variable?
-Chris
The important part is that the pde writes moved. (The DRM debug is also
dropped). As for this code, I just wanted to get rid of the if (!ret)
block. It looks weird.
Maybe I didn't get what you're asking though.
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/26] drm/i915: Split out gtt specific header file
On Tue, Mar 18, 2014 at 10:15:56AM +0100, Daniel Vetter wrote:
On Mon, Mar 17, 2014 at 10:48:41PM -0700, Ben Widawsky wrote:
TODO: Do header files need a copyright?
Yup ;-)
I like this though, especially since finer-grained files will make
kerneldoc inclusion (well, grouped into sensible chapters at least) much
simpler.
-Daniel
If I re-submit just this patch (with the copyright), will you merge it?
It will make my life so much easier.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_drv.h | 162 +-
drivers/gpu/drm/i915/i915_gem_gtt.c | 57 -
drivers/gpu/drm/i915/i915_gem_gtt.h | 225
3 files changed, 227 insertions(+), 217 deletions(-)
create mode 100644 drivers/gpu/drm/i915/i915_gem_gtt.h
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 084e82f..b19442c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -44,6 +44,8 @@
#include linux/kref.h
#include linux/pm_qos.h
+#include i915_gem_gtt.h
+
/* General customization:
*/
@@ -572,166 +574,6 @@ enum i915_cache_level {
I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */
};
-typedef uint32_t gen6_gtt_pte_t;
-
-/**
- * A VMA represents a GEM BO that is bound into an address space.
Therefore, a
- * VMA's presence cannot be guaranteed before binding, or after unbinding
the
- * object into/from the address space.
- *
- * To make things as simple as possible (ie. no refcounting), a VMA's
lifetime
- * will always be = an objects lifetime. So object refcounting should
cover us.
- */
-struct i915_vma {
- struct drm_mm_node node;
- struct drm_i915_gem_object *obj;
- struct i915_address_space *vm;
-
- /** This object's place on the active/inactive lists */
- struct list_head mm_list;
-
- struct list_head vma_link; /* Link in the object's VMA list */
-
- /** This vma's place in the batchbuffer or on the eviction list */
- struct list_head exec_list;
-
- /**
-* Used for performing relocations during execbuffer insertion.
-*/
- struct hlist_node exec_node;
- unsigned long exec_handle;
- struct drm_i915_gem_exec_object2 *exec_entry;
-
- /**
-* How many users have pinned this object in GTT space. The following
-* users can each hold at most one reference: pwrite/pread, pin_ioctl
-* (via user_pin_count), execbuffer (objects are not allowed multiple
-* times for the same batchbuffer), and the framebuffer code. When
-* switching/pageflipping, the framebuffer code has at most two buffers
-* pinned per crtc.
-*
-* In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3
-* bits with absolutely no headroom. So use 4 bits. */
- unsigned int pin_count:4;
-#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf
-
- /** Unmap an object from an address space. This usually consists of
-* setting the valid PTE entries to a reserved scratch page. */
- void (*unbind_vma)(struct i915_vma *vma);
- /* Map an object into an address space with the given cache flags. */
-#define GLOBAL_BIND (10)
- void (*bind_vma)(struct i915_vma *vma,
-enum i915_cache_level cache_level,
-u32 flags);
-};
-
-struct i915_address_space {
- struct drm_mm mm;
- struct drm_device *dev;
- struct list_head global_link;
- unsigned long start;/* Start offset always 0 for dri2 */
- size_t total; /* size addr space maps (ex. 2GB for ggtt) */
-
- struct {
- dma_addr_t addr;
- struct page *page;
- } scratch;
-
- /**
-* List of objects currently involved in rendering.
-*
-* Includes buffers having the contents of their GPU caches
-* flushed, not necessarily primitives. last_rendering_seqno
-* represents when the rendering involved will be completed.
-*
-* A reference is held on the buffer while on this list.
-*/
- struct list_head active_list;
-
- /**
-* LRU list of objects which are not in the ringbuffer and
-* are ready to unbind, but are still in the GTT.
-*
-* last_rendering_seqno is 0 while an object is in this list.
-*
-* A reference is not held on the buffer while on this list,
-* as merely being GTT-bound shouldn't prevent its being
-* freed, and we'll pull it off the list in the free path.
-*/
- struct list_head inactive_list;
-
- /* FIXME: Need a more generic return type */
- gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr,
-enum i915_cache_level level,
-bool valid); /* Create a valid PTE */
- void (*clear_range)(struct i915_address_space
Re: [Intel-gfx] [PATCH 14/26] drm/i915: Complete page table structures
On Tue, Mar 18, 2014 at 09:09:45AM +, Chris Wilson wrote: On Mon, Mar 17, 2014 at 10:48:46PM -0700, Ben Widawsky wrote: Move the remaining members over to the new page table structures. This can be squashed with the previous commit if desire. The reasoning is the same as that patch. I simply felt it is easier to review if split. I'm not liking the shorter names much. Is there precedence elsewhere (e.g. daddr)? -Chris I'm not particularly attached to daddr. It was fun to say in my head. A lot of code does use daddr but it seems to vary between dma, device, data, destination Not exactly precedence. Initially I had the prefix p[td]_daddr, but I thought you might complain about it because it's implicit. dma_addr seemed kinda redundant to me. Recommendation? -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/26] drm/i915: Create page table allocators
On Tue, Mar 18, 2014 at 09:14:09AM +, Chris Wilson wrote: On Mon, Mar 17, 2014 at 10:48:47PM -0700, Ben Widawsky wrote: As we move toward dynamic page table allocation, it becomes much easier to manage our data structures if break do things less coarsely by breaking up all of our actions into individual tasks. This makes the code easier to write, read, and verify. Aside from the dissection of the allocation functions, the patch statically allocates the page table structures without a page directory. This remains the same for all platforms, The patch itself should not have much functional difference. The primary noticeable difference is the fact that page tables are no longer allocated, but rather statically declared as part of the page directory. This has non-zero overhead, but things gain non-trivial complexity as a result. We increase overhead for increased complexity. What's the selling point of this patch then? I'd argue about the complexity. Personally, I think the result is easier to read. I'll add this all to the commit message, but hopefully you agree: 1. Splitting out the functions allows easily combining GEN6 and GEN8 code. Page tables have no difference based on GEN8. As we'll see in a future patch when we add the dma mappings to the allocations, it requires only one small change to make work, and error handling should just fall into place. 2. Unless we always want to allocate all page tables under a given PDE, we'll have to eventually break this up into an array of pointers (or pointer to pointer). 3. Having the discrete functions is easier to review, and understand. All allocations and frees now take place in just a couple of locations. Reviewing, and cathcing leaks should be easy. 4. Less important: the gfp flags are confined to one location, which makes playing around with such things trivial.o Hopefully you're more convinced after you went through more of the patch series. If you want to try to optimize the overhead of managing the page tables, I think this is a worthy thing to do (for instance, not statically declaring the array). It takes a little more work though, and I'd prefer to do it after the code is doing what it's supposed to do. Otherwise, patch does as you say. -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 -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/26] drm/i915: clean up PPGTT init error path
On Sat, Mar 22, 2014 at 12:43:28PM -0700, Ben Widawsky wrote:
On Tue, Mar 18, 2014 at 08:44:28AM +, Chris Wilson wrote:
On Mon, Mar 17, 2014 at 10:48:39PM -0700, Ben Widawsky wrote:
The old code (I'm having trouble finding the commit) had a reason for
doing things when there was an error, and would continue on, thus the
!ret. For the newer code however, this looks completely silly.
Follow the normal idiom of if (ret) return ret.
Also, put the pde wiring in the gen specific init, now that GEN8 exists.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +-
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 1620211..5f73284 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1202,6 +1202,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt
*ppgtt)
ppgtt-pd_offset =
ppgtt-node.start / PAGE_SIZE * sizeof(gen6_gtt_pte_t);
+ gen6_write_pdes(ppgtt);
+
ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true);
DRM_DEBUG_DRIVER(Allocated pde space (%ldM) at GTT entry: %lx\n,
@@ -1226,20 +1228,14 @@ int i915_gem_init_ppgtt(struct drm_device *dev,
struct i915_hw_ppgtt *ppgtt)
else
BUG();
- if (!ret) {
- struct drm_i915_private *dev_priv = dev-dev_private;
- kref_init(ppgtt-ref);
- drm_mm_init(ppgtt-base.mm, ppgtt-base.start,
- ppgtt-base.total);
- i915_init_vm(dev_priv, ppgtt-base);
- if (INTEL_INFO(dev)-gen 8) {
- gen6_write_pdes(ppgtt);
- DRM_DEBUG(Adding PPGTT at offset %x\n,
- ppgtt-pd_offset 10);
- }
- }
+ if (ret)
+ return ret;
- return ret;
+ kref_init(ppgtt-ref);
+ drm_mm_init(ppgtt-base.mm, ppgtt-base.start, ppgtt-base.total);
+ i915_init_vm(dev_priv, ppgtt-base);
Didn't we just delete the dev_priv local variable?
-Chris
The important part is that the pde writes moved. (The DRM debug is also
dropped). As for this code, I just wanted to get rid of the if (!ret)
block. It looks weird.
Maybe I didn't get what you're asking though.
I was wondering if this patch compiles because of the removal of the
dev_priv local variable. (Or if the original was a shadow.)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/26] drm/i915: Split out verbose PPGTT dumping
On Sat, Mar 22, 2014 at 11:13:17AM -0700, Ben Widawsky wrote:
On Thu, Mar 20, 2014 at 12:08:00PM +, Chris Wilson wrote:
On Thu, Mar 20, 2014 at 11:57:42AM +, Chris Wilson wrote:
static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev,
bool verbose)
@@ -1838,14 +1841,11 @@ static void gen6_ppgtt_info(struct seq_file *m,
struct drm_device *dev, bool ver
list_for_each_entry_reverse(file, dev-filelist, lhead) {
struct drm_i915_file_private *file_priv = file-driver_priv;
- struct i915_hw_ppgtt *pvt_ppgtt;
- pvt_ppgtt = ctx_to_ppgtt(file_priv-private_default_ctx);
seq_printf(m, proc: %s\n,
get_pid_task(file-pid, PIDTYPE_PID)-comm);
And
seq_printf(m, \nproc: %s\n,
for good measure
- print_ppgtt(m, pvt_ppgtt, Default context);
- if (verbose)
- idr_for_each(file_priv-context_idr, per_file_ctx, m);
+ idr_for_each(file_priv-context_idr, per_file_ctx,
+ (void *)((unsigned long)m | verbose));
}
}
--
Thanks, I like it. I'm assuming you didn't want the count_pt_pages stuck
in at this point (your diff was based on the end result)? I can do that
if you prefer it. It seems pointless to me though.
Maybe pointless to you, but it helped me to know when they were fully
populated - without having to look back at the constants.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/12] drm/i915/bdw: Set initial rps freq to RP0
On Sat, Mar 22, 2014 at 11:42:17AM -0700, Ben Widawsky wrote: On Thu, Mar 20, 2014 at 07:24:38AM +, Chris Wilson wrote: On Wed, Mar 19, 2014 at 06:31:14PM -0700, Ben Widawsky wrote: Programming it outside of the rp0-rp1 range is considered a programming error. Since we do not know that the previous value would actually be in the range, program something we've read from the hardware, and therefore know will work. This is potentially an issue for platforms whose ranges are outside the norms given in the programming guide (ie. early silicon) v2: Use RP1 instead of RPn Signed-off-by: Ben Widawsky b...@bwidawsk.net Do you have a reference for GEN6_RC_VIDEO_FREQ? I still have no idea what that controls, nor its valid range. -Chris I have no reference for the video freq other than the brief mention in the programming guide, and know nothing more than you do about it. It's there because the original spec I had said to program it to 600MHz. The reason for /this/ patch was that I noticed the default values happened to be a *really* close to our max freq. and figured someone, somewhere might get a part whose lower, or upper bound precludes setting the constant provided in the programming guide. Interestingly, the programming guide has been updated since I originally wrote this patch to clearly indicate both of these registers need to be programmed between Rp1-Rp0. So I guess that means that Rp1-Rp0 is the valid range. Therefore, I think we should either proceed with this patch, or create a new patch to avoid writing it at all. The current code seems like the worst solution of all. If you want to argue we can drop the write to GEN6_RPNSWREQ since we do the correct thing after step 6: gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) 0xff00) 8); I wouldn't be too opposed. I was just trying to follow the spec as closely as possible, and it says to write the register value in this sequence, so I did. Let's mark that as Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk and move on. Though I may double check to see if I can find some information on the video frequency. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/26] drm/i915: Create page table allocators
On Sat, Mar 22, 2014 at 01:21:39PM -0700, Ben Widawsky wrote: On Tue, Mar 18, 2014 at 09:14:09AM +, Chris Wilson wrote: On Mon, Mar 17, 2014 at 10:48:47PM -0700, Ben Widawsky wrote: As we move toward dynamic page table allocation, it becomes much easier to manage our data structures if break do things less coarsely by breaking up all of our actions into individual tasks. This makes the code easier to write, read, and verify. Aside from the dissection of the allocation functions, the patch statically allocates the page table structures without a page directory. This remains the same for all platforms, The patch itself should not have much functional difference. The primary noticeable difference is the fact that page tables are no longer allocated, but rather statically declared as part of the page directory. This has non-zero overhead, but things gain non-trivial complexity as a result. We increase overhead for increased complexity. What's the selling point of this patch then? I'd argue about the complexity. Personally, I think the result is easier to read. I'll add this all to the commit message, but hopefully you agree: 1. Splitting out the functions allows easily combining GEN6 and GEN8 code. Page tables have no difference based on GEN8. As we'll see in a future patch when we add the dma mappings to the allocations, it requires only one small change to make work, and error handling should just fall into place. 2. Unless we always want to allocate all page tables under a given PDE, we'll have to eventually break this up into an array of pointers (or pointer to pointer). 3. Having the discrete functions is easier to review, and understand. All allocations and frees now take place in just a couple of locations. Reviewing, and cathcing leaks should be easy. 4. Less important: the gfp flags are confined to one location, which makes playing around with such things trivial.o Hopefully you're more convinced after you went through more of the patch series. Right, the patches and the resulting code look good. I just felt the changelog here was self-contradictory. And now we have a great spiel to include ;-) If you want to try to optimize the overhead of managing the page tables, I think this is a worthy thing to do (for instance, not statically declaring the array). It takes a little more work though, and I'd prefer to do it after the code is doing what it's supposed to do. Agreed. I'm still watching PT pages come and go with great joy, and not yet worrying about the impact. (Though I think I did notices some side-effect when a reap of the userspace cache lead to a sudden release of lots of pages, and a dip in throughput.) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/26] drm/i915: Complete page table structures
On Sat, Mar 22, 2014 at 01:10:47PM -0700, Ben Widawsky wrote: On Tue, Mar 18, 2014 at 09:09:45AM +, Chris Wilson wrote: On Mon, Mar 17, 2014 at 10:48:46PM -0700, Ben Widawsky wrote: Move the remaining members over to the new page table structures. This can be squashed with the previous commit if desire. The reasoning is the same as that patch. I simply felt it is easier to review if split. I'm not liking the shorter names much. Is there precedence elsewhere (e.g. daddr)? -Chris I'm not particularly attached to daddr. It was fun to say in my head. A lot of code does use daddr but it seems to vary between dma, device, data, destination Not exactly precedence. Initially I had the prefix p[td]_daddr, but I thought you might complain about it because it's implicit. dma_addr seemed kinda redundant to me. Recommendation? I am still attached to dma_addr, as it seems to be common (or dma_address) for dma-mapped addresses. But by the end, I had stopped caring, so Acked-by: Chris Wilson ch...@chris-wilson.co.uk I'll come back to this if nobody has a better idea and see if I can make some sensisble suggestions, or just give in completely. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/26] drm/i915: Split out gtt specific header file
On Sat, Mar 22, 2014 at 8:44 PM, Ben Widawsky b...@bwidawsk.net wrote: On Tue, Mar 18, 2014 at 10:15:56AM +0100, Daniel Vetter wrote: On Mon, Mar 17, 2014 at 10:48:41PM -0700, Ben Widawsky wrote: TODO: Do header files need a copyright? Yup ;-) I like this though, especially since finer-grained files will make kerneldoc inclusion (well, grouped into sensible chapters at least) much simpler. -Daniel If I re-submit just this patch (with the copyright), will you merge it? It will make my life so much easier. Yeah. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Split out GTT specific header file
This file contains all necessary defines, prototypes and typesdefs for
manipulating GEN graphics address translation (this does not include the
legacy AGP driver)
Reiterating the comment in the header,
Please try to maintain the following order within this file unless it
makes sense to do otherwise. From top to bottom:
1. typedefs
2. #defines, and macros
3. structure definitions
4. function prototypes
Within each section, please try to order by generation in ascending
order, from top to bottom (ie. GEN6 on the top, GEN8 on the bottom).
I've made some minor cleanups, and fixed a couple of typos while here -
but there should be no functional changes.
The purpose of the patch is to reduce clutter in our main header file,
making room for new growth, and make documentation of our interfaces
easier by splitting things out.
With a little more work, like making i915_gtt a pointer, we could
potentially completely isolate this header from i915_drv.h. At the
moment however, I don't think it's worth the effort.
Personally, I would have liked to put the PTE encoding functions in this
file too, but I didn't want to rock the boat too much.
A similar patch has been in use on my machine for some time. This exact
patch though has only been compile tested.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_drv.h | 178 +--
drivers/gpu/drm/i915/i915_gem_gtt.c | 69 -
drivers/gpu/drm/i915/i915_gem_gtt.h | 283
3 files changed, 286 insertions(+), 244 deletions(-)
create mode 100644 drivers/gpu/drm/i915/i915_gem_gtt.h
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3f62be0..ef7e0ff 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -35,6 +35,7 @@
#include i915_reg.h
#include intel_bios.h
#include intel_ringbuffer.h
+#include i915_gem_gtt.h
#include linux/io-mapping.h
#include linux/i2c.h
#include linux/i2c-algo-bit.h
@@ -572,168 +573,6 @@ enum i915_cache_level {
I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */
};
-typedef uint32_t gen6_gtt_pte_t;
-
-/**
- * A VMA represents a GEM BO that is bound into an address space. Therefore, a
- * VMA's presence cannot be guaranteed before binding, or after unbinding the
- * object into/from the address space.
- *
- * To make things as simple as possible (ie. no refcounting), a VMA's lifetime
- * will always be = an objects lifetime. So object refcounting should cover
us.
- */
-struct i915_vma {
- struct drm_mm_node node;
- struct drm_i915_gem_object *obj;
- struct i915_address_space *vm;
-
- /** This object's place on the active/inactive lists */
- struct list_head mm_list;
-
- struct list_head vma_link; /* Link in the object's VMA list */
-
- /** This vma's place in the batchbuffer or on the eviction list */
- struct list_head exec_list;
-
- /**
-* Used for performing relocations during execbuffer insertion.
-*/
- struct hlist_node exec_node;
- unsigned long exec_handle;
- struct drm_i915_gem_exec_object2 *exec_entry;
-
- /**
-* How many users have pinned this object in GTT space. The following
-* users can each hold at most one reference: pwrite/pread, pin_ioctl
-* (via user_pin_count), execbuffer (objects are not allowed multiple
-* times for the same batchbuffer), and the framebuffer code. When
-* switching/pageflipping, the framebuffer code has at most two buffers
-* pinned per crtc.
-*
-* In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3
-* bits with absolutely no headroom. So use 4 bits. */
- unsigned int pin_count:4;
-#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf
-
- /** Unmap an object from an address space. This usually consists of
-* setting the valid PTE entries to a reserved scratch page. */
- void (*unbind_vma)(struct i915_vma *vma);
- /* Map an object into an address space with the given cache flags. */
-#define GLOBAL_BIND (10)
- void (*bind_vma)(struct i915_vma *vma,
-enum i915_cache_level cache_level,
-u32 flags);
-};
-
-struct i915_address_space {
- struct drm_mm mm;
- struct drm_device *dev;
- struct list_head global_link;
- unsigned long start;/* Start offset always 0 for dri2 */
- size_t total; /* size addr space maps (ex. 2GB for ggtt) */
-
- struct {
- dma_addr_t addr;
- struct page *page;
- } scratch;
-
- /**
-* List of objects currently involved in rendering.
-*
-* Includes buffers having the contents of their GPU caches
-* flushed, not necessarily primitives. last_rendering_seqno
-* represents when the rendering involved will be completed.
-*
-*
