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 <ito...@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"
I did not identify the need to do this for that kind, at least I did not
find any tests that failed in either deqp or piglit.
> > - /* 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 <b...@bwidawsk.net>
>
Thanks Ben!
Iago
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
