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

[Iago Toral]

On Tue, 2015-06-16 at 23:52 -0700, Ben Widawsky wrote:
> 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?

Yes, I think nobody gave it a formal reviewed-by yet.

Iago

> > ---
> >  .../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>
>