On Mon, Aug 24, 2015 at 12:32 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote: > On Sun, Aug 23, 2015 at 10:42 AM, Marek Olšák <mar...@gmail.com> wrote: >> On Fri, Aug 8, 2014 at 8:55 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote: >>> This adds the API entrypoint, error checking logic, and a driver hook for >>> the ARB_copy_image extension. >>> >>> v2: Fix a typo in ARB_copy_image.xml and add it to the makefile >>> v3: Put ARB_copy_image.xml in the right place alphebetically in the >>> makefile and properly prefix the commit message >>> v4: Fixed some line wrapping and added a check for null >>> v5: Check for incomplete renderbuffers >>> >>> Signed-off-by: Jason Ekstrand <jason.ekstr...@intel.com> >>> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikk...@gmail.com> >>> Reviewed-by: Neil Roberts <n...@linux.intel.com> >>> >>> v6: Update dispatch_sanity for the addition of CopyImageSubData >>> --- >>> src/mapi/glapi/gen/ARB_copy_image.xml | 28 +++ >>> src/mapi/glapi/gen/Makefile.am | 1 + >>> src/mapi/glapi/gen/gl_API.xml | 2 +- >>> src/mapi/glapi/gen/gl_genexec.py | 1 + >>> src/mesa/Makefile.sources | 1 + >>> src/mesa/main/copyimage.c | 356 >>> ++++++++++++++++++++++++++++++++ >>> src/mesa/main/copyimage.h | 49 +++++ >>> src/mesa/main/dd.h | 16 ++ >>> src/mesa/main/extensions.c | 1 + >>> src/mesa/main/mtypes.h | 1 + >>> src/mesa/main/tests/dispatch_sanity.cpp | 2 +- >>> src/mesa/main/textureview.c | 36 ++-- >>> src/mesa/main/textureview.h | 4 + >>> 13 files changed, 477 insertions(+), 21 deletions(-) >>> create mode 100644 src/mapi/glapi/gen/ARB_copy_image.xml >>> create mode 100644 src/mesa/main/copyimage.c >>> create mode 100644 src/mesa/main/copyimage.h >>> >>> diff --git a/src/mapi/glapi/gen/ARB_copy_image.xml >>> b/src/mapi/glapi/gen/ARB_copy_image.xml >>> new file mode 100644 >>> index 0000000..2fbd845 >>> --- /dev/null >>> +++ b/src/mapi/glapi/gen/ARB_copy_image.xml >>> @@ -0,0 +1,28 @@ >>> +<?xml version="1.0"?> >>> +<!DOCTYPE OpenGLAPI SYSTEM "gl_API.dtd"> >>> + >>> +<OpenGLAPI> >>> + >>> +<category name="GL_ARB_copy_image" number="123"> >>> + >>> + <function name="CopyImageSubData" offset="assign"> >>> + <param name="srcName" type="GLuint"/> >>> + <param name="srcTarget" type="GLenum"/> >>> + <param name="srcLevel" type="GLint"/> >>> + <param name="srcX" type="GLint"/> >>> + <param name="srcY" type="GLint"/> >>> + <param name="srcZ" type="GLint"/> >>> + <param name="dstName" type="GLuint"/> >>> + <param name="dstTarget" type="GLenum"/> >>> + <param name="dstLevel" type="GLint"/> >>> + <param name="dstX" type="GLint"/> >>> + <param name="dstY" type="GLint"/> >>> + <param name="dstZ" type="GLint"/> >>> + <param name="srcWidth" type="GLsizei"/> >>> + <param name="srcHeight" type="GLsizei"/> >>> + <param name="srcDepth" type="GLsizei"/> >>> + </function> >>> + >>> +</category> >>> + >>> +</OpenGLAPI> >>> diff --git a/src/mapi/glapi/gen/Makefile.am b/src/mapi/glapi/gen/Makefile.am >>> index 212731f..645def4 100644 >>> --- a/src/mapi/glapi/gen/Makefile.am >>> +++ b/src/mapi/glapi/gen/Makefile.am >>> @@ -117,6 +117,7 @@ API_XML = \ >>> ARB_compressed_texture_pixel_storage.xml \ >>> ARB_compute_shader.xml \ >>> ARB_copy_buffer.xml \ >>> + ARB_copy_image.xml \ >>> ARB_debug_output.xml \ >>> ARB_depth_buffer_float.xml \ >>> ARB_depth_clamp.xml \ >>> diff --git a/src/mapi/glapi/gen/gl_API.xml b/src/mapi/glapi/gen/gl_API.xml >>> index e011509..619717d 100644 >>> --- a/src/mapi/glapi/gen/gl_API.xml >>> +++ b/src/mapi/glapi/gen/gl_API.xml >>> @@ -8302,7 +8302,7 @@ >>> >>> <xi:include href="ARB_compute_shader.xml" >>> xmlns:xi="http://www.w3.org/2001/XInclude"/> >>> >>> -<!-- ARB extension #123 --> >>> +<xi:include href="ARB_copy_image.xml" >>> xmlns:xi="http://www.w3.org/2001/XInclude"/> >>> >>> <xi:include href="ARB_texture_view.xml" >>> xmlns:xi="http://www.w3.org/2001/XInclude"/> >>> >>> diff --git a/src/mapi/glapi/gen/gl_genexec.py >>> b/src/mapi/glapi/gen/gl_genexec.py >>> index 4609193..d479e66 100644 >>> --- a/src/mapi/glapi/gen/gl_genexec.py >>> +++ b/src/mapi/glapi/gen/gl_genexec.py >>> @@ -62,6 +62,7 @@ header = """/** >>> #include "main/condrender.h" >>> #include "main/context.h" >>> #include "main/convolve.h" >>> +#include "main/copyimage.h" >>> #include "main/depth.h" >>> #include "main/dlist.h" >>> #include "main/drawpix.h" >>> diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources >>> index 45c53ca..d02c174 100644 >>> --- a/src/mesa/Makefile.sources >>> +++ b/src/mesa/Makefile.sources >>> @@ -31,6 +31,7 @@ MAIN_FILES = \ >>> $(SRCDIR)main/condrender.c \ >>> $(SRCDIR)main/context.c \ >>> $(SRCDIR)main/convolve.c \ >>> + $(SRCDIR)main/copyimage.c \ >>> $(SRCDIR)main/cpuinfo.c \ >>> $(SRCDIR)main/debug.c \ >>> $(SRCDIR)main/depth.c \ >>> diff --git a/src/mesa/main/copyimage.c b/src/mesa/main/copyimage.c >>> new file mode 100644 >>> index 0000000..e1110dd >>> --- /dev/null >>> +++ b/src/mesa/main/copyimage.c >>> @@ -0,0 +1,356 @@ >>> +/* >>> + * Mesa 3-D graphics library >>> + * >>> + * Copyright (C) 2014 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. >>> + * >>> + * Authors: >>> + * Jason Ekstrand <jason.ekstr...@intel.com> >>> + */ >>> + >>> +#include "glheader.h" >>> +#include "errors.h" >>> +#include "enums.h" >>> +#include "copyimage.h" >>> +#include "teximage.h" >>> +#include "texobj.h" >>> +#include "fbobject.h" >>> +#include "textureview.h" >>> + >>> +static bool >>> +prepare_target(struct gl_context *ctx, GLuint name, GLenum *target, int >>> level, >>> + struct gl_texture_object **tex_obj, >>> + struct gl_texture_image **tex_image, GLuint *tmp_tex, >>> + const char *dbg_prefix) >>> +{ >>> + struct gl_renderbuffer *rb; >>> + >>> + if (name == 0) { >>> + _mesa_error(ctx, GL_INVALID_VALUE, >>> + "glCopyImageSubData(%sName = %d)", dbg_prefix, name); >>> + return false; >>> + } >>> + >>> + /* >>> + * INVALID_ENUM is generated >>> + * * if either <srcTarget> or <dstTarget> >>> + * - is not RENDERBUFFER or a valid non-proxy texture target >>> + * - is TEXTURE_BUFFER, or >>> + * - is one of the cubemap face selectors described in table 3.17, >>> + */ >>> + switch (*target) { >>> + case GL_RENDERBUFFER: >>> + /* Not a texture target, but valid */ >>> + case GL_TEXTURE_1D: >>> + case GL_TEXTURE_1D_ARRAY: >>> + case GL_TEXTURE_2D: >>> + case GL_TEXTURE_3D: >>> + case GL_TEXTURE_CUBE_MAP: >>> + case GL_TEXTURE_RECTANGLE: >>> + case GL_TEXTURE_2D_ARRAY: >>> + case GL_TEXTURE_CUBE_MAP_ARRAY: >>> + case GL_TEXTURE_2D_MULTISAMPLE: >>> + case GL_TEXTURE_2D_MULTISAMPLE_ARRAY: >>> + /* These are all valid */ >>> + break; >>> + case GL_TEXTURE_EXTERNAL_OES: >>> + /* Only exists in ES */ >>> + case GL_TEXTURE_BUFFER: >>> + default: >>> + _mesa_error(ctx, GL_INVALID_ENUM, >>> + "glCopyImageSubData(%sTarget = %s)", dbg_prefix, >>> + _mesa_lookup_enum_by_nr(*target)); >>> + return false; >>> + } >>> + >>> + if (*target == GL_RENDERBUFFER) { >>> + rb = _mesa_lookup_renderbuffer(ctx, name); >>> + if (!rb) { >>> + _mesa_error(ctx, GL_INVALID_VALUE, >>> + "glCopyImageSubData(%sName = %u)", dbg_prefix, name); >>> + return false; >>> + } >>> + >>> + if (!rb->Name) { >>> + _mesa_error(ctx, GL_INVALID_OPERATION, >>> + "glCopyImageSubData(%sName incomplete)", dbg_prefix); >>> + return false; >>> + } >>> + >>> + if (level != 0) { >>> + _mesa_error(ctx, GL_INVALID_VALUE, >>> + "glCopyImageSubData(%sLevel = %u)", dbg_prefix, >>> level); >>> + return false; >>> + } >>> + >>> + if (rb->NumSamples > 1) >>> + *target = GL_TEXTURE_2D_MULTISAMPLE; >>> + else >>> + *target = GL_TEXTURE_2D; >>> + >>> + *tmp_tex = 0; >>> + _mesa_GenTextures(1, tmp_tex); >>> + if (*tmp_tex == 0) >>> + return false; /* Error already set by GenTextures */ >>> + >>> + _mesa_BindTexture(*target, *tmp_tex); >> >> Sorry for the very late review, but this is wrong. This changes the > > This code has been merged for months. Patches would be better than review. > :-) > >> currently-bound texture object and therefore has a side effect that >> the specification doesn't allow. The code later unbinds the texture by >> calling _mesa_DeleteTextures. The result is that glCopyImageSubData >> unbinds the current texture. Another problem with this is that it >> needlessly causes flagging of _NEW_TEXTURE, which can have undesirable >> performance impact. > > Yes, you're correct. I seem to recall that this may have been fixed. > At the very least, I think there is a patch somewhere that fixes it > even if it's not merged. Now that we have DSA, creating a texture > with the appropreate target should be pretty trivial.
But we don't need (or want) to create a texture. gl_renderbuffer is perfectly fine for copying. Yes, CopyImageSubData would need parameters srcTex, srcRB, dstTex, dstRB, and one from each pair should be NULL. The thing is, if I attempt to do it right without creating a texture, it will inevitably break the feature for i965. Marek _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
