Anuj Phogat <anuj.pho...@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.pho...@gmail.com>
> Cc: Neil Roberts <n...@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.
> 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.
> 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.
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
