Re: [Freedreno] [Mesa-dev] [PATCH 1/2] nir: allow texture offsets with cube maps
On Mon, Nov 20, 2017 at 4:10 PM, Ilia Mirkin wrote: > On Mon, Nov 20, 2017 at 7:08 PM, Jason Ekstrand > wrote: > > On Mon, Nov 20, 2017 at 3:11 PM, Ilia Mirkin > wrote: > >> > >> On Mon, Nov 20, 2017 at 5:16 PM, Jason Ekstrand > >> wrote: > >> > On Sun, Nov 19, 2017 at 11:54 AM, Ilia Mirkin > >> > wrote: > >> >> > >> >> GL doesn't have this, but some hardware supports it. This is > convenient > >> >> for lowering tg4 to plain texture calls, which is necessary on Adreno > >> >> A4xx hardware. > >> >> > >> >> Signed-off-by: Ilia Mirkin > >> >> --- > >> >> src/compiler/nir/nir.h | 15 +-- > >> >> 1 file changed, 13 insertions(+), 2 deletions(-) > >> >> > >> >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > >> >> index f46f6147110..64965ae16d6 100644 > >> >> --- a/src/compiler/nir/nir.h > >> >> +++ b/src/compiler/nir/nir.h > >> >> @@ -1364,8 +1364,7 @@ nir_tex_instr_src_size(const nir_tex_instr > >> >> *instr, > >> >> unsigned src) > >> >> if (instr->src[src].src_type == nir_tex_src_ms_mcs) > >> >>return 4; > >> >> > >> >> - if (instr->src[src].src_type == nir_tex_src_offset || > >> >> - instr->src[src].src_type == nir_tex_src_ddx || > >> >> + if (instr->src[src].src_type == nir_tex_src_ddx || > >> >> instr->src[src].src_type == nir_tex_src_ddy) { > >> >>if (instr->is_array) > >> >> return instr->coord_components - 1; > >> >> @@ -1373,6 +1372,18 @@ nir_tex_instr_src_size(const nir_tex_instr > >> >> *instr, > >> >> unsigned src) > >> >> return instr->coord_components; > >> >> } > >> >> > >> >> + /* Usual APIs don't allow cube + offset, but we allow it, with 2 > >> >> coords for > >> >> +* the offset, since a cube maps to a single face. > >> >> +*/ > >> >> + if (instr->src[src].src_type == nir_tex_src_offset) { > >> >> + unsigned ret = instr->coord_components; > >> >> + if (instr->is_array) > >> >> + ret--; > >> >> + if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) > >> >> + ret--; > >> >> + return ret; > >> > > >> > > >> > I think I'd rather this look more like the one above: > >> > > >> > if (instr->is_array) > >> >return instr->coord_components; > >> > else if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) > >> >return 2; > >> > else > >> >return instr->coord_components - 1; > >> > > >> > It seems a bit cleaner and/or more explicit to me. > >> > >> OK. Although your version is slightly wrong, but I get the idea. Will > >> do that in a v2. (array should get -1, and cube should always get 2 > >> even if it's an array) > > > > > > I'd forgotten about cube arrays, yes, those would naturally be -2. In > that > > case, maybe -- for each subtraction is a good idea... > > Well, rearranging it, we get > > if (instr->sampler_dim == CUBE) > return 2; > else if (instr->is_array) > return comp - 1; > else > return comp; > Right. With that if-ladder, Reviewed-by: Jason Ekstrand ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Mesa-dev] [PATCH 1/2] nir: allow texture offsets with cube maps
On Mon, Nov 20, 2017 at 7:08 PM, Jason Ekstrand wrote: > On Mon, Nov 20, 2017 at 3:11 PM, Ilia Mirkin wrote: >> >> On Mon, Nov 20, 2017 at 5:16 PM, Jason Ekstrand >> wrote: >> > On Sun, Nov 19, 2017 at 11:54 AM, Ilia Mirkin >> > wrote: >> >> >> >> GL doesn't have this, but some hardware supports it. This is convenient >> >> for lowering tg4 to plain texture calls, which is necessary on Adreno >> >> A4xx hardware. >> >> >> >> Signed-off-by: Ilia Mirkin >> >> --- >> >> src/compiler/nir/nir.h | 15 +-- >> >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h >> >> index f46f6147110..64965ae16d6 100644 >> >> --- a/src/compiler/nir/nir.h >> >> +++ b/src/compiler/nir/nir.h >> >> @@ -1364,8 +1364,7 @@ nir_tex_instr_src_size(const nir_tex_instr >> >> *instr, >> >> unsigned src) >> >> if (instr->src[src].src_type == nir_tex_src_ms_mcs) >> >>return 4; >> >> >> >> - if (instr->src[src].src_type == nir_tex_src_offset || >> >> - instr->src[src].src_type == nir_tex_src_ddx || >> >> + if (instr->src[src].src_type == nir_tex_src_ddx || >> >> instr->src[src].src_type == nir_tex_src_ddy) { >> >>if (instr->is_array) >> >> return instr->coord_components - 1; >> >> @@ -1373,6 +1372,18 @@ nir_tex_instr_src_size(const nir_tex_instr >> >> *instr, >> >> unsigned src) >> >> return instr->coord_components; >> >> } >> >> >> >> + /* Usual APIs don't allow cube + offset, but we allow it, with 2 >> >> coords for >> >> +* the offset, since a cube maps to a single face. >> >> +*/ >> >> + if (instr->src[src].src_type == nir_tex_src_offset) { >> >> + unsigned ret = instr->coord_components; >> >> + if (instr->is_array) >> >> + ret--; >> >> + if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) >> >> + ret--; >> >> + return ret; >> > >> > >> > I think I'd rather this look more like the one above: >> > >> > if (instr->is_array) >> >return instr->coord_components; >> > else if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) >> >return 2; >> > else >> >return instr->coord_components - 1; >> > >> > It seems a bit cleaner and/or more explicit to me. >> >> OK. Although your version is slightly wrong, but I get the idea. Will >> do that in a v2. (array should get -1, and cube should always get 2 >> even if it's an array) > > > I'd forgotten about cube arrays, yes, those would naturally be -2. In that > case, maybe -- for each subtraction is a good idea... Well, rearranging it, we get if (instr->sampler_dim == CUBE) return 2; else if (instr->is_array) return comp - 1; else return comp; Happy to leave it alone too though. ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Mesa-dev] [PATCH 1/2] nir: allow texture offsets with cube maps
On Mon, Nov 20, 2017 at 3:11 PM, Ilia Mirkin wrote: > On Mon, Nov 20, 2017 at 5:16 PM, Jason Ekstrand > wrote: > > On Sun, Nov 19, 2017 at 11:54 AM, Ilia Mirkin > wrote: > >> > >> GL doesn't have this, but some hardware supports it. This is convenient > >> for lowering tg4 to plain texture calls, which is necessary on Adreno > >> A4xx hardware. > >> > >> Signed-off-by: Ilia Mirkin > >> --- > >> src/compiler/nir/nir.h | 15 +-- > >> 1 file changed, 13 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > >> index f46f6147110..64965ae16d6 100644 > >> --- a/src/compiler/nir/nir.h > >> +++ b/src/compiler/nir/nir.h > >> @@ -1364,8 +1364,7 @@ nir_tex_instr_src_size(const nir_tex_instr *instr, > >> unsigned src) > >> if (instr->src[src].src_type == nir_tex_src_ms_mcs) > >>return 4; > >> > >> - if (instr->src[src].src_type == nir_tex_src_offset || > >> - instr->src[src].src_type == nir_tex_src_ddx || > >> + if (instr->src[src].src_type == nir_tex_src_ddx || > >> instr->src[src].src_type == nir_tex_src_ddy) { > >>if (instr->is_array) > >> return instr->coord_components - 1; > >> @@ -1373,6 +1372,18 @@ nir_tex_instr_src_size(const nir_tex_instr > *instr, > >> unsigned src) > >> return instr->coord_components; > >> } > >> > >> + /* Usual APIs don't allow cube + offset, but we allow it, with 2 > >> coords for > >> +* the offset, since a cube maps to a single face. > >> +*/ > >> + if (instr->src[src].src_type == nir_tex_src_offset) { > >> + unsigned ret = instr->coord_components; > >> + if (instr->is_array) > >> + ret--; > >> + if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) > >> + ret--; > >> + return ret; > > > > > > I think I'd rather this look more like the one above: > > > > if (instr->is_array) > >return instr->coord_components; > > else if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) > >return 2; > > else > >return instr->coord_components - 1; > > > > It seems a bit cleaner and/or more explicit to me. > > OK. Although your version is slightly wrong, but I get the idea. Will > do that in a v2. (array should get -1, and cube should always get 2 > even if it's an array) > I'd forgotten about cube arrays, yes, those would naturally be -2. In that case, maybe -- for each subtraction is a good idea... > -ilia > > > > > Also, bonus points to anyone who converts this function to use a switch. > :-P > > > > --Jason > > > >> > >> + } > >> + > >> return 1; > >> } > >> > >> -- > >> 2.13.6 > >> > >> ___ > >> mesa-dev mailing list > >> mesa-...@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > > > > ___ > > Freedreno mailing list > > Freedreno@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/freedreno > > > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Mesa-dev] [PATCH 1/2] nir: allow texture offsets with cube maps
On Mon, Nov 20, 2017 at 5:16 PM, Jason Ekstrand wrote: > On Sun, Nov 19, 2017 at 11:54 AM, Ilia Mirkin wrote: >> >> GL doesn't have this, but some hardware supports it. This is convenient >> for lowering tg4 to plain texture calls, which is necessary on Adreno >> A4xx hardware. >> >> Signed-off-by: Ilia Mirkin >> --- >> src/compiler/nir/nir.h | 15 +-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h >> index f46f6147110..64965ae16d6 100644 >> --- a/src/compiler/nir/nir.h >> +++ b/src/compiler/nir/nir.h >> @@ -1364,8 +1364,7 @@ nir_tex_instr_src_size(const nir_tex_instr *instr, >> unsigned src) >> if (instr->src[src].src_type == nir_tex_src_ms_mcs) >>return 4; >> >> - if (instr->src[src].src_type == nir_tex_src_offset || >> - instr->src[src].src_type == nir_tex_src_ddx || >> + if (instr->src[src].src_type == nir_tex_src_ddx || >> instr->src[src].src_type == nir_tex_src_ddy) { >>if (instr->is_array) >> return instr->coord_components - 1; >> @@ -1373,6 +1372,18 @@ nir_tex_instr_src_size(const nir_tex_instr *instr, >> unsigned src) >> return instr->coord_components; >> } >> >> + /* Usual APIs don't allow cube + offset, but we allow it, with 2 >> coords for >> +* the offset, since a cube maps to a single face. >> +*/ >> + if (instr->src[src].src_type == nir_tex_src_offset) { >> + unsigned ret = instr->coord_components; >> + if (instr->is_array) >> + ret--; >> + if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) >> + ret--; >> + return ret; > > > I think I'd rather this look more like the one above: > > if (instr->is_array) >return instr->coord_components; > else if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) >return 2; > else >return instr->coord_components - 1; > > It seems a bit cleaner and/or more explicit to me. OK. Although your version is slightly wrong, but I get the idea. Will do that in a v2. (array should get -1, and cube should always get 2 even if it's an array) -ilia > > Also, bonus points to anyone who converts this function to use a switch. :-P > > --Jason > >> >> + } >> + >> return 1; >> } >> >> -- >> 2.13.6 >> >> ___ >> mesa-dev mailing list >> mesa-...@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > ___ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [Mesa-dev] [PATCH 1/2] nir: allow texture offsets with cube maps
On Sun, Nov 19, 2017 at 11:54 AM, Ilia Mirkin wrote: > GL doesn't have this, but some hardware supports it. This is convenient > for lowering tg4 to plain texture calls, which is necessary on Adreno > A4xx hardware. > > Signed-off-by: Ilia Mirkin > --- > src/compiler/nir/nir.h | 15 +-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > index f46f6147110..64965ae16d6 100644 > --- a/src/compiler/nir/nir.h > +++ b/src/compiler/nir/nir.h > @@ -1364,8 +1364,7 @@ nir_tex_instr_src_size(const nir_tex_instr *instr, > unsigned src) > if (instr->src[src].src_type == nir_tex_src_ms_mcs) >return 4; > > - if (instr->src[src].src_type == nir_tex_src_offset || > - instr->src[src].src_type == nir_tex_src_ddx || > + if (instr->src[src].src_type == nir_tex_src_ddx || > instr->src[src].src_type == nir_tex_src_ddy) { >if (instr->is_array) > return instr->coord_components - 1; > @@ -1373,6 +1372,18 @@ nir_tex_instr_src_size(const nir_tex_instr *instr, > unsigned src) > return instr->coord_components; > } > > + /* Usual APIs don't allow cube + offset, but we allow it, with 2 > coords for > +* the offset, since a cube maps to a single face. > +*/ > + if (instr->src[src].src_type == nir_tex_src_offset) { > + unsigned ret = instr->coord_components; > + if (instr->is_array) > + ret--; > + if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) > + ret--; > + return ret; > I think I'd rather this look more like the one above: if (instr->is_array) return instr->coord_components; else if (instr->sampler_dim == GLSL_SAMPLER_DIM_CUBE) return 2; else return instr->coord_components - 1; It seems a bit cleaner and/or more explicit to me. Also, bonus points to anyone who converts this function to use a switch. :-P --Jason > + } > + > return 1; > } > > -- > 2.13.6 > > ___ > mesa-dev mailing list > mesa-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno