Re: [Mesa-dev] [PATCH 2/6] gallium: implement clamping controls (ARB_color_buffer_float)

2011-03-21 Thread Keith Whitwell
On Mon, 2011-03-21 at 02:12 +0100, Marek Olšák wrote:
 diff --git a/src/gallium/include/pipe/p_state.h
 b/src/gallium/include/pipe/p_state.h
 index cf6c5b5..f6ad456 100644
 --- a/src/gallium/include/pipe/p_state.h
 +++ b/src/gallium/include/pipe/p_state.h
 @@ -81,6 +81,8 @@ struct pipe_rasterizer_state
  {
 unsigned flatshade:1;
 unsigned light_twoside:1;
 +   unsigned clamp_vertex_color:1;
 +   unsigned clamp_fragment_color:1;
 unsigned front_ccw:1;
 unsigned cull_face:2;  /** PIPE_FACE_x */
 unsigned fill_front:2; /** PIPE_POLYGON_MODE_x */ 

Don't know if this affects the overall packing of the struct.  Have you
been able to check?

Otherwise the interface changes look good to me.

Keith

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


Re: [Mesa-dev] [PATCH 2/6] gallium: implement clamping controls (ARB_color_buffer_float)

2011-03-21 Thread Christoph Bumiller
On 03/21/2011 02:12 AM, Marek Olšák wrote:

 diff --git a/src/gallium/include/pipe/p_state.h 
 b/src/gallium/include/pipe/p_state.h
 index cf6c5b5..f6ad456 100644
 --- a/src/gallium/include/pipe/p_state.h
 +++ b/src/gallium/include/pipe/p_state.h
 @@ -81,6 +81,8 @@ struct pipe_rasterizer_state
  {
 unsigned flatshade:1;
 unsigned light_twoside:1;
 +   unsigned clamp_vertex_color:1;
 +   unsigned clamp_fragment_color:1;
 unsigned front_ccw:1;
 unsigned cull_face:2;  /** PIPE_FACE_x */
 unsigned fill_front:2; /** PIPE_POLYGON_MODE_x */

Hadn't you put clamp_fragment_color in the blend state initially ?
It seems like a more logical place to me.

Beyond that I don't mind where it ends up though, both CSOs are growing
a little too large for my taste.

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


Re: [Mesa-dev] [PATCH 2/6] gallium: implement clamping controls (ARB_color_buffer_float)

2011-03-21 Thread Keith Whitwell
On Mon, 2011-03-21 at 16:23 +0100, Christoph Bumiller wrote:
 On 03/21/2011 02:12 AM, Marek Olšák wrote:
 
  diff --git a/src/gallium/include/pipe/p_state.h 
  b/src/gallium/include/pipe/p_state.h
  index cf6c5b5..f6ad456 100644
  --- a/src/gallium/include/pipe/p_state.h
  +++ b/src/gallium/include/pipe/p_state.h
  @@ -81,6 +81,8 @@ struct pipe_rasterizer_state
   {
  unsigned flatshade:1;
  unsigned light_twoside:1;
  +   unsigned clamp_vertex_color:1;
  +   unsigned clamp_fragment_color:1;
  unsigned front_ccw:1;
  unsigned cull_face:2;  /** PIPE_FACE_x */
  unsigned fill_front:2; /** PIPE_POLYGON_MODE_x */
 
 Hadn't you put clamp_fragment_color in the blend state initially ?
 It seems like a more logical place to me.

Indeed you're right.  Fragment color clamping takes place in the part of
the pipeline governed by the blend CSO.

Keith

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


Re: [Mesa-dev] [PATCH 2/6] gallium: implement clamping controls (ARB_color_buffer_float)

2011-03-21 Thread Brian Paul

On 03/21/2011 09:10 AM, Keith Whitwell wrote:

On Mon, 2011-03-21 at 02:12 +0100, Marek Olšák wrote:

diff --git a/src/gallium/include/pipe/p_state.h
b/src/gallium/include/pipe/p_state.h
index cf6c5b5..f6ad456 100644
--- a/src/gallium/include/pipe/p_state.h
+++ b/src/gallium/include/pipe/p_state.h
@@ -81,6 +81,8 @@ struct pipe_rasterizer_state
  {
 unsigned flatshade:1;
 unsigned light_twoside:1;
+   unsigned clamp_vertex_color:1;
+   unsigned clamp_fragment_color:1;
 unsigned front_ccw:1;
 unsigned cull_face:2;  /**  PIPE_FACE_x */
 unsigned fill_front:2; /**  PIPE_POLYGON_MODE_x */


Don't know if this affects the overall packing of the struct.  Have you
been able to check?

Otherwise the interface changes look good to me.



How do most GPUs handle this clamping?  Is it done with extra shader 
code or is it a discrete operation?  If most hardware does it with 
shader code, maybe we should just do it that way too in Gallium and 
avoid the new cap bits, etc.


OpenGL 3.0 deprecates the per-vertex and per-fragment clamp controls 
so people will have to do it in their shaders eventually.



Re: your comment in rasterizer.rst:

XXX: this happens _before_ the geometry shader and thus might not 
belong here.


So is there some question about where the per-vertex clamp is to be 
done?  It seems to me that it should be done after the geometry shader.


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


Re: [Mesa-dev] [PATCH 2/6] gallium: implement clamping controls (ARB_color_buffer_float)

2011-03-21 Thread Marek Olšák
Yeah I moved it from the rasterizer state to the blend state, but then I
realized the rasterizer state had been a better place. The thing is the
fragment color clamping is done right after the fragment shader output is
written, that is before the alpha test, before the depth/stencil test, and
therefore long before blending. So I don't know where to put it. Obviously,
the depth-stencil-alpha state is a better candidate than the blend state.
Also you can do some nasty things with it. For example, you can use the
clamping to clamp the fragment color after the fragment shader and then
multiply it by 2 in the blending stage, effectively outputting a value in
the range [0, 2].

Marek

On Mon, Mar 21, 2011 at 4:36 PM, Keith Whitwell kei...@vmware.com wrote:

 On Mon, 2011-03-21 at 16:23 +0100, Christoph Bumiller wrote:
  On 03/21/2011 02:12 AM, Marek Olšák wrote:
 
   diff --git a/src/gallium/include/pipe/p_state.h
 b/src/gallium/include/pipe/p_state.h
   index cf6c5b5..f6ad456 100644
   --- a/src/gallium/include/pipe/p_state.h
   +++ b/src/gallium/include/pipe/p_state.h
   @@ -81,6 +81,8 @@ struct pipe_rasterizer_state
{
   unsigned flatshade:1;
   unsigned light_twoside:1;
   +   unsigned clamp_vertex_color:1;
   +   unsigned clamp_fragment_color:1;
   unsigned front_ccw:1;
   unsigned cull_face:2;  /** PIPE_FACE_x */
   unsigned fill_front:2; /** PIPE_POLYGON_MODE_x */
 
  Hadn't you put clamp_fragment_color in the blend state initially ?
  It seems like a more logical place to me.

 Indeed you're right.  Fragment color clamping takes place in the part of
 the pipeline governed by the blend CSO.

 Keith


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


Re: [Mesa-dev] [PATCH 2/6] gallium: implement clamping controls (ARB_color_buffer_float)

2011-03-21 Thread Marek Olšák
On Mon, Mar 21, 2011 at 4:50 PM, Brian Paul bri...@vmware.com wrote:

 On 03/21/2011 09:10 AM, Keith Whitwell wrote:

 On Mon, 2011-03-21 at 02:12 +0100, Marek Olšák wrote:

 diff --git a/src/gallium/include/pipe/p_state.h
 b/src/gallium/include/pipe/p_state.h
 index cf6c5b5..f6ad456 100644
 --- a/src/gallium/include/pipe/p_state.h
 +++ b/src/gallium/include/pipe/p_state.h
 @@ -81,6 +81,8 @@ struct pipe_rasterizer_state
  {
 unsigned flatshade:1;
 unsigned light_twoside:1;
 +   unsigned clamp_vertex_color:1;
 +   unsigned clamp_fragment_color:1;
 unsigned front_ccw:1;
 unsigned cull_face:2;  /**  PIPE_FACE_x */
 unsigned fill_front:2; /**  PIPE_POLYGON_MODE_x */


 Don't know if this affects the overall packing of the struct.  Have you
 been able to check?

 Otherwise the interface changes look good to me.



 How do most GPUs handle this clamping?  Is it done with extra shader code
 or is it a discrete operation?  If most hardware does it with shader code,
 maybe we should just do it that way too in Gallium and avoid the new cap
 bits, etc.


There is a bit in the CB_COLOR[0-7]_INFO register on R600 which controls
color clamping before blending. I am not sure whether it can be used for
ARB_color_buffer_float or whether it's strictly dependent on the colorbuffer
format.

Christopher, do you happen to know whether NV50/NVC0 has a hardware state
for it too?



 OpenGL 3.0 deprecates the per-vertex and per-fragment clamp controls so
 people will have to do it in their shaders eventually.


 Re: your comment in rasterizer.rst:


 XXX: this happens _before_ the geometry shader and thus might not belong
 here.

 So is there some question about where the per-vertex clamp is to be done?
  It seems to me that it should be done after the geometry shader.


You're right. I will remove that comment.

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


Re: [Mesa-dev] [PATCH 2/6] gallium: implement clamping controls (ARB_color_buffer_float)

2011-03-21 Thread Christoph Bumiller
On 03/21/2011 05:43 PM, Marek Olšák wrote:
 On Mon, Mar 21, 2011 at 4:50 PM, Brian Paul bri...@vmware.com wrote:
 
 On 03/21/2011 09:10 AM, Keith Whitwell wrote:

 On Mon, 2011-03-21 at 02:12 +0100, Marek Olšák wrote:

 diff --git a/src/gallium/include/pipe/p_state.h
 b/src/gallium/include/pipe/p_state.h
 index cf6c5b5..f6ad456 100644
 --- a/src/gallium/include/pipe/p_state.h
 +++ b/src/gallium/include/pipe/p_state.h
 @@ -81,6 +81,8 @@ struct pipe_rasterizer_state
  {
 unsigned flatshade:1;
 unsigned light_twoside:1;
 +   unsigned clamp_vertex_color:1;
 +   unsigned clamp_fragment_color:1;
 unsigned front_ccw:1;
 unsigned cull_face:2;  /**  PIPE_FACE_x */
 unsigned fill_front:2; /**  PIPE_POLYGON_MODE_x */


 Don't know if this affects the overall packing of the struct.  Have you
 been able to check?

 Otherwise the interface changes look good to me.



 How do most GPUs handle this clamping?  Is it done with extra shader code
 or is it a discrete operation?  If most hardware does it with shader code,
 maybe we should just do it that way too in Gallium and avoid the new cap
 bits, etc.

 
 There is a bit in the CB_COLOR[0-7]_INFO register on R600 which controls
 color clamping before blending. I am not sure whether it can be used for
 ARB_color_buffer_float or whether it's strictly dependent on the colorbuffer
 format.
 
 Christoph, do you happen to know whether NV50/NVC0 has a hardware state
 for it too?
 
 
Sure,
there's a command to control per-RT fragment colour clamping and one to
enable/disable vertex colour clamping. No fiddling with shaders.

Not sure where exactly the clamping happens, but it's supposed to behave
like OpenGL requires it to.

(So we just call them FRAG_COLOR_CLAMP_EN and VERT_COLOR_CLAMP_EN,
on nv50 the latter is part of the color semantic/linkage setup, on nvc0
it always applies to the dedicated colour outputs.)

 
 OpenGL 3.0 deprecates the per-vertex and per-fragment clamp controls so
 people will have to do it in their shaders eventually.


 Re: your comment in rasterizer.rst:


 XXX: this happens _before_ the geometry shader and thus might not belong
 here.

 So is there some question about where the per-vertex clamp is to be done?
  It seems to me that it should be done after the geometry shader.

 
 You're right. I will remove that comment.
 
 Marek
 

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