Re: [Mesa-dev] [PATCH 2/5] mesa/main: Add generic bits of ARB_clear_texture implementation

2014-07-01 Thread Neil Roberts
Jason Ekstrand ja...@jlekstrand.net writes:

 +   texImages[0] = _mesa_select_tex_image(ctx, texObj, texObj-Target,
 level);


 Do you want this inside an else block?

I think it's quite a common idiom in Mesa to handle the special cases as
a series of if-statements with a return upfront before handling the
normal case at the end without an else. But yes, in this instance it
does look a bit odd because the there is only one special case and the
normal case is very short, so I don't mind changing it.

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


Re: [Mesa-dev] [PATCH 2/5] mesa/main: Add generic bits of ARB_clear_texture implementation

2014-07-01 Thread Neil Roberts
Ian Romanick i...@freedesktop.org writes:

 Are there potentially cases where the first clear_tex_image could
 succeed but the second fail?  If so, then this is no good.  If a GL call
 generates an error (other than GL_NO_MEMORY), it is supposed to be as
 though nothing happened.

Urgh, yes, you're right because for example GL would let you make one
face of a cubemap texture be compressed while the others aren't. I'll
change it to have a separate loop for the checks as you suggest.

Thanks for the review.

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


Re: [Mesa-dev] [PATCH 2/5] mesa/main: Add generic bits of ARB_clear_texture implementation

2014-07-01 Thread Jason Ekstrand
On Tue, Jul 1, 2014 at 10:09 AM, Neil Roberts n...@linux.intel.com wrote:

 Jason Ekstrand ja...@jlekstrand.net writes:

  +   texImages[0] = _mesa_select_tex_image(ctx, texObj, texObj-Target,
  level);
 
 
  Do you want this inside an else block?

 I think it's quite a common idiom in Mesa to handle the special cases as
 a series of if-statements with a return upfront before handling the
 normal case at the end without an else. But yes, in this instance it
 does look a bit odd because the there is only one special case and the
 normal case is very short, so I don't mind changing it.


Sorry, I missed the return.  In that case, I don't think it matters; feel
free to leave it alone.
--Jason



 Regards,
 - Neil

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


Re: [Mesa-dev] [PATCH 2/5] mesa/main: Add generic bits of ARB_clear_texture implementation

2014-06-30 Thread Jason Ekstrand
On Fri, Jun 13, 2014 at 5:59 PM, Neil Roberts n...@linux.intel.com wrote:

 This adds the driver entry point for glClearTexSubImage and fills in the
 _mesa_ClearTexImage and _mesa_ClearTexSubImage functions that call it.
 ---
  src/mesa/main/dd.h   |  14 +++
  src/mesa/main/teximage.c | 241
 ++-
  src/mesa/main/teximage.h |  12 +++
  3 files changed, 266 insertions(+), 1 deletion(-)

 diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
 index 633ea2c..8976535 100644
 --- a/src/mesa/main/dd.h
 +++ b/src/mesa/main/dd.h
 @@ -239,6 +239,20 @@ struct dd_function_table {
  struct gl_texture_image *texImage );

 /**
 +* Called by glClearTex[Sub]Image
 +*
 +* Clears a rectangular region of the image to a given value. The
 +* clearValue argument is either NULL or points to a single texel to
 use as
 +* the clear value in the same internal format as the texture image.
 If it
 +* is NULL then the texture should be cleared to zeroes.
 +*/
 +   void (*ClearTexSubImage)(struct gl_context *ctx,
 +struct gl_texture_image *texImage,
 +GLint xoffset, GLint yoffset, GLint zoffset,
 +GLsizei width, GLsizei height, GLsizei depth,
 +const GLvoid *clearValue);
 +
 +   /**
  * Called by glCopyTex[Sub]Image[123]D().
  *
  * This function should copy a rectangular region in the rb to a single
 diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
 index a893c70..d5baac8 100644
 --- a/src/mesa/main/teximage.c
 +++ b/src/mesa/main/teximage.c
 @@ -51,6 +51,7 @@
  #include textureview.h
  #include mtypes.h
  #include glformats.h
 +#include texstore.h


  /**
 @@ -3848,20 +3849,259 @@ _mesa_CopyTexSubImage3D( GLenum target, GLint
 level,
 x, y, width, height);
  }

 +static bool
 +clear_tex_image(struct gl_context *ctx,
 +const char *function,
 +struct gl_texture_image *texImage, GLint level,
 +GLint xoffset, GLint yoffset, GLint zoffset,
 +GLsizei width, GLsizei height, GLsizei depth,
 +GLenum format, GLenum type,
 +const void *data)
 +{
 +   struct gl_texture_object *texObj = texImage-TexObject;
 +   static const GLubyte zeroData[MAX_PIXEL_BYTES];
 +   GLubyte clearValue[MAX_PIXEL_BYTES];
 +   GLubyte *clearValuePtr = clearValue;
 +   GLenum internalFormat = texImage-InternalFormat;
 +   GLenum err;
 +
 +   if (texObj-Target == GL_TEXTURE_BUFFER) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(buffer texture), function);
 +  return false;
 +   }
 +
 +   if (_mesa_is_compressed_format(ctx, internalFormat)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(compressed texture), function);
 +  return false;
 +   }
 +
 +   err = _mesa_error_check_format_and_type(ctx, format, type);
 +   if (err != GL_NO_ERROR) {
 +  _mesa_error(ctx, err,
 +  %s(incompatible format = %s, type = %s),
 +  function,
 +  _mesa_lookup_enum_by_nr(format),
 +  _mesa_lookup_enum_by_nr(type));
 +  return false;
 +   }
 +
 +   /* make sure internal format and format basically agree */
 +   if (!texture_formats_agree(internalFormat, format)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(incompatible internalFormat = %s, format = %s),
 +  function,
 +  _mesa_lookup_enum_by_nr(internalFormat),
 +  _mesa_lookup_enum_by_nr(format));
 +  return false;
 +   }
 +
 +   if (ctx-Version = 30 || ctx-Extensions.EXT_texture_integer) {
 +  /* both source and dest must be integer-valued, or neither */
 +  if (_mesa_is_format_integer_color(texImage-TexFormat) !=
 +  _mesa_is_enum_format_integer(format)) {
 + _mesa_error(ctx, GL_INVALID_OPERATION,
 + %s(integer/non-integer format mismatch),
 + function);
 + return false;
 +  }
 +   }
 +
 +   if (!_mesa_texstore(ctx,
 +   1, /* dims */
 +   texImage-_BaseFormat,
 +   texImage-TexFormat,
 +   0, /* dstRowStride */
 +   clearValuePtr,
 +   1, 1, 1, /* srcWidth/Height/Depth */
 +   format, type,
 +   data ? data : zeroData,
 +   ctx-DefaultPacking)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION, %s(invalid format),
 function);
 +  return false;
 +   }
 +
 +   ctx-Driver.ClearTexSubImage(ctx,
 +texImage,
 +xoffset, yoffset, zoffset,
 +width, height, depth,
 +data ? clearValue : NULL);
 +
 +   return true;
 

Re: [Mesa-dev] [PATCH 2/5] mesa/main: Add generic bits of ARB_clear_texture implementation

2014-06-30 Thread Ian Romanick
On 06/13/2014 05:59 PM, Neil Roberts wrote:
 This adds the driver entry point for glClearTexSubImage and fills in the
 _mesa_ClearTexImage and _mesa_ClearTexSubImage functions that call it.
 ---
  src/mesa/main/dd.h   |  14 +++
  src/mesa/main/teximage.c | 241 
 ++-
  src/mesa/main/teximage.h |  12 +++
  3 files changed, 266 insertions(+), 1 deletion(-)
 
 diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
 index 633ea2c..8976535 100644
 --- a/src/mesa/main/dd.h
 +++ b/src/mesa/main/dd.h
 @@ -239,6 +239,20 @@ struct dd_function_table {
  struct gl_texture_image *texImage );
  
 /**
 +* Called by glClearTex[Sub]Image
 +*
 +* Clears a rectangular region of the image to a given value. The
 +* clearValue argument is either NULL or points to a single texel to use 
 as
 +* the clear value in the same internal format as the texture image. If it
 +* is NULL then the texture should be cleared to zeroes.
 +*/
 +   void (*ClearTexSubImage)(struct gl_context *ctx,
 +struct gl_texture_image *texImage,
 +GLint xoffset, GLint yoffset, GLint zoffset,
 +GLsizei width, GLsizei height, GLsizei depth,
 +const GLvoid *clearValue);
 +
 +   /**
  * Called by glCopyTex[Sub]Image[123]D().
  *
  * This function should copy a rectangular region in the rb to a single
 diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
 index a893c70..d5baac8 100644
 --- a/src/mesa/main/teximage.c
 +++ b/src/mesa/main/teximage.c
 @@ -51,6 +51,7 @@
  #include textureview.h
  #include mtypes.h
  #include glformats.h
 +#include texstore.h
  
  
  /**
 @@ -3848,20 +3849,259 @@ _mesa_CopyTexSubImage3D( GLenum target, GLint level,
 x, y, width, height);
  }
  
 +static bool
 +clear_tex_image(struct gl_context *ctx,
 +const char *function,
 +struct gl_texture_image *texImage, GLint level,
 +GLint xoffset, GLint yoffset, GLint zoffset,
 +GLsizei width, GLsizei height, GLsizei depth,
 +GLenum format, GLenum type,
 +const void *data)
 +{
 +   struct gl_texture_object *texObj = texImage-TexObject;
 +   static const GLubyte zeroData[MAX_PIXEL_BYTES];
 +   GLubyte clearValue[MAX_PIXEL_BYTES];
 +   GLubyte *clearValuePtr = clearValue;
 +   GLenum internalFormat = texImage-InternalFormat;
 +   GLenum err;
 +
 +   if (texObj-Target == GL_TEXTURE_BUFFER) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(buffer texture), function);
 +  return false;
 +   }
 +
 +   if (_mesa_is_compressed_format(ctx, internalFormat)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(compressed texture), function);
 +  return false;
 +   }
 +
 +   err = _mesa_error_check_format_and_type(ctx, format, type);
 +   if (err != GL_NO_ERROR) {
 +  _mesa_error(ctx, err,
 +  %s(incompatible format = %s, type = %s),
 +  function,
 +  _mesa_lookup_enum_by_nr(format),
 +  _mesa_lookup_enum_by_nr(type));
 +  return false;
 +   }
 +
 +   /* make sure internal format and format basically agree */
 +   if (!texture_formats_agree(internalFormat, format)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(incompatible internalFormat = %s, format = %s),
 +  function,
 +  _mesa_lookup_enum_by_nr(internalFormat),
 +  _mesa_lookup_enum_by_nr(format));
 +  return false;
 +   }
 +
 +   if (ctx-Version = 30 || ctx-Extensions.EXT_texture_integer) {
 +  /* both source and dest must be integer-valued, or neither */
 +  if (_mesa_is_format_integer_color(texImage-TexFormat) !=
 +  _mesa_is_enum_format_integer(format)) {
 + _mesa_error(ctx, GL_INVALID_OPERATION,
 + %s(integer/non-integer format mismatch),
 + function);
 + return false;
 +  }
 +   }
 +
 +   if (!_mesa_texstore(ctx,
 +   1, /* dims */
 +   texImage-_BaseFormat,
 +   texImage-TexFormat,
 +   0, /* dstRowStride */
 +   clearValuePtr,
 +   1, 1, 1, /* srcWidth/Height/Depth */
 +   format, type,
 +   data ? data : zeroData,
 +   ctx-DefaultPacking)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION, %s(invalid format), function);
 +  return false;
 +   }
 +
 +   ctx-Driver.ClearTexSubImage(ctx,
 +texImage,
 +xoffset, yoffset, zoffset,
 +width, height, depth,
 +data ? clearValue : NULL);
 +
 +   return true;
 +}
 +
 +static struct 

Re: [Mesa-dev] [PATCH 2/5] mesa/main: Add generic bits of ARB_clear_texture implementation

2014-06-16 Thread Neil Roberts
Ilia Mirkin imir...@alum.mit.edu writes:

 +   if (ctx-Version = 30 || ctx-Extensions.EXT_texture_integer) {

 Just ctx-Extensions.EXT_texture_integer should be enough here, no?

I'm reluctant to change this because every other place in the code that
checks for integer textures does it in the same way. Perhaps if we
wanted to change it then we should do it in a separate patch and change
it everywhere. I think it might theoretically make sense to expose GL 3
without GL_EXT_texture_integer in some cases because the extension
implies supporting luminance integer textures whereas GL 3 does not.

This bit of code implies that Mesa treats the two types of support for
integer textures distinctly:

http://is.gd/HM3SIw

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


Re: [Mesa-dev] [PATCH 2/5] mesa/main: Add generic bits of ARB_clear_texture implementation

2014-06-16 Thread Marek Olšák
The presence of EXT_texture_integer implies EXT_gpu_shader4, so I
added ctx-Version = 30 || ctx-Extensions.EXT_texture_integer
everywhere to allow drivers to disable EXT_texture_integer but not
lose integer textures from GL 3.0.

Marek

On Mon, Jun 16, 2014 at 12:49 PM, Neil Roberts n...@linux.intel.com wrote:
 Ilia Mirkin imir...@alum.mit.edu writes:

 +   if (ctx-Version = 30 || ctx-Extensions.EXT_texture_integer) {

 Just ctx-Extensions.EXT_texture_integer should be enough here, no?

 I'm reluctant to change this because every other place in the code that
 checks for integer textures does it in the same way. Perhaps if we
 wanted to change it then we should do it in a separate patch and change
 it everywhere. I think it might theoretically make sense to expose GL 3
 without GL_EXT_texture_integer in some cases because the extension
 implies supporting luminance integer textures whereas GL 3 does not.

 This bit of code implies that Mesa treats the two types of support for
 integer textures distinctly:

 http://is.gd/HM3SIw

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


Re: [Mesa-dev] [PATCH 2/5] mesa/main: Add generic bits of ARB_clear_texture implementation

2014-06-13 Thread Ilia Mirkin
On Fri, Jun 13, 2014 at 8:59 PM, Neil Roberts n...@linux.intel.com wrote:
 This adds the driver entry point for glClearTexSubImage and fills in the
 _mesa_ClearTexImage and _mesa_ClearTexSubImage functions that call it.
 ---
  src/mesa/main/dd.h   |  14 +++
  src/mesa/main/teximage.c | 241 
 ++-
  src/mesa/main/teximage.h |  12 +++
  3 files changed, 266 insertions(+), 1 deletion(-)

 diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
 index 633ea2c..8976535 100644
 --- a/src/mesa/main/dd.h
 +++ b/src/mesa/main/dd.h
 @@ -239,6 +239,20 @@ struct dd_function_table {
  struct gl_texture_image *texImage );

 /**
 +* Called by glClearTex[Sub]Image
 +*
 +* Clears a rectangular region of the image to a given value. The
 +* clearValue argument is either NULL or points to a single texel to use 
 as
 +* the clear value in the same internal format as the texture image. If it
 +* is NULL then the texture should be cleared to zeroes.
 +*/
 +   void (*ClearTexSubImage)(struct gl_context *ctx,
 +struct gl_texture_image *texImage,
 +GLint xoffset, GLint yoffset, GLint zoffset,
 +GLsizei width, GLsizei height, GLsizei depth,
 +const GLvoid *clearValue);
 +
 +   /**
  * Called by glCopyTex[Sub]Image[123]D().
  *
  * This function should copy a rectangular region in the rb to a single
 diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
 index a893c70..d5baac8 100644
 --- a/src/mesa/main/teximage.c
 +++ b/src/mesa/main/teximage.c
 @@ -51,6 +51,7 @@
  #include textureview.h
  #include mtypes.h
  #include glformats.h
 +#include texstore.h


  /**
 @@ -3848,20 +3849,259 @@ _mesa_CopyTexSubImage3D( GLenum target, GLint level,
 x, y, width, height);
  }

 +static bool
 +clear_tex_image(struct gl_context *ctx,
 +const char *function,
 +struct gl_texture_image *texImage, GLint level,
 +GLint xoffset, GLint yoffset, GLint zoffset,
 +GLsizei width, GLsizei height, GLsizei depth,
 +GLenum format, GLenum type,
 +const void *data)
 +{
 +   struct gl_texture_object *texObj = texImage-TexObject;
 +   static const GLubyte zeroData[MAX_PIXEL_BYTES];
 +   GLubyte clearValue[MAX_PIXEL_BYTES];
 +   GLubyte *clearValuePtr = clearValue;
 +   GLenum internalFormat = texImage-InternalFormat;
 +   GLenum err;
 +
 +   if (texObj-Target == GL_TEXTURE_BUFFER) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(buffer texture), function);
 +  return false;
 +   }
 +
 +   if (_mesa_is_compressed_format(ctx, internalFormat)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(compressed texture), function);
 +  return false;
 +   }
 +
 +   err = _mesa_error_check_format_and_type(ctx, format, type);
 +   if (err != GL_NO_ERROR) {
 +  _mesa_error(ctx, err,
 +  %s(incompatible format = %s, type = %s),
 +  function,
 +  _mesa_lookup_enum_by_nr(format),
 +  _mesa_lookup_enum_by_nr(type));
 +  return false;
 +   }
 +
 +   /* make sure internal format and format basically agree */
 +   if (!texture_formats_agree(internalFormat, format)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION,
 +  %s(incompatible internalFormat = %s, format = %s),
 +  function,
 +  _mesa_lookup_enum_by_nr(internalFormat),
 +  _mesa_lookup_enum_by_nr(format));
 +  return false;
 +   }
 +
 +   if (ctx-Version = 30 || ctx-Extensions.EXT_texture_integer) {

Just ctx-Extensions.EXT_texture_integer should be enough here, no?

 +  /* both source and dest must be integer-valued, or neither */
 +  if (_mesa_is_format_integer_color(texImage-TexFormat) !=
 +  _mesa_is_enum_format_integer(format)) {
 + _mesa_error(ctx, GL_INVALID_OPERATION,
 + %s(integer/non-integer format mismatch),
 + function);
 + return false;
 +  }
 +   }
 +
 +   if (!_mesa_texstore(ctx,
 +   1, /* dims */
 +   texImage-_BaseFormat,
 +   texImage-TexFormat,
 +   0, /* dstRowStride */
 +   clearValuePtr,
 +   1, 1, 1, /* srcWidth/Height/Depth */
 +   format, type,
 +   data ? data : zeroData,
 +   ctx-DefaultPacking)) {
 +  _mesa_error(ctx, GL_INVALID_OPERATION, %s(invalid format), function);
 +  return false;
 +   }
 +
 +   ctx-Driver.ClearTexSubImage(ctx,
 +texImage,
 +xoffset, yoffset, zoffset,
 +width, height, depth,
 +