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

[Marek Olšák]

On Thu, Aug 2, 2018 at 2:44 PM, Ian Romanick <idr at freedesktop.org> wrote:
> 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.

I agree.

Marek