[PATCH] drm/i915/execbuffer: Reorder relocations to match new object order

[PATCH] drm/i915/execbuffer: Reorder relocations to match new object order

[Chris Wilson]

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.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---

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(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index e698343..6b34e98 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -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;
 }
 
-- 
1.7.2.3

Re: [PATCH] drm/i915/execbuffer: Reorder relocations to match new object order

[Linus Torvalds]

On Wed, Jan 12, 2011 at 4:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> 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

Re: [PATCH] drm/i915/execbuffer: Reorder relocations to match new object order

[Chris Wilson]

On Wed, 12 Jan 2011 16:27:46 -0800, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Wed, Jan 12, 2011 at 4:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > 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
 
-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/execbuffer: Reorder relocations to match new object order

[Linus Torvalds]

On Wed, Jan 12, 2011 at 4:48 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, 12 Jan 2011 16:27:46 -0800, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>
>> 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.

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] drm/i915/execbuffer: Clear domains before beginning reloc processing

[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.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6b34e98..8db88e3 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -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)
-- 
1.7.2.3

Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing

[Linus Torvalds]

On Thu, Jan 13, 2011 at 3:07 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 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

[Chris Wilson]

On Thu, 13 Jan 2011 07:54:59 -0800, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Thu, Jan 13, 2011 at 3:07 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 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 processing

Indan Zupancic (1):
      drm/i915/panel: The backlight is enabled if the current value is non-zero

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing

[Alessandro Suardi]

On Thu, Jan 13, 2011 at 5:10 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 13 Jan 2011 07:54:59 -0800, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> On Thu, Jan 13, 2011 at 3:07 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > 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

>> 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 processing
>
> Indan 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
--alessandro

 "There's always a siren singing you to shipwreck"

   (Radiohead, "There There")

Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing

[Linus Torvalds]

On Thu, Jan 13, 2011 at 8:10 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> 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

[Chris Wilson]

[Let's try this again. There are times when I merely dislike my VPN.]

On Fri, 14 Jan 2011 09:37:20 -0800, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Thu, Jan 13, 2011 at 8:10 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > 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(-)

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/execbuffer: Clear domains before beginning reloc processing

[Dave Airlie]

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.

On Fri, 14 Jan 2011, Chris Wilson wrote:

> [Let's try this again. There are times when I merely dislike my VPN.]
> 
> On Fri, 14 Jan 2011 09:37:20 -0800, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > On Thu, Jan 13, 2011 at 8:10 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >
> > > 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(-)
> 
>