[Mesa-dev] [RFC][PATCH 0/5] mesa: Add types for AMD_depth_clamp_separate.

[Ian Romanick]

On 08/02/2018 11:30 AM, Ian Romanick wrote:
> On 08/01/2018 08:31 PM, Sagar Ghuge wrote:
>> Add some basic types and storage for the
>> AMD_depth_clamp_separate extension.

I mentioned this on patch 5, but you should word wrap the commit message
to 70 or 72 columns.

More substantive comments are below...

>> Signed-off-by: Sagar Ghuge <sagar.ghuge at intel.com>
>> ---
>>  include/GL/glcorearb.h           | 2 ++
>>  src/mesa/main/extensions_table.h | 1 +
>>  src/mesa/main/mtypes.h           | 9 +++++++++
>>  3 files changed, 12 insertions(+)
>>
>> diff --git a/include/GL/glcorearb.h b/include/GL/glcorearb.h
>> index a78bbb6e18..d73ca5a8df 100644
>> --- a/include/GL/glcorearb.h
>> +++ b/include/GL/glcorearb.h
>> @@ -1558,6 +1558,8 @@ typedef int64_t GLint64;
>>  #define GL_MAX_FRAGMENT_INPUT_COMPONENTS  0x9125
>>  #define GL_CONTEXT_PROFILE_MASK           0x9126
>>  #define GL_DEPTH_CLAMP                    0x864F
>> +#define GL_DEPTH_CLAMP_NEAR_AMD           0x901E
>> +#define GL_DEPTH_CLAMP_FAR_AMD            0x901F
>>  #define GL_QUADS_FOLLOW_PROVOKING_VERTEX_CONVENTION 0x8E4C
>>  #define GL_FIRST_VERTEX_CONVENTION        0x8E4D
>>  #define GL_LAST_VERTEX_CONVENTION         0x8E4E
> 
> We should just import the updated versions of the Khronos headers.  I
> think Marek sent out a patch to do this.  Does that work?
> 
>> diff --git a/src/mesa/main/extensions_table.h b/src/mesa/main/extensions_table.h
>> index 3f01896cae..8dc668e087 100644
>> --- a/src/mesa/main/extensions_table.h
>> +++ b/src/mesa/main/extensions_table.h
>> @@ -9,6 +9,7 @@
>>  EXT(3DFX_texture_compression_FXT1           , TDFX_texture_compression_FXT1          , GLL, GLC,  x ,  x , 1999)
>>  
>>  EXT(AMD_conservative_depth                  , ARB_conservative_depth                 , GLL, GLC,  x ,  x , 2009)
>> +EXT(AMD_depth_clamp_separate                , AMD_depth_clamp_separate               ,  x , GLC,  x ,  x , 2009)
>>  EXT(AMD_draw_buffers_blend                  , ARB_draw_buffers_blend                 , GLL, GLC,  x ,  x , 2009)
>>  EXT(AMD_performance_monitor                 , AMD_performance_monitor                , GLL, GLC,  x , ES2, 2007)
>>  EXT(AMD_pinned_memory                       , AMD_pinned_memory                      , GLL, GLC,  x ,  x , 2013)
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index d71872835d..406746a84c 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -1280,6 +1280,8 @@ struct gl_transform_attrib
>>     GLboolean RescaleNormals;			/**< GL_EXT_rescale_normal */
>>     GLboolean RasterPositionUnclipped;           /**< GL_IBM_rasterpos_clip */
>>     GLboolean DepthClamp;			/**< GL_ARB_depth_clamp */
>> +   GLboolean DepthClampNear;			/**< GL_AMD_depth_clamp_separate */
>> +   GLboolean DepthClampFar;			/**< GL_AMD_depth_clamp_separate */

I think we actually need two more flags here: _DepthClampNear and
_DepthClampFar.  The spec is a little unclear, so you may need to test
on some AMD closed-source drivers.  Specifically, the spec says

    "In addition to DEPTH_CLAMP_NEAR_AMD and DEPTH_CLAMP_FAR_AMD, the
    token DEPTH_CLAMP may be used to simultaneously enable or disable
    depth clamping at both the near and far planes."

Based on that, I'm not sure what you're supposed to get if you do:

    glDisable(GL_DEPTH_CLAMP_NEAR_AMD);
    glEnable(GL_DEPTH_CLAMP);
    glGetIntegerv(GL_DEPTH_CLAMP_NEAR_AMD, &v);

Should v contain GL_TRUE or GL_FALSE?  It seems clear that rendering
should have the near plane clamped.

Depending on the results of testing on AMD drivers, we either need
enable / disable of GL_DEPTH_CLAMP to set / reset
gl_transform_attrib::DepthClampNear and
gl_transform_attrib::DepthClampFar or we need to add _DepthClampNear and
_DepthClampFar that get modified.  In the latter case, the driver would
only ever look at _DepthClampNear and _DepthClampFar.

>>     /** GL_ARB_clip_control */
>>     GLenum16 ClipOrigin;   /**< GL_LOWER_LEFT or GL_UPPER_LEFT */
>>     GLenum16 ClipDepthMode;/**< GL_NEGATIVE_ONE_TO_ONE or GL_ZERO_TO_ONE */
>> @@ -4235,6 +4237,7 @@ struct gl_extensions
>>     GLboolean OES_texture_view;
>>     GLboolean OES_viewport_array;
>>     /* vendor extensions */
>> +   GLboolean AMD_depth_clamp_separate;
>>     GLboolean AMD_performance_monitor;
>>     GLboolean AMD_pinned_memory;
>>     GLboolean AMD_seamless_cubemap_per_texture;
>> @@ -4577,6 +4580,12 @@ struct gl_driver_flags
>>     /** gl_context::Transform::DepthClamp */
>>     uint64_t NewDepthClamp;
>>  
>> +   /** gl_context::Transform::DepthClampNear */
>> +   uint64_t NewDepthClampNear;
>> +
>> +   /** gl_context::Transform::DepthClampFar */
>> +   uint64_t NewDepthClampFar;
>> +

I don't think we need separate flags for near and far.  I think we can
just use the NewDepthClamp flag for all three.

>>     /** gl_context::Line */
>>     uint64_t NewLineState;
>>