<div dir="ltr">Yeah, I'm trying to get some bug fixes finished for meta_tex_subimage.c. The original doesn't handle array textures properly. </div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Feb 24, 2015 at 8:31 AM, Neil Roberts <<a href="mailto:neil@linux.intel.com" target="_blank">neil@linux.intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> writes: > using a flag passed in as function parameter. This will enable > _mesa_meta_pbo_GetTexSubImage to be used for reading in to > non-pbo buffers. > > This will be useful to support reading from YF/YS tiled surfaces > in Skylake. > > Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> > --- > src/mesa/drivers/common/meta.h | 1 + > src/mesa/drivers/common/meta_tex_subimage.c | 18 ++++++++++++++++-- > src/mesa/drivers/dri/i965/intel_pixel_read.c | 9 ++++----- > src/mesa/drivers/dri/i965/intel_tex_image.c | 3 ++- > 4 files changed, 23 insertions(+), 8 deletions(-) > > diff --git a/src/mesa/drivers/common/meta.h b/src/mesa/drivers/common/meta.h > index e7d894d..3de0d87 100644 > --- a/src/mesa/drivers/common/meta.h > +++ b/src/mesa/drivers/common/meta.h > @@ -542,6 +542,7 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims, > int xoffset, int yoffset, int zoffset, > int width, int height, int depth, > GLenum format, GLenum type, const void *pixels, > + bool create_pbo, > const struct gl_pixelstore_attrib *packing); > > extern void > diff --git a/src/mesa/drivers/common/meta_tex_subimage.c b/src/mesa/drivers/common/meta_tex_subimage.c > index 68c8273..cd87a72 100644 > --- a/src/mesa/drivers/common/meta_tex_subimage.c > +++ b/src/mesa/drivers/common/meta_tex_subimage.c > @@ -34,6 +34,7 @@ > #include "macros.h" > #include "meta.h" > #include "pbo.h" > +#include "readpix.h" > #include "shaderapi.h" > #include "state.h" > #include "teximage.h" > @@ -246,6 +247,7 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims, > int xoffset, int yoffset, int zoffset, > int width, int height, int depth, > GLenum format, GLenum type, const void *pixels, > + bool create_pbo, > const struct gl_pixelstore_attrib *packing) > { > GLuint pbo = 0, pbo_tex = 0, fbos[2] = { 0, 0 }; > @@ -257,7 +259,7 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims, > /* XXX: This should probably be passed in from somewhere */ > const char *where = "_mesa_meta_pbo_GetTexSubImage"; > > - if (!_mesa_is_bufferobj(packing->BufferObj)) > + if (!_mesa_is_bufferobj(packing->BufferObj) && !create_pbo) > return false; > > if (format == GL_DEPTH_COMPONENT || > @@ -282,7 +284,7 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims, > return true; > } > > - pbo_tex_image = create_texture_for_pbo(ctx, false, GL_PIXEL_PACK_BUFFER, > + pbo_tex_image = create_texture_for_pbo(ctx, create_pbo, GL_PIXEL_PACK_BUFFER, > width, height, depth, > format, type, pixels, packing, > &pbo, &pbo_tex); > @@ -348,6 +350,18 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims, > GL_COLOR_BUFFER_BIT, GL_NEAREST); > } > > + /* If we created a temporary pbo in meta to read the pixel data, that > + * data will now get copied to memory allocated by client. > + */ > + if (create_pbo) { </div></div>I think this should be changed to something like the below: if (create_pbo && !_mesa_is_bufferobj(packing->BufferObj)) It looks like the meaning of create_pbo in create_texture_for_pbo is ‘create pbo unless there is already a pbo’. With this patch _mesa_meta_pbo_GetTexSubImage seems to interpret it to mean that it will always create the pbo and if there already was a pbo it would end up clobbering the state. > + /* Unbind the pbo from pack binding. */ > + _mesa_BindBuffer(GL_PIXEL_PACK_BUFFER, 0); I don't think this unbind is necessary. create_texture_for_pbo is careful not to modify the PBO binding state and once that function is finished the PBO is only accessed via the texture so we shouldn't need to touch the PBO binding. > + _mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, fbos[1]); > + _mesa_update_state(ctx); > + _mesa_readpixels(ctx, 0, 0, width, height, format, type, > + packing, (void *) pixels); Doesn't this only read the last slice of the texture? Maybe this patch should wait until this patch from Laura Ekstrand is landed: <a href="http://lists.freedesktop.org/archives/mesa-dev/2015-February/077487.html" target="_blank">http://lists.freedesktop.org/archives/mesa-dev/2015-February/077487.html</a> Regards, - Neil _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div>