Re: [Intel-gfx] [PATCH 1/2] drm/i915: ring irq cleanups
On Thu, 29 Mar 2012 19:11:26 -0700, Ben Widawsky b...@bwidawsk.net wrote: - gen6 put/get only need one argument rflags and gflags are always the same (see above explanation) - remove a couple redundantly defined IRQs - reordered some lines to make things go in descending order Every ring has its own interrupts, enables, masks, and status bits that are fed into the main interrupt enable/mask/status registers. At one point in time it seemed like a good idea to make our functions support the notion that each interrupt may have a different bit position in the corresponding register (blitter parser error may be bit n in IMR, but bit m in blitter IMR). It turned out though that the HW designers did us a solid on Gen6+ and this unfortunate situation has been avoided. This allows our interrupt code to be cleaned up a bit. I jammed this into one commit because there should be no functional change with this commit, and staging it into multiple commits was unnecessarily artificial IMO. CC: Chris Wilson ch...@chris-wilson.co.uk CC: Jesse Barnes jbar...@virtuousgeek.org Signed-off-by: Ben Widawsky benjamin.widaw...@intel.com Those two patches are Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk We can go further in simplifying get/put irq. First requires a dev_priv-gt.enable_irq() and then refactor the common (pro|epi)logue. -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/1] intel_driver: add support for Ivy Bridge GT2 Server chipset
On Thu, 29 Mar 2012 21:08:29 -0300, Eugeni Dodonov eugeni.dodo...@intel.com wrote: This is the GT2 variant of Ivy Bridge server chip. Signed-off-by: Eugeni Dodonov eugeni.dodo...@intel.com Pushed, thanks. I think we should push out a 2.18.1 so that userspace support is available for when the kernel enabling lands as well. -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/2] drm/i915: ring irq cleanups
On Fri, Mar 30, 2012 at 09:33:22AM +0100, Chris Wilson wrote: On Thu, 29 Mar 2012 19:11:26 -0700, Ben Widawsky b...@bwidawsk.net wrote: - gen6 put/get only need one argument rflags and gflags are always the same (see above explanation) - remove a couple redundantly defined IRQs - reordered some lines to make things go in descending order Every ring has its own interrupts, enables, masks, and status bits that are fed into the main interrupt enable/mask/status registers. At one point in time it seemed like a good idea to make our functions support the notion that each interrupt may have a different bit position in the corresponding register (blitter parser error may be bit n in IMR, but bit m in blitter IMR). It turned out though that the HW designers did us a solid on Gen6+ and this unfortunate situation has been avoided. This allows our interrupt code to be cleaned up a bit. I jammed this into one commit because there should be no functional change with this commit, and staging it into multiple commits was unnecessarily artificial IMO. CC: Chris Wilson ch...@chris-wilson.co.uk CC: Jesse Barnes jbar...@virtuousgeek.org Signed-off-by: Ben Widawsky benjamin.widaw...@intel.com Those two patches are Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Both patches applied, with a few things added to i915_reg.h for the first one - I got confused about this stuff too much. Thanks for wrestling that red dragon. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Sanitize BIOS debugging bits from PIPECONF
On Thu, 22 Mar 2012 15:00:50 +, Chris Wilson ch...@chris-wilson.co.uk wrote: Quoting the BSpec from time immemorial: PIPEACONF, bits 28:27: Frame Start Delay (Debug) Used to delay the frame start signal that is sent to the display planes. Care must be taken to insure that there are enough lines during VBLANK to support this setting. An instance of the BIOS leaving these bits set was found in the wild, where it caused our modesetting to go all squiffy and skewiff. References: https://bugs.freedesktop.org/show_bug.cgi?id=47271 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: sta...@kernel.org Reported-and-tested-by: Carl Richell c...@system76.com Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=43012 -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 18/22] drm/i915: ValleyView IRQ support
On Wed, Mar 28, 2012 at 01:39:38PM -0700, Jesse Barnes wrote:
ValleyView has a new interrupt architecture; best to put it in a new set
of functions. Also make sure the ring mask functions handle ValleyView.
FIXME: fix flipping; need to enable interrupts and call prepare/finish
Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org
---
drivers/gpu/drm/i915/i915_debugfs.c | 40 -
drivers/gpu/drm/i915/i915_irq.c | 338
++-
drivers/gpu/drm/i915/i915_reg.h |7 +-
drivers/gpu/drm/i915/intel_ringbuffer.c |4 +-
4 files changed, 383 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index e74674b..d226f2f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -468,7 +468,45 @@ static int i915_interrupt_info(struct seq_file *m, void
*data)
if (ret)
return ret;
- if (!HAS_PCH_SPLIT(dev)) {
+ if (IS_VALLEYVIEW(dev)) {
+ seq_printf(m, Display IER:\t%08x\n,
+I915_READ(VLV_IER));
+ seq_printf(m, Display IIR:\t%08x\n,
+I915_READ(VLV_IIR));
+ seq_printf(m, Display IIR_RW:\t%08x\n,
+I915_READ(VLV_IIR_RW));
+ seq_printf(m, Display IMR:\t%08x\n,
+I915_READ(VLV_IMR));
+ for_each_pipe(pipe)
+ seq_printf(m, Pipe %c stat:\t%08x\n,
+pipe_name(pipe),
+I915_READ(PIPESTAT(pipe)));
+
+ seq_printf(m, Master IER:\t%08x\n,
+I915_READ(VLV_MASTER_IER));
+
+ seq_printf(m, Render IER:\t%08x\n,
+I915_READ(GTIER));
+ seq_printf(m, Render IIR:\t%08x\n,
+I915_READ(GTIIR));
+ seq_printf(m, Render IMR:\t%08x\n,
+I915_READ(GTIMR));
+
+ seq_printf(m, PM IER:\t\t%08x\n,
+I915_READ(GEN6_PMIER));
+ seq_printf(m, PM IIR:\t\t%08x\n,
+I915_READ(GEN6_PMIIR));
+ seq_printf(m, PM IMR:\t\t%08x\n,
+I915_READ(GEN6_PMIMR));
+
+ seq_printf(m, Port hotplug:\t%08x\n,
+I915_READ(PORT_HOTPLUG_EN));
+ seq_printf(m, DPFLIPSTAT:\t%08x\n,
+I915_READ(VLV_DPFLIPSTAT));
+ seq_printf(m, DPINVGTT:\t%08x\n,
+I915_READ(DPINVGTT));
+
+ } else if (!HAS_PCH_SPLIT(dev)) {
seq_printf(m, Interrupt enable:%08x\n,
I915_READ(IER));
seq_printf(m, Interrupt identity: %08x\n,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 998116e..01fb650 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -120,6 +120,10 @@ void intel_enable_asle(struct drm_device *dev)
drm_i915_private_t *dev_priv = dev-dev_private;
unsigned long irqflags;
+ /* FIXME: opregion/asle for VLV */
+ if (IS_VALLEYVIEW(dev))
+ return;
+
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
if (HAS_PCH_SPLIT(dev))
@@ -426,6 +430,119 @@ static void gen6_pm_rps_work(struct work_struct *work)
mutex_unlock(dev_priv-dev-struct_mutex);
}
+static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
+{
+ struct drm_device *dev = (struct drm_device *) arg;
+ drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev-dev_private;
+ u32 iir, gt_iir, pm_iir;
+ irqreturn_t ret = IRQ_NONE;
+ unsigned long irqflags;
+ int pipe;
+ u32 pipe_stats[I915_MAX_PIPES];
+ u32 vblank_status;
+ int vblank = 0;
+ bool blc_event;
+
+ atomic_inc(dev_priv-irq_received);
+
+ vblank_status = PIPE_START_VBLANK_INTERRUPT_STATUS |
+ PIPE_VBLANK_INTERRUPT_STATUS;
+
+ while (true) {
+ iir = I915_READ(VLV_IIR);
+ gt_iir = I915_READ(GTIIR);
+ pm_iir = I915_READ(GEN6_PMIIR);
+
+ if (gt_iir == 0 pm_iir == 0 iir == 0)
+ goto out;
+
+ ret = IRQ_HANDLED;
+
+ if (gt_iir (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
+ notify_ring(dev, dev_priv-ring[RCS]);
+ if (gt_iir GT_GEN6_BSD_USER_INTERRUPT)
+ notify_ring(dev, dev_priv-ring[VCS]);
+ if (gt_iir GT_BLT_USER_INTERRUPT)
+ notify_ring(dev, dev_priv-ring[BCS]);
+
+ if (gt_iir (GT_GEN6_BLT_CS_ERROR_INTERRUPT |
+ GT_GEN6_BSD_CS_ERROR_INTERRUPT |
+ GT_RENDER_CS_ERROR_INTERRUPT)) {
+ DRM_ERROR(GT error interrupt 0x%08x\n,
Re: [Intel-gfx] [PATCH 18/22] drm/i915: ValleyView IRQ support
On Fri, Mar 30, 2012 at 06:46:55PM +0200, Daniel Vetter wrote: On Wed, Mar 28, 2012 at 01:39:38PM -0700, Jesse Barnes wrote: ValleyView has a new interrupt architecture; best to put it in a new set of functions. Also make sure the ring mask functions handle ValleyView. FIXME: fix flipping; need to enable interrupts and call prepare/finish Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org Meh, just noticed that I've already applied this. I'll write a few patches ... -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/18] drm/i915: try to reset the gpu before unload
On Fri, Mar 30, 2012 at 09:54:31AM -0700, Jesse Barnes wrote: On Sun, 18 Mar 2012 13:39:52 -0700 Ben Widawsky b...@bwidawsk.net wrote: paranoia For context support the HW expects the default context to always be available as there is no way to shut off HW contexts once turned on (afaics). This is problematic when unloading the driver as we have no way to prevent the GPU from expecting the BO to still be present once unloaded. The best we can do to remedy the situation is to attempt a GPU reset when doing the unload. NOTE: this patch isn't *really* required to go with the rest of the context serious. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c1aab45..848cc45 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3910,6 +3910,9 @@ i915_gem_lastclose(struct drm_device *dev) ret = i915_gem_idle(dev); if (ret) DRM_ERROR(failed to idle hardware: %d\n, ret); + ret = i915_reset(dev, GRDOM_FULL); + if (ret) + DRM_ERROR(failed to reset gpu: %d\n, ret); } static void This reminds me that we should make our reset finer grained. We generally only need to reset the render engine when we detect an error, but today we reset display and media unconditionally too. Not resetting the display would make reset more invisible like it ought to be... I haven't tested it, but it's yet another thing for our TODO list. I think with the pch split stuff we don't already reset display. And in any cases the global gtt seems to just keep going. The real problem is that you will notice the reset anyway, because we tend to wait a few seconds before we declare the gpu dead. For that we also need some improvements (and better culprit detection so that we can start to reset earlier). That one is on my todo ;-) -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/18] drm/i915: context switch implementation
On Thu, 29 Mar 2012 20:49:00 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Thu, Mar 29, 2012 at 11:43:12AM -0700, Ben Widawsky wrote: On Thu, 29 Mar 2012 20:24:11 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sun, Mar 18, 2012 at 01:39:45PM -0700, Ben Widawsky wrote: Implement the context switch code as well as the interfaces to do the context switch. This patch also doesn't match 1:1 with the RFC patches. The main difference is that from Daniel's responses the last context object is now stored instead of the last context. This aids in allows us to free the context data structure, and context object independently. There is room for optimization: this code will pin the context object until the next context is active. The optimal way to do it is to actually pin the object, move it to the active list, do the context switch, and then unpin it. This allows the eviction code to actually evict the context object if needed. The context switch code is missing workarounds, they will be implemented in future patches. Signed-off-by: Ben Widawsky b...@bwidawsk.net Ok, I've looked at the use-sites of context_get and all this refcounting and noticed that: - we always hold dev-struct_mutex - we always drop the acquired reference to the context structure in the same function without dropping struct_mutex in between. So we don't seem to require any reference counting on these (and additional locking on the idr). Additionally the idr locking seems to give us a false sense of security because afaics the locking/refcounting would be broken when we would _not_ hold struct_mutex. So can we just rip this out or do we need this (in which case it needs some more work imo)? -Daniel I think it can be ripped out. I was on the fence about this before submitting the patches and left it out of laziness; it doesn't hurt as there is no lock contention assuming your statement is true with no bugs in the code, and it follows the canonical use of idrs. Let me look it over some more to make sure after you finish reviewing the other stuff. The idea was supposed to be contexts can be created and looked up without struct mutex, but that isn't actually done in the code. Well, if you want to leave it I have to add some more review comments about it - atm I think it would be buggy and racy without holding struct_mutex ... -Daniel As I said before, I'll either find a good use for it, or remove it. Context creation is the only currently viable case for it- but probably not worth the extra lock. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: rip out old HWSTAM missed irq WA for vlv
This got copy-pasted from an older version. The newer kinds of
workarounds don't need this anymore.
Shame on me for not noticing when picking up the vlv irq patch.
Signed-Off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_irq.c | 12
1 files changed, 0 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 06286a2..8496fa5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1998,18 +1998,6 @@ static void valleyview_irq_preinstall(struct drm_device
*dev)
I915_WRITE(RING_IMR(GEN6_BSD_RING_BASE), 0);
I915_WRITE(RING_IMR(BLT_RING_BASE), 0);
- if (IS_GEN6(dev) || IS_GEN7(dev)) {
- /* Workaround stalls observed on Sandy Bridge GPUs by
-* making the blitter command streamer generate a
-* write to the Hardware Status Page for
-* MI_USER_INTERRUPT. This appears to serialize the
-* previous seqno write out before the interrupt
-* happens.
-*/
- I915_WRITE(GEN6_BLITTER_HWSTAM, ~GEN6_BLITTER_USER_INTERRUPT);
- I915_WRITE(GEN6_BSD_HWSTAM, ~GEN6_BSD_USER_INTERRUPT);
- }
-
/* and GT */
I915_WRITE(GTIIR, I915_READ(GTIIR));
I915_WRITE(GTIIR, I915_READ(GTIIR));
--
1.7.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/3] drm/i915: extract gt interrupt handler
vlv, ivb and snb all share the gen6+ gt irq handling. 3 copies of the
same stuff is a bit much, so extract it into a little helper.
Now ilk has a different gt irq handling than snb, but shares the same
irq handler (due to the similar display block). So also extract the
ilk gt irq handling to clearly separate these two things.
Nice side effect of this is that we can complete Ben Widawsky's gen6+
irq bit #define cleanup and call the render irq also with the GEN6
alias. Beforehand that code was shared with ilk, and neither option
really made much sense.
As a bonus this enables the error interrupt handling lifted from the
vlv code on snb and ivb, too.
Signed-Off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_irq.c | 66 ++-
1 files changed, 37 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8496fa5..6d76ee5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -430,6 +430,27 @@ static void gen6_pm_rps_work(struct work_struct *work)
mutex_unlock(dev_priv-dev-struct_mutex);
}
+static void snb_gt_irq_handler(struct drm_device *dev,
+ struct drm_i915_private *dev_priv,
+ u32 gt_iir)
+{
+
+ if (gt_iir (GEN6_RENDER_USER_INTERRUPT |
+ GEN6_RENDER_PIPE_CONTROL_NOTIFY_INTERRUPT))
+ notify_ring(dev, dev_priv-ring[RCS]);
+ if (gt_iir GEN6_BSD_USER_INTERRUPT)
+ notify_ring(dev, dev_priv-ring[VCS]);
+ if (gt_iir GEN6_BLITTER_USER_INTERRUPT)
+ notify_ring(dev, dev_priv-ring[BCS]);
+
+ if (gt_iir (GT_GEN6_BLT_CS_ERROR_INTERRUPT |
+ GT_GEN6_BSD_CS_ERROR_INTERRUPT |
+ GT_RENDER_CS_ERROR_INTERRUPT)) {
+ DRM_ERROR(GT error interrupt 0x%08x\n, gt_iir);
+ i915_handle_error(dev, false);
+ }
+}
+
static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
{
struct drm_device *dev = (struct drm_device *) arg;
@@ -458,19 +479,7 @@ static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
ret = IRQ_HANDLED;
- if (gt_iir (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
- notify_ring(dev, dev_priv-ring[RCS]);
- if (gt_iir GT_GEN6_BSD_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[VCS]);
- if (gt_iir GT_GEN6_BLT_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[BCS]);
-
- if (gt_iir (GT_GEN6_BLT_CS_ERROR_INTERRUPT |
- GT_GEN6_BSD_CS_ERROR_INTERRUPT |
- GT_RENDER_CS_ERROR_INTERRUPT)) {
- DRM_ERROR(GT error interrupt 0x%08x\n, gt_iir);
- i915_handle_error(dev, false);
- }
+ snb_gt_irq_handler(dev, dev_priv, gt_iir);
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
for_each_pipe(pipe) {
@@ -618,12 +627,7 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
READ_BREADCRUMB(dev_priv);
}
- if (gt_iir (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
- notify_ring(dev, dev_priv-ring[RCS]);
- if (gt_iir GEN6_BSD_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[VCS]);
- if (gt_iir GEN6_BLITTER_USER_INTERRUPT)
- notify_ring(dev, dev_priv-ring[BCS]);
+ snb_gt_irq_handler(dev, dev_priv, gt_iir);
if (de_iir DE_GSE_IVB)
intel_opregion_gse_intr(dev);
@@ -675,6 +679,16 @@ done:
return ret;
}
+static void ilk_gt_irq_handler(struct drm_device *dev,
+ struct drm_i915_private *dev_priv,
+ u32 gt_iir)
+{
+ if (gt_iir (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
+ notify_ring(dev, dev_priv-ring[RCS]);
+ if (gt_iir GT_BSD_USER_INTERRUPT)
+ notify_ring(dev, dev_priv-ring[VCS]);
+}
+
static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
{
struct drm_device *dev = (struct drm_device *) arg;
@@ -683,13 +697,9 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
u32 de_iir, gt_iir, de_ier, pch_iir, pm_iir;
u32 hotplug_mask;
struct drm_i915_master_private *master_priv;
- u32 bsd_usr_interrupt = GT_BSD_USER_INTERRUPT;
atomic_inc(dev_priv-irq_received);
- if (IS_GEN6(dev))
- bsd_usr_interrupt = GEN6_BSD_USER_INTERRUPT;
-
/* disable master interrupt before clearing iir */
de_ier = I915_READ(DEIER);
I915_WRITE(DEIER, de_ier ~DE_MASTER_IRQ_CONTROL);
@@ -718,12 +728,10 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
READ_BREADCRUMB(dev_priv);
}
- if (gt_iir (GT_USER_INTERRUPT | GT_PIPE_NOTIFY))
-
Re: [Intel-gfx] [PATCH 3/3] drm/i915: extract gt interrupt handler
On Fri, 30 Mar 2012 20:24:35 +0200 Daniel Vetter daniel.vet...@ffwll.ch wrote: vlv, ivb and snb all share the gen6+ gt irq handling. 3 copies of the same stuff is a bit much, so extract it into a little helper. Now ilk has a different gt irq handling than snb, but shares the same irq handler (due to the similar display block). So also extract the ilk gt irq handling to clearly separate these two things. Nice side effect of this is that we can complete Ben Widawsky's gen6+ irq bit #define cleanup and call the render irq also with the GEN6 alias. Beforehand that code was shared with ilk, and neither option really made much sense. As a bonus this enables the error interrupt handling lifted from the vlv code on snb and ivb, too. Signed-Off-by: Daniel Vetter daniel.vet...@ffwll.ch Nice cleanup. Though I don't really like the IS_GEN5 branch in ironlake_irq_handler... might be nicer to just bite the bullet and have a mostly duplicate snb irq handler. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org -- Jesse Barnes, Intel Open Source Technology Center signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/18] drm/i915: possibly invalidate TLB before context switch
On Thu, 29 Mar 2012 21:25:49 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sun, Mar 18, 2012 at 01:39:49PM -0700, Ben Widawsky wrote: From http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol1_Part3.pdf [DevSNB] If Flush TLB invalidation Mode is enabled it’s the driver’s responsibility to invalidate the TLBs at least once after the previous context switch after any GTT mappings changed (including new GTT entries). This can be done by a pipelined PIPE_CONTROL with TLB inv bit set immediately before MI_SET_CONTEXT. Signed-off-by: Ben Widawsky b...@bwidawsk.net Hm, I've beend decently confused about the meaning of GFX_TLB_INVALIDATE_ALWAYS - it actually means that we flush tlbs on every full flush (i.e. always) when it's reset. And we don't set this so this workaround is pretty much just informational. I'm hence wondering whether a big comment wouldn't be better? -Daniel It's probably not much difference in terms of LOC with the plus that the comment turns into correctly functioning code if we flip the bit. Just from a quick glance at code though, it seems we don't explicitly touch this bit for GEN6. Which was probably why I did it in the first place. if (IS_GEN7(dev)) I915_WRITE(GFX_MODE_GEN7, GFX_MODE_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) | GFX_MODE_ENABLE(GFX_REPLAY_MODE)); Anyway, I really don't care enough to argue, but I like the way it is, and it's been tested the way it is; call the ball. Ben ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/18] drm/i915: try to reset the gpu before unload
On Thu, 29 Mar 2012 21:31:21 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sun, Mar 18, 2012 at 01:39:52PM -0700, Ben Widawsky wrote: paranoia For context support the HW expects the default context to always be available as there is no way to shut off HW contexts once turned on (afaics). This is problematic when unloading the driver as we have no way to prevent the GPU from expecting the BO to still be present once unloaded. The best we can do to remedy the situation is to attempt a GPU reset when doing the unload. NOTE: this patch isn't *really* required to go with the rest of the context serious. Signed-off-by: Ben Widawsky b...@bwidawsk.net I think the paranoia here is justified (albeit it would benefit from some commit-message love imo). But we do not support i915_reset on all gens, so I think you need to add a gen = 5 check here. I think i915_reset does the right thing, but I'm not sure. It has a big gen switch statement in it. --- drivers/gpu/drm/i915/i915_gem.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c1aab45..848cc45 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3910,6 +3910,9 @@ i915_gem_lastclose(struct drm_device *dev) ret = i915_gem_idle(dev); if (ret) DRM_ERROR(failed to idle hardware: %d\n, ret); + ret = i915_reset(dev, GRDOM_FULL); + if (ret) + DRM_ERROR(failed to reset gpu: %d\n, ret); } static void -- 1.7.9.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/18] drm/i915/context: create destroy ioctls
On Thu, 29 Mar 2012 21:35:35 +0200
Daniel Vetter dan...@ffwll.ch wrote:
On Sun, Mar 18, 2012 at 01:39:53PM -0700, Ben Widawsky wrote:
Add the interfaces to allow user space to create and destroy contexts.
Contexts are destroyed automatically if the file descriptor for the dri
device is closed.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_dma.c |2 +
drivers/gpu/drm/i915/i915_drv.h |4 ++
drivers/gpu/drm/i915/i915_gem_context.c | 64
+++
include/drm/i915_drm.h | 16
4 files changed, 86 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 4c7c1dc..fb3fccb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2333,6 +2333,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs,
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey,
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, intel_sprite_get_colorkey,
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+ DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE,
i915_gem_context_create_ioctl, DRM_UNLOCKED),
+ DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY,
i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
};
int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index c6c2ada..d49615e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1312,6 +1312,10 @@ void i915_gem_context_close(struct drm_device *dev,
struct drm_file *file);
int i915_switch_context(struct intel_ring_buffer *ring,
struct drm_file *file,
int to_id, u32 seqno, u32 flags);
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file);
+int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file);
/* i915_gem_gtt.c */
int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index d9a08f2..accb3de 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -411,3 +411,67 @@ out:
kref_put(to-nref, destroy_hw_context);
return ret;
}
+
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_i915_gem_context_create *args = data;
+ struct drm_i915_file_private *file_priv = file-driver_priv;
+ struct intel_ring_buffer *ring = dev_priv-ring[RCS];
+ struct i915_hw_context *ctx;
+ int ret;
+
+ if (dev_priv-hw_contexts_disabled)
+ return -EPERM;
+
+ ret = create_hw_context(dev, file_priv, ctx);
Note that the gem_alloc_object call in here is rather unsafe, due to the
unsafe statistics-keeping in i915_gem_info_add_obj. Pre-existing looking
goof-up, but can I maybe volunteer you to fix this? I think we need an
mm stats spinlock because these counters could be rather big (and atomic_t
is only 32 bits).
I can do it. In this series?
+ if (ret)
+ return ret;
+
+ /* We need to do a special switch (save-only) either now, or before the
+* context is actually used in order to keep the context switch request
+* in execbuffer fairly simple.
+*/
+ mutex_lock(dev-struct_mutex);
+ ret = i915_switch_context(ring, file, ctx-id, ring-get_seqno(ring),
+ I915_CONTEXT_INITIAL_SWITCH);
With the hw_context-is_intialized change you could ditch this (imo).
You could ditch it, but I can't make an argument that one way is better
than another. As you stated earlier, you want me to ditch the spinlock
for the context id creation, so I'll still need to acquire struct mutex
anyway. At which point, I think it doesn't hurt to switch now too.
I'm willing to change this is you can describe a benefit for it.
+ mutex_unlock(dev-struct_mutex);
+ if (ret)
+ return ret;
+
+ args-ctx_id = ctx-id;
+ DRM_DEBUG_DRIVER(HW context %d created\n, args-ctx_id);
+ return ret;
+}
+
+int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct drm_i915_gem_context_destroy *args = data;
+ struct drm_i915_file_private *file_priv = file-driver_priv;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct i915_hw_context *ctx;
+
+ if
Re: [Intel-gfx] [PATCH 14/18] drm/i915/context: switch contexts with execbuf2
On Thu, 29 Mar 2012 21:38:20 +0200
Daniel Vetter dan...@ffwll.ch wrote:
On Sun, Mar 18, 2012 at 01:39:54PM -0700, Ben Widawsky wrote:
Use the rsvd1 field in execbuf2 to specify the context ID associated
with the workload. This will allow the driver to do the proper context
switch when/if needed.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c |6 ++
include/drm/i915_drm.h |4 +++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 81687af..c365e12 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1058,6 +1058,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
*data,
struct drm_i915_gem_object *batch_obj;
struct drm_clip_rect *cliprects = NULL;
struct intel_ring_buffer *ring;
+ u32 ctx_id = args-context_info I915_EXEC_CONTEXT_ID_MASK;
u32 exec_start, exec_len;
u32 seqno;
u32 mask;
@@ -1266,6 +1267,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
*data,
goto err;
}
+ ret = i915_switch_context(ring, file, ctx_id, seqno, 0);
+ if (ret)
+ goto err;
+
Already complained a bit about these:
- Imo the seqno and hw_flags param can/should go away.
Responding to the other checks for this.
- You need to add some sanity checks somewhere so that we correctly bail
out of the do_execbuffer stuff without launching the batch. Obviously
also a prime candidate for an i-g-t tests (because it'll nicely exercise
a piece of code which usually is rather hard to tests).
I'm not sure what you're asking for, aside from - this is hairy code; be
careful. What was your idea?
Aside: I haven't yet looked at your testcases, so maybe I'll come up with
a crazy idea for another testcase ;-)
Testcases are pretty sparse in i-g-t. Most of it is done implicitly with
mesa. If you have ideas, I'll be happy to write them
trace_i915_gem_ring_dispatch(ring, seqno);
exec_start = batch_obj-gtt_offset + args-batch_start_offset;
@@ -1372,6 +1377,7 @@ i915_gem_execbuffer(struct drm_device *dev, void
*data,
exec2.num_cliprects = args-num_cliprects;
exec2.cliprects_ptr = args-cliprects_ptr;
exec2.flags = I915_EXEC_RENDER;
+ exec2.context_info = 0;
ret = i915_gem_do_execbuffer(dev, data, file, exec2, exec2_list);
if (!ret) {
diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
index bead13e..03d159f 100644
--- a/include/drm/i915_drm.h
+++ b/include/drm/i915_drm.h
@@ -660,13 +660,15 @@ struct drm_i915_gem_execbuffer2 {
#define I915_EXEC_CONSTANTS_ABSOLUTE (16)
#define I915_EXEC_CONSTANTS_REL_SURFACE (26) /* gen4/5 only */
__u64 flags;
- __u64 rsvd1;
+ __u64 context_info;
__u64 rsvd2;
};
/** Resets the SO write offset registers for transform feedback on gen7. */
#define I915_EXEC_GEN7_SOL_RESET (18)
+#define I915_EXEC_CONTEXT_ID_MASK (0x)
+
struct drm_i915_gem_pin {
/** Handle of the buffer to be pinned. */
__u32 handle;
--
1.7.9.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/18] drm/i915: possibly invalidate TLB before context switch
On Fri, Mar 30, 2012 at 11:39:53AM -0700, Ben Widawsky wrote: On Thu, 29 Mar 2012 21:25:49 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sun, Mar 18, 2012 at 01:39:49PM -0700, Ben Widawsky wrote: From http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol1_Part3.pdf [DevSNB] If Flush TLB invalidation Mode is enabled it’s the driver’s responsibility to invalidate the TLBs at least once after the previous context switch after any GTT mappings changed (including new GTT entries). This can be done by a pipelined PIPE_CONTROL with TLB inv bit set immediately before MI_SET_CONTEXT. Signed-off-by: Ben Widawsky b...@bwidawsk.net Hm, I've beend decently confused about the meaning of GFX_TLB_INVALIDATE_ALWAYS - it actually means that we flush tlbs on every full flush (i.e. always) when it's reset. And we don't set this so this workaround is pretty much just informational. I'm hence wondering whether a big comment wouldn't be better? -Daniel It's probably not much difference in terms of LOC with the plus that the comment turns into correctly functioning code if we flip the bit. Just from a quick glance at code though, it seems we don't explicitly touch this bit for GEN6. Which was probably why I did it in the first place. if (IS_GEN7(dev)) I915_WRITE(GFX_MODE_GEN7, GFX_MODE_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) | GFX_MODE_ENABLE(GFX_REPLAY_MODE)); Anyway, I really don't care enough to argue, but I like the way it is, and it's been tested the way it is; call the ball. Add a quick comment where you insert the wa flush that in our current config this isn't used and keep it. I don't care too much. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/18] drm/i915: try to reset the gpu before unload
On Fri, Mar 30, 2012 at 11:50:42AM -0700, Ben Widawsky wrote: On Thu, 29 Mar 2012 21:31:21 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sun, Mar 18, 2012 at 01:39:52PM -0700, Ben Widawsky wrote: paranoia For context support the HW expects the default context to always be available as there is no way to shut off HW contexts once turned on (afaics). This is problematic when unloading the driver as we have no way to prevent the GPU from expecting the BO to still be present once unloaded. The best we can do to remedy the situation is to attempt a GPU reset when doing the unload. NOTE: this patch isn't *really* required to go with the rest of the context serious. Signed-off-by: Ben Widawsky b...@bwidawsk.net I think the paranoia here is justified (albeit it would benefit from some commit-message love imo). But we do not support i915_reset on all gens, so I think you need to add a gen = 5 check here. I think i915_reset does the right thing, but I'm not sure. It has a big gen switch statement in it. Yeah, it just returns. But on reading i915_reset I think you don't want all it does - it resets the entire modeset and gem state, too. I think it would be better to just do the hw reset and not bother with everything else - the only chance is that it accidentally breaks module unload. What about extracting the actual hw reset code (i.e. the switch(gen) stuff) into i915_do_reset and only calling that? If you want, add the is_gen5 check, but given that it's just module unload I don't care about fried hw due to a not-so-well-working gpu reset ;-) -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/18] drm/i915/context: create destroy ioctls
On Fri, Mar 30, 2012 at 11:55:14AM -0700, Ben Widawsky wrote:
On Thu, 29 Mar 2012 21:35:35 +0200
Daniel Vetter dan...@ffwll.ch wrote:
On Sun, Mar 18, 2012 at 01:39:53PM -0700, Ben Widawsky wrote:
Add the interfaces to allow user space to create and destroy contexts.
Contexts are destroyed automatically if the file descriptor for the dri
device is closed.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_dma.c |2 +
drivers/gpu/drm/i915/i915_drv.h |4 ++
drivers/gpu/drm/i915/i915_gem_context.c | 64
+++
include/drm/i915_drm.h | 16
4 files changed, 86 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 4c7c1dc..fb3fccb 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2333,6 +2333,8 @@ struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_OVERLAY_ATTRS, intel_overlay_attrs,
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF_DRV(I915_SET_SPRITE_COLORKEY, intel_sprite_set_colorkey,
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF_DRV(I915_GET_SPRITE_COLORKEY, intel_sprite_get_colorkey,
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+ DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE,
i915_gem_context_create_ioctl, DRM_UNLOCKED),
+ DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_DESTROY,
i915_gem_context_destroy_ioctl, DRM_UNLOCKED),
};
int i915_max_ioctl = DRM_ARRAY_SIZE(i915_ioctls);
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index c6c2ada..d49615e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1312,6 +1312,10 @@ void i915_gem_context_close(struct drm_device
*dev, struct drm_file *file);
int i915_switch_context(struct intel_ring_buffer *ring,
struct drm_file *file,
int to_id, u32 seqno, u32 flags);
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file);
+int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
+struct drm_file *file);
/* i915_gem_gtt.c */
int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index d9a08f2..accb3de 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -411,3 +411,67 @@ out:
kref_put(to-nref, destroy_hw_context);
return ret;
}
+
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file)
+{
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_i915_gem_context_create *args = data;
+ struct drm_i915_file_private *file_priv = file-driver_priv;
+ struct intel_ring_buffer *ring = dev_priv-ring[RCS];
+ struct i915_hw_context *ctx;
+ int ret;
+
+ if (dev_priv-hw_contexts_disabled)
+ return -EPERM;
+
+ ret = create_hw_context(dev, file_priv, ctx);
Note that the gem_alloc_object call in here is rather unsafe, due to the
unsafe statistics-keeping in i915_gem_info_add_obj. Pre-existing looking
goof-up, but can I maybe volunteer you to fix this? I think we need an
mm stats spinlock because these counters could be rather big (and atomic_t
is only 32 bits).
I can do it. In this series?
That bug is pre-existing since gem was merged afaik. So just when you're bored,
in a
separate series.
+ if (ret)
+ return ret;
+
+ /* We need to do a special switch (save-only) either now, or before the
+ * context is actually used in order to keep the context switch request
+ * in execbuffer fairly simple.
+ */
+ mutex_lock(dev-struct_mutex);
+ ret = i915_switch_context(ring, file, ctx-id, ring-get_seqno(ring),
+ I915_CONTEXT_INITIAL_SWITCH);
With the hw_context-is_intialized change you could ditch this (imo).
You could ditch it, but I can't make an argument that one way is better
than another. As you stated earlier, you want me to ditch the spinlock
for the context id creation, so I'll still need to acquire struct mutex
anyway. At which point, I think it doesn't hurt to switch now too.
I'm willing to change this is you can describe a benefit for it.
Imo the benefit is that we have one (and just one) clear place where we
switch context. Doing a funky switch at create time just feels wonky. Just
now I've also noticed that you call ring-get_seqno(ring) - that reads the
current seqno from the gpu hws and is absolutely not what you want. So
you'd have to add another request (and attach it to the file_priv).
I
Re: [Intel-gfx] [PATCH 14/18] drm/i915/context: switch contexts with execbuf2
On Fri, Mar 30, 2012 at 11:58:20AM -0700, Ben Widawsky wrote: On Thu, 29 Mar 2012 21:38:20 +0200 Daniel Vetter dan...@ffwll.ch wrote: On Sun, Mar 18, 2012 at 01:39:54PM -0700, Ben Widawsky wrote: Use the rsvd1 field in execbuf2 to specify the context ID associated with the workload. This will allow the driver to do the proper context switch when/if needed. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_execbuffer.c |6 ++ include/drm/i915_drm.h |4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 81687af..c365e12 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1058,6 +1058,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct drm_i915_gem_object *batch_obj; struct drm_clip_rect *cliprects = NULL; struct intel_ring_buffer *ring; + u32 ctx_id = args-context_info I915_EXEC_CONTEXT_ID_MASK; u32 exec_start, exec_len; u32 seqno; u32 mask; @@ -1266,6 +1267,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, goto err; } + ret = i915_switch_context(ring, file, ctx_id, seqno, 0); + if (ret) + goto err; + Already complained a bit about these: - Imo the seqno and hw_flags param can/should go away. Responding to the other checks for this. - You need to add some sanity checks somewhere so that we correctly bail out of the do_execbuffer stuff without launching the batch. Obviously also a prime candidate for an i-g-t tests (because it'll nicely exercise a piece of code which usually is rather hard to tests). I'm not sure what you're asking for, aside from - this is hairy code; be careful. What was your idea? If userspace tries to run an execbuffer with - an nonexisting context on render - non-default context on !render_ring it needs to get a -EINVAL and the kernel must not execute the batch. Your code fails at that. We also need a testcase for this case in i-g-t. Another testcase is trying to destroy a non-existing context. I haven't rechecked whether your current code handles that correctly. Aside: I haven't yet looked at your testcases, so maybe I'll come up with a crazy idea for another testcase ;-) Testcases are pretty sparse in i-g-t. Most of it is done implicitly with mesa. If you have ideas, I'll be happy to write them See above ;-) I'll look at your current set of testcase later and write another mail with the things I might find missing. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: properly clear SSC1 bit in the pch refclock init code
Noticed by staring at intel_reg_dumper diffs. Unfortunately it does
not seem to completely fix the bug.
Still, it's good to get this right, and maybe it helps someplace else.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=47117
Signed-Off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/intel_display.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a7c2ddc..86a9e73 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5560,7 +5560,8 @@ void ironlake_init_pch_refclk(struct drm_device *dev)
if (intel_panel_use_ssc(dev_priv) can_ssc) {
DRM_DEBUG_KMS(Using SSC on panel\n);
temp |= DREF_SSC1_ENABLE;
- }
+ } else
+ temp = ~DREF_SSC1_ENABLE;
/* Get SSC going before enabling the outputs */
I915_WRITE(PCH_DREF_CONTROL, temp);
--
1.7.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Scanline wait hack for IVB
You need to set DE_RRMR (offset 0x44050) to unmask scanline waits, but
this at least doesn't hang on my IVB.
However I still see tearing on my config, need to verify the pipe and
inclusive/exclusive setting.
--
Jesse Barnes, Intel Open Source Technology Center
diff --git a/src/i830_reg.h b/src/i830_reg.h
index 93d03cf..bd2980a 100644
--- a/src/i830_reg.h
+++ b/src/i830_reg.h
@@ -52,19 +52,39 @@
/* Wait for Events */
#define MI_WAIT_FOR_EVENT (0x0323)
+#define MI_WAIT_FOR_PIPEC_SVBLANK_SNB (121)
+#define MI_WAIT_FOR_PIPEB_SVBLANK_SNB (111)
+#define MI_WAIT_FOR_PIPEA_SVBLANK_SNB (13)
#define MI_WAIT_FOR_PIPEB_SVBLANK (118)
#define MI_WAIT_FOR_PIPEA_SVBLANK (117)
#define MI_WAIT_FOR_OVERLAY_FLIP (116)
+#define MI_WAIT_FOR_PIPEC_SCAN_LINE_WINDOW_SNB (114)
#define MI_WAIT_FOR_PIPEB_VBLANK (17)
#define MI_WAIT_FOR_PIPEB_SCAN_LINE_WINDOW (15)
+#define MI_WAIT_FOR_PIPEB_SCAN_LINE_WINDOW_SNB (18)
#define MI_WAIT_FOR_PIPEA_VBLANK (13)
#define MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW (11)
+#define MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW_SNB (10)
/* Set the scan line for MI_WAIT_FOR_PIPE?_SCAN_LINE_WINDOW */
#define MI_LOAD_SCAN_LINES_INCL(0x1223)
#define MI_LOAD_SCAN_LINES_DISPLAY_PIPEA (0)
#define MI_LOAD_SCAN_LINES_DISPLAY_PIPEB (0x120)
+#define MI_LOAD_SCAN_LINES_EXCL(0x1323)
+#define MI_LOAD_SCAN_LINES_EXCL_PIPEA (0)
+#define MI_LOAD_SCAN_LINES_EXCL_PIPEB (119)
+#define MI_LOAD_SCAN_LINES_EXCL_PIPEC (419)
+
+#define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
+
+#define _DE_LOAD_SL_A 0x70068
+#define _DE_LOAD_SL_B 0x71068
+#define _DE_LOAD_SL_C 0x72068
+#define DE_LOAD_SL(pipe) _PIPE(pipe, _DE_LOAD_SL_A, _DE_LOAD_SL_B)
+
+#define MI_LOAD_REGISTER_IMM (0x2223 | 1)
+
/* BLT commands */
#define COLOR_BLT_CMD ((229)|(0x4022)|(0x3))
#define COLOR_BLT_WRITE_ALPHA (121)
diff --git a/src/intel_dri.c b/src/intel_dri.c
index f6f0c86..87633e2 100644
--- a/src/intel_dri.c
+++ b/src/intel_dri.c
@@ -625,6 +625,65 @@ I830DRI2CopyRegion(DrawablePtr drawable, RegionPtr pRegion,
OUT_BATCH(MI_WAIT_FOR_EVENT | event);
ADVANCE_BATCH();
}
+ } else if (pixmap_is_scanout(get_drawable_pixmap(dst))
+ intel-swapbuffers_wait INTEL_INFO(intel)-gen = 60) {
+ BoxPtr box;
+ BoxRec crtcbox;
+ int y1, y2;
+ int pipe = -1, event, load_scan_lines_reg;
+ xf86CrtcPtr crtc;
+ Bool full_height = FALSE;
+
+ box = REGION_EXTENTS(unused, gc-pCompositeClip);
+ crtc = intel_covering_crtc(scrn, box, NULL, crtcbox);
+
+ /*
+* Make sure the CRTC is valid and this is the real front
+* buffer
+*/
+ if (crtc != NULL !crtc-rotatedData) {
+ pipe = intel_crtc_to_pipe(crtc);
+
+ load_scan_lines_reg = DE_LOAD_SL(pipe);
+
+ /*
+* Make sure we don't wait for a scanline that will
+* never occur
+*/
+ y1 = (crtcbox.y1 = box-y1) ? box-y1 - crtcbox.y1 : 0;
+ y2 = (box-y2 = crtcbox.y2) ?
+ box-y2 - crtcbox.y1 : crtcbox.y2 - crtcbox.y1;
+
+ if (y1 == 0 y2 == (crtcbox.y2 - crtcbox.y1))
+ full_height = TRUE;
+
+ if (pipe == 0) {
+ event = MI_WAIT_FOR_PIPEA_SCAN_LINE_WINDOW_SNB;
+ if (full_height)
+ event = MI_WAIT_FOR_PIPEA_SVBLANK_SNB;
+ } else if (pipe == 1) {
+ event = MI_WAIT_FOR_PIPEB_SCAN_LINE_WINDOW_SNB;
+ if (full_height)
+ event = MI_WAIT_FOR_PIPEB_SVBLANK_SNB;
+ } else {
+ event = MI_WAIT_FOR_PIPEC_SCAN_LINE_WINDOW_SNB;
+ if (full_height)
+ event = MI_WAIT_FOR_PIPEC_SVBLANK_SNB;
+ }
+
+ if (crtc-mode.Flags V_INTERLACE) {
+ /* DSL count field lines */
+ y1 /= 2;
+ y2 /= 2;
+ }
+
+ BEGIN_BATCH_BLT(4);
+ OUT_BATCH(MI_LOAD_REGISTER_IMM);
+ OUT_BATCH(load_scan_lines_reg);
+ OUT_BATCH((131) | (130) | (y1 16) | (y2-1));
+
Re: [Intel-gfx] [PATCH 11/18] drm/i915: switch to default context on idle
On Thu, 29 Mar 2012 21:29:05 +0200
Daniel Vetter dan...@ffwll.ch wrote:
On Sun, Mar 18, 2012 at 01:39:51PM -0700, Ben Widawsky wrote:
To keep things as sane as possible, switch to the default context before
idling. This should help free context objects, as well as put things in
a more well defined state before suspending.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
Context are yet another thing that will nicely fragment our global gtt
space. You don't pin them to mappable, which is a start, but I wonder
whether we should integrate this into our evict_something code? Although
that project is more involved with the current evict code, so maybe just
add a fixme.
---
drivers/gpu/drm/i915/i915_gem.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index 0985aa5..c1aab45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3685,6 +3685,8 @@ int
i915_gem_idle(struct drm_device *dev)
{
drm_i915_private_t *dev_priv = dev-dev_private;
+ struct intel_ring_buffer *ring = dev_priv-ring[RCS];
+ u32 seqno;
int ret;
mutex_lock(dev-struct_mutex);
@@ -3694,6 +3696,10 @@ i915_gem_idle(struct drm_device *dev)
return 0;
}
+ seqno = i915_gem_next_request_seqno(ring);
+ i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID, seqno, 0);
+ WARN_ON(i915_wait_request(ring, seqno, false));
This can block and potentially return early due to a signal, i.e. you need
to properly handle this (like the i915_gpu_idle below).
Can you elaborate? Whether or not the context switch fails, I want to
try to idle the GPU. So I *thought* this is doing just that.
+
ret = i915_gpu_idle(dev, true);
if (ret) {
mutex_unlock(dev-struct_mutex);
--
1.7.9.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/18] drm/i915: switch to default context on idle
On Fri, Mar 30, 2012 at 02:17:20PM -0700, Ben Widawsky wrote:
On Thu, 29 Mar 2012 21:29:05 +0200
Daniel Vetter dan...@ffwll.ch wrote:
On Sun, Mar 18, 2012 at 01:39:51PM -0700, Ben Widawsky wrote:
To keep things as sane as possible, switch to the default context before
idling. This should help free context objects, as well as put things in
a more well defined state before suspending.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
Context are yet another thing that will nicely fragment our global gtt
space. You don't pin them to mappable, which is a start, but I wonder
whether we should integrate this into our evict_something code? Although
that project is more involved with the current evict code, so maybe just
add a fixme.
---
drivers/gpu/drm/i915/i915_gem.c |6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index 0985aa5..c1aab45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3685,6 +3685,8 @@ int
i915_gem_idle(struct drm_device *dev)
{
drm_i915_private_t *dev_priv = dev-dev_private;
+ struct intel_ring_buffer *ring = dev_priv-ring[RCS];
+ u32 seqno;
int ret;
mutex_lock(dev-struct_mutex);
@@ -3694,6 +3696,10 @@ i915_gem_idle(struct drm_device *dev)
return 0;
}
+ seqno = i915_gem_next_request_seqno(ring);
+ i915_switch_context(ring, NULL, DEFAULT_CONTEXT_ID, seqno, 0);
+ WARN_ON(i915_wait_request(ring, seqno, false));
This can block and potentially return early due to a signal, i.e. you need
to properly handle this (like the i915_gpu_idle below).
Can you elaborate? Whether or not the context switch fails, I want to
try to idle the GPU. So I *thought* this is doing just that.
wait_request can return error codes, you need to handle them. E.g. gpu
hang, interrupt, out of memory, whatnotelse.
-Daniel
--
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
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: use semaphores for the display plane
On Wed, Mar 21, 2012 at 05:19:13PM -0700, Ben Widawsky wrote:
In theory this will have performance and power improvements. Performance
because we don't need to stall when the scanout BO is busy, and power
because we don't have to stall when the BO is busy ie. we can get the
work done sooner and put the CPU to sleep (and one less interrupt
required).
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem.c |8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ce2fee5..96ad162 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3041,9 +3041,7 @@ int i915_gem_object_set_cache_level(struct
drm_i915_gem_object *obj,
* any flushes to be pipelined (for pageflips).
*
* For the display plane, we want to be in the GTT but out of any write
- * domains. So in many ways this looks like set_to_gtt_domain() apart from
the
- * ability to pipeline the waits, pinning and any additional subtleties
- * that may differentiate the display plane from ordinary buffers.
+ * domains. So in many ways this looks like set_to_gtt_domain().
*/
For the comment bikeshed, what about:
Prepare buffer for display plane (scanout, cursors, etc).
Can be called from an uninterruptible phase (modesetting).
For display planes we need to flush, synchronize with any outstanding
rendering (pipelined using semaphores, if available, in case of a
pageflip) and additionally need to ensure that the cache_level is coherent
for the scanout engine.
Cheers, Daniel
int
i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
@@ -3058,8 +3056,8 @@ i915_gem_object_pin_to_display_plane(struct
drm_i915_gem_object *obj,
return ret;
if (pipelined != obj-ring) {
- ret = i915_gem_object_wait_rendering(obj);
- if (ret == -ERESTARTSYS)
+ ret = i915_gem_object_sync(obj, pipelined);
+ if (ret)
return ret;
}
--
1.7.9.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
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: use semaphores for the display plane
On Sat, 31 Mar 2012 01:38:24 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Wed, Mar 21, 2012 at 05:19:13PM -0700, Ben Widawsky wrote: In theory this will have performance and power improvements. Performance because we don't need to stall when the scanout BO is busy, and power because we don't have to stall when the BO is busy ie. we can get the work done sooner and put the CPU to sleep (and one less interrupt required). Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c |8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index ce2fee5..96ad162 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3041,9 +3041,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, * any flushes to be pipelined (for pageflips). * * For the display plane, we want to be in the GTT but out of any write - * domains. So in many ways this looks like set_to_gtt_domain() apart from the - * ability to pipeline the waits, pinning and any additional subtleties - * that may differentiate the display plane from ordinary buffers. + * domains. So in many ways this looks like set_to_gtt_domain(). */ For the comment bikeshed, what about: Prepare buffer for display plane (scanout, cursors, etc). Can be called from an uninterruptible phase (modesetting). For display planes we need to flush, synchronize with any outstanding rendering (pipelined using semaphores, if available, in case of a pageflip) and additionally need to ensure that the cache_level is coherent for the scanout engine. It's an improvement over what we have certainly. Still doesn't explain the why, but I've writers block as well. -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] Scanline wait hack for IVB
#part sign=pgpmime On Fri, 30 Mar 2012 14:45:16 -0700, Jesse Barnes jbar...@virtuousgeek.org wrote: So it looks like we can't use this on the BLT ring in SNB or IVB. So we'll need a render ring BLT routine to use here instead (hello SNA). Or a semaphore to block the render ring? -- keith.pack...@intel.com ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: extract gt interrupt handler
On Fri, 30 Mar 2012 11:28:40 -0700 Jesse Barnes jbar...@virtuousgeek.org wrote: On Fri, 30 Mar 2012 20:24:35 +0200 Daniel Vetter daniel.vet...@ffwll.ch wrote: vlv, ivb and snb all share the gen6+ gt irq handling. 3 copies of the same stuff is a bit much, so extract it into a little helper. Now ilk has a different gt irq handling than snb, but shares the same irq handler (due to the similar display block). So also extract the ilk gt irq handling to clearly separate these two things. Nice side effect of this is that we can complete Ben Widawsky's gen6+ irq bit #define cleanup and call the render irq also with the GEN6 alias. Beforehand that code was shared with ilk, and neither option really made much sense. As a bonus this enables the error interrupt handling lifted from the vlv code on snb and ivb, too. Signed-Off-by: Daniel Vetter daniel.vet...@ffwll.ch Nice cleanup. Though I don't really like the IS_GEN5 branch in ironlake_irq_handler... might be nicer to just bite the bullet and have a mostly duplicate snb irq handler. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org I agree with Jesse. Bite the bullet, you're already +LOC (see below), may as well give it a nice clean split. Personally, I'd like to give you crap about the fact that your cleanup had a +LOC, which came up recently regarding my ILK context stuff. Antagonized-by: Ben Widawsky b...@bwidawsk.net ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
