[Mesa-dev] [PATCH 2/2] gallivm: add comment for bogus min/mag filter selection with nearest mip filter

[Jose Fonseca]

Series looks good.

You might want to promote XXX to FIXME. Fixing wouldn't be hard -- just need to pass some additional parameters to this function, so that we can check that MIN/MAG filter are same.

Jose

----- Original Message -----
> From: Roland Scheidegger <sroland at vmware.com>
> 
> Detected this hunting some other bug, not sure if it really needs fixing but
> it is definitely wrong.
> ---
>  src/gallium/auxiliary/gallivm/lp_bld_sample.c     |    8 ++++++++
>  src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c |    2 +-
>  src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c |    2 +-
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_sample.c
> b/src/gallium/auxiliary/gallivm/lp_bld_sample.c
> index d339aba..696855b 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_sample.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_sample.c
> @@ -700,6 +700,14 @@ lp_build_lod_selector(struct lp_build_sample_context
> *bld,
>  
>              if (mip_filter == PIPE_TEX_MIPFILTER_NONE ||
>                  mip_filter == PIPE_TEX_MIPFILTER_NEAREST) {
> +               /*
> +                * XXX: this is not entirely correct, as out_lod_ipart is
> used
> +                * both for mip level determination as well as mag/min
> switchover
> +                * point (if different min/mag filters are used). In
> particular,
> +                * lod values between [-0.5,0] (rho between [sqrt(2), 1.0])
> will
> +                * incorrectly use min filter instead of mag (the
> non-optimized
> +                * calculation further down has exactly the same problem).
> +                */
>                 *out_lod_ipart = lp_build_ilog2(levelf_bld, rho);
>                 *out_lod_fpart = levelf_bld->zero;
>                 return;
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c
> b/src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c
> index da416aa..2573cec 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_sample_aos.c
> @@ -1611,7 +1611,7 @@ lp_build_sample_aos(struct lp_build_sample_context
> *bld,
>        LLVMValueRef minify;
>  
>        /*
> -       * XXX this should to all lods into account, if some are min
> +       * XXX this should take all lods into account, if some are min
>         * some max probably could hack up the coords/weights in the linear
>         * path with selects to work for nearest.
>         * If that's just two quads sitting next to each other it seems
> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
> b/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
> index 6d12587..34ab414 100644
> --- a/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
> +++ b/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
> @@ -1636,7 +1636,7 @@ lp_build_sample_general(struct lp_build_sample_context
> *bld,
>        LLVMValueRef minify;
>  
>        /*
> -       * XXX this should to all lods into account, if some are min
> +       * XXX this should take all lods into account, if some are min
>         * some max probably could hack up the coords/weights in the linear
>         * path with selects to work for nearest.
>         * If that's just two quads sitting next to each other it seems
> --
> 1.7.9.5
>