Bug 90734 - glBufferSubData is corrupting data when buffer is > 32k
Summary: glBufferSubData is corrupting data when buffer is > 32k
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Ian Romanick
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-29 03:42 UTC by kaillasse91
Modified: 2015-09-01 15:41 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
test case (6.58 KB, text/plain)
2015-06-02 00:37 UTC, kaillasse91
Details

Description kaillasse91 2015-05-29 03:42:17 UTC
The behaviour of my application is very different with mesa and a Sandy bridge HD2000 gpu.

Using INTEL_DEBUG=perf I see a lot of
'Using a blit copy to avoid stalling on glBufferSubData(6336, 6408) (0kb) to a busy (0-72000) buffer object.'
with whatever the range I was updating is.

Getting the buffer back with glGetBufferSubData shows that the buffer is sometimes not/impartially (?) updated.

Updating the whole buffer or forcing a sync with glFinish corrects the errors.

For info. :

void ShapeCollectionGl::updateBufferRange(int startIdx, int endIdx)
{
    glBindVertexArray(vao_);

    glBindBuffer(GL_ARRAY_BUFFER, vbo_);

    int offPos = shapesGl_[startIdx].offPos;
    int sizeData = shapesGl_[endIdx].offPos + shapesGl_[endIdx].pos.size() - offPos;

    // glFinish() here solves the problem

    glBufferSubData(GL_ARRAY_BUFFER,
                    offPos * sizeof(VertexData),
                    sizeData * sizeof(VertexData),
                    &bufData_[offPos]);
                    
    // reading the buffer back here with glGetBufferSubData shows the difference

    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, ebo_);

    int offIdx = shapesGl_[startIdx].offIdx;
    int sizeIdx = shapesGl_[endIdx].offIdx + shapesGl_[endIdx].idx.size() - offIdx;

    glBufferSubData(GL_ELEMENT_ARRAY_BUFFER,
                    offIdx * sizeof(GLuint),
                    sizeIdx * sizeof(GLuint),
                    &bufIdx_[offIdx]);

    glBindVertexArray(0);
}

00:02.0 VGA compatible controller [0300]: Intel Corporation 2nd Generation Core Processor Family Integrated Graphics Controller [8086:0102] (rev 09) (prog-if 00 [VGA controller])

OpenGL core profile version string: 3.3 (Core Profile) Mesa 10.7.0-devel (git-10aacf5 2015-05-28 trusty-oibaf-ppa)

Linux 4.0.4-040004-generic #201505171336 SMP Sun May 17 17:37:22 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
Comment 1 kaillasse91 2015-06-02 00:37:31 UTC
Created attachment 116223 [details]
test case
Comment 2 kaillasse91 2015-06-02 00:40:34 UTC
Comment on attachment 116223 [details]
test case


Added a test case.
Also tested on a gen4 chip, and the error appears.

From oibaf ppa:
xserver-xorg-video-intel
2:2.99.917+git1506011932.189ad4~gd~t
Comment 3 Chris Wilson 2015-06-06 08:17:11 UTC
diff --git a/src/mesa/drivers/dri/i965/intel_blit.c b/src/mesa/drivers/dri/i965/intel_blit.c
index 4bfa0ef..a73da35 100644
--- a/src/mesa/drivers/dri/i965/intel_blit.c
+++ b/src/mesa/drivers/dri/i965/intel_blit.c
@@ -539,23 +539,26 @@ intel_emit_linear_blit(struct brw_context *brw,
     */
    pitch = ROUND_DOWN_TO(MIN2(size, (1 << 15) - 1), 4);
    height = (pitch == 0) ? 1 : size / pitch;
+   if (height) {
+      src_x = src_offset % 64;
+      dst_x = dst_offset % 64;
+      ok = intelEmitCopyBlit(brw, 4,
+                             pitch, src_bo, src_offset - src_x, I915_TILING_NONE,
+                             pitch, dst_bo, dst_offset - dst_x, I915_TILING_NONE,
+                             src_x/4, 0, /* src x/y */
+                             dst_x/4, 0, /* dst x/y */
+                             pitch/4, height, /* w, h */
+                             GL_COPY);
+      if (!ok)
+         _mesa_problem(ctx, "Failed to linear blit %dx%d\n", pitch, height);
+
+      src_offset += pitch * height;
+      dst_offset += pitch * height;
+      size -= pitch * height;
+   }
+
    src_x = src_offset % 64;
    dst_x = dst_offset % 64;
-   ok = intelEmitCopyBlit(brw, 1,
-                         pitch, src_bo, src_offset - src_x, I915_TILING_NONE,
-                         pitch, dst_bo, dst_offset - dst_x, I915_TILING_NONE,
-                         src_x, 0, /* src x/y */
-                         dst_x, 0, /* dst x/y */
-                         pitch, height, /* w, h */
-                         GL_COPY);
-   if (!ok)
-      _mesa_problem(ctx, "Failed to linear blit %dx%d\n", pitch, height);
-
-   src_offset += pitch * height;
-   dst_offset += pitch * height;
-   src_x = src_offset % 64;
-   dst_x = dst_offset % 64;
-   size -= pitch * height;
    assert (size < (1 << 15));
    pitch = ALIGN(size, 4);
Comment 4 Chris Wilson 2015-06-06 08:18:41 UTC
No space in margin for explanation! (It's the coordinates after the alignment adjustment exceeding 32k, the fix here is to translate them into 32bpp copy instead for the well aligned large transfer.)
Comment 5 kaillasse91 2015-06-06 20:33:57 UTC
Well, I built mesa with the patch and the test is ok but my application is not. I use various buffer size and update is quasi random so I modified the test case to reflect this and the first errors were at the 64k boundary.

Changing from :
//#define ELEMENT_CNT (4095 * 2) // NO ERROR
#define ELEMENT_CNT (4096 * 2) // ERROR, >= 4096

to :

//#define ELEMENT_CNT (8191 * 2) // NO ERROR
#define ELEMENT_CNT (8192 * 2) //  ERROR, >= 8192

catches more errors.

Tested on sandy bridge hd 2000, in bytes as it seems to be the relevant factor :

buffer size 65536 updating [44-65536] size 65492 bytes
buffer size 65536 updating [64-65536] size 65472 bytes
buffer size 65536 updating [56-65536] size 65480 bytes

And increasing the buffer size:
buffer size 400000 updating [50820-149076] size 98256 bytes
buffer size 400000 updating [11312-142364] size 131052 bytes

buffer size 4000000 updating [3255368-3484676] size 229308 bytes
buffer size 4000000 updating [2179900-2802388] size 622488 bytes
...
Comment 6 Chris Wilson 2015-08-22 08:44:48 UTC
Sorry for the late reply, I had rewritten the patch to try and make the logic more robust, could you please test http://patchwork.freedesktop.org/patch/56678/ ?
Comment 7 kaillasse91 2015-08-24 20:05:12 UTC
(In reply to Chris Wilson from comment #6)
> Sorry for the late reply, I had rewritten the patch to try and make the
> logic more robust, could you please test
> http://patchwork.freedesktop.org/patch/56678/ ?

I have exhumed the old version of my application with the test case included, compiled current git mesa (Mesa 11.1.0-devel (git-000e225)) and applied the patch manually.

Testing without the patch applied give me the errors described in my initial bug report.

Testing with the patch applied and running with a buffer of max size 125000 * 3 vertices : millions of changes later, no errors logged and the output is visually as expected. I've only tested on sandy bridge hd2000, but this seems ok to me.
Comment 8 Chris Wilson 2015-09-01 15:41:59 UTC
commit d38a5601068ae1d923efece8f28757777f4474e4
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sat Jun 6 09:33:33 2015 +0100

    i965: Prevent coordinate overflow in intel_emit_linear_blit


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.