[Mesa-dev] [PATCH V2 01/22] meta: Enable _mesa_meta_pbo_GetTexSubImage() to read in to non-pbo buffers
Anuj Phogat
anuj.phogat at gmail.com
Fri May 15 15:55:17 PDT 2015
On Tue, Apr 21, 2015 at 3:20 PM, Neil Roberts <neil at linux.intel.com> wrote:
>
> Anuj Phogat <anuj.phogat at gmail.com> writes:
>
> > This will allow Skylake to use _mesa_meta_pbo_GetTexSubImage() for reading YF/YS
> > tiled surfaces.
> >
> > V2: Make changes suggested by Neil.
> >
> > Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> > Cc: Neil Roberts <neil at linux.intel.com>
> > ---
> > src/mesa/drivers/common/meta.h | 1 +
> > src/mesa/drivers/common/meta_tex_subimage.c | 43 ++++++++++++++++++++++++----
> > src/mesa/drivers/dri/i965/intel_pixel_read.c | 11 +++----
> > src/mesa/drivers/dri/i965/intel_tex_image.c | 3 +-
> > 4 files changed, 47 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/mesa/drivers/common/meta.h b/src/mesa/drivers/common/meta.h
> > index e7d894d..fd1ca1f 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, bool for_read_pixels,
> > 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 ad6e787..fbd7e76 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"
> > @@ -150,7 +151,8 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint dims,
> > bool success = false;
> > int z;
> >
> > - if (!_mesa_is_bufferobj(packing->BufferObj) && !create_pbo)
> > + if (!_mesa_is_bufferobj(packing->BufferObj) &&
> > + (!create_pbo || pixels == NULL))
> > return false;
>
> Is this hunk supposed to be here? It looks unrelated and I'm not sure
> what it's trying to achieve. I guess it would catch situations where a
> PBO is not used but pixels == NULL? It seems like this is a bit of a
> strange place to try and catch that as it's an API usage error rather
> than a failure for the driver function.
Sorry for the long delay in my response.
Earlier I had this in a separate patch but squashed in here after your comment
on v1. Now this does look unrelated here. I'll put it in a different patch.
What it's trying to achieve:
glTexImage{123}D use pixels=NULL to allocate the texture without initializing
it. This check here provides the early exit after texture allocation. We have
similar checks in other texture upload paths (intel_texsubimage_tiled_memcpy()
and _mesa_store_teximage()). This is not an API usage error.
>
> > if (format == GL_DEPTH_COMPONENT ||
> > @@ -252,6 +254,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, bool for_read_pixels,
> > const struct gl_pixelstore_attrib *packing)
> > {
> > GLuint pbo = 0, pbo_tex = 0, fbos[2] = { 0, 0 };
> > @@ -259,9 +262,11 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
> > struct gl_texture_image *pbo_tex_image;
> > GLenum status;
> > bool success = false;
> > + const bool is_bufferobj = _mesa_is_bufferobj(packing->BufferObj);
> > + const bool create_temp_pbo = create_pbo && !is_bufferobj;
> > int z;
> >
> > - if (!_mesa_is_bufferobj(packing->BufferObj))
> > + if (!is_bufferobj && !create_pbo)
> > return false;
> >
> > if (format == GL_DEPTH_COMPONENT ||
> > @@ -280,13 +285,26 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
> > image_height = packing->ImageHeight == 0 ? height : packing->ImageHeight;
> > full_height = image_height * (depth - 1) + height;
> >
> > - pbo_tex_image = create_texture_for_pbo(ctx, false, GL_PIXEL_PACK_BUFFER,
> > - width, full_height * depth,
> > - format, type, pixels, packing,
> > + pbo_tex_image = create_texture_for_pbo(ctx, create_pbo,
> > + GL_PIXEL_PACK_BUFFER,
> > + width, full_height,
> > + is_bufferobj ? format : GL_RGBA,
> > + is_bufferobj ? type: GL_FLOAT,
> > + pixels, packing,
> > &pbo, &pbo_tex);
>
> Why does this pass GL_RGBA/GL_FLOAT here? Won't that make
> create_texture_for_pbo use the wrong rowstride and size for the buffer?
> I guess it would probably work anyway because there's unlikely to be a
> format that needs a bigger rowstride than this, but it might end up
> making the buffer much larger than it needs to be.
>
You're right. I can avoid always creating unnecessary large buffer
here. I initially chose this format and type to handle few special
cases of reading back RGBA textures as luminance. ReadPixels
is required to return L= R + G + B. We will get L=R if we use
format=dst_format (i.e. Luminance) here. This fixed an existing
bug in meta pbo path:
http://patchwork.freedesktop.org/patch/49157/
I'll update my patch to fix this.
> > if (!pbo_tex_image)
> > return false;
> >
> > + /* Copy the _BaseFormat of tex_image to pbo_tex_image. Depending on the
> > + * base format involved we may need to apply a rebase transform later in
> > + * _mesa_meta_GetTexImage() (See get_tex_rgba_compressed() and
> > + * get_tex_rgba_uncompressed()). For example if we download to a Luminance
> > + * format we want G=0 and B=0.
> > + */
> > + if (tex_image && create_temp_pbo) {
> > + pbo_tex_image->_BaseFormat = tex_image->_BaseFormat;
> > + }
> > +
>
> I guess the plan here is to try and force the format of the temporary
> texture to be the same as the source texture? Is that so we can
> guarantee that we can create a texture for the PBO because otherwise the
> format that the application chooses might not work for blitting, and
> there is no fallback path in that case for YF/YS tiled buffers? It
> doesn't feel like this is the right place to implement this restriction
> because it is really driver specific and this is in the generic meta
> code. It might be better to handle this as a fallback in the driver
> code. It could try _mesa_meta_pbo_GetTexSubImage once using the
> format/type that the application requested and if it fails it could
> create its own PBO with the same format/type as the source texture and
> try again. In that case I guess it could assume that
> _mesa_meta_pbo_GetTexSubImage will aways succeed. Then it could manually
> copy and convert the data to the application's format.
>
I was trying to fix an existing bug in meta pbo path in above lines of code.
See: http://patchwork.freedesktop.org/patch/49156/
I also found a better way to fix the bug using glColorMask and glClear.
I realized that this patch ended up fixing two existing bugs along with changes
to enable reading in to non-pbo buffers. I have splitted this in to
few patches in my development branch.
I agree with you that we should not use by default the src format and type
for temporary pbo. I liked the idea of trying with dst format, type for pbo and
fallback to using src format, type in i965 driver. Thanks for the suggestions.
Updated patches coming soon on mailing list.
> Presumably there needs to be a similar fix for the TexSubImage case.
>
> - Neil
>
> > _mesa_meta_begin(ctx, ~(MESA_META_PIXEL_TRANSFER |
> > MESA_META_PIXEL_STORE));
> >
> > @@ -349,6 +367,21 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
> > GL_COLOR_BUFFER_BIT, GL_NEAREST);
> > }
> >
> > + /* If temporary pbo is created in meta to read the pixel/texture data,
> > + * now copy that data to client's memory.
> > + */
> > + if (create_temp_pbo) {
> > + if (for_read_pixels) {
> > + _mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, fbos[1]);
> > + _mesa_update_state(ctx);
> > + _mesa_readpixels(ctx, 0, 0, width, height, format, type,
> > + packing, (void *) pixels);
> > + } else {
> > + _mesa_meta_GetTexImage(ctx, format, type, (GLvoid *) pixels,
> > + pbo_tex_image);
> > + }
> > + }
> > +
> > success = true;
> >
> > fail:
> > diff --git a/src/mesa/drivers/dri/i965/intel_pixel_read.c b/src/mesa/drivers/dri/i965/intel_pixel_read.c
> > index 0972121..9ab5ed1 100644
> > --- a/src/mesa/drivers/dri/i965/intel_pixel_read.c
> > +++ b/src/mesa/drivers/dri/i965/intel_pixel_read.c
> > @@ -224,13 +224,14 @@ intelReadPixels(struct gl_context * ctx,
> >
> > DBG("%s\n", __FUNCTION__);
> >
> > - if (_mesa_is_bufferobj(pack->BufferObj)) {
> > - if (_mesa_meta_pbo_GetTexSubImage(ctx, 2, NULL, x, y, 0, width, height, 1,
> > - format, type, pixels, pack))
> > - return;
> > + if (_mesa_meta_pbo_GetTexSubImage(ctx, 2, NULL, x, y, 0, width,
> > + height, 1, format, type, pixels,
> > + false /*create_pbo*/,
> > + true /*for_readpixels*/, pack))
> > + return;
> >
> > + if (_mesa_is_bufferobj(ctx->Pack.BufferObj))
> > perf_debug("%s: fallback to CPU mapping in PBO case\n", __FUNCTION__);
> > - }
> >
> > ok = intel_readpixels_tiled_memcpy(ctx, x, y, width, height,
> > format, type, pixels, pack);
> > diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c
> > index 00bf9ce..31cbabe 100644
> > --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> > +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> > @@ -485,7 +485,8 @@ intel_get_tex_image(struct gl_context *ctx,
> > if (_mesa_meta_pbo_GetTexSubImage(ctx, 3, texImage, 0, 0, 0,
> > texImage->Width, texImage->Height,
> > texImage->Depth, format, type,
> > - pixels, &ctx->Pack))
> > + pixels, false /* create_pbo */,
> > + false /*for_readpixels*/, &ctx->Pack))
> > return;
> >
> > perf_debug("%s: fallback to CPU mapping in PBO case\n", __FUNCTION__);
> > --
> > 2.3.4
More information about the mesa-dev
mailing list