Re: [Intel-gfx] [PATCH 1/1] Revert drm/i915: drop i915_ prefix from enable_rc6, enable_fbc, enable_ppgtt parameters
On Wed, Jul 16, 2014 at 9:54 PM, Linus Torvalds torva...@linux-foundation.org wrote: Sorry for the top post, I'm on the road.. In wondering if we couldn't just keep both the old an the new names and have them both point at the same variable? Remove the description for the old name, but keep it working? I'm really surprised here ... We have rc6 enabled by default everywhere, and all the additional rc6 levels that users try to enable are known to hard-hang machines. I actually have plans to taint the kernel if you set any of them since I'm fed up with the random crash reports. Same for fbc, even more so or the ppgtt knob. My stance is that if you know about these knobs you _really_ should know the driver to its depths and so also be able to follow module parameter renamings. On Jul 16, 2014 8:34 AM, Amit Shah amit.s...@redhat.com wrote: This reverts commit 3adee7a7976012a20f1d3b5a529a3c105e29fef1. After upgrading to v3.15, my laptop's battery started draining quite fast. Powertop pointed to the deep RC6 states not being used. The kernel param I had put to enable them had stopped working the way it used to; so I disagree with the 'not maintaing ABI' part of the param name change. However weird the names may be, they're in active use and changing them only causes pain for users. This also isn't advertised (marked deprecated, big warning shown, etc.), so just reverting now. CC: Daniel Vetter daniel.vet...@ffwll.ch CC: Jani Nikula jani.nik...@linux.intel.com CC: David Airlie airl...@linux.ie CC: sta...@vger.kernel.org # v3.15+ Signed-off-by: Amit Shah amit.s...@redhat.com Anyway we need to figure out what went wrong here. Please share your exact kernelcmdline and lspci -nn. Also stats for before/after from powertop when idle please. Thanks, Daniel --- drivers/gpu/drm/i915/i915_params.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index d05a2af..053981d 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -69,16 +69,16 @@ MODULE_PARM_DESC(semaphores, Use semaphores for inter-ring sync (default: -1 (use per-chip defaults))); -module_param_named(enable_rc6, i915.enable_rc6, int, 0400); -MODULE_PARM_DESC(enable_rc6, +module_param_named(i915_enable_rc6, i915.enable_rc6, int, 0400); +MODULE_PARM_DESC(i915_enable_rc6, Enable power-saving render C-state 6. Different stages can be selected via bitmask values (0 = disable; 1 = enable rc6; 2 = enable deep rc6; 4 = enable deepest rc6). For example, 3 would enable rc6 and deep rc6, and 7 would enable everything. default: -1 (use per-chip default)); -module_param_named(enable_fbc, i915.enable_fbc, int, 0600); -MODULE_PARM_DESC(enable_fbc, +module_param_named(i915_enable_fbc, i915.enable_fbc, int, 0600); +MODULE_PARM_DESC(i915_enable_fbc, Enable frame buffer compression for power savings (default: -1 (use per-chip default))); @@ -111,8 +111,8 @@ MODULE_PARM_DESC(enable_hangcheck, WARNING: Disabling this can cause system wide hangs. (default: true)); -module_param_named(enable_ppgtt, i915.enable_ppgtt, int, 0400); -MODULE_PARM_DESC(enable_ppgtt, +module_param_named(i915_enable_ppgtt, i915.enable_ppgtt, int, 0400); +MODULE_PARM_DESC(i915_enable_ppgtt, Override PPGTT usage. (-1=auto [default], 0=disabled, 1=aliasing, 2=full)); -- 1.9.3 -- 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/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs
On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote: 2014-07-14 14:34 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote: 2014-07-07 18:53 GMT-03:00 Jesse Barnes jbar...@virtuousgeek.org: On Mon, 7 Jul 2014 18:48:47 -0300 Paulo Zanoni przan...@gmail.com wrote: (documenting what we discussed on IRC) 2014-06-20 13:29 GMT-03:00 Jesse Barnes jbar...@virtuousgeek.org: This was always the case on our suspend path, but it was recently exposed by the change to use our runtime IRQ disable routine rather than the full DRM IRQ disable. Keep the warning on the enable side, as that really would indicate a bug. While I understand the patch and think it's a reasonable thing to do, I feel the need to spend some time persuading you in replacing it with something that doesn't involve removing WARNs from our driver. While the driver is runtime suspended, no one should really be manipulating IRQs, even if they're disabling stuff that is already disabled: this reflects there's probably a problem somewhere. These WARNs are extremely helpful for the runtime PM feature because almost nobody actually uses runtime PM to notice any bugs with it, so the WARNs can make QA report bugs and bisect things for us. Also, it seems S3 suspend is currently a little disaster on our driver. Your 6 patches just solve some of the WARNs, not all of them. And last week I even solved another WARN on the S3 path. I just did some investigation, and the current bad commits are: 8abdc17941c71b37311bb93876ac83dce58160c8, e11aa362308f5de467ce355a2a2471321b15a35c and 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3 commits, S3 doesn't give me any WARNs. Instead of the change you proposed, can't we think of another solution that would maybe address all the 3 regressions we have? Since we're always submitting patches to change the order we do things at S3 suspend/resume, shouldn't we add something like dev_priv-suspending that could be used to avoid the strict ordering that is required during runtime? Hm I was running with those changes and didn't see additional warnings, so I'll have to look again. I agree we want sensible warnings in place, and maybe removing this one is too permissive, but I'd like to avoid a suspending flag if we can. Maybe we just need to bundle up our assertions and check all the relevant state after runtime suspend or S3 like we do for mode set state in various places. That would let us keep our warnings, but also save us from having them spread out in code paths that get used in many different contexts. I'm probably going to regret this, but since no one proposed a better patch and the bug is still there: Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com Not really eager to merge a patch soaking in preemptive regrets ... I'll punt on this for now. Possible solutions: 1 - Patch xxx_disable_vblank to do nothing in case dev_priv-pm.suspended 2 - Add a flag dev_priv-suspending and don't print those WARNs in case it is set. 3 - Merge Jesse's original patch 4 - Something else? I can implement any of the proposed solutions if needed... I've gone with 3 for now. It sounds like we need to work with this code a bit more still until the best solution is clear. The patch at least addresses the WARN. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: mark IRQs as disabled on unload
On Fri, Jun 20, 2014 at 11:57:33AM -0700, Jesse Barnes wrote: To avoid more spew with the new warnings. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org Ok, I've pulled this all into dinq. I guess we'll see how it all works out. -Daniel --- drivers/gpu/drm/i915/intel_display.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d993b69..7288d1d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12778,6 +12778,8 @@ void intel_modeset_cleanup(struct drm_device *dev) */ drm_irq_uninstall(dev); cancel_work_sync(dev_priv-hotplug_work); + dev_priv-pm._irqs_disabled = true; + /* * Due to the hpd irq storm handling the hotplug work can re-arm the * poll handlers. Hence disable polling after hpd handling is shut down. -- 1.8.3.2 ___ 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 2/2] drm/i915: BDW can also detect unclaimed registers
On Wed, Jul 16, 2014 at 05:49:30PM -0300, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zan...@intel.com
By the time I wrote this patch, it allowed me to catch some problems.
But due to patch reordering - in order to prevent fake regression
reports - this patch may be merged after the fixes of the problems
identified by this patch.
Reviewed-by: Rodrigo Vivi rodrigo.v...@intel.com
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
Pulled in, thanks for the resend. And my apology for the mess I've made
yesterday.
-Daniel
---
drivers/gpu/drm/i915/i915_drv.c | 4
drivers/gpu/drm/i915/intel_uncore.c | 3 +++
2 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5e4fefd..3315358 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -303,6 +303,7 @@ static const struct intel_device_info
intel_broadwell_d_info = {
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
.has_llc = 1,
.has_ddi = 1,
+ .has_fpga_dbg = 1,
.has_fbc = 1,
GEN_DEFAULT_PIPEOFFSETS,
IVB_CURSOR_OFFSETS,
@@ -314,6 +315,7 @@ static const struct intel_device_info
intel_broadwell_m_info = {
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
.has_llc = 1,
.has_ddi = 1,
+ .has_fpga_dbg = 1,
.has_fbc = 1,
GEN_DEFAULT_PIPEOFFSETS,
IVB_CURSOR_OFFSETS,
@@ -325,6 +327,7 @@ static const struct intel_device_info
intel_broadwell_gt3d_info = {
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
.has_llc = 1,
.has_ddi = 1,
+ .has_fpga_dbg = 1,
.has_fbc = 1,
GEN_DEFAULT_PIPEOFFSETS,
IVB_CURSOR_OFFSETS,
@@ -336,6 +339,7 @@ static const struct intel_device_info
intel_broadwell_gt3m_info = {
.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
.has_llc = 1,
.has_ddi = 1,
+ .has_fpga_dbg = 1,
.has_fbc = 1,
GEN_DEFAULT_PIPEOFFSETS,
IVB_CURSOR_OFFSETS,
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 6fee122..e81bc3b 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -747,6 +747,7 @@ static bool is_gen8_shadowed(struct drm_i915_private
*dev_priv, u32 reg)
static void \
gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool
trace) { \
REG_WRITE_HEADER; \
+ hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
if (reg 0x4 !is_gen8_shadowed(dev_priv, reg)) { \
if (dev_priv-uncore.forcewake_count == 0) \
dev_priv-uncore.funcs.force_wake_get(dev_priv, \
@@ -758,6 +759,8 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t
reg, u##x val, bool trace
} else { \
__raw_i915_write##x(dev_priv, reg, val); \
} \
+ hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
+ hsw_unclaimed_reg_detect(dev_priv); \
REG_WRITE_FOOTER; \
}
--
2.0.0
___
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: check the power wells on assert_{cursor, plane}
On Wed, Jul 16, 2014 at 05:06:34PM -0300, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zan...@intel.com
Since we merged runtime PM support for DPMS, it is possible that these
functions will be called when the power wells are disabled but a mode
is set, resulting in failed assertion and device suspended while
reading register WARNs.
To reproduce the bug: disable all screens using mode unset, do a
modeset on one screen, disable it using DPMS, then try to do a mode
unset on it again to see the WARNs.
Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
Hm, where do we call these asserts while the pipe is off? Do you have some
example backtraces at hand?
-Daniel
---
drivers/gpu/drm/i915/intel_display.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 54e3af9..7ad46e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1216,7 +1216,9 @@ static void assert_cursor(struct drm_i915_private
*dev_priv,
struct drm_device *dev = dev_priv-dev;
bool cur_state;
- if (IS_845G(dev) || IS_I865G(dev))
+ if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
+ cur_state = false;
+ else if (IS_845G(dev) || IS_I865G(dev))
cur_state = I915_READ(_CURACNTR) CURSOR_ENABLE;
else
cur_state = I915_READ(CURCNTR(pipe)) CURSOR_MODE;
@@ -1262,9 +1264,13 @@ static void assert_plane(struct drm_i915_private
*dev_priv,
u32 val;
bool cur_state;
- reg = DSPCNTR(plane);
- val = I915_READ(reg);
- cur_state = !!(val DISPLAY_PLANE_ENABLE);
+ if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(plane))) {
+ cur_state = false;
+ } else {
+ reg = DSPCNTR(plane);
+ val = I915_READ(reg);
+ cur_state = !!(val DISPLAY_PLANE_ENABLE);
+ }
WARN(cur_state != state,
plane %c assertion failure (expected %s, current %s)\n,
plane_name(plane), state_string(state), state_string(cur_state));
--
2.0.0
___
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 printing proper min/min/rpe values in debugfs
On Thu, Jul 17, 2014 at 02:21:14PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com This was fumbled while trying to use the cached min/min/rpe values in the vlv debugfs code. This is a regression from commit 03af20458a57a50735b12c1e3c23abc7ff70c6fa Author: Ville Syrjälä ville.syrj...@linux.intel.com Date: Sat Jun 28 02:03:53 2014 +0300 drm/i915: Use the cached min/min/rpe values in the vlv debugfs code Signed-off-by: Deepak S deepa...@linux.intel.com Queued for -next, thanks for the patch. -Daniel --- drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index fc39610..5bc65af 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1116,13 +1116,13 @@ static int i915_frequency_info(struct seq_file *m, void *unused) seq_printf(m, DDR freq: %d MHz\n, dev_priv-mem_freq); seq_printf(m, max GPU freq: %d MHz\n, -dev_priv-rps.max_freq); +vlv_gpu_freq(dev_priv, dev_priv-rps.max_freq)); seq_printf(m, min GPU freq: %d MHz\n, -dev_priv-rps.min_freq); +vlv_gpu_freq(dev_priv, dev_priv-rps.min_freq)); seq_printf(m, efficient (RPe) frequency: %d MHz\n, -dev_priv-rps.efficient_freq); +vlv_gpu_freq(dev_priv, dev_priv-rps.efficient_freq)); seq_printf(m, current GPU freq: %d MHz\n, vlv_gpu_freq(dev_priv, (freq_sts 8) 0xff)); -- 1.9.1 -- 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 06/16] drm/i915/error: Check the potential ctx obj's vm
On Tue, Jul 01, 2014 at 11:17:41AM -0700, Ben Widawsky wrote:
The bound list is global (all objects which back the VMAs are stored
here). Recently the BUG() in the offset lookup was demoted to a WARN,
but the fault actually lies in the caller, here.
This bug has existed since the initial introduction of PPGTT (however,
it was fixed in unmerged patches to fix up the error state).
Aside: This is another bug than the one which spurred me to demote the
BUG_ON to a WARN_ON. Patch merged with that patch referenced for clarity.
-Daniel
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gpu_error.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 66cf417..550ba38 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -871,6 +871,9 @@ static void i915_gem_record_active_context(struct
intel_engine_cs *ring,
return;
list_for_each_entry(obj, dev_priv-mm.bound_list, global_list) {
+ if (!i915_gem_obj_ggtt_bound(obj))
+ continue;
+
if ((error-ccid PAGE_MASK) == i915_gem_obj_ggtt_offset(obj))
{
ering-ctx = i915_error_ggtt_object_create(dev_priv,
obj);
break;
--
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 08/16] drm/i915/error: Do a better job of disambiguating VMAs
On Fri, Jul 04, 2014 at 09:56:54AM -0700, Ben Widawsky wrote:
On Fri, Jul 04, 2014 at 08:57:08AM +0100, Chris Wilson wrote:
On Tue, Jul 01, 2014 at 11:17:43AM -0700, Ben Widawsky wrote:
Some of the original PPGTT patches in this area where unmerged, and this
left a lot of confusion in our error capture with regard to which vm/obj
we want to capture. There have been at least a couple of patches from
Chris, and myself to try to fix this up; so here is another shot. Nobody
running without full PPGTT is effected by this, and that is probably why
nobody has bothered to fix it yet.
Instead of using any of the global lists to find the VMAs we want to
capture, we use the union of the active, and the inactive list in the
VM. This allows us to replace our capture_bo with capture_vma, and know
all the VMAs we want to capture are valid.
I could have probably figured out a way to reuse mm_list. As we've had
bugs here before in the shrinker, I think the best way forward is to get
it working, and then optimize it later.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 1 +
drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
drivers/gpu/drm/i915/i915_gpu_error.c | 39
++-
3 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a4153ee..88451dc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2114,6 +2114,7 @@ static struct i915_vma
*__i915_gem_vma_create(struct drm_i915_gem_object *obj,
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(vma-vma_link);
+ INIT_LIST_HEAD(vma-pin_capture_link);
INIT_LIST_HEAD(vma-mm_list);
INIT_LIST_HEAD(vma-exec_list);
vma-vm = vm;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8d6f7c1..1d75801 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -126,6 +126,8 @@ struct i915_vma {
struct list_head vma_link; /* Link in the object's VMA list */
+ struct list_head pin_capture_link; /* Link in the error capture */
+
/** This vma's place in the batchbuffer or on the eviction list */
struct list_head exec_list;
We already have a slot for temporary lists...
-Chris
I did mention that in the commit message, if I caught your meaning.
Chris is probably talking about exec_list which is our canonical temporary
list, mostly used by execbuf. But also in other places.
-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/1] Revert drm/i915: drop i915_ prefix from enable_rc6, enable_fbc, enable_ppgtt parameters
On (Thu) 17 Jul 2014 [09:35:20], Daniel Vetter wrote: On Wed, Jul 16, 2014 at 9:54 PM, Linus Torvalds torva...@linux-foundation.org wrote: Sorry for the top post, I'm on the road.. In wondering if we couldn't just keep both the old an the new names and have them both point at the same variable? Remove the description for the old name, but keep it working? I'm really surprised here ... We have rc6 enabled by default everywhere, and all the additional rc6 levels that users try to enable are known to hard-hang machines. I haven't had this problem on my hardware (ThinkPad T420s, lspci below) for a few kernel versions. I think I added the enable_rc6= setting back from the time the deeper states were enabled and then reverted for SandyBridge. Nevertheless, with the current state, RC6p and RC6pp states are not used. I actually have plans to taint the kernel if you set any of them since I'm fed up with the random crash reports. Same for fbc, even more so or the ppgtt knob. My stance is that if you know about these knobs you _really_ should know the driver to its depths and so also be able to follow module parameter renamings. I also remember there being bugzillas about power consumption, and using this setting was recommended (for Fedora, I think). I know a few people are using this setting. On Jul 16, 2014 8:34 AM, Amit Shah amit.s...@redhat.com wrote: This reverts commit 3adee7a7976012a20f1d3b5a529a3c105e29fef1. After upgrading to v3.15, my laptop's battery started draining quite fast. Powertop pointed to the deep RC6 states not being used. The kernel param I had put to enable them had stopped working the way it used to; so I disagree with the 'not maintaing ABI' part of the param name change. However weird the names may be, they're in active use and changing them only causes pain for users. This also isn't advertised (marked deprecated, big warning shown, etc.), so just reverting now. CC: Daniel Vetter daniel.vet...@ffwll.ch CC: Jani Nikula jani.nik...@linux.intel.com CC: David Airlie airl...@linux.ie CC: sta...@vger.kernel.org # v3.15+ Signed-off-by: Amit Shah amit.s...@redhat.com Anyway we need to figure out what went wrong here. Please share your exact kernelcmdline and lspci -nn. Also stats for before/after from powertop when idle please. Powertop stats for idle are a little difficult -- since this is my primary laptop. BOOT_IMAGE=/vmlinuz-3.15.4-200.fc20.x86_64 root=/dev/mapper/luks-3aff2acf-737d-4002-b644-15f599d09a18 ro rd.lvm.lv=fedora_grmbl/00 rd.lvm.lv=fedora_grmbl/01 vconsole.font=latarcyrheb-sun16 rd.luks.uuid=luks-0934d354-5b07-4e91-a699-9bfc57e76fdc rd.luks.uuid=luks-3aff2acf-737d-4002-b644-15f599d09a18 rhgb quiet slub_debug=- i915.i915_enable_rc6=7 LANG=en_IN.UTF-8 00:00.0 Host bridge [0600]: Intel Corporation 2nd Generation Core Processor Family DRAM Controller [8086:0104] (rev 09) 00:02.0 VGA compatible controller [0300]: Intel Corporation 2nd Generation Core Processor Family Integrated Graphics Controller [8086:0126] (rev 09) 00:16.0 Communication controller [0780]: Intel Corporation 6 Series/C200 Series Chipset Family MEI Controller #1 [8086:1c3a] (rev 04) 00:19.0 Ethernet controller [0200]: Intel Corporation 82579LM Gigabit Network Connection [8086:1502] (rev 04) 00:1a.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #2 [8086:1c2d] (rev 04) 00:1b.0 Audio device [0403]: Intel Corporation 6 Series/C200 Series Chipset Family High Definition Audio Controller [8086:1c20] (rev 04) 00:1c.0 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 1 [8086:1c10] (rev b4) 00:1c.1 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 2 [8086:1c12] (rev b4) 00:1c.3 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 4 [8086:1c16] (rev b4) 00:1c.4 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 5 [8086:1c18] (rev b4) 00:1d.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #1 [8086:1c26] (rev 04) 00:1f.0 ISA bridge [0601]: Intel Corporation QM67 Express Chipset Family LPC Controller [8086:1c4f] (rev 04) 00:1f.2 SATA controller [0106]: Intel Corporation 6 Series/C200 Series Chipset Family 6 port SATA AHCI Controller [8086:1c03] (rev 04) 00:1f.3 SMBus [0c05]: Intel Corporation 6 Series/C200 Series Chipset Family SMBus Controller [8086:1c22] (rev 04) 03:00.0 Network controller [0280]: Intel Corporation Centrino Advanced-N 6205 [Taylor Peak] [8086:0085] (rev 34) 05:00.0 SD Host controller [0805]: Ricoh Co Ltd MMC/SD Host Controller [1180:e822] (rev 07) 0d:00.0 USB controller [0c03]: NEC Corporation uPD720200 USB 3.0 Host Controller [1033:0194] (rev 04) Thanks, Amit
Re: [Intel-gfx] [PATCH 1/1] Revert drm/i915: drop i915_ prefix from enable_rc6, enable_fbc, enable_ppgtt parameters
On Thu, Jul 17, 2014 at 02:32:41PM +0530, Amit Shah wrote: On (Thu) 17 Jul 2014 [09:35:20], Daniel Vetter wrote: On Wed, Jul 16, 2014 at 9:54 PM, Linus Torvalds torva...@linux-foundation.org wrote: Sorry for the top post, I'm on the road.. In wondering if we couldn't just keep both the old an the new names and have them both point at the same variable? Remove the description for the old name, but keep it working? I'm really surprised here ... We have rc6 enabled by default everywhere, and all the additional rc6 levels that users try to enable are known to hard-hang machines. I haven't had this problem on my hardware (ThinkPad T420s, lspci below) for a few kernel versions. I think I added the enable_rc6= setting back from the time the deeper states were enabled and then reverted for SandyBridge. Nevertheless, with the current state, RC6p and RC6pp states are not used. Yeah, on snb they cause crashes and instability and also don't provide measurable power benefits (afaik). So I recommend you drop that one. I actually have plans to taint the kernel if you set any of them since I'm fed up with the random crash reports. Same for fbc, even more so or the ppgtt knob. My stance is that if you know about these knobs you _really_ should know the driver to its depths and so also be able to follow module parameter renamings. I also remember there being bugzillas about power consumption, and using this setting was recommended (for Fedora, I think). I know a few people are using this setting. I know, google is littered with such entries. Unfortunately by the time google thinks something is important (which usually takes a few months) it's already badly outdated: i915 graphics developement is charging ahead at a really brisk pace - we merge a few hundred patches per release for i915 alone. On Jul 16, 2014 8:34 AM, Amit Shah amit.s...@redhat.com wrote: This reverts commit 3adee7a7976012a20f1d3b5a529a3c105e29fef1. After upgrading to v3.15, my laptop's battery started draining quite fast. Powertop pointed to the deep RC6 states not being used. The kernel param I had put to enable them had stopped working the way it used to; so I disagree with the 'not maintaing ABI' part of the param name change. However weird the names may be, they're in active use and changing them only causes pain for users. This also isn't advertised (marked deprecated, big warning shown, etc.), so just reverting now. CC: Daniel Vetter daniel.vet...@ffwll.ch CC: Jani Nikula jani.nik...@linux.intel.com CC: David Airlie airl...@linux.ie CC: sta...@vger.kernel.org # v3.15+ Signed-off-by: Amit Shah amit.s...@redhat.com Anyway we need to figure out what went wrong here. Please share your exact kernelcmdline and lspci -nn. Also stats for before/after from powertop when idle please. Powertop stats for idle are a little difficult -- since this is my primary laptop. Now I'm a bit confused: How have you measured that the lack of rc6p/pp is the reason for your power consumption regression? -Daniel BOOT_IMAGE=/vmlinuz-3.15.4-200.fc20.x86_64 root=/dev/mapper/luks-3aff2acf-737d-4002-b644-15f599d09a18 ro rd.lvm.lv=fedora_grmbl/00 rd.lvm.lv=fedora_grmbl/01 vconsole.font=latarcyrheb-sun16 rd.luks.uuid=luks-0934d354-5b07-4e91-a699-9bfc57e76fdc rd.luks.uuid=luks-3aff2acf-737d-4002-b644-15f599d09a18 rhgb quiet slub_debug=- i915.i915_enable_rc6=7 LANG=en_IN.UTF-8 00:00.0 Host bridge [0600]: Intel Corporation 2nd Generation Core Processor Family DRAM Controller [8086:0104] (rev 09) 00:02.0 VGA compatible controller [0300]: Intel Corporation 2nd Generation Core Processor Family Integrated Graphics Controller [8086:0126] (rev 09) 00:16.0 Communication controller [0780]: Intel Corporation 6 Series/C200 Series Chipset Family MEI Controller #1 [8086:1c3a] (rev 04) 00:19.0 Ethernet controller [0200]: Intel Corporation 82579LM Gigabit Network Connection [8086:1502] (rev 04) 00:1a.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #2 [8086:1c2d] (rev 04) 00:1b.0 Audio device [0403]: Intel Corporation 6 Series/C200 Series Chipset Family High Definition Audio Controller [8086:1c20] (rev 04) 00:1c.0 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 1 [8086:1c10] (rev b4) 00:1c.1 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 2 [8086:1c12] (rev b4) 00:1c.3 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 4 [8086:1c16] (rev b4) 00:1c.4 PCI bridge [0604]: Intel Corporation 6 Series/C200 Series Chipset Family PCI Express Root Port 5 [8086:1c18] (rev b4) 00:1d.0 USB controller [0c03]: Intel Corporation 6 Series/C200 Series Chipset Family USB Enhanced Host Controller #1 [8086:1c26] (rev 04) 00:1f.0 ISA bridge
Re: [Intel-gfx] [PATCH 1/1] Revert drm/i915: drop i915_ prefix from enable_rc6, enable_fbc, enable_ppgtt parameters
On (Thu) 17 Jul 2014 [11:11:15], Daniel Vetter wrote: On Thu, Jul 17, 2014 at 02:32:41PM +0530, Amit Shah wrote: On (Thu) 17 Jul 2014 [09:35:20], Daniel Vetter wrote: On Wed, Jul 16, 2014 at 9:54 PM, Linus Torvalds torva...@linux-foundation.org wrote: Sorry for the top post, I'm on the road.. In wondering if we couldn't just keep both the old an the new names and have them both point at the same variable? Remove the description for the old name, but keep it working? I'm really surprised here ... We have rc6 enabled by default everywhere, and all the additional rc6 levels that users try to enable are known to hard-hang machines. I haven't had this problem on my hardware (ThinkPad T420s, lspci below) for a few kernel versions. I think I added the enable_rc6= setting back from the time the deeper states were enabled and then reverted for SandyBridge. Nevertheless, with the current state, RC6p and RC6pp states are not used. Yeah, on snb they cause crashes and instability and also don't provide measurable power benefits (afaik). So I recommend you drop that one. Not for me -- there have been no crashes / hangs / lockups as I mentioned. I actually have plans to taint the kernel if you set any of them since I'm fed up with the random crash reports. Same for fbc, even more so or the ppgtt knob. My stance is that if you know about these knobs you _really_ should know the driver to its depths and so also be able to follow module parameter renamings. I also remember there being bugzillas about power consumption, and using this setting was recommended (for Fedora, I think). I know a few people are using this setting. I know, google is littered with such entries. Unfortunately by the time google thinks something is important (which usually takes a few months) it's already badly outdated: i915 graphics developement is charging ahead at a really brisk pace - we merge a few hundred patches per release for i915 alone. But for SNB, there's really no improvement for the RC6 states, is there? On Jul 16, 2014 8:34 AM, Amit Shah amit.s...@redhat.com wrote: This reverts commit 3adee7a7976012a20f1d3b5a529a3c105e29fef1. After upgrading to v3.15, my laptop's battery started draining quite fast. Powertop pointed to the deep RC6 states not being used. The kernel param I had put to enable them had stopped working the way it used to; so I disagree with the 'not maintaing ABI' part of the param name change. However weird the names may be, they're in active use and changing them only causes pain for users. This also isn't advertised (marked deprecated, big warning shown, etc.), so just reverting now. CC: Daniel Vetter daniel.vet...@ffwll.ch CC: Jani Nikula jani.nik...@linux.intel.com CC: David Airlie airl...@linux.ie CC: sta...@vger.kernel.org # v3.15+ Signed-off-by: Amit Shah amit.s...@redhat.com Anyway we need to figure out what went wrong here. Please share your exact kernelcmdline and lspci -nn. Also stats for before/after from powertop when idle please. Powertop stats for idle are a little difficult -- since this is my primary laptop. Now I'm a bit confused: How have you measured that the lack of rc6p/pp is the reason for your power consumption regression? -Daniel What I meant was rebooting in the middle of something is a pain (usually a week or two between trying these things); and also for a fair comparison, the workloads have to be similar for both the powertop ratings. In any case, my daily work doesn't change, and I noticed this immediately upon booting into 3.15. The laptop heats up a bit more, that's the first clue; and the battery doesn't provide as much backup as it used to. Amit ___ 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 2014/7/16 22:20, Konrad Rzeszutek Wilk wrote:
On Thu, Jul 03, 2014 at 11:27:40PM +0300, Michael S. Tsirkin wrote:
On Thu, Jul 03, 2014 at 12:09:28PM -0700, Jesse Barnes wrote:
On Thu, 3 Jul 2014 14:26:12 -0400
Konrad Rzeszutek Wilk konrad.w...@oracle.com wrote:
On Thu, Jul 03, 2014 at 10:32:12AM +0300, Michael S. Tsirkin wrote:
On Wed, Jul 02, 2014 at 12:23:37PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Jul 02, 2014 at 04:50:15PM +0200, Paolo Bonzini wrote:
Il 02/07/2014 16:00, Konrad Rzeszutek Wilk ha scritto:
With this long thread I lost a bit context about the challenges
that exists. But let me try summarizing it here - which will hopefully
get some consensus.
1). Fix IGD hardware to not use Southbridge magic addresses.
We can moan and moan but I doubt it is going to change.
There are two problems:
- Northbridge (i.e. MCH i.e. PCI host bridge) configuration space addresses
Right. So in drivers/gpu/drm/i915/i915_dma.c:
1135 #define MCHBAR_I915 0x44
1136 #define MCHBAR_I965 0x48
1147 int reg = INTEL_INFO(dev)-gen = 4 ? MCHBAR_I965 : MCHBAR_I915;
1152 if (INTEL_INFO(dev)-gen = 4)
1153 pci_read_config_dword(dev_priv-bridge_dev, reg + 4,
temp_hi);
1154 pci_read_config_dword(dev_priv-bridge_dev, reg, temp_lo);
1155 mchbar_addr = ((u64)temp_hi 32) | temp_lo;
and
1139 #define DEVEN_REG 0x54
1193 int mchbar_reg = INTEL_INFO(dev)-gen = 4 ? MCHBAR_I965 :
MCHBAR_I915;
1202 if (IS_I915G(dev) || IS_I915GM(dev)) {
1203 pci_read_config_dword(dev_priv-bridge_dev, DEVEN_REG,
temp);
1204 enabled = !!(temp DEVEN_MCHBAR_EN);
1205 } else {
1206 pci_read_config_dword(dev_priv-bridge_dev, mchbar_reg,
temp);
1207 enabled = temp 1;
1208 }
- Southbridge (i.e. PCH i.e. ISA bridge) vendor/device ID; some versions of
the driver identify it by class, some versions identify it by slot (1f.0).
Right, So in drivers/gpu/drm/i915/i915_drv.c the giant intel_detect_pch
which sets the pch_type based on :
432 if (pch-vendor == PCI_VENDOR_ID_INTEL) {
433 unsigned short id = pch-device
INTEL_PCH_DEVICE_ID_MASK;
434 dev_priv-pch_id = id;
435
436 if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
It checks for 0x3b00, 0x1c00, 0x1e00, 0x8c00 and 0x9c00.
The INTEL_PCH_DEVICE_ID_MASK is 0xff00
To solve the first, make a new machine type, PIIX4-based, and pass through
the registers you need. The patch must document _exactly_ why the registers
are safe to pass. If they are not reserved on PIIX4, the patch must
document what the same offsets mean on PIIX4, and why it's sensible to
assume that firmware for virtual machine will not read/write them. Bonus
point for also documenting the same for Q35.
OK. They look to be related to setting up an MBAR , but I don't understand
why it is needed. Hopefully some of the i915 folks CC-ed here can answer.
In particular, I think setting up MBAR should move out of i915 and become
the bridge driver tweak on linux and windows.
That is an excellent idea.
However I am still curious to _why_ it has to be done in the first place.
The display part of the GPU is partly on the PCH, and it's possible to
mix match them a bit, so we have this detection code to figure things
out. In some cases, the PCH display portion may not even be present,
so we look for that too.
Practically speaking, we could probably assume specific CPU/PCH combos,
as I don't think they're generally mixed across generations, though SNB
and IVB did have compatible sockets, so there is the possibility of
mixing CPT and PPT PCHs, but those are register identical as far as the
graphics driver is concerned, so even that should be safe.
Beyond that, the other MCH data we need to look at is mirrored into the
GPU's MMIO space on current gens. On older gens, we do need to poke at
the memory controller a bit to get some info (see
intel_setup_mchbar()), but that's not true of newer stuff. Looks like
we only short circuit that on VLV though; we could probably do it on
SNB+.
That sounds great. Tiejun could you confirm that with
windows driver guys too?
ping?
Allen,
Could you reply this?
Thanks
Tiejun
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/16] drm/i915: More correct (slower) ppgtt cleanup
On Tue, Jul 01, 2014 at 11:17:48AM -0700, Ben Widawsky wrote:
If a VM still have objects which are bound (exactly: have a node
reserved in the drm_mm), and we are in the middle of a reset, we have no
hope of the standard methods fixing the situation (ring idle won't
work). We must therefore let the reset handler take it's course, and
then we can resume tearing down the VM.
This logic very much duplicates^Wresembles the logic in our wait for
error code. I've decided to leave it as open coded because I expect this
bit of code to require tweaks and changes over time.
Interruptions via signal causes a really similar problem.
This should deprecate the need for the yet unmerged patch from Chris
(and an identical patch from me, which was first!!):
drm/i915: Prevent signals from interrupting close()
I have a followup patch to implement deferred free, before you complain.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
Imo this goes in the wrong direction. ppgtt_cleanup really shouldn't ever
have a need to wait for the gpu. We need to rework the lifetimes such that
we keep the ppgtt alive until the gpu is done with it. Similarly to how we
keep the objects themselves around when the gpu is still using them. Even
when userspace has already dropped the last reference.
Having such a stark behaviour difference between ppgtt lifetimes and
object lifetimes only leads to unecessary complexity and fragility in the
code. And this patch here is a good example for this.
-Daniel
---
drivers/gpu/drm/i915/i915_gem_context.c | 51
+++--
1 file changed, 48 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 8d106d9..e1b5613 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -101,6 +101,32 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
struct drm_device *dev = ppgtt-base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct i915_address_space *vm = ppgtt-base;
+ bool do_idle = false;
+ int ret;
+
+ /* If we get here while in reset, we need to let the reset handler run
+ * first, or else our VM teardown isn't going to go smoothly. There are
+ * a could of options at this point, but letting the reset handler do
+ * it's thing is the most desirable. The reset handler will take care of
+ * retiring the stuck requests.
+ */
+ if (i915_reset_in_progress(dev_priv-gpu_error)) {
+ mutex_unlock(dev-struct_mutex);
+#define EXIT_COND (!i915_reset_in_progress(dev_priv-gpu_error) || \
+i915_terminally_wedged(dev_priv-gpu_error))
+ ret = wait_event_timeout(dev_priv-gpu_error.reset_queue,
+ EXIT_COND,
+ 10 * HZ);
+ if (!ret) {
+ /* it's unlikely idling will solve anything, but it
+ * shouldn't hurt to try. */
+ do_idle = true;
+ /* TODO: go down kicking and screaming harder */
+ }
+#undef EXIT_COND
+
+ mutex_lock(dev-struct_mutex);
+ }
if (ppgtt == dev_priv-mm.aliasing_ppgtt ||
(list_empty(vm-active_list) list_empty(vm-inactive_list))) {
@@ -117,14 +143,33 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt
*ppgtt)
if (!list_empty(vm-active_list)) {
struct i915_vma *vma;
+ do_idle = true;
list_for_each_entry(vma, vm-active_list, mm_list)
if (WARN_ON(list_empty(vma-vma_link) ||
list_is_singular(vma-vma_link)))
break;
- i915_gem_evict_vm(ppgtt-base, true, true);
- } else {
+ } else
i915_gem_retire_requests(dev);
- i915_gem_evict_vm(ppgtt-base, false, true);
+
+ /* We have a problem here where VM teardown cannot be interrupted, or
+ * else the ppgtt cleanup will fail. As an example, a precisely timed
+ * SIGKILL could leads to an OOPS, or worse. There are two options:
+ * 1. Make the eviction uninterruptible
+ * 2. Defer the eviction if it was interrupted.
+ *
+ * Option #1 is not the friendliest, but it's the easiest to implement,
+ * and least error prone.
+ * TODO: Implement option 2
+ */
+ ret = i915_gem_evict_vm(ppgtt-base, do_idle, !do_idle);
+ if (ret == -ERESTARTSYS)
+ ret = i915_gem_evict_vm(ppgtt-base, do_idle, false);
+ WARN_ON(ret);
+ WARN_ON(!list_empty(vm-active_list));
+
+ /* This is going to blow up badly if the mm is unclean */
+ if (WARN_ON(!list_empty(ppgtt-base.mm.head_node.node_list))) {
+ /* TODO: go down kicking and screaming harder++ */
}
Re: [Intel-gfx] [PATCH 12/16] drm/i915: Reorder ctx unref on ppgtt cleanup
On Tue, Jul 01, 2014 at 11:17:47AM -0700, Ben Widawsky wrote: The comment [which was mine] is wrong. The context object can never be bound in a PPGTT because it is only capable of living in the Global GTT. So, remove the comment, and reorder the unref. What's nice about the latter is it keeps the context object alive past the PPGTT. This makes the destroy ordering symmetric with the creation ordering. Create: 1. Create context 2. Create PPGTT Destroy: 1. Destroy PPGTT 2. Destroy context As far as I know, this does not fix a bug. The code previously kept the context data structure, only the object was gone. As the code was, nothing tried to use the object after this point. NOTE: If in the future we have cases where the PPGTT can/should outlive the context (which doesn't occur today, but the code permits it), this ordering does not matter. Even if this occurs, as it stands now, we do not expect that to be the normal case, and having this order makes debugging a bit easier if we're tracking object lifetimes for the context vs ppgtt Signed-off-by: Ben Widawsky b...@bwidawsk.net Queued for -next, thanks for the patch. -Daniel --- drivers/gpu/drm/i915/i915_gem_context.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index b6803b3..8d106d9 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -185,14 +185,12 @@ void i915_gem_context_free(struct kref *ctx_ref) /* We refcount even the aliasing PPGTT to keep the code symmetric */ if (USES_PPGTT(ctx-obj-base.dev)) ppgtt = ctx_to_ppgtt(ctx); - - /* XXX: Free up the object before tearing down the address space, in - * case we're bound in the PPGTT */ - drm_gem_object_unreference(ctx-obj-base); } if (ppgtt) kref_put(ppgtt-ref, ppgtt_release); + if (ctx-obj) + drm_gem_object_unreference(ctx-obj-base); list_del(ctx-link); kfree(ctx); } -- 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 00/16] Enabling GEN8 full PPGTT + fixes
On Tue, Jul 01, 2014 at 11:17:35AM -0700, Ben Widawsky wrote:
Here be all the patches to make full PPGTT relatively stable on Broadwell.
Most
of the work was actually to the generic PPGTT code, and not BDW specific.
There
are basically 3 fixes:
1. Make the error state not horrible, but more work still needed.
2. Fix up another tricky from != from case in do_switch
3. Generally more graceful handling in ppgtt_release
1. Various forms of this subseries have shown up on the list from both
myself, and Chris (and I feel like Mika, also). For whatever reason,
none have been merged. I don't necessarily mind missing information, but
without these patches we can OOPs and GP fault in the error state, which
is not acceptable.
2. The meat of the debugging came here. Essentially this problem has
already been seen, and solved. The issue is, there is another spot in
do_switch that can invoke a switch to the default context. We probably
want to get slightly better handling of this, but for now this solves my
problems.
3. There are 2 categories addressed in this bucket. Reset, and signals. The
patches themselves explain the situation. I take a two step approach with this
where first I make things correct (and slow as heck), and then I do the more
optimal and trickier solution. Both of these are in line to be replaced by
Daniel, but I needed something sooner.
Here is the test case that caught all of the above issues:
while [ 1 ] ; do
(glxgears) pid[0]=$!
(glxgears) pid[1]=$!
(glxgears) pid[2]=$!
sleep 3
kill ${pid[*]}
done
Ben Widawsky (16):
drm/i915: Split up do_switch
drm/i915: Extract l3 remapping out of ctx switch
drm/i915/ppgtt: Load address space after mi_set_context
drm/i915: Fix another another use-after-free in do_switch
drm/i915/ctx: Return earlier on failure
drm/i915/error: Check the potential ctx obj's vm
drm/i915/error: vma error capture prettyify
drm/i915/error: Do a better job of disambiguating VMAs
drm/i915/error: Capture vmas instead of BOs
drm/i915: Add some extra guards in evict_vm
drm/i915: Make an uninterruptible evict
drm/i915: Reorder ctx unref on ppgtt cleanup
drm/i915: More correct (slower) ppgtt cleanup
drm/i915: Defer PPGTT cleanup
drm/i915/bdw: Enable full PPGTT
drm/i915: Get the error state over the wire (HACKish)
I think we can pull the roughly first 9 patches (already merged one of
them). Just needs someone to do the in-depth review, and preferrably
someone who's looking at the relevant jira ppgtt tasks we're tracking
interannyl. For the changes affecting ppgtt_cleanup I've replied in a
separate mail.
-Daniel
drivers/gpu/drm/i915/i915_debugfs.c| 2 +-
drivers/gpu/drm/i915/i915_drv.h| 18 ++-
drivers/gpu/drm/i915/i915_gem.c| 110 +
drivers/gpu/drm/i915/i915_gem_context.c| 238
++---
drivers/gpu/drm/i915/i915_gem_evict.c | 39 +++--
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/i915_gem_gtt.c| 3 +-
drivers/gpu/drm/i915/i915_gem_gtt.h| 4 +
drivers/gpu/drm/i915/i915_gpu_error.c | 157 ---
drivers/gpu/drm/i915/i915_sysfs.c | 2 +-
10 files changed, 449 insertions(+), 126 deletions(-)
--
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: check the power wells on assert_{cursor, plane}
2014-07-17 5:38 GMT-03:00 Daniel Vetter dan...@ffwll.ch:
On Wed, Jul 16, 2014 at 05:06:34PM -0300, Paulo Zanoni wrote:
From: Paulo Zanoni paulo.r.zan...@intel.com
Since we merged runtime PM support for DPMS, it is possible that these
functions will be called when the power wells are disabled but a mode
is set, resulting in failed assertion and device suspended while
reading register WARNs.
To reproduce the bug: disable all screens using mode unset, do a
modeset on one screen, disable it using DPMS, then try to do a mode
unset on it again to see the WARNs.
Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
Hm, where do we call these asserts while the pipe is off? Do you have some
example backtraces at hand?
Function __intel_set_mode() directly calls intel_crtc_disable(), which
calls both assert_plane_disabled() and assert_cursor_disabled().
-Daniel
---
drivers/gpu/drm/i915/intel_display.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 54e3af9..7ad46e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1216,7 +1216,9 @@ static void assert_cursor(struct drm_i915_private
*dev_priv,
struct drm_device *dev = dev_priv-dev;
bool cur_state;
- if (IS_845G(dev) || IS_I865G(dev))
+ if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe)))
+ cur_state = false;
+ else if (IS_845G(dev) || IS_I865G(dev))
cur_state = I915_READ(_CURACNTR) CURSOR_ENABLE;
else
cur_state = I915_READ(CURCNTR(pipe)) CURSOR_MODE;
@@ -1262,9 +1264,13 @@ static void assert_plane(struct drm_i915_private
*dev_priv,
u32 val;
bool cur_state;
- reg = DSPCNTR(plane);
- val = I915_READ(reg);
- cur_state = !!(val DISPLAY_PLANE_ENABLE);
+ if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PIPE(plane))) {
+ cur_state = false;
+ } else {
+ reg = DSPCNTR(plane);
+ val = I915_READ(reg);
+ cur_state = !!(val DISPLAY_PLANE_ENABLE);
+ }
WARN(cur_state != state,
plane %c assertion failure (expected %s, current %s)\n,
plane_name(plane), state_string(state), state_string(cur_state));
--
2.0.0
___
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
--
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: check the power wells on assert_{cursor, plane}
On Thu, Jul 17, 2014 at 2:53 PM, Paulo Zanoni przan...@gmail.com wrote: 2014-07-17 5:38 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Wed, Jul 16, 2014 at 05:06:34PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Since we merged runtime PM support for DPMS, it is possible that these functions will be called when the power wells are disabled but a mode is set, resulting in failed assertion and device suspended while reading register WARNs. To reproduce the bug: disable all screens using mode unset, do a modeset on one screen, disable it using DPMS, then try to do a mode unset on it again to see the WARNs. Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Hm, where do we call these asserts while the pipe is off? Do you have some example backtraces at hand? Function __intel_set_mode() directly calls intel_crtc_disable(), which calls both assert_plane_disabled() and assert_cursor_disabled(). Hm, I think it makes more sense to drop the three asserts in there. The modeset state checker will already notice when we've failed to turn off the pipe. And we check cursors and plane state in the enable sequence, too. Since we use these asserts a lot to lock down the precise modeset sequence I actually prefer if they're a bit dumb and don't check the power wells. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: check the power wells on assert_{cursor, plane}
2014-07-17 10:23 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Thu, Jul 17, 2014 at 2:53 PM, Paulo Zanoni przan...@gmail.com wrote: 2014-07-17 5:38 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Wed, Jul 16, 2014 at 05:06:34PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Since we merged runtime PM support for DPMS, it is possible that these functions will be called when the power wells are disabled but a mode is set, resulting in failed assertion and device suspended while reading register WARNs. To reproduce the bug: disable all screens using mode unset, do a modeset on one screen, disable it using DPMS, then try to do a mode unset on it again to see the WARNs. Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Hm, where do we call these asserts while the pipe is off? Do you have some example backtraces at hand? Function __intel_set_mode() directly calls intel_crtc_disable(), which calls both assert_plane_disabled() and assert_cursor_disabled(). Hm, I think it makes more sense to drop the three asserts in there. The modeset state checker will already notice when we've failed to turn off the pipe. And we check cursors and plane state in the enable sequence, too. Since we use these asserts a lot to lock down the precise modeset sequence I actually prefer if they're a bit dumb and don't check the power wells. I can do this, but please notice that we already have power-well-checking code in many of the other assertions on our driver... And it will probably be just a matter of time since someone starts using the assertions again on a place where the power well can be disabled :) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs
2014-07-17 5:06 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote: 2014-07-14 14:34 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote: 2014-07-07 18:53 GMT-03:00 Jesse Barnes jbar...@virtuousgeek.org: On Mon, 7 Jul 2014 18:48:47 -0300 Paulo Zanoni przan...@gmail.com wrote: (documenting what we discussed on IRC) 2014-06-20 13:29 GMT-03:00 Jesse Barnes jbar...@virtuousgeek.org: This was always the case on our suspend path, but it was recently exposed by the change to use our runtime IRQ disable routine rather than the full DRM IRQ disable. Keep the warning on the enable side, as that really would indicate a bug. While I understand the patch and think it's a reasonable thing to do, I feel the need to spend some time persuading you in replacing it with something that doesn't involve removing WARNs from our driver. While the driver is runtime suspended, no one should really be manipulating IRQs, even if they're disabling stuff that is already disabled: this reflects there's probably a problem somewhere. These WARNs are extremely helpful for the runtime PM feature because almost nobody actually uses runtime PM to notice any bugs with it, so the WARNs can make QA report bugs and bisect things for us. Also, it seems S3 suspend is currently a little disaster on our driver. Your 6 patches just solve some of the WARNs, not all of them. And last week I even solved another WARN on the S3 path. I just did some investigation, and the current bad commits are: 8abdc17941c71b37311bb93876ac83dce58160c8, e11aa362308f5de467ce355a2a2471321b15a35c and 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3 commits, S3 doesn't give me any WARNs. Instead of the change you proposed, can't we think of another solution that would maybe address all the 3 regressions we have? Since we're always submitting patches to change the order we do things at S3 suspend/resume, shouldn't we add something like dev_priv-suspending that could be used to avoid the strict ordering that is required during runtime? Hm I was running with those changes and didn't see additional warnings, so I'll have to look again. I agree we want sensible warnings in place, and maybe removing this one is too permissive, but I'd like to avoid a suspending flag if we can. Maybe we just need to bundle up our assertions and check all the relevant state after runtime suspend or S3 like we do for mode set state in various places. That would let us keep our warnings, but also save us from having them spread out in code paths that get used in many different contexts. I'm probably going to regret this, but since no one proposed a better patch and the bug is still there: Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com Not really eager to merge a patch soaking in preemptive regrets ... I'll punt on this for now. Possible solutions: 1 - Patch xxx_disable_vblank to do nothing in case dev_priv-pm.suspended 2 - Add a flag dev_priv-suspending and don't print those WARNs in case it is set. 3 - Merge Jesse's original patch 4 - Something else? I can implement any of the proposed solutions if needed... I've gone with 3 for now. It sounds like we need to work with this code a bit more still until the best solution is clear. The patch at least addresses the WARN. Ok, so after I wrote this email, I remembered Ville's [10/24] drm/i915: Leave interrupts enabled while disabling crtcs during suspend, which is part of the watermarks series. Based on that, I produced the following fix that would replace Jesse's current patch series, fixing the bad WARN while keeping it in the places we want: diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 3315358..63675bf 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -520,12 +520,6 @@ static int i915_drm_freeze(struct drm_device *dev) return error; } - flush_delayed_work(dev_priv-rps.delayed_resume_work); - - intel_runtime_pm_disable_interrupts(dev); - - intel_suspend_gt_powersave(dev); - /* * Disable CRTCs directly since we want to preserve sw state * for _thaw. Also, power gate the CRTC power wells. @@ -535,6 +529,11 @@ static int i915_drm_freeze(struct drm_device *dev) intel_crtc_control(crtc, false); drm_modeset_unlock_all(dev); + flush_delayed_work(dev_priv-rps.delayed_resume_work); + intel_runtime_pm_disable_interrupts(dev); + + intel_suspend_gt_powersave(dev); + intel_modeset_suspend_hw(dev); } What do you think? -Daniel -- Daniel
[Intel-gfx] [PATCH] drm/i915: remove plane/cursor/pipe assertions from intel_crtc_disable
From: Paulo Zanoni paulo.r.zan...@intel.com
Since we merged runtime PM support for DPMS, it is possible that these
assertions will be called when the power wells are disabled but a mode
is set, resulting in failed assertion and device suspended while
reading register WARNs.
To reproduce the bug: disable all screens using mode unset, do a
modeset on one screen, disable it using DPMS, then try to do a mode
unset on it again to see the WARNs.
v2: The first version of this patch changed the assertions to also
check the power domains. Daniel suggested that it would be better to
just remove the assertions: The modeset state checker
will already notice when we've failed to turn off the pipe. And we
check cursors and plane state in the enable sequence, too. Since we
use these asserts a lot to lock down the precise modeset sequence I
actually prefer if they're a bit dumb and don't check the power
wells.
Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp
Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index cb40a94..f4ef59e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4930,10 +4930,6 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
intel_crtc_update_sarea(crtc, false);
dev_priv-display.off(crtc);
- assert_plane_disabled(dev-dev_private, to_intel_crtc(crtc)-plane);
- assert_cursor_disabled(dev_priv, pipe);
- assert_pipe_disabled(dev-dev_private, pipe);
-
if (crtc-primary-fb) {
mutex_lock(dev-struct_mutex);
intel_unpin_fb_obj(old_obj);
--
2.0.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC v2 2/3] drm/i915: duplicate i915_gem_ring_dispatch trace and add ctx parameter
On Wed, Jul 16, 2014 at 05:22:38PM +0100, daniele.ceraolospu...@intel.com wrote: From: Daniele Ceraolo Spurio daniele.ceraolospu...@intel.com The context used to execute a batchbuffer is becoming increasingly important. Duplicating to avoid modifications to the original trace. I am sure we don't want both. The structure encoding is exposed to userspace so we are free to update the tracepoints within reason. I would also like a better ctx identifier than its pointer. Using the pointer for tracking objects makes it more difficult to read traces (although it is easy for scripts). -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 1/4] drm/i915: don't warn if IRQs are disabled when shutting down display IRQs
On Thu, Jul 17, 2014 at 3:42 PM, Paulo Zanoni przan...@gmail.com wrote: On Wed, Jul 16, 2014 at 06:19:13PM -0300, Paulo Zanoni wrote: 2014-07-14 14:34 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Mon, Jul 14, 2014 at 11:56:57AM -0300, Paulo Zanoni wrote: 2014-07-07 18:53 GMT-03:00 Jesse Barnes jbar...@virtuousgeek.org: On Mon, 7 Jul 2014 18:48:47 -0300 Paulo Zanoni przan...@gmail.com wrote: (documenting what we discussed on IRC) 2014-06-20 13:29 GMT-03:00 Jesse Barnes jbar...@virtuousgeek.org: This was always the case on our suspend path, but it was recently exposed by the change to use our runtime IRQ disable routine rather than the full DRM IRQ disable. Keep the warning on the enable side, as that really would indicate a bug. While I understand the patch and think it's a reasonable thing to do, I feel the need to spend some time persuading you in replacing it with something that doesn't involve removing WARNs from our driver. While the driver is runtime suspended, no one should really be manipulating IRQs, even if they're disabling stuff that is already disabled: this reflects there's probably a problem somewhere. These WARNs are extremely helpful for the runtime PM feature because almost nobody actually uses runtime PM to notice any bugs with it, so the WARNs can make QA report bugs and bisect things for us. Also, it seems S3 suspend is currently a little disaster on our driver. Your 6 patches just solve some of the WARNs, not all of them. And last week I even solved another WARN on the S3 path. I just did some investigation, and the current bad commits are: 8abdc17941c71b37311bb93876ac83dce58160c8, e11aa362308f5de467ce355a2a2471321b15a35c and 85e90679335f56d162f4a0ff525573818e17ce44. If I just revert these 3 commits, S3 doesn't give me any WARNs. Instead of the change you proposed, can't we think of another solution that would maybe address all the 3 regressions we have? Since we're always submitting patches to change the order we do things at S3 suspend/resume, shouldn't we add something like dev_priv-suspending that could be used to avoid the strict ordering that is required during runtime? Hm I was running with those changes and didn't see additional warnings, so I'll have to look again. I agree we want sensible warnings in place, and maybe removing this one is too permissive, but I'd like to avoid a suspending flag if we can. Maybe we just need to bundle up our assertions and check all the relevant state after runtime suspend or S3 like we do for mode set state in various places. That would let us keep our warnings, but also save us from having them spread out in code paths that get used in many different contexts. I'm probably going to regret this, but since no one proposed a better patch and the bug is still there: Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com Not really eager to merge a patch soaking in preemptive regrets ... I'll punt on this for now. Possible solutions: 1 - Patch xxx_disable_vblank to do nothing in case dev_priv-pm.suspended 2 - Add a flag dev_priv-suspending and don't print those WARNs in case it is set. 3 - Merge Jesse's original patch 4 - Something else? I can implement any of the proposed solutions if needed... I've gone with 3 for now. It sounds like we need to work with this code a bit more still until the best solution is clear. The patch at least addresses the WARN. Ok, so after I wrote this email, I remembered Ville's [10/24] drm/i915: Leave interrupts enabled while disabling crtcs during suspend, which is part of the watermarks series. Based on that, I produced the following fix that would replace Jesse's current patch series, fixing the bad WARN while keeping it in the places we want: diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 3315358..63675bf 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -520,12 +520,6 @@ static int i915_drm_freeze(struct drm_device *dev) return error; } - flush_delayed_work(dev_priv-rps.delayed_resume_work); - - intel_runtime_pm_disable_interrupts(dev); - - intel_suspend_gt_powersave(dev); - /* * Disable CRTCs directly since we want to preserve sw state * for _thaw. Also, power gate the CRTC power wells. @@ -535,6 +529,11 @@ static int i915_drm_freeze(struct drm_device *dev) intel_crtc_control(crtc, false); drm_modeset_unlock_all(dev); + flush_delayed_work(dev_priv-rps.delayed_resume_work); + intel_runtime_pm_disable_interrupts(dev); + + intel_suspend_gt_powersave(dev); + intel_modeset_suspend_hw(dev);
Re: [Intel-gfx] [PATCH] drm/i915: check the power wells on assert_{cursor, plane}
On Thu, Jul 17, 2014 at 3:29 PM, Paulo Zanoni przan...@gmail.com wrote: I can do this, but please notice that we already have power-well-checking code in many of the other assertions on our driver... And it will probably be just a matter of time since someone starts using the assertions again on a place where the power well can be disabled :) The only one I've found outside of the hw state readout code and error capture code (and there we obviously need them) is in assert_pipe. This was added in 693101618a4be. Tbh I wonder whether we could revert that with the patch to ditch the assert from intel_crtc_disable? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: remove plane/cursor/pipe assertions from intel_crtc_disable
On Thu, Jul 17, 2014 at 11:16:39AM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Since we merged runtime PM support for DPMS, it is possible that these assertions will be called when the power wells are disabled but a mode is set, resulting in failed assertion and device suspended while reading register WARNs. To reproduce the bug: disable all screens using mode unset, do a modeset on one screen, disable it using DPMS, then try to do a mode unset on it again to see the WARNs. v2: The first version of this patch changed the assertions to also check the power domains. Daniel suggested that it would be better to just remove the assertions: The modeset state checker will already notice when we've failed to turn off the pipe. And we check cursors and plane state in the enable sequence, too. Since we use these asserts a lot to lock down the precise modeset sequence I actually prefer if they're a bit dumb and don't check the power wells. Testcase: igt/rpm_rpm/dpms-mode-unset-lpsp Testcase: igt/rpm_rpm/dpms-mode-unset-non-lpsp Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [drm:intel_dp_start_link_train] *ERROR* too many full retries, give up and others on an ASUS UX32A
Hi, I have an ASUS UX32A laptop, which have troubles booting up with an external HDMI display. The BIOS boot screen appears on the external display, and the internal one stays blank during the whole boot. After login I can make it work again if I turn it off and on, e.g. with Ubuntu's display setting tool, or with the following commands: xrandr --output eDP1 --off xrandr --output eDP1 --auto xrandr --output eDP1 --right-of HDMI1 If I don't do it and the displays going to sleep mode, I can't wake it up again, unless I restart the machine (but I can switch to console with Ctrl+Alt+Fx). I've tried it with various different kernels, from default 3.2 (I'm using Ubuntu 12.04) to 3.13 (the backport from Trusty). I've tried to search Google for a solution, but none of the relevant tips worked for me. I guess this should be a BIOS problem, as it doesn't turn the display on at the first place. Is there a better solution for this problem than running the above mentioned command during login? Some excerpts from kern.log after a recent boot: ( I can send the whole logs, but I don't want to spam the list with 100k) Jul 16 22:12:21 kernel: [1.560021] [drm] Initialized drm 1.1.0 20060810 Jul 16 22:12:21 kernel: [1.582925] [drm] Memory usable by graphics device = 2048M Jul 16 22:12:21 kernel: [1.582931] fb: conflicting fb hw usage inteldrmfb vs VESA VGA - removing generic driver Jul 16 22:12:21 kernel: [1.666837] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). Jul 16 22:12:21 kernel: [1.666838] [drm] Driver supports precise vblank timestamp query. Jul 16 22:12:21 kernel: [1.671238] [drm:intel_dp_i2c_aux_ch] *ERROR* too many retries, giving up Jul 16 22:12:21 kernel: [1.954512] fbcon: inteldrmfb (fb0) is primary device Jul 16 22:12:21 kernel: [2.124724] [drm:intel_dp_start_link_train] *ERROR* too many full retries, give up Jul 16 22:12:21 kernel: [3.305125] [drm:intel_dp_complete_link_train] *ERROR* failed to train DP, aborting Jul 16 22:12:21 kernel: [3.660395] [drm:cpt_verify_modeset] *ERROR* mode set failed: pipe A stuck Jul 16 22:12:21 kernel: [3.856571] [drm] Enabling RC6 states: RC6 on, RC6p on, RC6pp off Jul 16 22:12:21 kernel: [3.880588] i915 :00:02.0: fb0: inteldrmfb frame buffer device Jul 16 22:12:21 kernel: [3.892846] [drm] Initialized i915 1.6.0 20080730 for :00:02.0 on minor 0 Jul 16 22:12:22 kernel: [5.080210] ACPI Warning: 0x0428-0x042f SystemIO conflicts with Region \GPIS 1 (20131115/utaddress-251) Jul 16 22:12:22 kernel: [5.080218] ACPI Warning: 0x0428-0x042f SystemIO conflicts with Region \PMIO 2 (20131115/utaddress-251) Jul 16 22:12:22 kernel: [5.080223] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver Jul 16 22:12:22 kernel: [5.080229] ACPI Warning: 0x0530-0x053f SystemIO conflicts with Region \GPIO 1 (20131115/utaddress-251) Jul 16 22:12:22 kernel: [5.080233] ACPI Warning: 0x0530-0x053f SystemIO conflicts with Region \GP01 2 (20131115/utaddress-251) Jul 16 22:12:22 kernel: [5.080236] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver Jul 16 22:12:22 kernel: [5.080238] ACPI Warning: 0x0500-0x052f SystemIO conflicts with Region \GPIO 1 (20131115/utaddress-251) Jul 16 22:12:22 kernel: [5.080242] ACPI Warning: 0x0500-0x052f SystemIO conflicts with Region \GP01 2 (20131115/utaddress-251) Jul 16 22:12:22 kernel: [5.080245] ACPI: If an ACPI driver is available for this device, you should use it instead of the native driver Jul 16 22:12:42 kernel: [ 25.002527] WARNING: CPU: 1 PID: 1513 at /build/buildd/linux-lts-trusty-3.13.0/drivers/gpu/drm/i915/intel_display.c:851 intel_wait_for_pipe_off+0x1db/0x1f0 [i915]() Jul 16 22:12:42 kernel: [ 25.002529] Modules linked in: xts gf128mul pci_stub vboxpci(OF) vboxnetadp(OF) vboxnetflt(OF) vboxdrv(OF) dm_crypt xt_hl ip6t_rt arc4 nf_conntrack_ipv6 iwldvm nf_defrag_ipv6 ipt_REJECT mac80211 xt_LOG uvcvideo snd_hda_codec_hdmi xt_limit videobuf2_core rts5139(C) snd_hda_codec_realtek xt_tcpudp videodev xt_addrtype videobuf2_vmalloc snd_hda_intel iwlwifi snd_hda_codec videobuf2_memops nf_conntrack_ipv4 nf_defrag_ipv4 xt_state snd_hwdep joydev snd_pcm cfg80211 ip6table_filter btusb ip6_tables snd_seq_midi snd_rawmidi nf_conntrack_netbios_ns snd_seq_midi_event nf_conntrack_broadcast nf_nat_ftp nf_nat snd_seq nf_conntrack_ftp nf_conntrack asus_nb_wmi snd_timer asus_wmi iptable_filter snd_seq_device sparse_keymap ip_tables x_tables snd soundcore snd_page_alloc lpc_ich mei_me psmouse mei bnep serio_raw rfcomm bluetooth parport_pc mac_hid ppdev nls_utf8 cifs fscache binfmt_misc lp parport hid_generic usbhid hid wmi i915 drm_kms_helper ahci libahci drm i2c_algo_bit video
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
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 removed. The MCH PCI
registers that need to be read, we are working with HW team to get them
mirrored in GPU MMIO registers in future HW.
Allen
-Original Message-
From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
Michael S. Tsirkin
Sent: Thursday, July 03, 2014 1:28 PM
To: Jesse Barnes
Cc: peter.mayd...@linaro.org; xen-de...@lists.xensource.com; Ross Philipson;
Konrad Rzeszutek Wilk; 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 03, 2014 at 12:09:28PM -0700, Jesse Barnes wrote:
On Thu, 3 Jul 2014 14:26:12 -0400
Konrad Rzeszutek Wilk konrad.w...@oracle.com wrote:
On Thu, Jul 03, 2014 at 10:32:12AM +0300, Michael S. Tsirkin wrote:
On Wed, Jul 02, 2014 at 12:23:37PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Jul 02, 2014 at 04:50:15PM +0200, Paolo Bonzini wrote:
Il 02/07/2014 16:00, Konrad Rzeszutek Wilk ha scritto:
With this long thread I lost a bit context about the
challenges that exists. But let me try summarizing it here -
which will hopefully get some consensus.
1). Fix IGD hardware to not use Southbridge magic addresses.
We can moan and moan but I doubt it is going to change.
There are two problems:
- Northbridge (i.e. MCH i.e. PCI host bridge) configuration
space addresses
Right. So in drivers/gpu/drm/i915/i915_dma.c:
1135 #define MCHBAR_I915 0x44
1136 #define MCHBAR_I965 0x48
1147 int reg = INTEL_INFO(dev)-gen = 4 ? MCHBAR_I965 :
MCHBAR_I915;
1152 if (INTEL_INFO(dev)-gen = 4)
1153 pci_read_config_dword(dev_priv-bridge_dev, reg +
4, temp_hi);
1154 pci_read_config_dword(dev_priv-bridge_dev, reg,
temp_lo);
1155 mchbar_addr = ((u64)temp_hi 32) | temp_lo;
and
1139 #define DEVEN_REG 0x54
1193 int mchbar_reg = INTEL_INFO(dev)-gen = 4 ? MCHBAR_I965 :
MCHBAR_I915;
1202 if (IS_I915G(dev) || IS_I915GM(dev)) {
1203 pci_read_config_dword(dev_priv-bridge_dev,
DEVEN_REG, temp);
1204 enabled = !!(temp DEVEN_MCHBAR_EN);
1205 } else {
1206 pci_read_config_dword(dev_priv-bridge_dev,
mchbar_reg, temp);
1207 enabled = temp 1;
1208 }
- Southbridge (i.e. PCH i.e. ISA bridge) vendor/device ID;
some versions of the driver identify it by class, some versions
identify it by slot (1f.0).
Right, So in drivers/gpu/drm/i915/i915_drv.c the giant
intel_detect_pch which sets the pch_type based on :
432 if (pch-vendor == PCI_VENDOR_ID_INTEL) {
433 unsigned short id = pch-device
INTEL_PCH_DEVICE_ID_MASK;
434 dev_priv-pch_id = id;
435
436 if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
It checks for 0x3b00, 0x1c00, 0x1e00, 0x8c00 and 0x9c00.
The INTEL_PCH_DEVICE_ID_MASK is 0xff00
To solve the first, make a new machine type, PIIX4-based, and
pass through the registers you need. The patch must document
_exactly_ why the registers are safe to pass. If they are not
reserved on PIIX4, the patch must document what the same
offsets mean on PIIX4, and why it's sensible to assume that
firmware for virtual machine will not read/write them. Bonus point
for also documenting the same for Q35.
OK.
[Intel-gfx] [PATCH] drm/i915: Fix semaphore_seqno and semaphore_mboxes sizes
Otherwise some iteractions depending on the current number of active rings
could overflow.
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2b8308d..190f0e5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -342,7 +342,7 @@ struct drm_i915_error_state {
u32 cpu_ring_head;
u32 cpu_ring_tail;
- u32 semaphore_seqno[I915_NUM_RINGS - 1];
+ u32 semaphore_seqno[I915_NUM_RINGS];
/* Register state */
u32 tail;
@@ -361,7 +361,7 @@ struct drm_i915_error_state {
u32 fault_reg;
u64 faddr;
u32 rc_psmi; /* sleep state */
- u32 semaphore_mboxes[I915_NUM_RINGS - 1];
+ u32 semaphore_mboxes[I915_NUM_RINGS];
struct drm_i915_error_object {
int page_count;
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix semaphore_seqno and semaphore_mboxes sizes
just ignore this one..
On Thu, Jul 17, 2014 at 5:00 AM, Rodrigo Vivi rodrigo.v...@intel.com
wrote:
Otherwise some iteractions depending on the current number of active rings
could overflow.
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 2b8308d..190f0e5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -342,7 +342,7 @@ struct drm_i915_error_state {
u32 cpu_ring_head;
u32 cpu_ring_tail;
- u32 semaphore_seqno[I915_NUM_RINGS - 1];
+ u32 semaphore_seqno[I915_NUM_RINGS];
/* Register state */
u32 tail;
@@ -361,7 +361,7 @@ struct drm_i915_error_state {
u32 fault_reg;
u64 faddr;
u32 rc_psmi; /* sleep state */
- u32 semaphore_mboxes[I915_NUM_RINGS - 1];
+ u32 semaphore_mboxes[I915_NUM_RINGS];
struct drm_i915_error_object {
int page_count;
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: check the power wells on assert_{cursor, plane}
2014-07-17 13:58 GMT-03:00 Daniel Vetter dan...@ffwll.ch: On Thu, Jul 17, 2014 at 3:29 PM, Paulo Zanoni przan...@gmail.com wrote: I can do this, but please notice that we already have power-well-checking code in many of the other assertions on our driver... And it will probably be just a matter of time since someone starts using the assertions again on a place where the power well can be disabled :) The only one I've found outside of the hw state readout code and error capture code (and there we obviously need them) is in assert_pipe. This was added in 693101618a4be. Tbh I wonder whether we could revert that with the patch to ditch the assert from intel_crtc_disable? I was talking about all that hw state readout code (which are also used as assertions) and basically every single caller of intel_display_power_enabled(). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Fix possible overflow when recording semaphore states.
semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
This optimization is to remove the ring itself from the list and the logic to
do that
is at intel_ring_sync_index as below:
/*
* rcs - 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
* vcs - 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
* bcs - 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
* vecs - 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
* vcs2 - 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
*/
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_gpu_error.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9faebbc..36a7960 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct
drm_i915_private *dev_priv,
struct intel_engine_cs *ring,
struct drm_i915_error_ring *ering)
{
- struct intel_engine_cs *useless;
+ struct intel_engine_cs *to;
int i;
if (!i915_semaphore_is_enabled(dev_priv-dev))
@@ -776,13 +776,14 @@ static void gen8_record_semaphore_state(struct
drm_i915_private *dev_priv,
dev_priv-semaphore_obj,
dev_priv-gtt.base);
- for_each_ring(useless, dev_priv, i) {
+ for_each_ring(to, dev_priv, i) {
u16 signal_offset =
(GEN8_SIGNAL_OFFSET(ring, i) PAGE_MASK) / 4;
u32 *tmp = error-semaphore_obj-pages[0];
+ int idx = intel_ring_sync_index(ring, to);
- ering-semaphore_mboxes[i] = tmp[signal_offset];
- ering-semaphore_seqno[i] = ring-semaphore.sync_seqno[i];
+ ering-semaphore_mboxes[idx] = tmp[signal_offset];
+ ering-semaphore_seqno[idx] = ring-semaphore.sync_seqno[idx];
}
}
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: freeze display before the interrupts and GT
From: Paulo Zanoni paulo.r.zan...@intel.com
Since we started using intel_runtime_pm_disable_interrupts() at normal
(non-runtime) suspend/resume, we had to remove a WARN from
ironlake_disable_display_irq to avoid a case where we were doing the
correct thing and the WARN was not really needed. The problem is that
the WARN was useful in other cases, and its removal can hide some bugs
that we would catch automatically.
To be able to add back the WARN, we have to call intel_crtc_control()
before interrupts are disabled, which is what this patch currently
does.
Also notice that Ville's patch from the Watermarks series drm/i915:
Leave interrupts enabled while disabling crtcs during suspend also
did a change that's equivalent to the one we're doing on this patch,
with the exception that its original patch, when applied to the
current tree, procduces a WARN.
Related commits:
commit daa390e5ee45cc051d6bf37b296901f2f92b002d
Author: Jesse Barnes jbar...@virtuousgeek.org
drm/i915: don't warn if IRQs are disabled when shutting down display IRQs
commit e11aa362308f5de467ce355a2a2471321b15a35c
Author: Jesse Barnes jbar...@virtuousgeek.org
drm/i915: use runtime irq suspend/resume in freeze/thaw
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Jesse Barnes jbar...@virtuousgeek.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 11 +--
drivers/gpu/drm/i915/i915_irq.c | 2 +-
2 files changed, 6 insertions(+), 7 deletions(-)
No need to revert anything :)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3315358..63675bf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -520,12 +520,6 @@ static int i915_drm_freeze(struct drm_device *dev)
return error;
}
- flush_delayed_work(dev_priv-rps.delayed_resume_work);
-
- intel_runtime_pm_disable_interrupts(dev);
-
- intel_suspend_gt_powersave(dev);
-
/*
* Disable CRTCs directly since we want to preserve sw state
* for _thaw. Also, power gate the CRTC power wells.
@@ -535,6 +529,11 @@ static int i915_drm_freeze(struct drm_device *dev)
intel_crtc_control(crtc, false);
drm_modeset_unlock_all(dev);
+ flush_delayed_work(dev_priv-rps.delayed_resume_work);
+ intel_runtime_pm_disable_interrupts(dev);
+
+ intel_suspend_gt_powersave(dev);
+
intel_modeset_suspend_hw(dev);
}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 20c777c..af231ba 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -151,7 +151,7 @@ ironlake_disable_display_irq(struct drm_i915_private
*dev_priv, u32 mask)
{
assert_spin_locked(dev_priv-irq_lock);
- if (!intel_irqs_enabled(dev_priv))
+ if (WARN_ON(!intel_irqs_enabled(dev_priv)))
return;
if ((dev_priv-irq_mask mask) != mask) {
--
2.0.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] i915 / 3.15 intermittently boots into blank screeen
Hi list, you were very helpful to nail down the interrupt storm issue a year ago, I hope we can track this down, too. I'm experiencing an issue with 3.15.5 on my Lenovo T400: When my machine boots, I can see some of the boot messages flying by (quite fast because the machine has a SSD). On a usual boot, the screen would go blank with the backlight off, come back on and other boot messages are visible in a different resolution. Finally the screen flickers once and shows the GDM login. Since 3.15 (or 3.14, can't say for sure), the boot starts normally, but the first mode change doesn't occur, resulting in a black screen with backlight on. The system is entirely unresponsive and I can only press the power button until to switch it off. This happens with my custom built kernel 3.15.5 but not with the Debian standard kernel 3.2.0 and IIRC neither with my custom built 3.14 kernels. Would a drm debug log help and if so, what kernel parameters should I use to generate it? System environment Lenovo T400, GM45 Express Chipset, custom-built kernel 3.15.5 on Debian 7.5 /32bit, built-in LCD, issue happens 3 boots out of 4. Best regards Jan ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix possible overflow when recording semaphore states.
On Thu, Jul 17, 2014 at 05:35:20AM -0700, Rodrigo Vivi wrote:
semaphore _sync_seqno, _seqno and _mbox are smaller than number of rings.
This optimization is to remove the ring itself from the list and the logic to
do that
is at intel_ring_sync_index as below:
/*
* rcs - 0 = vcs, 1 = bcs, 2 = vecs, 3 = vcs2;
* vcs - 0 = bcs, 1 = vecs, 2 = vcs2, 3 = rcs;
* bcs - 0 = vecs, 1 = vcs2. 2 = rcs, 3 = vcs;
* vecs - 0 = vcs2, 1 = rcs, 2 = vcs, 3 = bcs;
* vcs2 - 0 = rcs, 1 = vcs, 2 = bcs, 3 = vecs;
*/
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_gpu_error.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 9faebbc..36a7960 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -764,7 +764,7 @@ static void gen8_record_semaphore_state(struct
drm_i915_private *dev_priv,
struct intel_engine_cs *ring,
struct drm_i915_error_ring *ering)
{
- struct intel_engine_cs *useless;
+ struct intel_engine_cs *to;
int i;
if (!i915_semaphore_is_enabled(dev_priv-dev))
@@ -776,13 +776,14 @@ static void gen8_record_semaphore_state(struct
drm_i915_private *dev_priv,
dev_priv-semaphore_obj,
dev_priv-gtt.base);
- for_each_ring(useless, dev_priv, i) {
+ for_each_ring(to, dev_priv, i) {
u16 signal_offset =
(GEN8_SIGNAL_OFFSET(ring, i) PAGE_MASK) / 4;
u32 *tmp = error-semaphore_obj-pages[0];
+ int idx = intel_ring_sync_index(ring, to);
- ering-semaphore_mboxes[i] = tmp[signal_offset];
- ering-semaphore_seqno[i] = ring-semaphore.sync_seqno[i];
+ ering-semaphore_mboxes[idx] = tmp[signal_offset];
+ ering-semaphore_seqno[idx] = ring-semaphore.sync_seqno[idx];
}
}
Actually, this patch does the opposite of my original intent. Since the
semaphore object should have the proper spacing, my original intent was
what your first patch was: Fix semaphore_seqno and semaphore_mboxes
sizes (with probably a comment in the error state why we do this).
That patch is simpler and gives potentially more useful information
(like if we write to the wrong offset in the semaphore page, we won't
see it here). UNFORTUNATELY it does not seem to correspond with how we
actually print out the error state. So I think unless you want to fix up
the other spots, your other patch is incorrect.
This patch looks correct to me, and should fix the bug with the least
amount of churn. In addition, assuming we have no bugs, it shows things
more concisely in the error state. Perhaps just add my concern about
missing capture certain offsets within the semaphore object in the
commit message and call it good.
Reviewed-by: Ben Widawsky b...@bwidawsk.net
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Optimize the i915_gem_gtt_finish_object function
On Fri, Jul 11, 2014 at 10:20:08AM -0700, armin.c.re...@intel.com wrote:
From: Armin Reese armin.c.re...@intel.com
Signed-off-by: Armin Reese armin.c.re...@intel.com
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index afd4eef..7e2190e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1624,18 +1624,17 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
{
- struct drm_device *dev = obj-base.dev;
- struct drm_i915_private *dev_priv = dev-dev_private;
- bool interruptible;
-
- interruptible = do_idling(dev_priv);
+ if (!obj-has_dma_mapping) {
+ struct drm_device *dev = obj-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ bool interruptible = do_idling(dev_priv);
- if (!obj-has_dma_mapping)
dma_unmap_sg(dev-pdev-dev,
obj-pages-sgl, obj-pages-nents,
PCI_DMA_BIDIRECTIONAL);
- undo_idling(dev_priv, interruptible);
+ undo_idling(dev_priv, interruptible);
+ }
}
static void i915_gtt_color_adjust(struct drm_mm_node *node,
Note that this doesn't do much on platforms you care about.
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: print full error ring semaphore mboxes and sync.
With the increasing number of rings,
we probably have more information to print than we were printing.
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_gpu_error.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 36a7960..0beeebf 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -242,6 +242,8 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
struct drm_device *dev,
struct drm_i915_error_ring *ring)
{
+ int i;
+
if (!ring-valid)
return;
@@ -264,23 +266,15 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
if (INTEL_INFO(dev)-gen = 6) {
err_printf(m, RC PSMI: 0x%08x\n, ring-rc_psmi);
err_printf(m, FAULT_REG: 0x%08x\n, ring-fault_reg);
- err_printf(m, SYNC_0: 0x%08x [last synced 0x%08x]\n,
- ring-semaphore_mboxes[0],
- ring-semaphore_seqno[0]);
- err_printf(m, SYNC_1: 0x%08x [last synced 0x%08x]\n,
- ring-semaphore_mboxes[1],
- ring-semaphore_seqno[1]);
- if (HAS_VEBOX(dev)) {
- err_printf(m, SYNC_2: 0x%08x [last synced 0x%08x]\n,
- ring-semaphore_mboxes[2],
- ring-semaphore_seqno[2]);
+ for (i = 0; i I915_NUM_RINGS - 1; i++) {
+ err_printf(m, SYNC_%d: 0x%08x [last synced
0x%08x]\n,
+ i, ring-semaphore_mboxes[i],
+ ring-semaphore_seqno[i]);
}
}
if (USES_PPGTT(dev)) {
err_printf(m, GFX_MODE: 0x%08x\n, ring-vm_info.gfx_mode);
-
if (INTEL_INFO(dev)-gen = 8) {
- int i;
for (i = 0; i 4; i++)
err_printf(m, PDP%d: 0x%016llx\n,
i, ring-vm_info.pdp[i]);
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Do not unmap object unless no other VMAs reference it
On Fri, Jul 11, 2014 at 10:20:07AM -0700, armin.c.re...@intel.com wrote:
From: Armin Reese armin.c.re...@intel.com
Signed-off-by: Armin Reese armin.c.re...@intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 87d0aac..676e5f4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2922,8 +2922,6 @@ int i915_vma_unbind(struct i915_vma *vma)
vma-unbind_vma(vma);
- i915_gem_gtt_finish_object(obj);
-
list_del_init(vma-mm_list);
/* Avoid an unnecessary call to unbind on rebind. */
if (i915_is_ggtt(vma-vm))
@@ -2934,8 +2932,10 @@ int i915_vma_unbind(struct i915_vma *vma)
/* Since the unbound list is global, only move to that list if
* no more VMAs exist. */
- if (list_empty(obj-vma_list))
+ if (list_empty(obj-vma_list)) {
+ i915_gem_gtt_finish_object(obj);
list_move_tail(obj-global_list, dev_priv-mm.unbound_list);
+ }
/* And finally now the object is completely decoupled from this vma,
* we can drop its hold on the backing storage and allow it to be
This patch is a fix, and therefore needs a commit message to explain the
bug that you're trying to fix. I will help.
When using an IOMMU, GEM objects are mapped by their DMA address as the
physical address is unknown. This depends on the underlying IOMMU
driver to map and unmap the physical pages properly as defined in
intel_iommu.c.
The current code will tell the IOMMU to unmap the GEM BO's pages on the
destruction of the first VMA that maps that BO. This is clearly wrong
as there may be other VMAs mapping that BO (using flink). The scanout
is one such example.
The patch fixes this issue by only unmapping the DMA maps when there are
no more VMAs mapping that object. This is equivalent to when an object
is considered unbound as can be seen by the code. On the first VMA that
again because bound, we will remap.
An alternate solution would be to move the dma mapping to object
creation and destrubtion. I am not sure if this is considered an
unfriendly thing to do.
Some notes to backporters:
The bug can never be hit without enabling the IOMMU. The existing code
will also do the right thing when the object is shared via dmabuf. The
failure should be demonstrable with flink. In cases when not using
intel_iommu_strict it is likely (likely, as defined by: off the top of
my head) on current workloads to *not* hit this bug since we often
teardown all VMAs for an object shared across multiple VMs. We also
finish access to that object before the first dma_unmapping.
intel_iommu_strict with flinked buffers is likely to hit this issue.
As a side note:
I was expecting this to be fixed as part of Daniels lifetime tracking
plans. I had imagined that would have a handler run when refcount hits 0
that can do the dma_unmap. Therefore I assume everything in that
list_empty() condition is a temporary hack.
This patch is:
Reviewed-by: Ben Widawsky b...@bwidawsk.net
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Do not unmap object unless no other VMAs reference it
On Thu, Jul 17, 2014 at 04:45:02PM -0700, Ben Widawsky wrote:
On Fri, Jul 11, 2014 at 10:20:07AM -0700, armin.c.re...@intel.com wrote:
From: Armin Reese armin.c.re...@intel.com
Signed-off-by: Armin Reese armin.c.re...@intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index 87d0aac..676e5f4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2922,8 +2922,6 @@ int i915_vma_unbind(struct i915_vma *vma)
vma-unbind_vma(vma);
- i915_gem_gtt_finish_object(obj);
-
list_del_init(vma-mm_list);
/* Avoid an unnecessary call to unbind on rebind. */
if (i915_is_ggtt(vma-vm))
@@ -2934,8 +2932,10 @@ int i915_vma_unbind(struct i915_vma *vma)
/* Since the unbound list is global, only move to that list if
* no more VMAs exist. */
- if (list_empty(obj-vma_list))
+ if (list_empty(obj-vma_list)) {
+ i915_gem_gtt_finish_object(obj);
list_move_tail(obj-global_list, dev_priv-mm.unbound_list);
+ }
/* And finally now the object is completely decoupled from this vma,
* we can drop its hold on the backing storage and allow it to be
This patch is a fix, and therefore needs a commit message to explain the
bug that you're trying to fix. I will help.
When using an IOMMU, GEM objects are mapped by their DMA address as the
physical address is unknown. This depends on the underlying IOMMU
driver to map and unmap the physical pages properly as defined in
intel_iommu.c.
The current code will tell the IOMMU to unmap the GEM BO's pages on the
destruction of the first VMA that maps that BO. This is clearly wrong
as there may be other VMAs mapping that BO (using flink). The scanout
is one such example.
The patch fixes this issue by only unmapping the DMA maps when there are
no more VMAs mapping that object. This is equivalent to when an object
is considered unbound as can be seen by the code. On the first VMA that
again because bound, we will remap.
An alternate solution would be to move the dma mapping to object
creation and destrubtion. I am not sure if this is considered an
unfriendly thing to do.
Some notes to backporters:
The bug can never be hit without enabling the IOMMU. The existing code
will also do the right thing when the object is shared via dmabuf. The
failure should be demonstrable with flink. In cases when not using
intel_iommu_strict it is likely (likely, as defined by: off the top of
my head) on current workloads to *not* hit this bug since we often
teardown all VMAs for an object shared across multiple VMs. We also
finish access to that object before the first dma_unmapping.
intel_iommu_strict with flinked buffers is likely to hit this issue.
Crap. I left out the important part. Note to backporters trying to
backport full PPGTT, which is disabled by default on all kernels to
date.
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: print full error ring semaphore mboxes and sync.
On Thu, Jul 17, 2014 at 09:39:55AM -0700, Rodrigo Vivi wrote: With the increasing number of rings, we probably have more information to print than we were printing. After our discussion were you going to send a new patch? [snip] -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: print full error ring semaphore mboxes and sync.
You mean you prefer the the loop with for (i = 0; i hweight(ring_mask); i++)
{
instead? I thought you were ok with either and I preferred this one just to be
on the safest side and let userspace parse it properly.
Or do you prefer that other version with double loop but with names of rings?
Thanks,
Rodrigo.
-Original Message-
From: Widawsky, Benjamin
Sent: Thursday, July 17, 2014 5:23 PM
To: Vivi, Rodrigo
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: print full error ring semaphore mboxes and sync.
On Thu, Jul 17, 2014 at 09:39:55AM -0700, Rodrigo Vivi wrote:
With the increasing number of rings,
we probably have more information to print than we were printing.
After our discussion were you going to send a new patch?
[snip]
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: print full error ring semaphore mboxes and sync.
On Thu, Jul 17, 2014 at 05:36:52PM -0700, Vivi, Rodrigo wrote:
You mean you prefer the the loop with for (i = 0; i hweight(ring_mask);
i++) {
instead? I thought you were ok with either and I preferred this one just to
be on the safest side and let userspace parse it properly.
I can live with either. I guess it's a little more obvious if we only
capture the relevant data on platforms that have actual registers. gen8
is special IMO here because we're writing to memory. But either way this
is an improvement.
Or do you prefer that other version with double loop but with names of rings?
I think it would be cool if we could have it as such, but I am not sure
what others think:
SYNC[RCS-VCS]: 0x%08x
But whatever.
Anyway, I was just asking because I wasn't sure if I should wait for
another patch.
Thanks,
Rodrigo.
-Original Message-
From: Widawsky, Benjamin
Sent: Thursday, July 17, 2014 5:23 PM
To: Vivi, Rodrigo
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: print full error ring semaphore mboxes and
sync.
On Thu, Jul 17, 2014 at 09:39:55AM -0700, Rodrigo Vivi wrote:
With the increasing number of rings,
we probably have more information to print than we were printing.
After our discussion were you going to send a new patch?
[snip]
--
Ben Widawsky, Intel Open Source Technology Center
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: print full error ring semaphore mboxes and sync.
With the increasing number of rings,
we probably have more information to print than we were printing.
v2: Loop only over active rings and print info with ring names.
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_gpu_error.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 36a7960..b1848e0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -242,6 +242,10 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
struct drm_device *dev,
struct drm_i915_error_ring *ring)
{
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct intel_engine_cs *from, *to;
+ int i, j;
+
if (!ring-valid)
return;
@@ -264,23 +268,19 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
if (INTEL_INFO(dev)-gen = 6) {
err_printf(m, RC PSMI: 0x%08x\n, ring-rc_psmi);
err_printf(m, FAULT_REG: 0x%08x\n, ring-fault_reg);
- err_printf(m, SYNC_0: 0x%08x [last synced 0x%08x]\n,
- ring-semaphore_mboxes[0],
- ring-semaphore_seqno[0]);
- err_printf(m, SYNC_1: 0x%08x [last synced 0x%08x]\n,
- ring-semaphore_mboxes[1],
- ring-semaphore_seqno[1]);
- if (HAS_VEBOX(dev)) {
- err_printf(m, SYNC_2: 0x%08x [last synced 0x%08x]\n,
- ring-semaphore_mboxes[2],
- ring-semaphore_seqno[2]);
+ for_each_ring(from, dev_priv, i) {
+ for_each_ring(to, dev_priv, j) {
+ int idx = intel_ring_sync_index(from, to);
+ err_printf(m, SYNC[%s - %s]: 0x%08x [last
synced 0x%08x]\n,
+ from-name, to-name,
+ ring-semaphore_mboxes[idx],
+ ring-semaphore_seqno[idx]);
+ }
}
}
if (USES_PPGTT(dev)) {
err_printf(m, GFX_MODE: 0x%08x\n, ring-vm_info.gfx_mode);
-
if (INTEL_INFO(dev)-gen = 8) {
- int i;
for (i = 0; i 4; i++)
err_printf(m, PDP%d: 0x%016llx\n,
i, ring-vm_info.pdp[i]);
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: print full error ring semaphore mboxes and sync.
On Thu, Jul 17, 2014 at 10:58:17AM -0700, Rodrigo Vivi wrote:
With the increasing number of rings,
we probably have more information to print than we were printing.
v2: Loop only over active rings and print info with ring names.
Cc: Ben Widawsky benjamin.widaw...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_gpu_error.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 36a7960..b1848e0 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -242,6 +242,10 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
struct drm_device *dev,
struct drm_i915_error_ring *ring)
{
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct intel_engine_cs *from, *to;
+ int i, j;
+
if (!ring-valid)
return;
@@ -264,23 +268,19 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
if (INTEL_INFO(dev)-gen = 6) {
err_printf(m, RC PSMI: 0x%08x\n, ring-rc_psmi);
err_printf(m, FAULT_REG: 0x%08x\n, ring-fault_reg);
- err_printf(m, SYNC_0: 0x%08x [last synced 0x%08x]\n,
-ring-semaphore_mboxes[0],
-ring-semaphore_seqno[0]);
- err_printf(m, SYNC_1: 0x%08x [last synced 0x%08x]\n,
-ring-semaphore_mboxes[1],
-ring-semaphore_seqno[1]);
- if (HAS_VEBOX(dev)) {
- err_printf(m, SYNC_2: 0x%08x [last synced 0x%08x]\n,
-ring-semaphore_mboxes[2],
-ring-semaphore_seqno[2]);
+ for_each_ring(from, dev_priv, i) {
+ for_each_ring(to, dev_priv, j) {
+ int idx = intel_ring_sync_index(from, to);
If you plan to get the gen8 gaps in the object:
if (i == j !IS_GEN8())
continue;
If not, with the existing code:
if (i == j)
continue;
Currently, I am also in favor of
for(i = 0; i NUM_RINGS - 1; i++) {
if (!(INTEL_INFO(dev)-ring_mask (1i)))
continue;
for(j = 0; j NUM_RINGS - 1; j++) {
if (!(INTEL_INFO(dev)-ring_mask (ji)))
continue;
err_printf(...)
}
}
or use if (ring_is_initialied()) instead of checking ring mask. For
error state, I prefer to use the mask because who knows what has
happened to the rings on reset or something.
Whatever you like. Fix the if (i == j) and lgtm
+ err_printf(m, SYNC[%s - %s]: 0x%08x [last
synced 0x%08x]\n,
+from-name, to-name,
+ring-semaphore_mboxes[idx],
+ring-semaphore_seqno[idx]);
+ }
}
}
if (USES_PPGTT(dev)) {
err_printf(m, GFX_MODE: 0x%08x\n, ring-vm_info.gfx_mode);
-
Should probably remove this if you do a respin.
if (INTEL_INFO(dev)-gen = 8) {
- int i;
for (i = 0; i 4; i++)
err_printf(m, PDP%d: 0x%016llx\n,
i, ring-vm_info.pdp[i]);
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix printing proper min/min/rpe values in debugfs
Oops, meant to reply-to-all On Thu, Jul 17, 2014 at 07:44:25PM -0700, Ben Widawsky wrote: On Thu, Jul 17, 2014 at 10:42:20AM +0200, Daniel Vetter wrote: On Thu, Jul 17, 2014 at 02:21:14PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com This was fumbled while trying to use the cached min/min/rpe values in the vlv debugfs code. This is a regression from commit 03af20458a57a50735b12c1e3c23abc7ff70c6fa Author: Ville Syrjälä ville.syrj...@linux.intel.com Date: Sat Jun 28 02:03:53 2014 +0300 drm/i915: Use the cached min/min/rpe values in the vlv debugfs code Signed-off-by: Deepak S deepa...@linux.intel.com Queued for -next, thanks for the patch. -Daniel Patch looks fine to me, and merge is fine, though I'd really prefer we stop improving the debugfs interfaces when the sysfs interface now exists for many releases, and are stable. If something here is not in sysfs, it should be. /rant --- drivers/gpu/drm/i915/i915_debugfs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index fc39610..5bc65af 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1116,13 +1116,13 @@ static int i915_frequency_info(struct seq_file *m, void *unused) seq_printf(m, DDR freq: %d MHz\n, dev_priv-mem_freq); seq_printf(m, max GPU freq: %d MHz\n, -dev_priv-rps.max_freq); +vlv_gpu_freq(dev_priv, dev_priv-rps.max_freq)); seq_printf(m, min GPU freq: %d MHz\n, -dev_priv-rps.min_freq); +vlv_gpu_freq(dev_priv, dev_priv-rps.min_freq)); seq_printf(m, efficient (RPe) frequency: %d MHz\n, -dev_priv-rps.efficient_freq); +vlv_gpu_freq(dev_priv, dev_priv-rps.efficient_freq)); seq_printf(m, current GPU freq: %d MHz\n, vlv_gpu_freq(dev_priv, (freq_sts 8) 0xff)); -- 1.9.1 -- 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 -- Ben Widawsky, Intel Open Source Technology Center -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/7] drm/i915: Allowing changing of wm latencies for valid platforms
From: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Sonika Jindal sonika.jin...@intel.com
---
drivers/gpu/drm/i915/i915_debugfs.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index fc39610..ffb83e0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3267,7 +3267,7 @@ static int pri_wm_latency_open(struct inode *inode,
struct file *file)
{
struct drm_device *dev = inode-i_private;
- if (!HAS_PCH_SPLIT(dev))
+ if (INTEL_INFO(dev)-gen = 5 || IS_VALLEYVIEW(dev))
return -ENODEV;
return single_open(file, pri_wm_latency_show, dev);
@@ -3277,7 +3277,7 @@ static int spr_wm_latency_open(struct inode *inode,
struct file *file)
{
struct drm_device *dev = inode-i_private;
- if (!HAS_PCH_SPLIT(dev))
+ if (INTEL_INFO(dev)-gen = 5 || IS_VALLEYVIEW(dev))
return -ENODEV;
return single_open(file, spr_wm_latency_show, dev);
@@ -3287,7 +3287,7 @@ static int cur_wm_latency_open(struct inode *inode,
struct file *file)
{
struct drm_device *dev = inode-i_private;
- if (!HAS_PCH_SPLIT(dev))
+ if (INTEL_INFO(dev)-gen = 5 || IS_VALLEYVIEW(dev))
return -ENODEV;
return single_open(file, cur_wm_latency_show, dev);
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/7] drm/i915: Replace HAS_PCH_SPLIT which incorrectly lets some platforms in
From: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Sonika Jindal sonika.jin...@intel.com
---
drivers/gpu/drm/i915/intel_hdmi.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
b/drivers/gpu/drm/i915/intel_hdmi.c
index 5f8f4ca..2e70132 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1562,7 +1562,7 @@ void intel_hdmi_init_connector(struct intel_digital_port
*intel_dig_port,
if (IS_VALLEYVIEW(dev)) {
intel_hdmi-write_infoframe = vlv_write_infoframe;
intel_hdmi-set_infoframes = vlv_set_infoframes;
- } else if (!HAS_PCH_SPLIT(dev)) {
+ } else if (INTEL_INFO(dev)-gen = 5) {
intel_hdmi-write_infoframe = g4x_write_infoframe;
intel_hdmi-set_infoframes = g4x_set_infoframes;
} else if (HAS_DDI(dev)) {
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/7] drm/i915: Writing proper check for reading of pipe status reg
From: Sonika Jindal sonika.jin...@intel.com Signed-off-by: Sonika Jindal sonika.jin...@intel.com --- drivers/gpu/drm/i915/intel_display.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e259c41..3f7089c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13244,7 +13244,7 @@ intel_display_capture_error_state(struct drm_device *dev) error-pipe[i].source = I915_READ(PIPESRC(i)); - if (!HAS_PCH_SPLIT(dev)) + if (INTEL_INFO(dev)-gen = 5 || IS_VALLEYVIEW(dev)) error-pipe[i].stat = I915_READ(PIPESTAT(i)); } -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/7] Future preparation patches
From: Sonika Jindal sonika.jin...@intel.com This series prepares future platform enabling by changing HAS_PCH_SPLIT to more appropriate check since the code accessed may not have anything to do with having PCH or not. Sonika Jindal (7): drm/i915: Allowing changing of wm latencies for other valid platforms drm/i915: Returning the right VGA control reg for other platforms drm/i915: Setting legacy palette correctly for different platforms drm/i915: Returning from increase/decrease of pllclock when invalid drm/i915: Writing proper check for reading of pipe status reg drm/i915: Replace HAS_PCH_SPLIT which incorrectly lets some platforms in drm/i915: Avoid incorrect returning for some platforms drivers/gpu/drm/i915/i915_debugfs.c |6 +++--- drivers/gpu/drm/i915/i915_drv.h |6 +++--- drivers/gpu/drm/i915/intel_display.c |8 drivers/gpu/drm/i915/intel_hdmi.c|4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/7] drm/i915: Returning the right VGA control reg for platforms
From: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Sonika Jindal sonika.jin...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 991b663..90a682c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2800,10 +2800,10 @@ int vlv_freq_opcode(struct drm_i915_private *dev_priv,
int val);
static inline uint32_t i915_vgacntrl_reg(struct drm_device *dev)
{
- if (HAS_PCH_SPLIT(dev))
- return CPU_VGACNTRL;
- else if (IS_VALLEYVIEW(dev))
+ if (IS_VALLEYVIEW(dev))
return VLV_VGACNTRL;
+ else if (INTEL_INFO(dev)-gen = 5)
+ return CPU_VGACNTRL;
else
return VGACNTRL;
}
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/7] drm/i915: Avoid incorrect returning for some platforms
From: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Sonika Jindal sonika.jin...@intel.com
---
drivers/gpu/drm/i915/intel_hdmi.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
b/drivers/gpu/drm/i915/intel_hdmi.c
index 2e70132..a15fc47 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -882,7 +882,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc *crtc)
struct intel_encoder *encoder;
int count = 0, count_hdmi = 0;
- if (!HAS_PCH_SPLIT(dev))
+ if (INTEL_INFO(dev)-gen = 5 || IS_VALLEYVIEW(dev))
return false;
list_for_each_entry(encoder, dev-mode_config.encoder_list, base.head)
{
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/7] drm/i915: Setting legacy palette correctly for different platforms
From: Sonika Jindal sonika.jin...@intel.com Signed-off-by: Sonika Jindal sonika.jin...@intel.com --- drivers/gpu/drm/i915/intel_display.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c89b4ac..d1c7105 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3847,7 +3847,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc) } /* use legacy palette for Ironlake */ - if (HAS_PCH_SPLIT(dev)) + if (INTEL_INFO(dev)-gen 5 !IS_VALLEYVIEW(dev)) palreg = LGC_PALETTE(pipe); /* Workaround : Do not read or write the pipe palette/gamma data while -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/7] drm/i915: Returning from increase/decrease of pllclock when invalid
From: Sonika Jindal sonika.jin...@intel.com Signed-off-by: Sonika Jindal sonika.jin...@intel.com --- drivers/gpu/drm/i915/intel_display.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d1c7105..e259c41 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8783,7 +8783,7 @@ static void intel_increase_pllclock(struct drm_device *dev, int dpll_reg = DPLL(pipe); int dpll; - if (HAS_PCH_SPLIT(dev)) + if (INTEL_INFO(dev)-gen 5 !IS_VALLEYVIEW(dev)) return; if (!dev_priv-lvds_downclock_avail) @@ -8811,7 +8811,7 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc) struct drm_i915_private *dev_priv = dev-dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - if (HAS_PCH_SPLIT(dev)) + if (INTEL_INFO(dev)-gen 5 !IS_VALLEYVIEW(dev)) return; if (!dev_priv-lvds_downclock_avail) -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
