On Jan 9, 2015 6:33 AM, "Neil Roberts" <<a href="mailto:neil@linux.intel.com">neil@linux.intel.com</a>> wrote: > > This patch looks really good. I have some comments below. > > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> 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: > > <a href="https://github.com/bpeel/piglit/tree/test-texsubimage-no-skip">https://github.com/bpeel/piglit/tree/test-texsubimage-no-skip</a> > > 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 > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>