Re: [Freedreno] [Mesa-dev] [PATCH 1/2] nir: allow texture offsets with cube maps

[Jason Ekstrand]

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

[Ilia Mirkin]

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

[Jason Ekstrand]

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

[Ilia Mirkin]

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

[Jason Ekstrand]

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