drm/i915/execbuffer: Reorder relocations to match new object order
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): id:[email protected]
PATCH 1/1 drm/i915/execbuffer: Reorder relocations to match new object order
On the fault path, commit 6fe4f140 introduction a regression whereby it changed the sequence of the objects but continued to use the original ordering of relocation entries. The result was that incorrect GTT offsets were being fed into the execbuffer causing lots of misrendering and potential hangs.
---
This is what I'm running with at the moment. It only appears to affect the slow path which hopefully explains why it is relatively infrequent... -Chris
--- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++++------ 1 files changed, 9 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_gem_execbuffer.c +9 -6
@@ -636,6 +636,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev, {struct drm_i915_gem_relocation_entry *reloc;struct drm_i915_gem_object *obj; +int *reloc_offset;int i, total, ret;/* We may process another execbuffer during the unlock... */
@@ -653,8 +654,11 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,for (i =0 ; i < count; i++) total += exec[i].relocation_count; + reloc_offset =drm_malloc_ab (count,sizeof (*reloc_offset)); reloc =drm_malloc_ab (total,sizeof (*reloc)); -if (reloc ==NULL ) { +if (reloc ==NULL || reloc_offset ==NULL ) { +drm_free_large (reloc); +drm_free_large (reloc_offset);mutex_lock (&dev->struct_mutex);return -ENOMEM ; }
@@ -672,6 +676,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev, goto err; } + reloc_offset[i] = total; total += exec[i].relocation_count; }
@@ -705,17 +710,14 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,if (ret) goto err; - total =0 ;list_for_each_entry (obj, objects, exec_list) { +int offset = obj->exec_entry - exec; obj->base.pending_read_domains =0 ; obj->base.pending_write_domain =0 ; ret =i915_gem_execbuffer_relocate_object_slow (obj, eb, - reloc + total); + reloc + reloc_offset[offset]);if (ret) goto err; - - total += exec->relocation_count; - exec++; }/* Leave the user relocations as are, this is the painfully slow path,
@@ -726,6 +728,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev, err:drm_free_large (reloc); +drm_free_large (reloc_offset);return ret; }
Signature · 1 line
1.7.2.3
Quoted 3 lines · Chris Wilson
This is what I'm running with at the moment. It only appears to affect the slow path which hopefully explains why it is relatively infrequent...
I think you're still missing some case.
This improves the video case a lot - I can't see any artifacts there - but I still get occasional corruption when moving between the "share" and "rate" buttons, and they end up drawing incorrectly.
So you're close. But no cigar.
Linus
Quoted 10 lines · Linus Torvalds
Quoted 3 lines · Chris Wilson
This is what I'm running with at the moment. It only appears to affect the slow path which hopefully explains why it is relatively infrequent...
I think you're still missing some case.
This improves the video case a lot - I can't see any artifacts there - but I still get occasional corruption when moving between the "share" and "rate" buttons, and they end up drawing incorrectly.
Now we're probably drifting into other bug territory... ;-)
I'll look again in the morning. Usually running the conformance test suites in parallel is enough to exercise the worst-case behaviour. -Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Quoted 4 lines · Chris Wilson
Quoted 3 lines · Linus Torvalds
I think you're still missing some case.
Quoted 8 lines
Quoted 3 lines
This improves the video case a lot - I can't see any artifacts there - but I still get occasional corruption when moving between the "share" and "rate" buttons, and they end up drawing incorrectly.
Now we're probably drifting into other bug territory... ;-)
I'll look again in the morning. Usually running the conformance test suites in parallel is enough to exercise the worst-case behaviour.
I'm pretty dang sure it's still the same bug.
I went back to the revert, and tried very hard, but I cannot reproduce even a whiff of the corruption with the revert. No corruption what-so-ever.
With your patch, the corruption is _less_, but it's absolutely there. So the re-ordering of the object list is doing something else too.
Linus
PATCH 1/1 drm/i915/execbuffer: Clear domains before beginning reloc processing
After reordering the sequence of relocating objects, commit 6fe4f1404, we can no longer rely on seeing all reloc targets prior to performing the relocation. So we need to clear the relocation domains earlier.
--- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_gem_execbuffer.c +3 -4
@@ -464,8 +464,6 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,int ret;list_for_each_entry (obj, objects, exec_list) { - obj->base.pending_read_domains =0 ; - obj->base.pending_write_domain =0 ; ret =i915_gem_execbuffer_relocate_object (obj, eb);if (ret)return ret;
@@ -505,6 +503,9 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring,list_move (&obj->exec_list, &ordered_objects); else list_move_tail(&obj->exec_list, &ordered_objects); + + obj->base.pending_read_domains =0 ; + obj->base.pending_write_domain =0 ; }list_splice (&ordered_objects, objects);
@@ -712,8 +713,6 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,list_for_each_entry (obj, objects, exec_list) {int offset = obj->exec_entry - exec; - obj->base.pending_read_domains =0 ; - obj->base.pending_write_domain =0 ; ret =i915_gem_execbuffer_relocate_object_slow (obj, eb, reloc + reloc_offset[offset]);if (ret)
Signature · 1 line
1.7.2.3
Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing
Quoted 3 lines · Chris Wilson
After reordering the sequence of relocating objects, commit
6fe4f1404, we can no longer rely on seeing all reloc targets prior to performing the relocation. So we need to clear the relocation domains earlier.
Yup, this one (together with the previous fix) seems to get rid of all the artifacts.
Should I just apply them as patches, or do you have a git branch to pull from?
Linus
Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing
Quoted 9 lines · Linus Torvalds
Quoted 3 lines · Chris Wilson
After reordering the sequence of relocating objects, commit
6fe4f1404, we can no longer rely on seeing all reloc targets prior to performing the relocation. So we need to clear the relocation domains earlier.Yup, this one (together with the previous fix) seems to get rid of all the artifacts.
Should I just apply them as patches, or do you have a git branch to pull from?
They will be in drm-intel-fixes. There have been a couple of other patches, for the compiler warning, the typo that Indan spotted and for modparam to workaround the U160, so I'll send the pull request it via Dave. -Chris
$ git shortlog linus..upstream/drm-intel-fixes
Chris Wilson (5):
drm/i915/debugfs: Correct format after changing type of err object 'size'
drm/i915: Add a module option to override the use of SSC
drm/i915: Fix error handler to capture the first batch after the seqno
drm/i915/execbuffer: Reorder relocations to match new object order
drm/i915/execbuffer: Clear domains before beginning reloc processingIndan Zupancic (1):
drm/i915/panel: The backlight is enabled if the current value is non-zero
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing
Quoted 8 lines · Chris Wilson
Quoted 7 lines · Linus Torvalds
Quoted 3 lines · Chris Wilson
After reordering the sequence of relocating objects, commit
6fe4f1404, we can no longer rely on seeing all reloc targets prior to performing the relocation. So we need to clear the relocation domains earlier.Yup, this one (together with the previous fix) seems to get rid of all the artifacts.
Just to add that this patch also fixes my gnome top and bottom panels
on F14 where mouseover on the icons caused them to disappear and reappear, partly or entirely; in detail,
2.6.37-git7 => ok
2.6.37-git8 => ok
2.6.37-git9 => bad (mouseover on panels has icons disappearing/reappearing) 2.6.37-git9 + patch => ok
Quoted 26 lines
Quoted 1 line
Should I just apply them as patches, or do you have a git branch to pull from?
They will be in drm-intel-fixes. There have been a couple of other patches, for the compiler warning, the typo that Indan spotted and for modparam to workaround the U160, so I'll send the pull request it via Dave. -Chris
$ git shortlog linus..upstream/drm-intel-fixes Chris Wilson (5): drm/i915/debugfs: Correct format after changing type of err object 'size' drm/i915: Add a module option to override the use of SSC drm/i915: Fix error handler to capture the first batch after the seqno drm/i915/execbuffer: Reorder relocations to match new object order drm/i915/execbuffer: Clear domains before beginning reloc processingIndan Zupancic (1):
drm/i915/panel: The backlight is enabled if the current value is non-zero--
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 5 lines
--alessandro "There's always a siren singing you to shipwreck" (Radiohead, "There There")
Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing
In reply to Chris WilsonQuoted 4 lines · Chris Wilson
They will be in drm-intel-fixes. There have been a couple of other patches, for the compiler warning, the typo that Indan spotted and for modparam to workaround the U160, so I'll send the pull request it via Dave.
My tree has been in a known-broken state for too damn long now. Please send the pull request to me directly. Dave may be flooded out in Brisbane or something.
And if you're not certain about some of the other changes, just tell me, and I'll apply the fix patches only. Or I will revert the thing that caused the problem. Having a known-broken tree without the fixes in it is _not_ acceptable, it just makes testing harder for everybody else!
Linus
Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing
[Let's try this again. There are times when I merely dislike my VPN.]
Quoted 9 lines · Linus Torvalds
Quoted 4 lines · Chris Wilson
They will be in drm-intel-fixes. There have been a couple of other patches, for the compiler warning, the typo that Indan spotted and for modparam to workaround the U160, so I'll send the pull request it via Dave.
My tree has been in a known-broken state for too damn long now. Please send the pull request to me directly. Dave may be flooded out in Brisbane or something.
This is what I have waiting in drm-intel-fixes. Besides the regression noted above, there is also a critical stability fix for SNB mobile, a few patches for older regressions (LVDS on U160, no LVDS on AOpen, preserve the backlight value from boot and correct a couple of device strings) and one to keep the compiler quiet.
-Chris
The following changes since commit f878133bf022717b880d0e0995b8f91436fd605c:
Merge branch 'drm-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6 (2011-01-12 08:40:25 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/ickle/drm-intel.git drm-intel-fixes
Chris Wilson (6):
drm/i915/debugfs: Correct format after changing type of err object 'size' drm/i915: Add a module option to override the use of SSC drm/i915: Fix error handler to capture the first batch after the seqno drm/i915/execbuffer: Reorder relocations to match new object order drm/i915/execbuffer: Clear domains before beginning reloc processing drm/i915: Disable GPU semaphores on SandyBridge mobile
Indan Zupancic (1):
drm/i915/panel: The backlight is enabled if the current value is non-zero
Knut Petersen (1):
drm/i915/lvds: Add AOpen i915GMm-HFS to the list of false-positive LVDS
Oswald Buddenhagen (1):
agp/intel: Fix device names of i845 and 845G
drivers/char/agp/intel-agp.c | 4 ++-- drivers/char/agp/intel-gtt.c | 2 +- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 3 +++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem_execbuffer.c | 25 ++++++++++++++----------- drivers/gpu/drm/i915/i915_irq.c | 2 +- drivers/gpu/drm/i915/intel_bios.c | 17 ++++++----------- drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++------ drivers/gpu/drm/i915/intel_lvds.c | 8 ++++++++ drivers/gpu/drm/i915/intel_panel.c | 2 +- 11 files changed, 49 insertions(+), 34 deletions(-)
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing
Ack from me for direct pull, I'm not flooded but I have no access to my dev machines in the office which all got powered down.
I've also been distracted with some deadlines for the bill payers.
Dave.
Quoted 59 lines · Chris Wilson
[Let's try this again. There are times when I merely dislike my VPN.]
Quoted 9 lines · Linus Torvalds
Quoted 4 lines · Chris Wilson
They will be in drm-intel-fixes. There have been a couple of other patches, for the compiler warning, the typo that Indan spotted and for modparam to workaround the U160, so I'll send the pull request it via Dave.
My tree has been in a known-broken state for too damn long now. Please send the pull request to me directly. Dave may be flooded out in Brisbane or something.
This is what I have waiting in drm-intel-fixes. Besides the regression noted above, there is also a critical stability fix for SNB mobile, a few patches for older regressions (LVDS on U160, no LVDS on AOpen, preserve the backlight value from boot and correct a couple of device strings) and one to keep the compiler quiet.
-ChrisThe following changes since commit
f878133bf022717b880d0e0995b8f91436fd605c:Merge branch 'drm-fixes' of
git://git.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6(2011-01-12 08:40:25 -0800)are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/ickle/drm-intel.gitdrm-intel-fixesChris Wilson (6):
drm/i915/debugfs: Correct format after changing type of err object 'size' drm/i915: Add a module option to override the use of SSC drm/i915: Fix error handler to capture the first batch after the seqno drm/i915/execbuffer: Reorder relocations to match new object order drm/i915/execbuffer: Clear domains before beginning reloc processing drm/i915: Disable GPU semaphores on SandyBridge mobileIndan Zupancic (1):
drm/i915/panel: The backlight is enabled if the current value is non-zeroKnut Petersen (1):
drm/i915/lvds: Add AOpen i915GMm-HFS to the list of false-positive LVDSOswald Buddenhagen (1):
agp/intel: Fix device names of i845 and 845Gdrivers/char/agp/intel-agp.c | 4 ++-- drivers/char/agp/intel-gtt.c | 2 +- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 3 +++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem_execbuffer.c | 25 ++++++++++++++----------- drivers/gpu/drm/i915/i915_irq.c | 2 +- drivers/gpu/drm/i915/intel_bios.c | 17 ++++++----------- drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++------ drivers/gpu/drm/i915/intel_lvds.c | 8 ++++++++ drivers/gpu/drm/i915/intel_panel.c | 2 +- 11 files changed, 49 insertions(+), 34 deletions(-)