Re: [Intel-gfx] [Mesa-dev] [PATCH 0/5] First batch of gm45 clipping/interpolation fixes
On Fri, Jul 13, 2012 at 02:45:10PM -0700, Kenneth Graunke wrote: Sorry...been really busy, and most of us haven't actually spent much if any time in the clipper shaders. I'll try and review it within a week. Ok cool, lack of time is something I completely understand :-) Despite the lack of response, I am really excited to see that you're working on this---this is a huge step toward bringing GL 3.x back to Gen4/5, and we're all really glad to see it happen! Excellent. I was starting to wonder if gen4/5 was abandoned (by lack of resources if anything), nice to see it isn't. OG. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/13] drm/i915: Replace the pending_gpu_write flag with an explicit seqno
On Fri, 13 Jul 2012 17:41:01 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Jul 13, 2012 at 02:14:07PM +0100, Chris Wilson wrote: As we always flush the GPU cache prior to emitting the breadcrumb, we no longer have to worry about the deferred flush causing the pending_gpu_write to be delayed. So we can instead utilize the known last_write_seqno to hopefully minimise the wait times. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk I like this, but I'd prefer if this would be at the end of the series as a neat improvement - I'd really prefer if we get together a somewhat minimal fix to take care of the flushing list and intel_begin_ring interruptable patch. The reason is that the merge window approaches fast, and if we're unlucky the current -next cycle won't make it into 3.6, so I'd have to send Dave a smaller pile of fixes. Which this doesn't really look like. Or do I again miss something? It's not really my brightest day ;-) This is just to enable removing the pending_gpu_write bit, and really if the flushing list removal wasn't to enable this patch, what was the point? :-p -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] Panning broken in i915?
On 2012/07/13 20:59 (GMT-0400) Felix Miata composed: I wouldn't ask here, except Bugzilla seems stuck on unreachable. Anyone know when it should be back online? Anyone know the status of panning? For me, mouse cannot move into the extended area, so only content can't go there, not my eyes. It's also broken on Nouveau. I finally got through to Bugzilla and found https://bugs.freedesktop.org/show_bug.cgi?id=39949 -- The wise are known for their understanding, and pleasant words are persuasive. Proverbs 16:21 (New Living Translation) Team OS/2 ** Reg. Linux User #211409 ** a11y rocks! Felix Miata *** http://fm.no-ip.com/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: fix invalid reference handling of the default ctx obj
On Fri, 13 Jul 2012 17:37:14 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Jul 13, 2012 at 02:14:05PM +0100, Chris Wilson wrote: Otherwise we end up trying to unpin a freed object and BUG. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky b...@bwidawsk.net Afact this patch contains quite some code refactoring that does not relate directly to the fix (or if it does, I fail to see the direct relevance). So I think this either needs an explanation in the commit message or be put into a separate patch (I agree though for actual code cleanups). For the fix itself I seem to be a bit dense again - the only thing I see is that you move the refcount handling into do_switch. Afacs we do the ref-handling in both cases only when do_switch is successful, and also right at the end of do_switch (or right afterwards). So can you please enlighten your clueless maintainer a bit an explain how things blow up? The fix is that the reference handling was only done on one path, not both. Hence the default_ctx ends up being used-after-free. The rest of it was just unwinding the code to get to finding the bug... -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 05/13] drm/i915: Insert a flush between batches if the breadcrumb was dropped
On Fri, 13 Jul 2012 17:46:20 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Jul 13, 2012 at 02:14:08PM +0100, Chris Wilson wrote: If we drop the breadcrumb request after a batch due to a signal for example we aim to fix it up at the next opportunity. In this case we emit a second batchbuffer with no waits upon the first and so no opportunity to insert the missing request, so we need to emit the missing flush for coherency. (Note that that invalidating the render cache is the same as flushing it, so there should have been no observable corruption.) Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Imo still too meager commit message ;-) As I've said in the previous mail, I'd like some mention of the two commits that made this disaster possible (put the blame on me where it is due). And I think some more in-detail walk-thru of how things blow up would be great. And the Bugzilla link for the QA bugreport. Sure, in the patch I thought I was sending I had an extra paragraph: As a side effect this will also paper over issues such as https://bugs.freedesktop.org/show_bug.cgi?id=52040 whereby we clear the write_domain on objects on the defunct gpu_write_list. References: https://bugs.freedesktop.org/show_bug.cgi?id=52040 Also, I still don't understand why this patch here isn't enough to fix up the fallout. So if you can enlighten me where/why stuff blows up even with this I'd highly appreciate. Not just because not understanding bugs makes me queasy, but also to have a clear picture of what I'd need to send to Dave it this -next cycle misses 3.6. The remaining fallout is that we still end up using the flushing-list, as revealed by *adding* a WARN. To end up in that situation we must retire an object with a write-domain still set. But how can this be possible if we always clear the write_list prior to the request/retirment? I thought I had it, being sneaky with the use of INSTRUCTION write domain for pipe-control. However, looks like I'm going to have to reproduce with some more debugging. -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 02/13] drm/i915: fix invalid reference handling of the default ctx obj
On Sat, Jul 14, 2012 at 10:55:19AM +0100, Chris Wilson wrote: On Fri, 13 Jul 2012 17:37:14 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Jul 13, 2012 at 02:14:05PM +0100, Chris Wilson wrote: Otherwise we end up trying to unpin a freed object and BUG. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky b...@bwidawsk.net Afact this patch contains quite some code refactoring that does not relate directly to the fix (or if it does, I fail to see the direct relevance). So I think this either needs an explanation in the commit message or be put into a separate patch (I agree though for actual code cleanups). For the fix itself I seem to be a bit dense again - the only thing I see is that you move the refcount handling into do_switch. Afacs we do the ref-handling in both cases only when do_switch is successful, and also right at the end of do_switch (or right afterwards). So can you please enlighten your clueless maintainer a bit an explain how things blow up? The fix is that the reference handling was only done on one path, not both. Hence the default_ctx ends up being used-after-free. The rest of it was just unwinding the code to get to finding the bug... Yeah, I've figured this out somewhat later yesterday. Still, a bugfix shouldn't resemble a riddle more than a simple patch ... Afacs we only need to move the reference count handling from i915_switch_context to do_switch (and mention in the commit message that it's been missing when creating the default context). -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 01/13] drm/i915: Flush the context object from the CPU caches upon creation
On Fri, Jul 13, 2012 at 02:14:04PM +0100, Chris Wilson wrote:
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 9ae3f2c..90857f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -225,6 +225,13 @@ static int create_default_context(struct
drm_i915_private *dev_priv)
return ret;
}
+ ret = i915_gem_object_set_to_gtt_domain(ctx-obj, true);
+ if (ret) {
+ i915_gem_object_unpin(ctx-obj);
+ do_destroy(ctx);
+ return ret;
+ }
+
ret = do_switch(NULL, ctx, 0);
if (ret) {
i915_gem_object_unpin(ctx-obj);
@@ -396,8 +403,6 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
* MI_SET_CONTEXT instead of when the next seqno has completed.
*/
if (from_obj != NULL) {
- from_obj-base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
- i915_gem_object_move_to_active(from_obj, ring, seqno);
/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
* whole damn pipeline, we don't need to explicitly mark the
* object dirty. The only exception is that the context must be
@@ -405,6 +410,9 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
* able to defer doing this until we know the object would be
* swapped, but there is no way to do that yet.
*/
+ from_obj-base.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+ from_obj-base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
Presuming I understand things correctly, setting write_domain to non-zero
will result in the ctx object landing on the flushing list when we retire
it from the active list. But it isn't being added to the ring's
gpu_write_list, so it won't ever get off that flushing list and eventually
result in the BUG_ON(seqno == 0) when we try to wait for it after a flush.
So afact this first patch here seems to add another instance of the very
bug this patch series tries squash ... Additionally I'm still hunting for
that other failure case, which can't be fixed by adding the flush in
execbuffer if ring-gpu_caches_dirty is set.
/me is still lost
-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 01/13] drm/i915: Flush the context object from the CPU caches upon creation
On Sat, 14 Jul 2012 13:58:58 +0200, Daniel Vetter dan...@ffwll.ch wrote: So afact this first patch here seems to add another instance of the very bug this patch series tries squash ... Additionally I'm still hunting for that other failure case, which can't be fixed by adding the flush in execbuffer if ring-gpu_caches_dirty is set. /me is still lost Now all that you are missing is that we only flush GPU_DOMAINS when on the gpu_write_list. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/13] drm/i915: Flush the context object from the CPU caches upon creation
On Sat, Jul 14, 2012 at 2:48 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Sat, 14 Jul 2012 13:58:58 +0200, Daniel Vetter dan...@ffwll.ch wrote: So afact this first patch here seems to add another instance of the very bug this patch series tries squash ... Additionally I'm still hunting for that other failure case, which can't be fixed by adding the flush in execbuffer if ring-gpu_caches_dirty is set. /me is still lost Now all that you are missing is that we only flush GPU_DOMAINS when on the gpu_write_list. Hm, I guess we've seen that one before ;-) But that would require that we add an object with a non-gpu write domain to the active list, but both with or without this patch I don't see how that should happen ... I guess a WARN in move_to_active won't hurt (and we should have done that when fixing the GTT_DOMAIN relocation hole in execbuf), but besides that I'm still as dense as ever, it seems. -Daniel -- Daniel Vetter daniel.vet...@ffwll.ch - +41 (0) 79 364 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 05/13] drm/i915: Insert a flush between batches if the breadcrumb was dropped
On Fri, Jul 13, 2012 at 02:14:08PM +0100, Chris Wilson wrote: If we drop the breadcrumb request after a batch due to a signal for example we aim to fix it up at the next opportunity. In this case we emit a second batchbuffer with no waits upon the first and so no opportunity to insert the missing request, so we need to emit the missing flush for coherency. (Note that that invalidating the render cache is the same as flushing it, so there should have been no observable corruption.) Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Ok, now that I've got some more clue about how this all blows up, I've merged this patch here (with a rather decently pimped commit message). Thanks for digging into this feeding me the lacking clue. Cheers, 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: don't forget the PCH backlight registers
From: Paulo Zanoni paulo.r.zan...@intel.com
When we enable/disable the CPU backlight registers we can't forget to
enable/disable the PCH backlight registers. Since we're using the CPU
registers we should also unset the override bit.
Fixes a regression on the following commit:
drm/i915: properly enable the blc controller on the right pipe
The commit just deleted the code that sets the PCH registers, so it
was relying on the values set by the BIOS. I told my BIOS to boot on
the DVI monitor instead of the LVDS panel, so I noticed the bug.
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
drivers/gpu/drm/i915/intel_panel.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
I really think this patch fixes fd.o bug #51463. I'm waiting
confirmation from the bug reporter.
Also, I did not really do the git bisect: I understood it is a
regression from that commit just by reading the commit (and by
the fact that backlight used to work until I upgraded my
Kernel).
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index 58c7ee7..10c7d39 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -289,11 +289,17 @@ void intel_panel_disable_backlight(struct drm_device *dev)
intel_panel_actually_set_backlight(dev, 0);
if (INTEL_INFO(dev)-gen = 4) {
- uint32_t reg;
+ uint32_t reg, tmp;
reg = HAS_PCH_SPLIT(dev) ? BLC_PWM_CPU_CTL2 : BLC_PWM_CTL2;
I915_WRITE(reg, I915_READ(reg) ~BLM_PWM_ENABLE);
+
+ if (HAS_PCH_SPLIT(dev)) {
+ tmp = I915_READ(BLC_PWM_PCH_CTL1);
+ tmp = ~BLM_PCH_PWM_ENABLE;
+ I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
+ }
}
}
@@ -333,6 +339,13 @@ void intel_panel_enable_backlight(struct drm_device *dev,
I915_WRITE(reg, tmp);
POSTING_READ(reg);
I915_WRITE(reg, tmp | BLM_PWM_ENABLE);
+
+ if (HAS_PCH_SPLIT(dev)) {
+ tmp = I915_READ(BLC_PWM_PCH_CTL1);
+ tmp |= BLM_PCH_PWM_ENABLE;
+ tmp = ~BLM_PCH_OVERRIDE_ENABLE;
+ I915_WRITE(BLC_PWM_PCH_CTL1, tmp);
+ }
}
}
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert drm/i915: allow PCH PWM override on IVB
Hi 2012/6/27 Daniel Vetter daniel.vet...@ffwll.ch: This reverts commit f82cfb6bcda164ef3a66b8c3fc549b1f9bdd09ad. This breaks the backlight controls on my IVB asus zenbook with an eDP panel. I guess the right fix would be to read this bit and use either the pch or the cpu register to frob the backlight values. But that is stuff for -next. I was about to propose *exactly* this patch and just noticed it is here, but for -fixes instead of dinq. After playing with the CPU/PCH backlight registers, this is what I concluded: - If you want to control backlight just using the PCH: - You have to turn on BLM_PCH_PWM_ENABLE (PCH backlight enable) - You have to turn on the BLM_PCH_OVERRIDE_ENABLE bit - You have to use BLC_PWM_PCH_CTL2 to set the backlight value - Just ignore the CPU registers - If you want to control backlight using the CPU: - You have to turn on BLM_PWM_ENABLE (CPU backlight enable) - You have to turn on BLM_PCH_PWM_ENABLE (PCH backlight enable) - You have to turn *OFF* the BLM_PCH_OVERRIDE_ENABLE bit - You have to use BLC_PWM_CPU_CTL2 to set the backlight value This ivb_pcm_pwm_override function turns the override bit ON, so it completely relies on the PCH registers. The problem is that our code only changes the CPU registers... It was probably relying on the values set by the bios, and I would guess that changing the backlight levels was probably not working at all (I don't have an IVB machine with LVDS to test). If people start complaining that your revert causes problems my suggestion would be: - Undo your revert, and instead of removing the whole function, just remove the code that turns the override bit on (1 30). Explicitly turn it off. - Then resend the patch that kills the whole function to dinq and apply it on top of the patch I just sent a few minutes ago. Your rework of the backlight registers + my fix should probably make it safe to completely remove ivb_pch_pwm. -- 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: don't forget the PCH backlight registers
On Sat, Jul 14, 2012 at 11:57:12AM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com When we enable/disable the CPU backlight registers we can't forget to enable/disable the PCH backlight registers. Since we're using the CPU registers we should also unset the override bit. Fixes a regression on the following commit: drm/i915: properly enable the blc controller on the right pipe The commit just deleted the code that sets the PCH registers, so it was relying on the values set by the BIOS. I told my BIOS to boot on the DVI monitor instead of the LVDS panel, so I noticed the bug. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Ok, I've double-check bspec, and at least on ilk, snb ivb this seems to be a sane thing to do. Patch queued for -next, thanks. -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
