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

[Jose Fonseca]

----- 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?

Jose