Hi Pierre,

Thanks!

I've modifed the patch to cast data, and added an additional check to ensure unaligned offsets produce INVALID_VALUE. I'm not sure how to use git send-mail to update a review, so I've just inlined the updated patch here and attached it as well.

 - Anthony


commit 770effba655f97be229e13f894884f0e4c3e604a
Author: Anthony Pesch <ape...@nvidia.com>
Date:   Thu Apr 11 12:37:07 2019 -0400

    arb_texture_buffer_range: Fix buffer alignment in ranges-2 test

The ranges-2 test was failing due to glTexBufferRange returning GL_INVALID_VALUE when the offset parameter wasn't GL_TEXTURE_BUFFER_OFFSET_ALIGNMENT byte aligned.

    From the OpenGL 4.6 Core spec:

An INVALID_VALUE error is generated if offset is not an integer multiple of the
    value of TEXTURE_BUFFER_OFFSET_ALIGNMENT.

diff --git a/tests/spec/arb_texture_buffer_range/ranges-2.c b/tests/spec/arb_texture_buffer_range/ranges-2.c
index 3477e4b52..83a009875 100644
--- a/tests/spec/arb_texture_buffer_range/ranges-2.c
+++ b/tests/spec/arb_texture_buffer_range/ranges-2.c
@@ -91,22 +91,29 @@ float data[] = {
        0, 0,           0.5, 0,
 };

+int aligned_size = 0;
+int chunk_size = 24 * sizeof(float);
+int num_chunks = 4;
+
 enum piglit_result
 piglit_display(void) {
        int i;
-       int chunk_size = 24 * sizeof(float);
        bool pass = true;

        glClearColor(0.2, 0.2, 0.2, 0.2);
        glClear(GL_COLOR_BUFFER_BIT);

-       for (i = 0; i < sizeof(data) / chunk_size; i++) {
+       /* verify unaligned offsets produce an error */
+       glTexBufferRange(GL_TEXTURE_BUFFER, GL_RGBA32F, tbo, aligned_size - 1, 
1);
+       pass &= glGetError() == GL_INVALID_VALUE;
+
+       for (i = 0; i < num_chunks; i++) {
                glTexBufferRange(GL_TEXTURE_BUFFER, GL_RGBA32F,
-                                tbo, i * chunk_size, chunk_size);
+                                tbo, i * aligned_size, chunk_size);
                glDrawArrays(GL_TRIANGLES, 0, 6);
        }

-       for (i = 0; i < sizeof(data) / chunk_size; i++) {
+       for (i = 0; i < num_chunks; i++) {
                float c[4] = {
                        data[i * 24 + 2],
                        data[i * 24 + 3],
@@ -114,7 +121,7 @@ piglit_display(void) {
                        1
                };

-               pass = piglit_probe_rect_rgba(
+               pass &= piglit_probe_rect_rgba(
                        piglit_width * 0.5 * (1 + data[i * 24 + 0]),
                        piglit_height * 0.5 * (1 + data[i * 24 + 1]),
                        piglit_width/2,
@@ -128,8 +135,14 @@ piglit_display(void) {

 void
 piglit_init(int argc, char **argv) {
+       GLint align, i;
+       uint8_t *chunk;
+
        piglit_require_extension("GL_ARB_texture_buffer_range");

+       glGetIntegerv(GL_TEXTURE_BUFFER_OFFSET_ALIGNMENT, &align);
+       aligned_size = chunk_size % align == 0 ? chunk_size : align;
+
        prog = piglit_build_simple_program(vs_source, fs_source);
        glUseProgram(prog);

@@ -138,7 +151,12 @@ piglit_init(int argc, char **argv) {

        glGenBuffers(1, &tbo);
        glBindBuffer(GL_ARRAY_BUFFER, tbo);
-       glBufferData(GL_ARRAY_BUFFER, sizeof(data), data, GL_STATIC_DRAW);
+ glBufferData(GL_ARRAY_BUFFER, aligned_size * num_chunks, NULL, GL_STATIC_DRAW);
+
+       for (i = 0, chunk = (uint8_t *)data; i < num_chunks; i++) {
+               glBufferSubData(GL_ARRAY_BUFFER, aligned_size * i, chunk_size, 
chunk);
+               chunk += chunk_size;
+       }

        glGenTextures(1, &tex);
        glBindTexture(GL_TEXTURE_BUFFER, tex);


On 5/17/19 6:04 AM, Pierre-Eric Pelloux-Prayer wrote:
Hi,

The patch looks good, except ranges-2.c now cause a compiler warning:

tests/spec/arb_texture_buffer_range/ranges-2.c:152:20: warning: assignment to 
‘uint8_t *’ {aka ‘unsigned char *’} from incompatible pointer type ‘float *’ 
[-Wincompatible-pointer-types]
   for (i = 0, chunk = data; i < num_chunks; i++) {

Another suggestion: it could be interesting to verify that INVALID_VALUE is 
generated when the alignment constraint is not respected.

Thanks,

Pierre-Eric




On 09/05/2019 20:41, Anthony Pesch wrote:
Pinging to see if anyone has the time to review.

  - Anthony

On 4/15/19 4:24 PM, Anthony Pesch wrote:
From: Anthony Pesch <ape...@nvidia.com>

The ranges-2 test was failing due to glTexBufferRange returning GL_INVALID_VALUE
when the offset parameter wasn't GL_TEXTURE_BUFFER_OFFSET_ALIGNMENT byte 
aligned.

  From the OpenGL 4.6 Core spec:

An INVALID_VALUE error is generated if offset is not an integer multiple of the
value of TEXTURE_BUFFER_OFFSET_ALIGNMENT.
---
   .../spec/arb_texture_buffer_range/ranges-2.c  | 24 +++++++++++++++----
   1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tests/spec/arb_texture_buffer_range/ranges-2.c 
b/tests/spec/arb_texture_buffer_range/ranges-2.c
index 3477e4b52..b9cd543b0 100644
--- a/tests/spec/arb_texture_buffer_range/ranges-2.c
+++ b/tests/spec/arb_texture_buffer_range/ranges-2.c
@@ -91,22 +91,25 @@ float data[] = {
       0, 0,        0.5, 0,
   };
   +int aligned_size = 0;
+int chunk_size = 24 * sizeof(float);
+int num_chunks = 4;
+
   enum piglit_result
   piglit_display(void) {
       int i;
-    int chunk_size = 24 * sizeof(float);
       bool pass = true;
         glClearColor(0.2, 0.2, 0.2, 0.2);
       glClear(GL_COLOR_BUFFER_BIT);
   -    for (i = 0; i < sizeof(data) / chunk_size; i++) {
+    for (i = 0; i < num_chunks; i++) {
           glTexBufferRange(GL_TEXTURE_BUFFER, GL_RGBA32F,
-                 tbo, i * chunk_size, chunk_size);
+                 tbo, i * aligned_size, chunk_size);
           glDrawArrays(GL_TRIANGLES, 0, 6);
       }
   -    for (i = 0; i < sizeof(data) / chunk_size; i++) {
+    for (i = 0; i < num_chunks; i++) {
           float c[4] = {
               data[i * 24 + 2],
               data[i * 24 + 3],
@@ -128,8 +131,14 @@ piglit_display(void) {
     void
   piglit_init(int argc, char **argv) {
+    GLint align, i;
+    uint8_t *chunk;
+
       piglit_require_extension("GL_ARB_texture_buffer_range");
   +    glGetIntegerv(GL_TEXTURE_BUFFER_OFFSET_ALIGNMENT, &align);
+    aligned_size = chunk_size % align == 0 ? chunk_size : align;
+
       prog = piglit_build_simple_program(vs_source, fs_source);
       glUseProgram(prog);
   @@ -138,7 +147,12 @@ piglit_init(int argc, char **argv) {
         glGenBuffers(1, &tbo);
       glBindBuffer(GL_ARRAY_BUFFER, tbo);
-    glBufferData(GL_ARRAY_BUFFER, sizeof(data), data, GL_STATIC_DRAW);
+    glBufferData(GL_ARRAY_BUFFER, aligned_size * num_chunks, NULL, 
GL_STATIC_DRAW);
+
+    for (i = 0, chunk = data; i < num_chunks; i++) {
+        glBufferSubData(GL_ARRAY_BUFFER, aligned_size * i, chunk_size, chunk);
+        chunk += chunk_size;
+    }
         glGenTextures(1, &tex);
       glBindTexture(GL_TEXTURE_BUFFER, tex);

_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
commit 770effba655f97be229e13f894884f0e4c3e604a
Author: Anthony Pesch <ape...@nvidia.com>
Date:   Thu Apr 11 12:37:07 2019 -0400

    arb_texture_buffer_range: Fix buffer alignment in ranges-2 test
    
    The ranges-2 test was failing due to glTexBufferRange returning GL_INVALID_VALUE
    when the offset parameter wasn't GL_TEXTURE_BUFFER_OFFSET_ALIGNMENT byte aligned.
    
    From the OpenGL 4.6 Core spec:
    
    An INVALID_VALUE error is generated if offset is not an integer multiple of the
    value of TEXTURE_BUFFER_OFFSET_ALIGNMENT.

diff --git a/tests/spec/arb_texture_buffer_range/ranges-2.c b/tests/spec/arb_texture_buffer_range/ranges-2.c
index 3477e4b52..83a009875 100644
--- a/tests/spec/arb_texture_buffer_range/ranges-2.c
+++ b/tests/spec/arb_texture_buffer_range/ranges-2.c
@@ -91,22 +91,29 @@ float data[] = {
 	0, 0,		0.5, 0,
 };
 
+int aligned_size = 0;
+int chunk_size = 24 * sizeof(float);
+int num_chunks = 4;
+
 enum piglit_result
 piglit_display(void) {
 	int i;
-	int chunk_size = 24 * sizeof(float);
 	bool pass = true;
 
 	glClearColor(0.2, 0.2, 0.2, 0.2);
 	glClear(GL_COLOR_BUFFER_BIT);
 
-	for (i = 0; i < sizeof(data) / chunk_size; i++) {
+	/* verify unaligned offsets produce an error */
+	glTexBufferRange(GL_TEXTURE_BUFFER, GL_RGBA32F, tbo, aligned_size - 1, 1);
+	pass &= glGetError() == GL_INVALID_VALUE;
+
+	for (i = 0; i < num_chunks; i++) {
 		glTexBufferRange(GL_TEXTURE_BUFFER, GL_RGBA32F,
-				 tbo, i * chunk_size, chunk_size);
+				 tbo, i * aligned_size, chunk_size);
 		glDrawArrays(GL_TRIANGLES, 0, 6);
 	}
 
-	for (i = 0; i < sizeof(data) / chunk_size; i++) {
+	for (i = 0; i < num_chunks; i++) {
 		float c[4] = {
 			data[i * 24 + 2],
 			data[i * 24 + 3],
@@ -114,7 +121,7 @@ piglit_display(void) {
 			1
 		};
 
-		pass = piglit_probe_rect_rgba(
+		pass &= piglit_probe_rect_rgba(
 			piglit_width * 0.5 * (1 + data[i * 24 + 0]),
 			piglit_height * 0.5 * (1 + data[i * 24 + 1]),
 			piglit_width/2,
@@ -128,8 +135,14 @@ piglit_display(void) {
 
 void
 piglit_init(int argc, char **argv) {
+	GLint align, i;
+	uint8_t *chunk;
+
 	piglit_require_extension("GL_ARB_texture_buffer_range");
 
+	glGetIntegerv(GL_TEXTURE_BUFFER_OFFSET_ALIGNMENT, &align);
+	aligned_size = chunk_size % align == 0 ? chunk_size : align;
+
 	prog = piglit_build_simple_program(vs_source, fs_source);
 	glUseProgram(prog);
 
@@ -138,7 +151,12 @@ piglit_init(int argc, char **argv) {
 
 	glGenBuffers(1, &tbo);
 	glBindBuffer(GL_ARRAY_BUFFER, tbo);
-	glBufferData(GL_ARRAY_BUFFER, sizeof(data), data, GL_STATIC_DRAW);
+	glBufferData(GL_ARRAY_BUFFER, aligned_size * num_chunks, NULL, GL_STATIC_DRAW);
+
+	for (i = 0, chunk = (uint8_t *)data; i < num_chunks; i++) {
+		glBufferSubData(GL_ARRAY_BUFFER, aligned_size * i, chunk_size, chunk);
+		chunk += chunk_size;
+	}
 
 	glGenTextures(1, &tex);
 	glBindTexture(GL_TEXTURE_BUFFER, tex);
_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit

Reply via email to