Re: [Mesa-dev] [PATCH 6/7] i965: add functional changes for AMD_depth_clamp_separate

2018-08-22 Thread Ian Romanick
On 08/21/2018 05:02 PM, Sagar Ghuge wrote:
> Gen >= 9 have ability to control clamping of depth values separately at
> near and far plane.
> 
> z_w is clamped to the range [min(n,f), 0] if clamping at near plane is
> enabled, [0, max(n,f)] if clamping at far plane is enabled and [min(n,f)
> max(n,f)] if clamping at both plane is enabled.
> 
> Signed-off-by: Sagar Ghuge 
> ---
>  src/mesa/drivers/dri/i965/genX_state_upload.c | 23 +++
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c 
> b/src/mesa/drivers/dri/i965/genX_state_upload.c
> index dc54cb67af..4e978bed91 100644
> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> @@ -2343,7 +2343,13 @@ genX(upload_cc_viewport)(struct brw_context *brw)
>if (ctx->Transform.DepthClampNear && ctx->Transform.DepthClampFar) {
>   ccv.MinimumDepth = MIN2(vp->Near, vp->Far);
>   ccv.MaximumDepth = MAX2(vp->Near, vp->Far);
> -  } else {
> +  } else if (ctx->Transform.DepthClampNear) {
> + ccv.MinimumDepth = MIN2(vp->Near, vp->Far);
> + ccv.MaximumDepth = 0.0;
> +  } else if (ctx->Transform.DepthClampFar) {
> + ccv.MinimumDepth = 0.0;
> + ccv.MaximumDepth = MAX2(vp->Near, vp->Far);
> +  }else {
  ^
Missing space here.

With that fixed, this patch is

Reviewed-by: Ian Romanick 

>   ccv.MinimumDepth = 0.0;
>   ccv.MaximumDepth = 1.0;
>}
> @@ -4607,16 +4613,23 @@ genX(upload_raster)(struct brw_context *brw)
>raster.ScissorRectangleEnable = ctx->Scissor.EnableFlags;
>  
>/* _NEW_TRANSFORM */
> +#if GEN_GEN < 9
>if (!(ctx->Transform.DepthClampNear &&
>  ctx->Transform.DepthClampFar)) {
> -#if GEN_GEN >= 9
> - raster.ViewportZFarClipTestEnable = true;
> - raster.ViewportZNearClipTestEnable = true;
> -#else
>   raster.ViewportZClipTestEnable = true;
> +  }
>  #endif
> +
> +#if GEN_GEN >= 9
> +  if (!ctx->Transform.DepthClampNear) {
> + raster.ViewportZNearClipTestEnable = true;
>}

If the condition and the body of an if-statement are each a single text
line, it's okay to omit the { }.  If either the condition or the single
statement of the body are long enough to require line-wrapping, we still
require the { } because it improves readability.

You don't need to change this here.  I just thought I'd let you know for
future reference.  It's "coder's preference." :)

>  
> +  if (!ctx->Transform.DepthClampFar) {
> + raster.ViewportZFarClipTestEnable = true;
> +  }
> +#endif
> +
>/* BRW_NEW_CONSERVATIVE_RASTERIZATION */
>  #if GEN_GEN >= 9
>raster.ConservativeRasterizationEnable =
> 

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


[Mesa-dev] [PATCH 6/7] i965: add functional changes for AMD_depth_clamp_separate

2018-08-21 Thread Sagar Ghuge
Gen >= 9 have ability to control clamping of depth values separately at
near and far plane.

z_w is clamped to the range [min(n,f), 0] if clamping at near plane is
enabled, [0, max(n,f)] if clamping at far plane is enabled and [min(n,f)
max(n,f)] if clamping at both plane is enabled.

Signed-off-by: Sagar Ghuge 
---
 src/mesa/drivers/dri/i965/genX_state_upload.c | 23 +++
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c 
b/src/mesa/drivers/dri/i965/genX_state_upload.c
index dc54cb67af..4e978bed91 100644
--- a/src/mesa/drivers/dri/i965/genX_state_upload.c
+++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
@@ -2343,7 +2343,13 @@ genX(upload_cc_viewport)(struct brw_context *brw)
   if (ctx->Transform.DepthClampNear && ctx->Transform.DepthClampFar) {
  ccv.MinimumDepth = MIN2(vp->Near, vp->Far);
  ccv.MaximumDepth = MAX2(vp->Near, vp->Far);
-  } else {
+  } else if (ctx->Transform.DepthClampNear) {
+ ccv.MinimumDepth = MIN2(vp->Near, vp->Far);
+ ccv.MaximumDepth = 0.0;
+  } else if (ctx->Transform.DepthClampFar) {
+ ccv.MinimumDepth = 0.0;
+ ccv.MaximumDepth = MAX2(vp->Near, vp->Far);
+  }else {
  ccv.MinimumDepth = 0.0;
  ccv.MaximumDepth = 1.0;
   }
@@ -4607,16 +4613,23 @@ genX(upload_raster)(struct brw_context *brw)
   raster.ScissorRectangleEnable = ctx->Scissor.EnableFlags;
 
   /* _NEW_TRANSFORM */
+#if GEN_GEN < 9
   if (!(ctx->Transform.DepthClampNear &&
 ctx->Transform.DepthClampFar)) {
-#if GEN_GEN >= 9
- raster.ViewportZFarClipTestEnable = true;
- raster.ViewportZNearClipTestEnable = true;
-#else
  raster.ViewportZClipTestEnable = true;
+  }
 #endif
+
+#if GEN_GEN >= 9
+  if (!ctx->Transform.DepthClampNear) {
+ raster.ViewportZNearClipTestEnable = true;
   }
 
+  if (!ctx->Transform.DepthClampFar) {
+ raster.ViewportZFarClipTestEnable = true;
+  }
+#endif
+
   /* BRW_NEW_CONSERVATIVE_RASTERIZATION */
 #if GEN_GEN >= 9
   raster.ConservativeRasterizationEnable =
-- 
2.17.1

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