[Mesa-dev] [PATCH 04/14] meta: Create temporary pbo in _mesa_meta_pbo_GetTexSubImage()

[Laura Ekstrand]

Yeah, I'm trying to get some bug fixes finished for meta_tex_subimage.c.
The original doesn't handle array textures properly.

On Tue, Feb 24, 2015 at 8:31 AM, Neil Roberts <neil at linux.intel.com> wrote:

> Anuj Phogat <anuj.phogat at 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.phogat at 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.
>
> > +      /* 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
>
> Regards,
> - Neil
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150224/b2eb9e0c/attachment.html>