[Mesa-dev] [PATCH 2/9] i965: Fix textureGrad with cube samplers

[Ben Widawsky]

On Tue, Feb 24, 2015 at 07:02:50PM +0100, Eduardo Lima Mitev wrote:
> From: Iago Toral Quiroga <itoral at igalia.com>
> 
> We can't use sampler messages with gradient information (like
> sample_g or sample_d) to deal with this scenario because according
> to the PRM:
> 
> "The r coordinate and its gradients are required only for surface
> types that use the third coordinate. Usage of this message type on
> cube surfaces assumes that the u, v, and gradients have already been
> transformed onto the appropriate face, but still in [-1,+1] range.
> The r coordinate contains the faceid, and the r gradients are ignored
> by hardware."
> 
> Instead, we should lower this to compute the LOD manually based on the
> gradients and use a different sample message that takes the computed
> LOD instead of the gradients. This is already being done in
> brw_lower_texture_gradients.cpp, but it is restricted to shadow
> samplers only, although there is a comment stating that we should
> probably do this also for samplerCube and samplerCubeArray.
> 
> Because of this, both dEQP and Piglit test cases for textureGrad with
> cube maps currently fail.
> 
> This patch does two things:
> 1) Activates the texturegrad lowering pass for all cube samplers.
> 2) Corrects the computation of the LOD value for cube samplers.
> 
> I had to do 2) because for cube maps the calculations implemented
> in the lowering pass always compute a value of rho that is twice
> the value we want (so we get a LOD value one unit larger than we
> want). This only happens for cube map samplers (all kinds). I am
> not sure about why we need to do this, but I suspect that it is
> related to the fact that cube map coordinates, when transported
> to a specific face in the cube, are in the range [-1, 1] instead of
> [0, 1] so we probably need to divide the derivatives by 2 when
> we compute the LOD. Doing that would produce the same result as
> dividing the final rho computation by 2 (or removing a unit
> from the computed LOD, which is what we are doing here).
> 
> Fixes the following piglit tests:
> bin/tex-miplevel-selection textureGrad Cube -auto -fbo
> bin/tex-miplevel-selection textureGrad CubeArray -auto -fbo
> bin/tex-miplevel-selection textureGrad CubeShadow -auto -fbo
> 
> Fixes 10 dEQP tests in the following category:
> dEQP-GLES3.functional.shaders.texture_functions.texturegrad.*cube*

What$ happened to this patch? It seems like we still need/want it, and it seemed
like people had been looking at it. Was it just missing a formal review?

> ---
>  .../dri/i965/brw_lower_texture_gradients.cpp       | 26 +++++++++++++++-------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_lower_texture_gradients.cpp b/src/mesa/drivers/dri/i965/brw_lower_texture_gradients.cpp
> index 9679d28..878a54e 100644
> --- a/src/mesa/drivers/dri/i965/brw_lower_texture_gradients.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_lower_texture_gradients.cpp
> @@ -89,19 +89,18 @@ txs_type(const glsl_type *type)
>  ir_visitor_status
>  lower_texture_grad_visitor::visit_leave(ir_texture *ir)
>  {
> -   /* Only lower textureGrad with shadow samplers */
> -   if (ir->op != ir_txd || !ir->shadow_comparitor)
> +   /* Only lower textureGrad with cube maps or shadow samplers */
> +   if (ir->op != ir_txd ||
> +      (ir->sampler->type->sampler_dimensionality != GLSL_SAMPLER_DIM_CUBE &&
> +       !ir->shadow_comparitor))
>        return visit_continue;
>  

Do you need this for GLSL_SAMPLER_DIM_3D? It seems to fit with the PRM blurb
about "use the third coordinate"

> -   /* Lower textureGrad() with samplerCubeShadow even if we have the sample_d_c
> +   /* Lower textureGrad() with samplerCube* even if we have the sample_d_c
>      * message.  GLSL provides gradients for the 'r' coordinate.  Unfortunately:
>      *
>      * From the Ivybridge PRM, Volume 4, Part 1, sample_d message description:
>      * "The r coordinate contains the faceid, and the r gradients are ignored
>      *  by hardware."
> -    *
> -    * We likely need to do a similar treatment for samplerCube and
> -    * samplerCubeArray, but we have insufficient testing for that at the moment.
>      */
>     bool need_lowering = !has_sample_d_c ||
>        ir->sampler->type->sampler_dimensionality == GLSL_SAMPLER_DIM_CUBE;
> @@ -153,9 +152,20 @@ lower_texture_grad_visitor::visit_leave(ir_texture *ir)
>  			       expr(ir_unop_sqrt, dot(dPdy, dPdy)));
>     }
>  
> -   /* lambda_base = log2(rho).  We're ignoring GL state biases for now. */
> +   /* lambda_base = log2(rho).  We're ignoring GL state biases for now.
> +    *
> +    * For cube maps the result of these formulas is giving us a value of rho
> +    * that is twice the value we should use, so divide it by 2 or,
> +    * alternatively, remove one unit from the result of the log2 computation.
> +    */
>     ir->op = ir_txl;
> -   ir->lod_info.lod = expr(ir_unop_log2, rho);
> +   if (ir->sampler->type->sampler_dimensionality == GLSL_SAMPLER_DIM_CUBE) {
> +      ir->lod_info.lod = expr(ir_binop_add,
> +                              expr(ir_unop_log2, rho),
> +                              new(mem_ctx) ir_constant(-1.0f));
> +   } else {
> +      ir->lod_info.lod = expr(ir_unop_log2, rho);
> +   }
>  
>     progress = true;
>     return visit_continue;

Patch seems to do what it's advertising. I am not really an expert here, but
fwiw:
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>