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

[Jason Ekstrand]

On Tue, Feb 24, 2015 at 7:01 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:

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

The biggest of her changes were pushed this afternoon.  Go ahead and rebase.


> > 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/4c548596/attachment-0001.html>