On Mon, Aug 24, 2015 at 4:55 PM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Monday, August 24, 2015 04:51:48 PM Kenneth Graunke wrote:
>> On Monday, August 24, 2015 04:14:13 PM Jason Ekstrand wrote:
>> > On Mon, Aug 24, 2015 at 4:03 PM, Kenneth Graunke <kenn...@whitecape.org>
>> > wrote:
>> > > On Wednesday, August 19, 2015 10:45:44 PM Jason Ekstrand wrote:
>> > >> v2 (Jason Ekstrand): Fix up image uniforms
>> > >> ---
>> > >> src/glsl/nir/nir_lower_io.c | 9 +++++++--
>> > >> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 13 +------------
>> > >> 2 files changed, 8 insertions(+), 14 deletions(-)
>> > >>
>> > >> diff --git a/src/glsl/nir/nir_lower_io.c b/src/glsl/nir/nir_lower_io.c
>> > >> index 15a4edc..70645b6 100644
>> > >> --- a/src/glsl/nir/nir_lower_io.c
>> > >> +++ b/src/glsl/nir/nir_lower_io.c
>> > >> @@ -244,9 +244,14 @@ nir_lower_io_block(nir_block *block, void
>> > >> *void_state)
>> > >> nir_src indirect;
>> > >> unsigned offset = get_io_offset(intrin->variables[0],
>> > >> &intrin->instr, &indirect,
>> > >> state);
>> > >> - offset += intrin->variables[0]->var->data.driver_location;
>> > >>
>> > >> - load->const_index[0] = offset;
>> > >> + unsigned location =
>> > >> intrin->variables[0]->var->data.driver_location;
>> > >> + if (mode == nir_var_uniform) {
>> > >> + load->const_index[0] = location;
>> > >> + load->const_index[1] = offset;
>> > >> + } else {
>> > >> + load->const_index[0] = location + offset;
>> > >> + }
>> > >>
>> > >> if (has_indirect)
>> > >> load->src[0] = indirect;
>> > >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> > >> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> > >> index 5b54b95..32a05dc 100644
>> > >> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> > >> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> > >> @@ -239,18 +239,7 @@ fs_visitor::nir_setup_uniform(nir_variable *var)
>> > >> }
>> > >>
>> > >> if (storage->type->is_image()) {
>> > >> - /* Images don't get a valid location assigned by
>> > >> nir_lower_io()
>> > >> - * because their size is driver-specific, so we need to
>> > >> allocate
>> > >> - * space for them here at the end of the parameter array.
>> > >> - */
>> > >> - var->data.driver_location = uniforms;
>> > >> - unsigned size =
>> > >> - BRW_IMAGE_PARAM_SIZE * MAX2(storage->array_elements, 1);
>> > >> -
>> > >> - setup_image_uniform_values(uniforms, storage);
>> > >> -
>> > >> - param_size[uniforms] = size;
>> > >> - uniforms += size;
>> > >> + setup_image_uniform_values(index, storage);
>> > >> } else {
>> > >> unsigned slots = storage->type->component_slots();
>> > >> if (storage->array_elements)
>> > >>
>> > >
>> > > This hunk really looks like it was squashed into the wrong patch...
>> >
>> > You're right. This hunk should go into the patch that starts passing
>> > type_size into nir_lower_io.
>>
>> Ahh, that makes so much more sense. Squashing this hunk into that
>> patch (07) sounds great. Perhaps add something like this to the commit
>> message of that patch:
>>
>> "In fact, this causes nir_lower_io to allocate space for our image
>> uniforms, allowing us to drop hacks in the backend."
>>
>> Patches 1-13 (other than the ones I wrote) are:
>> Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
>>
>> This definitely cleans up a lot of mess. Great work!
>
> Erp. Except for patch 06. I don't like that one, and proposed an
> alternate solution.
Hunh? I'm not seeing any comments on patch 6 (the one dropping
asserts). Also, could you look at patch 17 as well; it's also a
cleanup, it just got in with the actual new changes.
--Jason
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
