[PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

[PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

[Peter Clifton]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

Fixes corruption with glBufferSubData on my machine,

Can someone review and push?

-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)

[-- Attachment #2: 0001-intel-Fix-emit_linear_blit-to-use-DWORD-aligned-widt.patch --]
[-- Type: text/x-patch, Size: 1480 bytes --]

>From 1e07dce2b9664861d92917426cbb153cc89a1c8d Mon Sep 17 00:00:00 2001
From: Peter Clifton <pcjc2@cam.ac.uk>
Date: Fri, 5 Nov 2010 14:26:24 +0000
Subject: [PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

The width of the 2D blits used to copy the data is defined as a 16-bit
signed integer, but the pitch must be DWORD aligned. Limit to an integral
number of DWORDs, (1 << 15 - 4) rather than (1 << 15 -1).

Fixes corruption to data uploaded with glBufferSubData.

Signed-off-by: Peter Clifton <pcjc2@cam.ac.uk>
---
 src/mesa/drivers/dri/intel/intel_blit.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/intel/intel_blit.c b/src/mesa/drivers/dri/intel/intel_blit.c
index a74e217..7118898 100644
--- a/src/mesa/drivers/dri/intel/intel_blit.c
+++ b/src/mesa/drivers/dri/intel/intel_blit.c
@@ -483,8 +483,11 @@ intel_emit_linear_blit(struct intel_context *intel,
    /* Blits are in a different ringbuffer so we don't use them. */
    assert(intel->gen < 6);
 
-   /* The pitch is a signed value. */
-   pitch = MIN2(size, (1 << 15) - 1);
+   /* The pitch hits the GPU as a is a signed value, IN DWORDs.
+    * But we want width to match pitch. Max width is (1 << 15 - 1),
+    * rounding that down to the nearest DWORD is 1 << 15 - 4
+    */
+   pitch = MIN2(size, (1 << 15) - 4);
    height = size / pitch;
    ok = intelEmitCopyBlit(intel, 1,
 			  pitch, src_bo, src_offset, I915_TILING_NONE,
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

[Peter Clifton]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

Fixes corruption with glBufferSubData on my machine,

Can someone review and push?

(Resent with code comment fixed)

-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)


[-- Attachment #2: 0001-intel-Fix-emit_linear_blit-to-use-DWORD-aligned-widt.patch --]
[-- Type: text/x-patch, Size: 1480 bytes --]

>From 1e07dce2b9664861d92917426cbb153cc89a1c8d Mon Sep 17 00:00:00 2001
From: Peter Clifton <pcjc2@cam.ac.uk>
Date: Fri, 5 Nov 2010 14:26:24 +0000
Subject: [PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

The width of the 2D blits used to copy the data is defined as a 16-bit
signed integer, but the pitch must be DWORD aligned. Limit to an integral
number of DWORDs, (1 << 15 - 4) rather than (1 << 15 -1).

Fixes corruption to data uploaded with glBufferSubData.

Signed-off-by: Peter Clifton <pcjc2@cam.ac.uk>
---
 src/mesa/drivers/dri/intel/intel_blit.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/intel/intel_blit.c b/src/mesa/drivers/dri/intel/intel_blit.c
index a74e217..7118898 100644
--- a/src/mesa/drivers/dri/intel/intel_blit.c
+++ b/src/mesa/drivers/dri/intel/intel_blit.c
@@ -483,8 +483,11 @@ intel_emit_linear_blit(struct intel_context *intel,
    /* Blits are in a different ringbuffer so we don't use them. */
    assert(intel->gen < 6);
 
-   /* The pitch is a signed value. */
-   pitch = MIN2(size, (1 << 15) - 1);
+   /* The pitch hits the GPU as a is a signed value, IN DWORDs.
+    * But we want width to match pitch. Max width is (1 << 15 - 1),
+    * rounding that down to the nearest DWORD is 1 << 15 - 4
+    */
+   pitch = MIN2(size, (1 << 15) - 4);
    height = size / pitch;
    ok = intelEmitCopyBlit(intel, 1,
 			  pitch, src_bo, src_offset, I915_TILING_NONE,
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

[Chris Wilson]

On Sat, 06 Nov 2010 10:04:31 +0000, Peter Clifton <pcjc2@cam.ac.uk> wrote:
> Fixes corruption with glBufferSubData on my machine,
> 
> Can someone review and push?

Oddly, the pitch for BLT is in bytes and it should be sufficient to be a
multiple of element-size. Or two element-size depending on circumstances. 

Just to satisfy my curiosity, what happens with (1<<15 - 2)?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

[Peter Clifton]

On Sun, 2010-11-07 at 10:25 +0000, Chris Wilson wrote:
> On Sat, 06 Nov 2010 10:04:31 +0000, Peter Clifton <pcjc2@cam.ac.uk> wrote:
> > Fixes corruption with glBufferSubData on my machine,
> > 
> > Can someone review and push?
> 
> Oddly, the pitch for BLT is in bytes and it should be sufficient to be a
> multiple of element-size. Or two element-size depending on circumstances. 
> 
> Just to satisfy my curiosity, what happens with (1<<15 - 2)?

I've not tried that yet, but the PRM does state that BLT pitch is in
DWORDs.


We do this:

   src_pitch *= cpp;
   dst_pitch *= cpp;

Converting a pitch passed to the blit function in _pixels_, to a pitch
in bytes, but further dragons lurking I think:

Also, only on non I915 defined builds of intel_blit.c, we also do things
like:

   if (dst_tiling != I915_TILING_NONE) {
      CMD |= XY_DST_TILED;
      dst_pitch /= 4;
   }


The G45 PRM doesn't appear to specify pitch is in DWORDS _only_ for
tiled surfaces, merely that there is further restriction:

"
Source Pitch (double word aligned) and in DWords: [15:00] 2’s
complement.

For Tiled Src (bit 15 enabled) this pitch is of 512Byte granularity and
can be upto 128Kbytes (or 32KDwords).
"

Truthfully, I'm not quite sure I understand what the implication for a
tiled surface is.. or how pitch should be calculated correctly for that
case.. but it does seem clear that our handing for non-tiled surfaces is
suspect.


-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

[Peter Clifton]

On Tue, 2010-11-09 at 10:52 +0000, Peter Clifton wrote:
> On Sun, 2010-11-07 at 10:25 +0000, Chris Wilson wrote:

> I've not tried that yet, but the PRM does state that BLT pitch is in
> DWORDs.

Gah.. the PRM is badly written in places! In one place it states DWORDs,
then in another you get the actual detail:

"Source Pitch (Offset)
For non-XY Blits with color source operand (SRC_COPY_BLT), the signed
16bit field allows for specifying upto + 32Kbytes signed pitch in bytes
(same as before).

For X, Y Blits with tiled (X) surfaces, the pitch for Color Source will
be 512Byte aligned and should be programmable upto + 128Kbytes. In this
case, this 16bit signed pitch field is used to specify upto + 32KDWords.
For X, Y blits with nontiled color source surfaces (linear surfaces),
this 16bit field can be programmed to byte specification of upto +
32Kbytes (same as before).
"

The similar description for the Destination pitch field DOES NOT go into
any of this detail, nor refer to the source pitch entry.

I'm not sure where it is written that BLITs must be DWORD aligned for
non-tiled XY blits, but I certainly saw corruption on my G45 when it was
not, so my original patch (which Eric committed) seems to help.

Chris, I can try word-aligned if you wish, but am chasing some other
random GPU hangs / crashes at the moment. Fun fun ;)

(PS. Any idea where batchbuffer containing 3D commands, but with a
string of 6-8 MI_NOOP commands would come from? I can't find code to
emit like that in MESA or the 2D driver - I am wondering if the buffer
had become corrupted).

-- 
Peter Clifton

Electrical Engineering Division,
Engineering Department,
University of Cambridge,
9, JJ Thomson Avenue,
Cambridge
CB3 0FA

Tel: +44 (0)7729 980173 - (No signal in the lab!)
Tel: +44 (0)1223 748328 - (Shared lab phone, ask for me)

Re: [PATCH] intel: Fix emit_linear_blit to use DWORD aligned width blits

[Chris Wilson]

On Tue, 09 Nov 2010 11:34:52 +0000, Peter Clifton <pcjc2@cam.ac.uk> wrote:
> On Tue, 2010-11-09 at 10:52 +0000, Peter Clifton wrote:
> > On Sun, 2010-11-07 at 10:25 +0000, Chris Wilson wrote:
> 
> > I've not tried that yet, but the PRM does state that BLT pitch is in
> > DWORDs.
> 
> Gah.. the PRM is badly written in places! In one place it states DWORDs,
> then in another you get the actual detail:

Of course, I was reading another document which made no mention of the
tiling or dword restrictions ;-)

Contradictory, uncrossreferenced, incomplete docs are all we have. 

> Chris, I can try word-aligned if you wish, but am chasing some other
> random GPU hangs / crashes at the moment. Fun fun ;)

No need, life is too short.
 
> (PS. Any idea where batchbuffer containing 3D commands, but with a
> string of 6-8 MI_NOOP commands would come from? I can't find code to
> emit like that in MESA or the 2D driver - I am wondering if the buffer
> had become corrupted).

Keep digging and you'll find a reference to some silicon bugs for which
certain commands (in this case it's probably the URB_FENCE) must not cross
cache-lines. I'm guessing that the MI_NOOPs you've seen are due to us
aligning the following commands to meet such constraints.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre