On Jan 9, 2015 6:33 AM, "Neil Roberts" <n...@linux.intel.com> wrote:
>
> This patch looks really good. I have some comments below.
>
> Jason Ekstrand <ja...@jlekstrand.net> writes:
>
> > This meta path, designed for use with PBO's, creates a temporary texture
> > out of the PBO and uses BlitFramebuffers to do the actual texture
upload.
> > ---
> > src/mesa/Makefile.sources | 1 +
> > src/mesa/drivers/common/meta.h | 9 ++
> > src/mesa/drivers/common/meta_tex_subimage.c | 211
++++++++++++++++++++++++++++
> > 3 files changed, 221 insertions(+)
> > create mode 100644 src/mesa/drivers/common/meta_tex_subimage.c
> >
> > diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
> > index 44cfafa..3261b28 100644
> > --- a/src/mesa/Makefile.sources
> > +++ b/src/mesa/Makefile.sources
> > @@ -585,6 +585,7 @@ COMMON_DRIVER_FILES = \
> > $(SRCDIR)drivers/common/meta_blit.c \
> > $(SRCDIR)drivers/common/meta_copy_image.c \
> > $(SRCDIR)drivers/common/meta_generate_mipmap.c \
> > + $(SRCDIR)drivers/common/meta_tex_subimage.c \
> > $(SRCDIR)drivers/common/meta.c \
> > $(SRCDIR)drivers/common/meta.h
> >
> > diff --git a/src/mesa/drivers/common/meta.h
b/src/mesa/drivers/common/meta.h
> > index 6ecf3c0..90cbd55 100644
> > --- a/src/mesa/drivers/common/meta.h
> > +++ b/src/mesa/drivers/common/meta.h
> > @@ -519,6 +519,15 @@ extern void
> > _mesa_meta_GenerateMipmap(struct gl_context *ctx, GLenum target,
> > struct gl_texture_object *texObj);
> >
> > +extern bool
> > +_mesa_meta_TexSubImage(struct gl_context *ctx, GLuint dims,
> > + struct gl_texture_image *tex_image,
> > + int xoffset, int yoffset, int zoffset,
> > + int width, int height, int depth,
> > + GLenum format, GLenum type, const void *pixels,
> > + bool allocate_storage, bool create_pbo,
> > + const struct gl_pixelstore_attrib *packing);
> > +
> > extern void
> > _mesa_meta_CopyTexSubImage(struct gl_context *ctx, GLuint dims,
> > struct gl_texture_image *texImage,
> > diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
b/src/mesa/drivers/common/meta_tex_subimage.c
> > new file mode 100644
> > index 0000000..195f600
> > --- /dev/null
> > +++ b/src/mesa/drivers/common/meta_tex_subimage.c
> > @@ -0,0 +1,211 @@
> > +/*
> > + * Mesa 3-D graphics library
> > + *
> > + * Copyright (C) 2015 Intel Corporation. All Rights Reserved.
> > + *
> > + * Permission is hereby granted, free of charge, to any person
obtaining a
> > + * copy of this software and associated documentation files (the
"Software"),
> > + * to deal in the Software without restriction, including without
limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
included
> > + * in all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS
> > + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#include "glheader.h"
> > +#include "context.h"
> > +#include "enums.h"
> > +#include "imports.h"
> > +#include "macros.h"
> > +#include "teximage.h"
> > +#include "texobj.h"
> > +#include "fbobject.h"
> > +#include "buffers.h"
> > +#include "state.h"
> > +
> > +#include "bufferobj.h"
> > +#include "pbo.h"
> > +#include "meta.h"
> > +#include "glformats.h"
> > +#include "shaderapi.h"
> > +#include "uniforms.h"
> > +#include "texstate.h"
> > +#include "varray.h"
> > +
> > +
> > +bool
> > +_mesa_meta_TexSubImage(struct gl_context *ctx, GLuint dims,
> > + struct gl_texture_image *tex_image,
> > + int xoffset, int yoffset, int zoffset,
> > + int width, int height, int depth,
> > + GLenum format, GLenum type, const void *pixels,
> > + bool allocate_storage, bool create_pbo,
> > + const struct gl_pixelstore_attrib *packing)
> > +{
> > + uint32_t pbo_format;
> > + GLenum internal_format, status;
> > + GLuint pbo = 0, pbo_tex = 0, row_stride, size;
> > + GLuint fbos[2] = { 0, 0 };
> > + struct gl_texture_object *pbo_tex_obj;
> > + struct gl_texture_image *pbo_tex_image;
> > + struct gl_buffer_object *buffer_obj;
> > + bool success = false;
> > +
> > + /* XXX: This should probably be passed in from somewhere */
> > + const char *where = "_mesa_meta_TexSubImage";
> > +
> > + if (!_mesa_is_bufferobj(packing->BufferObj) && !create_pbo)
> > + return false;
> > +
> > + if (format == GL_DEPTH_COMPONENT ||
> > + format == GL_DEPTH_STENCIL ||
> > + format == GL_STENCIL_INDEX ||
> > + format == GL_COLOR_INDEX)
> > + return false;
> > +
> > + if (packing->Alignment > 4 ||
> > + packing->SkipPixels > 0 ||
> > + packing->SkipRows > 0 ||
> > + (packing->RowLength != 0 && packing->RowLength != width) ||
> > + packing->SwapBytes ||
> > + packing->LsbFirst ||
> > + packing->Invert)
> > + return false;
>
> Is there any reason for all these restrictions on the row length,
> alignment and skip offsets? If ‘pixels’ is already an arbitrary byte
> offset we can use _mesa_image_offset to just add the skip values to the
> offset.
No good one. We can't handle swapbytes or lsbfirst, but the pointer offset
ones and RowLength are totally doable. I'll get that hooked up.
> These restrictions effectively make the texsubimage tests skip this code
> path because they all use the skip offsets. I've made a little tweak to
> the test cases here so that it will work:
>
> https://github.com/bpeel/piglit/tree/test-texsubimage-no-skip
>
> With that branch the test fails when it needs to do a conversion such as
> when it uploads RGBA/UNSIGNED_BYTE data to an internal format of
> GL_RGB5. This might be something to do with having different rounding
> when the hardware converts rather than whatever mesa does to convert
> when no PBO is used. If that's the case I guess it's not really a
> problem but it is a bit weird to have different results depending on
> whether you upload via a PBO or not.
>
> I don't like all the places in Mesa that try to restrict the possible
> row stride values by checking Alignment and RowLength. For example if
> the image is RGBA with a width of 2 then an alignment of 8 is equivalent
> to an alignment of 1 and this check would needlessly disallow it. I
> think it is better to just treat those values as a way to calculate the
> row stride and then check for the restrictions based on the row stride
> value instead. In this case I think it would be better to let the driver
> check for limitations in the implementation of
> SetTextureStorageForBufferObject instead. I think in this case because
> the source buffer is not tiled the hardware doesn't even have any
> limitations, does it?
Correct. I was just lazy and wanted to get stuff on the list so people like
you could look at it.
>
> > +
> > + pbo_format = _mesa_format_from_format_and_type(format, type);
> > + if (_mesa_format_is_mesa_array_format(pbo_format))
> > + pbo_format = _mesa_format_from_array_format(pbo_format);
> > +
> > + if (!pbo_format)
> > + return false;
> > +
> > + if (!ctx->TextureFormatSupported[pbo_format])
> > + return false;
> > +
> > + internal_format = _mesa_get_format_base_format(pbo_format);
> > + row_stride = _mesa_format_row_stride(pbo_format, width);
> > +
> > + if (!_mesa_validate_pbo_access(dims, packing, width, height, depth,
> > + format, type, INT_MAX, pixels)) {
> > + _mesa_error(ctx, GL_INVALID_OPERATION,
> > + "%s(out of bounds PBO access)", where);
> > + return true;
> > + }
> > +
> > + if (_mesa_check_disallowed_mapping(packing->BufferObj)) {
> > + /* buffer is mapped - that's an error */
> > + _mesa_error(ctx, GL_INVALID_OPERATION, "%s(PBO is mapped)",
where);
> > + return true;
> > + }
> > +
> > + if (allocate_storage)
> > + ctx->Driver.AllocTextureImageBuffer(ctx, tex_image);
> > +
> > + /* Only stash the current FBO */
> > + _mesa_meta_begin(ctx, 0);
> > +
> > + pbo = 0;
> > + if (_mesa_is_bufferobj(packing->BufferObj)) {
> > + buffer_obj = packing->BufferObj;
> > + } else {
> > + assert(create_pbo);
> > +
> > + _mesa_GenBuffers(1, &pbo);
> > +
> > + /* We know the client doesn't have this bound */
> > + _mesa_BindBuffer(GL_PIXEL_UNPACK_BUFFER, pbo);
> > +
> > + size = _mesa_format_image_size(pbo_format, width, height, depth);
> > + _mesa_BufferData(GL_PIXEL_UNPACK_BUFFER, size, pixels,
GL_STREAM_DRAW);
> > +
> > + buffer_obj = ctx->Unpack.BufferObj;
> > + pixels = NULL;
> > +
> > + _mesa_BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
> > + }
> > +
> > + _mesa_GenTextures(1, &pbo_tex);
> > + pbo_tex_obj = _mesa_lookup_texture(ctx, pbo_tex);
> > + pbo_tex_obj->Target = depth > 2 ? GL_TEXTURE_2D_ARRAY :
GL_TEXTURE_2D;
>
> Should that be depth > 1?
>
> > + pbo_tex_obj->Immutable = GL_TRUE;
> > + _mesa_initialize_texture_object(ctx, pbo_tex_obj, pbo_tex,
GL_TEXTURE_2D);
> > +
> > + pbo_tex_image = _mesa_get_tex_image(ctx, pbo_tex_obj,
> > + pbo_tex_obj->Target, 0);
> > + _mesa_init_teximage_fields(ctx, pbo_tex_image, width, height, depth,
> > + 0, internal_format, pbo_format);
> > +
> > + if (!ctx->Driver.SetTextureStorageForBufferObject(ctx, pbo_tex_obj,
> > + buffer_obj,
> > + (intptr_t)pixels,
> > + row_stride)) {
> > + goto fail;
> > + }
> > +
> > + _mesa_GenFramebuffers(2, fbos);
> > + _mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, fbos[0]);
> > + _mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbos[1]);
> > +
> > + if (tex_image->TexObject->Target == GL_TEXTURE_1D_ARRAY) {
> > + assert(depth == 1);
> > + depth = height;
> > + height = 1;
> > + }
> > +
> > + _mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
> > + pbo_tex_image, 0);
> > + /* If this passes on the first layer it should pass on the others */
> > + status = _mesa_CheckFramebufferStatus(GL_READ_FRAMEBUFFER);
> > + if (status != GL_FRAMEBUFFER_COMPLETE)
> > + goto fail;
> > +
> > + _mesa_meta_bind_fbo_image(GL_DRAW_FRAMEBUFFER, GL_COLOR_ATTACHMENT0,
> > + tex_image, zoffset);
> > + /* If this passes on the first layer it should pass on the others */
> > + status = _mesa_CheckFramebufferStatus(GL_DRAW_FRAMEBUFFER);
> > + if (status != GL_FRAMEBUFFER_COMPLETE)
> > + goto fail;
> > +
> > + _mesa_update_state(ctx);
> > +
> > + if (_mesa_meta_BlitFramebuffer(ctx, 0, 0, width, height,
> > + xoffset, yoffset,
> > + xoffset + width, yoffset + height,
> > + GL_COLOR_BUFFER_BIT, GL_NEAREST))
> > + goto fail;
>
> Is there a good reason to use _mesa_meta_BlitFramebuffer here and not
> _mesa_BlitFramebuffer? The driver might have a faster blit
> implementation than the meta path. intel_blit_framebuffer seems to
> prefer the blorp blitter and then the BLT blitter over the meta path. I
> guess the meta blitter might be faster but we should probably fix that
> in intel_blit_framebuffer instead if that is the case.
Yes, that is a risk we face here. However, using the meta call explicitly
guarantees that we never hit a software fallback. Doing so would be worse
than the regular upload path. Also, for BDW+, we don't have blorp, so we
wouldn't really get any benignity anyway.
> > +
> > + for (int z = 1; z < depth; z++) {
> > + _mesa_meta_bind_fbo_image(GL_READ_FRAMEBUFFER,
GL_COLOR_ATTACHMENT0,
> > + pbo_tex_image, z);
> > + _mesa_meta_bind_fbo_image(GL_DRAW_FRAMEBUFFER,
GL_COLOR_ATTACHMENT0,
> > + tex_image, zoffset + z);
> > +
> > + _mesa_update_state(ctx);
> > +
> > + _mesa_meta_BlitFramebuffer(ctx, 0, 0, width, height,
> > + xoffset, yoffset,
> > + xoffset + width, yoffset + height,
> > + GL_COLOR_BUFFER_BIT, GL_NEAREST);
> > + }
> > +
> > + success = true;
> > +
> > +fail:
> > + _mesa_DeleteFramebuffers(2, fbos);
> > + _mesa_DeleteTextures(1, &pbo_tex);
> > + _mesa_DeleteBuffers(1, &pbo);
> > +
> > + _mesa_meta_end(ctx);
> > +
> > + return success;
> > +}
> > --
> > 2.2.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
