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

[Sagar Ghuge]

Hi everyone,

I am kind of stuck on this part actually. I don't have 
latest AMD graphics card to test following behavior which
Ian and Marek suggested me. 

I have written a piglit test :
https://gitlab.freedesktop.org/sagarghuge/piglit/blob/320b91ffb131b380f1d27d9c05ab141e0cd9e557/tests/spec/amd_depth_clamp_separate/depth_clamp_get_test.c

It would be great if someone can help me or test it in their 
spare time on latest AMD graphics card and provide some input
on the extension behavior on AMD's closed source driver.     


On 08/09/2018 01:11 PM, Marek Olšák wrote:
> 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
>