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.
> Regards,
> - Neil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
