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"
> - /* 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>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
