<div dir="ltr">On Sun, Aug 23, 2015 at 4:51 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"> On Aug 23, 2015 3:48 PM, "Marek Olšák" <<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a>> wrote: > > On Mon, Aug 24, 2015 at 12:32 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: > > On Sun, Aug 23, 2015 at 10:42 AM, Marek Olšák <<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a>> wrote: > >> On Fri, Aug 8, 2014 at 8:55 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> 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 <<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a>> > >>> Reviewed-by: Juha-Pekka Heikkila <<a href="mailto:juhapekka.heikkila@gmail.com" target="_blank">juhapekka.heikkila@gmail.com</a>> > >>> Reviewed-by: Neil Roberts <<a href="mailto:neil@linux.intel.com" target="_blank">neil@linux.intel.com</a>> > >>> > >>> 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="<a href="http://www.w3.org/2001/XInclude" target="_blank">http://www.w3.org/2001/XInclude</a>"/> > >>> > >>> - > >>> +<xi:include href="ARB_copy_image.xml" xmlns:xi="<a href="http://www.w3.org/2001/XInclude" target="_blank">http://www.w3.org/2001/XInclude</a>"/> > >>> > >>> <xi:include href="ARB_texture_view.xml" xmlns:xi="<a href="http://www.w3.org/2001/XInclude" target="_blank">http://www.w3.org/2001/XInclude</a>"/> > >>> > >>> 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 <<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a>> > >>> + */ > >>> + > >>> +#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. </div></div>Right... Like I said, I think there was a patch somewhere that cleaned this up. IIRC, it passed both the render buffer and the texture (one of them was null) and punted texture creation to meta. I can try and dig it up some time next week.</blockquote><div> </div><div>I've got a patch series for GL_ARB_copy_image. I'll try to finish cleaning it up this week and post it. Been on vacation. </div><div>-Brian </div></div></div></div>