On Tue, Feb 24, 2015 at 7:01 PM, Anuj Phogat <anuj.pho...@gmail.com> wrote:
> On Tue, Feb 24, 2015 at 8:31 AM, Neil Roberts <n...@linux.intel.com>
> wrote:
> > Anuj Phogat <anuj.pho...@gmail.com> 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 <anuj.pho...@gmail.com>
> >> ---
> >> 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.
> >
> 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:
> >
> > http://lists.freedesktop.org/archives/mesa-dev/2015-February/077487.html
> >
> I'll hold this patch until her changes land and send out a v2.
>
The biggest of her changes were pushed this afternoon. Go ahead and rebase.
> > Regards,
> > - Neil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
