Thank you for volunteering to test my branch. But before I point you to the 
branch,
I will rework patches according to your comment on patch 3.

Again thanks a lot for your and Ian's input. 

- Sagar  

On 08/20/2018 04:21 PM, Marek Olšák wrote:
> I can try to test the extension with the radeonsi driver. Do you have
> a Mesa branch with the final patches?
> 
> Marek
> 
> On Mon, Aug 13, 2018 at 5:35 PM Sagar Ghuge <sagar.gh...@intel.com> wrote:
>>
>> 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 <i...@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.gh...@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
>>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to