Re: [Mesa-dev] [PATCH 3/4] common: Fix PBOs for 1D_ARRAY.
Sorry for the late review.
Can you explain what this patch does? The previous code was doing a blit
like this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z * height, width, (z + 1) * height,
xoffset, yoffset,
xoffset + width, yoffset + height,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
However, it was first setting the height to 1 so that would end up like
this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z * 1, width, (z + 1) * 1,
xoffset, yoffset,
xoffset + width, yoffset + 1,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
The patch makes it do this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z, width, z + 1,
xoffset, yoffset,
xoffset + width, yoffset + 1,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
This looks like it would have exactly the same result.
The patch doesn't modify the blit call for slice 0 which looks wrong to
me.
Neither the version with or without the patch appears to handle the
yoffset correctly because for the 1D array case this needs to be
interpreted as a slice offset.
I'm attaching a patch which I think fixes it, although I haven't managed
to test it with the arb_direct_state_access/gettextureimage-targets test
so I don't know if I'm misunderstanding something. It is also not
complete because it doesn't touch GetTexSubImage. I have tested it with
the texsubimage test which does use the yoffset, but in order to use
that test the code path first needs to be able to accept the
IMAGE_HEIGHT packing option which I will also post a patch for.
Regards,
- Neil
--- 8 --- (use git am --scissors to automatically chop here)
Subject: meta: Fix the y offset for 1D_ARRAY in _mesa_meta_pbo_TexSubImage
This partially reverts 546aba143d13ba3f99 and brings back the if
statement to move the height over to the depth. However it
additionally moves the yoffset to the zoffset. This fixes texsubimage
array pbo for 1D_ARRAY textures.
The previous fix in 546aba143 wasn't taking into account the yoffset
correctly because it needs to be used to alter the selected layer.
---
src/mesa/drivers/common/meta_tex_subimage.c | 36 ++---
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
b/src/mesa/drivers/common/meta_tex_subimage.c
index 3965d31..be89102 100644
--- a/src/mesa/drivers/common/meta_tex_subimage.c
+++ b/src/mesa/drivers/common/meta_tex_subimage.c
@@ -138,7 +138,7 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint
dims,
struct gl_texture_image *pbo_tex_image;
GLenum status;
bool success = false;
- int z, iters;
+ int z;
/* XXX: This should probably be passed in from somewhere */
const char *where = _mesa_meta_pbo_TexSubImage;
@@ -193,6 +193,16 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint
dims,
_mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, fbos[0]);
_mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbos[1]);
+ if (tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY) {
+ assert(depth == 1);
+ assert(zoffset == 0);
+ depth = height;
+ height = 1;
+ image_height = 1;
+ zoffset = yoffset;
+ yoffset = 0;
+ }
+
_mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
pbo_tex_image, 0);
/* If this passes on the first layer it should pass on the others */
@@ -216,28 +226,18 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint
dims,
GL_COLOR_BUFFER_BIT, GL_NEAREST))
goto fail;
- iters = tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY ?
- height : depth;
-
- for (z = 1; z iters; z++) {
+ for (z = 1; z depth; z++) {
_mesa_meta_bind_fbo_image(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
tex_image, zoffset + z);
_mesa_update_state(ctx);
- if (tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY)
- _mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
-0, z, width, z + 1,
-xoffset, yoffset,
-xoffset + width, yoffset + 1,
-GL_COLOR_BUFFER_BIT, GL_NEAREST);
- else
- _mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
-0, z * image_height,
-width, z * image_height + height,
-xoffset, yoffset,
-
Re: [Mesa-dev] [PATCH 3/4] common: Fix PBOs for 1D_ARRAY.
Ah, now that I looked at your other patch I see why you have image_height.
On Wed, Feb 25, 2015 at 5:33 PM, Laura Ekstrand la...@jlekstrand.net
wrote:
This was an unfortunate artifact of rebasing; I fixed the 1D array bug
before figuring out a robust fix for the 2D array bug (7a49f2e). I think
that you are right, although I'm confused what you are doing with the
variable image_height that you added. It seems like you should be able to
get rid of image_height here. (But that's just a quick gut feeling.)
On Wed, Feb 25, 2015 at 8:19 AM, Neil Roberts n...@linux.intel.com
wrote:
Sorry for the late review.
Can you explain what this patch does? The previous code was doing a blit
like this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z * height, width, (z + 1) * height,
xoffset, yoffset,
xoffset + width, yoffset + height,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
However, it was first setting the height to 1 so that would end up like
this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z * 1, width, (z + 1) * 1,
xoffset, yoffset,
xoffset + width, yoffset + 1,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
The patch makes it do this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z, width, z + 1,
xoffset, yoffset,
xoffset + width, yoffset + 1,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
This looks like it would have exactly the same result.
The patch doesn't modify the blit call for slice 0 which looks wrong to
me.
Neither the version with or without the patch appears to handle the
yoffset correctly because for the 1D array case this needs to be
interpreted as a slice offset.
I'm attaching a patch which I think fixes it, although I haven't managed
to test it with the arb_direct_state_access/gettextureimage-targets test
so I don't know if I'm misunderstanding something. It is also not
complete because it doesn't touch GetTexSubImage. I have tested it with
the texsubimage test which does use the yoffset, but in order to use
that test the code path first needs to be able to accept the
IMAGE_HEIGHT packing option which I will also post a patch for.
Regards,
- Neil
--- 8 --- (use git am --scissors to automatically chop
here)
Subject: meta: Fix the y offset for 1D_ARRAY in _mesa_meta_pbo_TexSubImage
This partially reverts 546aba143d13ba3f99 and brings back the if
statement to move the height over to the depth. However it
additionally moves the yoffset to the zoffset. This fixes texsubimage
array pbo for 1D_ARRAY textures.
The previous fix in 546aba143 wasn't taking into account the yoffset
correctly because it needs to be used to alter the selected layer.
---
src/mesa/drivers/common/meta_tex_subimage.c | 36
++---
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
b/src/mesa/drivers/common/meta_tex_subimage.c
index 3965d31..be89102 100644
--- a/src/mesa/drivers/common/meta_tex_subimage.c
+++ b/src/mesa/drivers/common/meta_tex_subimage.c
@@ -138,7 +138,7 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx,
GLuint dims,
struct gl_texture_image *pbo_tex_image;
GLenum status;
bool success = false;
- int z, iters;
+ int z;
/* XXX: This should probably be passed in from somewhere */
const char *where = _mesa_meta_pbo_TexSubImage;
@@ -193,6 +193,16 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx,
GLuint dims,
_mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, fbos[0]);
_mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbos[1]);
+ if (tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY) {
+ assert(depth == 1);
+ assert(zoffset == 0);
+ depth = height;
+ height = 1;
+ image_height = 1;
+ zoffset = yoffset;
+ yoffset = 0;
+ }
+
_mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
pbo_tex_image, 0);
/* If this passes on the first layer it should pass on the others */
@@ -216,28 +226,18 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx,
GLuint dims,
GL_COLOR_BUFFER_BIT, GL_NEAREST))
goto fail;
- iters = tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY ?
- height : depth;
-
- for (z = 1; z iters; z++) {
+ for (z = 1; z depth; z++) {
_mesa_meta_bind_fbo_image(GL_DRAW_FRAMEBUFFER,
GL_COLOR_ATTACHMENT0,
tex_image, zoffset + z);
Re: [Mesa-dev] [PATCH 3/4] common: Fix PBOs for 1D_ARRAY.
This was an unfortunate artifact of rebasing; I fixed the 1D array bug
before figuring out a robust fix for the 2D array bug (7a49f2e). I think
that you are right, although I'm confused what you are doing with the
variable image_height that you added. It seems like you should be able to
get rid of image_height here. (But that's just a quick gut feeling.)
On Wed, Feb 25, 2015 at 8:19 AM, Neil Roberts n...@linux.intel.com wrote:
Sorry for the late review.
Can you explain what this patch does? The previous code was doing a blit
like this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z * height, width, (z + 1) * height,
xoffset, yoffset,
xoffset + width, yoffset + height,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
However, it was first setting the height to 1 so that would end up like
this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z * 1, width, (z + 1) * 1,
xoffset, yoffset,
xoffset + width, yoffset + 1,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
The patch makes it do this:
_mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
0, z, width, z + 1,
xoffset, yoffset,
xoffset + width, yoffset + 1,
GL_COLOR_BUFFER_BIT, GL_NEAREST);
This looks like it would have exactly the same result.
The patch doesn't modify the blit call for slice 0 which looks wrong to
me.
Neither the version with or without the patch appears to handle the
yoffset correctly because for the 1D array case this needs to be
interpreted as a slice offset.
I'm attaching a patch which I think fixes it, although I haven't managed
to test it with the arb_direct_state_access/gettextureimage-targets test
so I don't know if I'm misunderstanding something. It is also not
complete because it doesn't touch GetTexSubImage. I have tested it with
the texsubimage test which does use the yoffset, but in order to use
that test the code path first needs to be able to accept the
IMAGE_HEIGHT packing option which I will also post a patch for.
Regards,
- Neil
--- 8 --- (use git am --scissors to automatically chop
here)
Subject: meta: Fix the y offset for 1D_ARRAY in _mesa_meta_pbo_TexSubImage
This partially reverts 546aba143d13ba3f99 and brings back the if
statement to move the height over to the depth. However it
additionally moves the yoffset to the zoffset. This fixes texsubimage
array pbo for 1D_ARRAY textures.
The previous fix in 546aba143 wasn't taking into account the yoffset
correctly because it needs to be used to alter the selected layer.
---
src/mesa/drivers/common/meta_tex_subimage.c | 36
++---
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
b/src/mesa/drivers/common/meta_tex_subimage.c
index 3965d31..be89102 100644
--- a/src/mesa/drivers/common/meta_tex_subimage.c
+++ b/src/mesa/drivers/common/meta_tex_subimage.c
@@ -138,7 +138,7 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx,
GLuint dims,
struct gl_texture_image *pbo_tex_image;
GLenum status;
bool success = false;
- int z, iters;
+ int z;
/* XXX: This should probably be passed in from somewhere */
const char *where = _mesa_meta_pbo_TexSubImage;
@@ -193,6 +193,16 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx,
GLuint dims,
_mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, fbos[0]);
_mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbos[1]);
+ if (tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY) {
+ assert(depth == 1);
+ assert(zoffset == 0);
+ depth = height;
+ height = 1;
+ image_height = 1;
+ zoffset = yoffset;
+ yoffset = 0;
+ }
+
_mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
pbo_tex_image, 0);
/* If this passes on the first layer it should pass on the others */
@@ -216,28 +226,18 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx,
GLuint dims,
GL_COLOR_BUFFER_BIT, GL_NEAREST))
goto fail;
- iters = tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY ?
- height : depth;
-
- for (z = 1; z iters; z++) {
+ for (z = 1; z depth; z++) {
_mesa_meta_bind_fbo_image(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
tex_image, zoffset + z);
_mesa_update_state(ctx);
- if (tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY)
- _mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
-
[Mesa-dev] [PATCH 3/4] common: Fix PBOs for 1D_ARRAY.
Corrects the way that _mesa_meta_pbo_TexSubImage and
_mesa_meta_pbo_GetTexSubImage handle 1D_ARRAY textures. Fixes a failure in
the Piglit arb_direct_state_access/gettextureimage-targets test.
---
src/mesa/drivers/common/meta_tex_subimage.c | 62 +
1 file changed, 36 insertions(+), 26 deletions(-)
diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
b/src/mesa/drivers/common/meta_tex_subimage.c
index ee3295b..24a72ba 100644
--- a/src/mesa/drivers/common/meta_tex_subimage.c
+++ b/src/mesa/drivers/common/meta_tex_subimage.c
@@ -141,7 +141,7 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint
dims,
struct gl_texture_image *pbo_tex_image;
GLenum status;
bool success = false;
- int z;
+ int z, iters;
/* XXX: This should probably be passed in from somewhere */
const char *where = _mesa_meta_pbo_TexSubImage;
@@ -189,12 +189,6 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint
dims,
_mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, fbos[0]);
_mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbos[1]);
- if (tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY) {
- assert(depth == 1);
- depth = height;
- height = 1;
- }
-
_mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
pbo_tex_image, 0);
/* If this passes on the first layer it should pass on the others */
@@ -218,7 +212,10 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint
dims,
GL_COLOR_BUFFER_BIT, GL_NEAREST))
goto fail;
- for (z = 1; z depth; z++) {
+ iters = tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY ?
+ height : depth;
+
+ for (z = 1; z iters; z++) {
_mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
pbo_tex_image, z);
_mesa_meta_bind_fbo_image(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
@@ -226,11 +223,18 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint
dims,
_mesa_update_state(ctx);
- _mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
- 0, z * height, width, (z + 1) * height,
- xoffset, yoffset,
- xoffset + width, yoffset + height,
- GL_COLOR_BUFFER_BIT, GL_NEAREST);
+ if (tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY)
+ _mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
+0, z, width, z + 1,
+xoffset, yoffset,
+xoffset + width, yoffset + 1,
+GL_COLOR_BUFFER_BIT, GL_NEAREST);
+ else
+ _mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
+0, z * height, width, (z + 1) * height,
+xoffset, yoffset,
+xoffset + width, yoffset + height,
+GL_COLOR_BUFFER_BIT, GL_NEAREST);
}
success = true;
@@ -257,7 +261,7 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx,
GLuint dims,
struct gl_texture_image *pbo_tex_image;
GLenum status;
bool success = false;
- int z;
+ int z, iters;
/* XXX: This should probably be passed in from somewhere */
const char *where = _mesa_meta_pbo_GetTexSubImage;
@@ -299,12 +303,6 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx,
GLuint dims,
_mesa_GenFramebuffers(2, fbos);
- if (tex_image tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY) {
- assert(depth == 1);
- depth = height;
- height = 1;
- }
-
/* If we were given a texture, bind it to the read framebuffer. If not,
* we're doing a ReadPixels and we should just use whatever framebuffer
* the client has bound.
@@ -338,7 +336,12 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx,
GLuint dims,
GL_COLOR_BUFFER_BIT, GL_NEAREST))
goto fail;
- for (z = 1; z depth; z++) {
+ if (tex_image tex_image-TexObject-Target == GL_TEXTURE_1D_ARRAY)
+ iters = height;
+ else
+ iters = depth;
+
+ for (z = 1; z iters; z++) {
_mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
tex_image, zoffset + z);
_mesa_meta_bind_fbo_image(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
@@ -346,11 +349,18 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx,
GLuint dims,
_mesa_update_state(ctx);
- _mesa_meta_BlitFramebuffer(ctx, ctx-ReadBuffer, ctx-DrawBuffer,
- xoffset, yoffset,
- xoffset + width, yoffset + height,
- 0, z * height, width, (z + 1) * height,
-
