[Mesa-dev] [PATCH 2/2] llvmpipe: fix txq for 1d/2d arrays.

[Dave Airlie]

On Tue, Dec 11, 2012 at 5:52 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
>
>
> ----- Original Message -----
>> Am 08.12.2012 07:02, schrieb Dave Airlie:
>> > From: Dave Airlie <airlied at redhat.com>
>> >
>> > Noticed would fail, we were doing two things wrong
>> >
>> > a) 1d arrays require the layers in height
>> > b) minifying the layers field.
>> >
>> > Signed-off-by: Dave Airlie <airlied at redhat.com>
>> > ---
>> >  src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c | 22
>> >  +++++++++++++++++-----
>> >  src/gallium/drivers/llvmpipe/lp_setup.c           |  6 +++++-
>> >  2 files changed, 22 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
>> > b/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
>> > index ba265b2..c0389a8 100644
>> > --- a/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
>> > +++ b/src/gallium/auxiliary/gallivm/lp_bld_sample_soa.c
>> > @@ -1680,23 +1680,30 @@ lp_build_size_query_soa(struct
>> > gallivm_state *gallivm,
>> >  {
>> >     LLVMValueRef lod;
>> >     LLVMValueRef size;
>> > -   int dims, i;
>> > +   int dims, i, num_min;
>> >     struct lp_build_context bld_int_vec;
>> >
>> >     switch (static_state->target) {
>> >     case PIPE_TEXTURE_1D:
>> >     case PIPE_BUFFER:
>> > -      dims = 1;
>> > +      num_min = dims = 1;
>> > +      break;
>> > +   case PIPE_TEXTURE_1D_ARRAY:
>> > +      num_min = 1;
>> > +      dims = 2;
>> >        break;
>> >     case PIPE_TEXTURE_2D:
>> >     case PIPE_TEXTURE_CUBE:
>> >     case PIPE_TEXTURE_RECT:
>> > -      dims = 2;
>> > +      num_min = dims = 2;
>> >        break;
>> >     case PIPE_TEXTURE_3D:
>> > +      num_min = dims = 3;
>> > +      break;
>> > +   case PIPE_TEXTURE_2D_ARRAY:
>> > +      num_min = 2;
>> >        dims = 3;
>> >        break;
>> > -
>> >     default:
>> >        assert(0);
>> >        return;
>> > @@ -1723,19 +1730,24 @@ lp_build_size_query_soa(struct
>> > gallivm_state *gallivm,
>> >                                   dynamic_state->width(dynamic_state,
>> >                                   gallivm, unit),
>> >                                   lp_build_const_int32(gallivm, 0),
>> >                                   "");
>> >
>> > +   if (num_min == 1)
>> > +      size = lp_build_minify(&bld_int_vec, size, lod);
>> >     if (dims >= 2) {
>> >        size = LLVMBuildInsertElement(gallivm->builder, size,
>> >                                      dynamic_state->height(dynamic_state,
>> >                                      gallivm, unit),
>> >                                      lp_build_const_int32(gallivm,
>> >                                      1), "");
>> >     }
>> >
>> > +   if (num_min == 2)
>> > +      size = lp_build_minify(&bld_int_vec, size, lod);
>> >     if (dims >= 3) {
>> >        size = LLVMBuildInsertElement(gallivm->builder, size,
>> >                                      dynamic_state->depth(dynamic_state,
>> >                                      gallivm, unit),
>> >                                      lp_build_const_int32(gallivm,
>> >                                      2), "");
>> >     }
>> >
>> > -   size = lp_build_minify(&bld_int_vec, size, lod);
>> > +   if (num_min == 3)
>> > +      size = lp_build_minify(&bld_int_vec, size, lod);
>> >
>> >     for (i=0; i < dims; i++) {
>> >        sizes_out[i] = lp_build_extract_broadcast(gallivm,
>> >        bld_int_vec.type, int_type,
>> > diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c
>> > b/src/gallium/drivers/llvmpipe/lp_setup.c
>> > index 7d40d8c..3dfe335 100644
>> > --- a/src/gallium/drivers/llvmpipe/lp_setup.c
>> > +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
>> > @@ -678,7 +678,11 @@ lp_setup_set_fragment_sampler_views(struct
>> > lp_setup_context *setup,
>> >           struct lp_jit_texture *jit_tex;
>> >           jit_tex = &setup->fs.current.jit_context.textures[i];
>> >           jit_tex->width = tex->width0;
>> > -         jit_tex->height = tex->height0;
>> > +         if (tex->target == PIPE_TEXTURE_1D_ARRAY) {
>> > +            jit_tex->height = tex->array_size;
>> > +         } else {
>> > +            jit_tex->height = tex->height0;
>> > +         }
>> >           jit_tex->first_level = view->u.tex.first_level;
>> >           jit_tex->last_level = tex->last_level;
>> >
>> >
>> I'm not convinced this is the right thing to do. Nothing requires us
>> to
>> put the array_size into jit_tex->height, I'd prefer it if we'd always
>> put it into depth (which is what the rest of the sampling code
>> assumes
>> where it is). There's actually a very good reason for not putting
>> array_size into either width or depth depending on 1d or 2d array,
>> since
>> eventually we'll need some min/max layer instead anyway, having
>> array_size always put into the same variable makes things simpler.
>> Also, I'm not entirely happy with the different semantic meaning of
>> "dims" here. While this was always used somewhat confusingly and
>> didn't
>> really mean "dims" it was at least used consistently in the sampling
>> code to mean "the number of "ordinary" coords subject to
>> minification,
>> wrapping etc" (and I've just recently added a comment somewhere to
>> indicate this finally).
>> But otherwise this looks good, certainly that query code couldn't
>> handle
>> texture arrays (I was unable to get the piglit test to run when I
>> tried
>> even with version overrides).
>
> Yes, you make valid points. But given that this seems to improve things, maybe we could address this in a follow on change?

When I looked at it again, I realised I could drop the height usage
myself, I'll see if I can clean it up and push it today.

thanks
Dave.