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

[Neil Roberts]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 472 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150224/724b4b5e/attachment.sig>