On Tue, Dec 11, 2012 at 5:52 AM, Jose Fonseca <jfons...@vmware.com> wrote:
>
>
> ----- Original Message -----
>> Am 08.12.2012 07:02, schrieb Dave Airlie:
>> > From: Dave Airlie <airl...@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 <airl...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
