Re: [Mesa-dev] [PATCH 02/23] main: Add entry point for TextureBufferRange.

2015-02-19 Thread Martin Peres 

On 18/02/2015 20:10, Ian Romanick wrote:

On 02/17/2015 07:59 AM, Martin Peres wrote:

Hey Laura,

Thanks for this code. I'll be sending reviews throughout the week :)

On 12/02/15 04:05, Laura Ekstrand wrote:

diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
index 336feff..ce6f446 100644
--- a/src/mesa/main/teximage.c
+++ b/src/mesa/main/teximage.c
@@ -5197,6 +5197,96 @@ _mesa_TextureBuffer(GLuint texture, GLenum
internalFormat, GLuint buffer)
 bufObj, 0, buffer ? -1 : 0, false, true);
   }
   +void GLAPIENTRY
+_mesa_TextureBufferRange(GLuint texture, GLenum internalFormat,
GLuint buffer,
+ GLintptr offset, GLsizeiptr size)
+{
+   struct gl_texture_object *texObj;
+   struct gl_buffer_object *bufObj;
+
+   GET_CURRENT_CONTEXT(ctx);
+
+   /* NOTE: ARB_texture_buffer_object has interactions with
+* the compatibility profile that are not implemented.
+*/
+   if (!(ctx-API == API_OPENGL_CORE 
+ ctx-Extensions.ARB_texture_buffer_object)) {
+  _mesa_error(ctx, GL_INVALID_OPERATION,
+  glTextureBufferRange(ARB_texture_buffer_object is
not
+   implemented for the compatibility profile));
+  return;
+   }

Can you point me to the relevant part of the spec that explains the
interaction?

In compatibility profile, TexBos can support luminance, luminance-alpha,
and intensity textures.  Intel hardware doesn't support that, and none
of the TexBo common code supports it either.

But... at least for glTexBuffer and glTexBufferRange there is (almost) a
better way to do this.  We should change the GL API generator code so
that we can omit functions from compatibility profile.  We can already
do this for core profile (look for the deprecated='3.1' bits in
src/mapi/glapi/gen).

Due to previously mentioned issues with libraries like GLEW, we may not
be able to do this with DSA functions that are core-only if we're going
to expose DSA on compatibility profile.


Thanks Ian. How should I check for interactions with the compatibility mode
on the entry points I added? I guess I need to check Opengl 4.5 compat, 
right?

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/23] main: Add entry point for TextureBufferRange.

2015-02-18 Thread Ian Romanick 
On 02/17/2015 07:59 AM, Martin Peres wrote:
 Hey Laura,
 
 Thanks for this code. I'll be sending reviews throughout the week :)
 
 On 12/02/15 04:05, Laura Ekstrand wrote:
 diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
 index 336feff..ce6f446 100644
 --- a/src/mesa/main/teximage.c
 +++ b/src/mesa/main/teximage.c
 @@ -5197,6 +5197,96 @@ _mesa_TextureBuffer(GLuint texture, GLenum
 internalFormat, GLuint buffer)
 bufObj, 0, buffer ? -1 : 0, false, true);
   }
   +void GLAPIENTRY
 +_mesa_TextureBufferRange(GLuint texture, GLenum internalFormat,
 GLuint buffer,
 + GLintptr offset, GLsizeiptr size)
 +{
 +   struct gl_texture_object *texObj;
 +   struct gl_buffer_object *bufObj;
 +
 +   GET_CURRENT_CONTEXT(ctx);
 +
 +   /* NOTE: ARB_texture_buffer_object has interactions with
 +* the compatibility profile that are not implemented.
 +*/
 +   if (!(ctx-API == API_OPENGL_CORE 
 + ctx-Extensions.ARB_texture_buffer_object)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  glTextureBufferRange(ARB_texture_buffer_object is
 not
 +   implemented for the compatibility profile));
 +  return;
 +   }
 
 Can you point me to the relevant part of the spec that explains the
 interaction?

In compatibility profile, TexBos can support luminance, luminance-alpha,
and intensity textures.  Intel hardware doesn't support that, and none
of the TexBo common code supports it either.

But... at least for glTexBuffer and glTexBufferRange there is (almost) a
better way to do this.  We should change the GL API generator code so
that we can omit functions from compatibility profile.  We can already
do this for core profile (look for the deprecated='3.1' bits in
src/mapi/glapi/gen).

Due to previously mentioned issues with libraries like GLEW, we may not
be able to do this with DSA functions that are core-only if we're going
to expose DSA on compatibility profile.

 Did you simply took this code out of _mesa_TexBufferRange?
 +
 +   bufObj = _mesa_lookup_bufferobj(ctx, buffer);
 +   if (bufObj) {
 +  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
 +   * Textures:
 +   *An INVALID_VALUE error is generated if offset is
 negative, if
 +   *size is less than or equal to zero, or if offset + size
 is greater
 +   *than the value of BUFFER_SIZE for the buffer bound to
 target.
 +   */
 +  if (offset  0) {
 + _mesa_error(ctx, GL_INVALID_VALUE,
 + glTextureBufferRange(offset %d  0), (int)
 offset);
 + return;
 +  }
 +
 +  if (size = 0) {
 + _mesa_error(ctx, GL_INVALID_VALUE,
 + glTextureBufferRange(size %d = 0), (int) size);
 + return;
 +  }
 +
 +  if (offset + size  bufObj-Size) {
 + _mesa_error(ctx, GL_INVALID_VALUE,
 + glTextureBufferRange(
 + offset %d + size %d  buffer_size %d), (int)
 offset,
 + (int) size, (int) bufObj-Size);
 + return;
 +  }
 +
 +  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
 +   * Textures:
 +   *An INVALID_VALUE error is generated if offset is not an
 integer
 +   *multiple of the value of TEXTURE_BUFFER_OFFSET_ALIGNMENT.
 +   */
 +  if (offset % ctx-Const.TextureBufferOffsetAlignment) {
 + _mesa_error(ctx, GL_INVALID_VALUE,
 + glTextureBufferRange(invalid offset alignment));
 + return;
 +  }
 +   } else if (buffer) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  glTextureBufferRange(unrecognized buffer %u),
 buffer);
 +  return;
 +   } else {
 +
 +  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
 +   * Textures:
 +   *If buffer is zero, then any buffer object attached to
 the buffer
 +   *texture is detached, the values offset and size are
 ignored and
 +   *the state for offset and size for the buffer texture are
 reset to
 +   *zero.
 +   */
 +  offset = 0;
 +  size = 0;
 +   }
 +
 +   /* Get the texture object by Name. */
 +   texObj = _mesa_lookup_texture_err(ctx, texture,
 glTextureBufferRange);
 +   if (!texObj)
 +  return;
 
 This should return INVALID_OPERATION:
 
 An INVALID_OPERATION error is generated by TextureBufferRange if
 texture is not the name of an existing texture object.
 +
 +   if (texObj-Target != GL_TEXTURE_BUFFER_ARB) {
 +  _mesa_error(ctx, GL_INVALID_ENUM, glTextureBufferRange(texture
 target
 + is not GL_TEXTURE_BUFFER));
 +  return;
 +   }
 
 What are you trying to do here? First, I see no mention of this in the
 spec (but I am quite possibly wrong).
 
 Secondly, you are checking the target before calling
 _mesa_texture_buffer_range which does not care about the target.
 
 Is it a copy/paste from

Re: [Mesa-dev] [PATCH 02/23] main: Add entry point for TextureBufferRange.

2015-02-18 Thread Ian Romanick 
On 02/11/2015 06:05 PM, Laura Ekstrand wrote:
 ---
  src/mapi/glapi/gen/ARB_direct_state_access.xml |  8 +++
  src/mesa/main/tests/dispatch_sanity.cpp|  1 +
  src/mesa/main/teximage.c   | 90 
 ++
  src/mesa/main/teximage.h   |  4 ++
  4 files changed, 103 insertions(+)
 
 diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml 
 b/src/mapi/glapi/gen/ARB_direct_state_access.xml
 index 2fe1638..86c00f9 100644
 --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml
 +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml
 @@ -21,6 +21,14 @@
param name=buffer type=GLuint /
 /function
  
 +   function name=TextureBufferRange offset=assign
 +  param name=texture type=GLuint /
 +  param name=internalformat type=GLenum /
 +  param name=buffer type=GLuint /
 +  param name=offset type=GLintptr /
 +  param name=size type=GLsizeiptr /
 +   /function
 +
 function name=TextureStorage1D offset=assign
param name=texture type=GLuint /
param name=levels type=GLsizei /
 diff --git a/src/mesa/main/tests/dispatch_sanity.cpp 
 b/src/mesa/main/tests/dispatch_sanity.cpp
 index 1f1a3a8..b78c1ce 100644
 --- a/src/mesa/main/tests/dispatch_sanity.cpp
 +++ b/src/mesa/main/tests/dispatch_sanity.cpp
 @@ -987,6 +987,7 @@ const struct function gl_core_functions_possible[] = {
 { glTextureStorage2DMultisample, 45, -1 },
 { glTextureStorage3DMultisample, 45, -1 },
 { glTextureBuffer, 45, -1 },
 +   { glTextureBufferRange, 45, -1 },
  
 /* GL_EXT_polygon_offset_clamp */
 { glPolygonOffsetClampEXT, 11, -1 },
 diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
 index 336feff..ce6f446 100644
 --- a/src/mesa/main/teximage.c
 +++ b/src/mesa/main/teximage.c
 @@ -5197,6 +5197,96 @@ _mesa_TextureBuffer(GLuint texture, GLenum 
 internalFormat, GLuint buffer)
bufObj, 0, buffer ? -1 : 0, false, true);
  }
  
 +void GLAPIENTRY
 +_mesa_TextureBufferRange(GLuint texture, GLenum internalFormat, GLuint 
 buffer,
 + GLintptr offset, GLsizeiptr size)
 +{
 +   struct gl_texture_object *texObj;
 +   struct gl_buffer_object *bufObj;
 +
 +   GET_CURRENT_CONTEXT(ctx);
 +
 +   /* NOTE: ARB_texture_buffer_object has interactions with
 +* the compatibility profile that are not implemented.
 +*/
 +   if (!(ctx-API == API_OPENGL_CORE 
 + ctx-Extensions.ARB_texture_buffer_object)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  glTextureBufferRange(ARB_texture_buffer_object is not
 +   implemented for the compatibility profile));
 +  return;
 +   }
 +
 +   bufObj = _mesa_lookup_bufferobj(ctx, buffer);
 +   if (bufObj) {
 +  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
 +   * Textures:
 +   *An INVALID_VALUE error is generated if offset is negative, if
 +   *size is less than or equal to zero, or if offset + size is 
 greater
 +   *than the value of BUFFER_SIZE for the buffer bound to target.
 +   */
 +  if (offset  0) {
 + _mesa_error(ctx, GL_INVALID_VALUE,
 + glTextureBufferRange(offset %d  0), (int) offset);
 + return;
 +  }
 +
 +  if (size = 0) {
 + _mesa_error(ctx, GL_INVALID_VALUE,
 + glTextureBufferRange(size %d = 0), (int) size);
 + return;
 +  }
 +
 +  if (offset + size  bufObj-Size) {
 + _mesa_error(ctx, GL_INVALID_VALUE,
 + glTextureBufferRange(
 + offset %d + size %d  buffer_size %d), (int) offset,
 + (int) size, (int) bufObj-Size);
 + return;
 +  }
 +
 +  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
 +   * Textures:
 +   *An INVALID_VALUE error is generated if offset is not an integer
 +   *multiple of the value of TEXTURE_BUFFER_OFFSET_ALIGNMENT.
 +   */
 +  if (offset % ctx-Const.TextureBufferOffsetAlignment) {
 + _mesa_error(ctx, GL_INVALID_VALUE,
 + glTextureBufferRange(invalid offset alignment));
 + return;
 +  }
 +   } else if (buffer) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  glTextureBufferRange(unrecognized buffer %u), buffer);
 +  return;
 +   } else {
 +
 +  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
 +   * Textures:
 +   *If buffer is zero, then any buffer object attached to the buffer
 +   *texture is detached, the values offset and size are ignored and
 +   *the state for offset and size for the buffer texture are reset to
 +   *zero.
 +   */
 +  offset = 0;
 +  size = 0;
 +   }

We usually try to keep flow control for error checking more flat.  It
makes it easier to follow.  For example, the above block would be much
better as

   struct gl_buffer_object *bufObj =

Re: [Mesa-dev] [PATCH 02/23] main: Add entry point for TextureBufferRange.

2015-02-17 Thread Martin Peres 

Hey Laura,

Thanks for this code. I'll be sending reviews throughout the week :)

On 12/02/15 04:05, Laura Ekstrand wrote:

diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
index 336feff..ce6f446 100644
--- a/src/mesa/main/teximage.c
+++ b/src/mesa/main/teximage.c
@@ -5197,6 +5197,96 @@ _mesa_TextureBuffer(GLuint texture, GLenum 
internalFormat, GLuint buffer)
bufObj, 0, buffer ? -1 : 0, false, true);
  }
  
+void GLAPIENTRY

+_mesa_TextureBufferRange(GLuint texture, GLenum internalFormat, GLuint buffer,
+ GLintptr offset, GLsizeiptr size)
+{
+   struct gl_texture_object *texObj;
+   struct gl_buffer_object *bufObj;
+
+   GET_CURRENT_CONTEXT(ctx);
+
+   /* NOTE: ARB_texture_buffer_object has interactions with
+* the compatibility profile that are not implemented.
+*/
+   if (!(ctx-API == API_OPENGL_CORE 
+ ctx-Extensions.ARB_texture_buffer_object)) {
+  _mesa_error(ctx, GL_INVALID_OPERATION,
+  glTextureBufferRange(ARB_texture_buffer_object is not
+   implemented for the compatibility profile));
+  return;
+   }


Can you point me to the relevant part of the spec that explains the 
interaction?


Did you simply took this code out of _mesa_TexBufferRange?

+
+   bufObj = _mesa_lookup_bufferobj(ctx, buffer);
+   if (bufObj) {
+  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
+   * Textures:
+   *An INVALID_VALUE error is generated if offset is negative, if
+   *size is less than or equal to zero, or if offset + size is greater
+   *than the value of BUFFER_SIZE for the buffer bound to target.
+   */
+  if (offset  0) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ glTextureBufferRange(offset %d  0), (int) offset);
+ return;
+  }
+
+  if (size = 0) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ glTextureBufferRange(size %d = 0), (int) size);
+ return;
+  }
+
+  if (offset + size  bufObj-Size) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ glTextureBufferRange(
+ offset %d + size %d  buffer_size %d), (int) offset,
+ (int) size, (int) bufObj-Size);
+ return;
+  }
+
+  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
+   * Textures:
+   *An INVALID_VALUE error is generated if offset is not an integer
+   *multiple of the value of TEXTURE_BUFFER_OFFSET_ALIGNMENT.
+   */
+  if (offset % ctx-Const.TextureBufferOffsetAlignment) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ glTextureBufferRange(invalid offset alignment));
+ return;
+  }
+   } else if (buffer) {
+  _mesa_error(ctx, GL_INVALID_OPERATION,
+  glTextureBufferRange(unrecognized buffer %u), buffer);
+  return;
+   } else {
+
+  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
+   * Textures:
+   *If buffer is zero, then any buffer object attached to the buffer
+   *texture is detached, the values offset and size are ignored and
+   *the state for offset and size for the buffer texture are reset to
+   *zero.
+   */
+  offset = 0;
+  size = 0;
+   }
+
+   /* Get the texture object by Name. */
+   texObj = _mesa_lookup_texture_err(ctx, texture, glTextureBufferRange);
+   if (!texObj)
+  return;


This should return INVALID_OPERATION:

An INVALID_OPERATION error is generated by TextureBufferRange if 
texture is not the name of an existing texture object.

+
+   if (texObj-Target != GL_TEXTURE_BUFFER_ARB) {
+  _mesa_error(ctx, GL_INVALID_ENUM, glTextureBufferRange(texture target
+ is not GL_TEXTURE_BUFFER));
+  return;
+   }


What are you trying to do here? First, I see no mention of this in the 
spec (but I am quite possibly wrong).


Secondly, you are checking the target before calling 
_mesa_texture_buffer_range which does not care about the target.


Is it a copy/paste from _mesa_TextureBuffer? Please explain why this is 
needed since I surely have not read the spec

as closely as you did on this matter.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 02/23] main: Add entry point for TextureBufferRange.

2015-02-11 Thread Laura Ekstrand 
---
 src/mapi/glapi/gen/ARB_direct_state_access.xml |  8 +++
 src/mesa/main/tests/dispatch_sanity.cpp|  1 +
 src/mesa/main/teximage.c   | 90 ++
 src/mesa/main/teximage.h   |  4 ++
 4 files changed, 103 insertions(+)

diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml 
b/src/mapi/glapi/gen/ARB_direct_state_access.xml
index 2fe1638..86c00f9 100644
--- a/src/mapi/glapi/gen/ARB_direct_state_access.xml
+++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml
@@ -21,6 +21,14 @@
   param name=buffer type=GLuint /
/function
 
+   function name=TextureBufferRange offset=assign
+  param name=texture type=GLuint /
+  param name=internalformat type=GLenum /
+  param name=buffer type=GLuint /
+  param name=offset type=GLintptr /
+  param name=size type=GLsizeiptr /
+   /function
+
function name=TextureStorage1D offset=assign
   param name=texture type=GLuint /
   param name=levels type=GLsizei /
diff --git a/src/mesa/main/tests/dispatch_sanity.cpp 
b/src/mesa/main/tests/dispatch_sanity.cpp
index 1f1a3a8..b78c1ce 100644
--- a/src/mesa/main/tests/dispatch_sanity.cpp
+++ b/src/mesa/main/tests/dispatch_sanity.cpp
@@ -987,6 +987,7 @@ const struct function gl_core_functions_possible[] = {
{ glTextureStorage2DMultisample, 45, -1 },
{ glTextureStorage3DMultisample, 45, -1 },
{ glTextureBuffer, 45, -1 },
+   { glTextureBufferRange, 45, -1 },
 
/* GL_EXT_polygon_offset_clamp */
{ glPolygonOffsetClampEXT, 11, -1 },
diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
index 336feff..ce6f446 100644
--- a/src/mesa/main/teximage.c
+++ b/src/mesa/main/teximage.c
@@ -5197,6 +5197,96 @@ _mesa_TextureBuffer(GLuint texture, GLenum 
internalFormat, GLuint buffer)
   bufObj, 0, buffer ? -1 : 0, false, true);
 }
 
+void GLAPIENTRY
+_mesa_TextureBufferRange(GLuint texture, GLenum internalFormat, GLuint buffer,
+ GLintptr offset, GLsizeiptr size)
+{
+   struct gl_texture_object *texObj;
+   struct gl_buffer_object *bufObj;
+
+   GET_CURRENT_CONTEXT(ctx);
+
+   /* NOTE: ARB_texture_buffer_object has interactions with
+* the compatibility profile that are not implemented.
+*/
+   if (!(ctx-API == API_OPENGL_CORE 
+ ctx-Extensions.ARB_texture_buffer_object)) {
+  _mesa_error(ctx, GL_INVALID_OPERATION,
+  glTextureBufferRange(ARB_texture_buffer_object is not
+   implemented for the compatibility profile));
+  return;
+   }
+
+   bufObj = _mesa_lookup_bufferobj(ctx, buffer);
+   if (bufObj) {
+  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
+   * Textures:
+   *An INVALID_VALUE error is generated if offset is negative, if
+   *size is less than or equal to zero, or if offset + size is greater
+   *than the value of BUFFER_SIZE for the buffer bound to target.
+   */
+  if (offset  0) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ glTextureBufferRange(offset %d  0), (int) offset);
+ return;
+  }
+
+  if (size = 0) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ glTextureBufferRange(size %d = 0), (int) size);
+ return;
+  }
+
+  if (offset + size  bufObj-Size) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ glTextureBufferRange(
+ offset %d + size %d  buffer_size %d), (int) offset,
+ (int) size, (int) bufObj-Size);
+ return;
+  }
+
+  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
+   * Textures:
+   *An INVALID_VALUE error is generated if offset is not an integer
+   *multiple of the value of TEXTURE_BUFFER_OFFSET_ALIGNMENT.
+   */
+  if (offset % ctx-Const.TextureBufferOffsetAlignment) {
+ _mesa_error(ctx, GL_INVALID_VALUE,
+ glTextureBufferRange(invalid offset alignment));
+ return;
+  }
+   } else if (buffer) {
+  _mesa_error(ctx, GL_INVALID_OPERATION,
+  glTextureBufferRange(unrecognized buffer %u), buffer);
+  return;
+   } else {
+
+  /* OpenGL 4.5 core spec (30.10.2014) says in Section 8.9 Buffer
+   * Textures:
+   *If buffer is zero, then any buffer object attached to the buffer
+   *texture is detached, the values offset and size are ignored and
+   *the state for offset and size for the buffer texture are reset to
+   *zero.
+   */
+  offset = 0;
+  size = 0;
+   }
+
+   /* Get the texture object by Name. */
+   texObj = _mesa_lookup_texture_err(ctx, texture, glTextureBufferRange);
+   if (!texObj)
+  return;
+
+   if (texObj-Target != GL_TEXTURE_BUFFER_ARB) {
+  _mesa_error(ctx, GL_INVALID_ENUM, glTextureBufferRange(texture target
+ is not GL_TEXTURE_BUFFER));
+  return;