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

[Anuj Phogat]

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.
>
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.

> Regards,
> - Neil