<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Feb 24, 2015 at 7:01 PM, Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Feb 24, 2015 at 8:31 AM, Neil Roberts <<a href="mailto:neil@linux.intel.com">neil@linux.intel.com</a>> wrote: <div><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) { > > 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. > </div></div>I agree. I will make this change. >> + /* 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> > I'll hold this patch until her changes land and send out a v2. </blockquote><div> </div><div>The biggest of her changes were pushed this afternoon. Go ahead and rebase. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > Regards, > - Neil <div class="HOEnZb"><div class="h5">_______________________________________________ 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> </div></div></blockquote></div> </div></div>