[Mesa-dev] [PATCH 1/5] mesa: Add GL API support for ARB_copy_image
Jason Ekstrand
jason at jlekstrand.net
Sun Aug 23 15:51:44 PDT 2015
On Aug 23, 2015 3:48 PM, "Marek Olšák" <maraeo at gmail.com> wrote:
>
> On Mon, Aug 24, 2015 at 12:32 AM, Jason Ekstrand <jason at jlekstrand.net>
wrote:
> > On Sun, Aug 23, 2015 at 10:42 AM, Marek Olšák <maraeo at gmail.com> wrote:
> >> On Fri, Aug 8, 2014 at 8:55 PM, Jason Ekstrand <jason at 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.ekstrand at intel.com>
> >>> Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> >>> Reviewed-by: Neil Roberts <neil at 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.ekstrand at 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.
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.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150823/370ce9b9/attachment-0001.html>
More information about the mesa-dev
mailing list