Re: [Mesa-dev] [PATCH] anv/state: if enabled, use anisotropic filtering also with VK_FILTER_NEAREST

2016-11-25 Thread Jason Ekstrand 
On Nov 24, 2016 11:19 PM, "Iago Toral"  wrote:
>
> On Thu, 2016-11-24 at 16:28 -0800, Jason Ekstrand wrote:
> >
> > On Nov 24, 2016 7:12 AM, "Iago Toral"  wrote:
> > >
> > > On Thu, 2016-11-24 at 14:33 +0100, Iago Toral wrote:
> > > > Hi Lionel,
> > > >
> > > > On Thu, 2016-11-24 at 13:08 +, Lionel Landwerlin wrote:
> > > > >
> > > > > Hi Iago,
> > > > >
> > > > > Looking at the history, before
> > > > > ed4fe3e9ba9018e68afe6fdd4f267218a537fdaa
> > > > > we seem to set min/mag filter to MAPFILTER_ANISOTROPIC if
> > > > > maxAnisotropy
> > > > >  > 1. It seems your patch makes sense in using
> > > > > MAPFILTER_ANISOTROPIC
> > > > > in
> > > > > the NEAREST case, but I wonder whether we should also check
> > for
> > > > > maxAnisotropy > 1.
> > > > Right, good catch, although I think that if we do that it should
> > be a
> > > > separate change since we are not currently checking that for the
> > > > linear
> > > > filter either.
> > In GL, there is no explicit enable.  It's just assumed that it's
> > "always on" and the anisotropy value being 1.0 or > 1.0 enables and
> > disables it.  Vulkan has an explicit bit so we should use that.
> > > > It seems that we do check for this in OpenGL so I think we
> > probably
> > > > should do that here as well unless Jason dropped it for Vulkan on
> > > > purpose for some reason in that commit.
> > > >
> > > > I'll send a separate patch for this after I confirm that it does
> > not
> > > > alter the results for the tests in CTS if we add that check.
> > >
> > > Actually, thinking about this a bit more, I don't think we need
> >  That
> > > commit is about honoring SamplerCreateInfo.anisotropyEnable to
> > decide
> > > whether to activate anisotropic filtering, so if that is true we
> > want
> > > to use that. Notice that since that commit we clamp the anisotropy
> > > ratio to ensure it is in a valid range. If we pass a maxAnisotropy
> > > value < 1, it will clamp it to the minimum value we can work with,
> > so I
> > > think Jason did that change on purpose.
> > You overestimate the amount of thought I put into anisotropic
> > filtering.  :-)
> > I think this patch is probably correct.  At the very least, I don't
> > think it's wrong.  I'm not sure that it makes sense to use
> > anisotropic filtering with NEAREST.  I'm not even quite sure what
> > that would mean.  The spec certainly doesn't say.  Anyway, I think
> > I'm fine with this but a bit more digging to make sure we're actually
> > doing it right might be in order.
>
> If you're using anisotropic filtering to enhance texture quality it
> would seem odd that someone would use it with a nearest filter, but I
> have not seen any indication that this combination should not be
> allowed. After all, anisotropic filtering is conceptually orthogonal to
> nearest/linear filtering.
>
> I did not find any references in the Vulkan docs, but at least in
> OpenGL this is explained in EXT_texture_filter_anisotropic  and they
> actually mention the combination of nearest+anisotropic filtering
> explicitly:
>
> https://www.opengl.org/registry/specs/EXT/texture_filter_anisotropic.tx
> t
>
>"A texture's maximum degree of anisotropy is specified independent
> from the texture's minification and magnification filter (as
> opposed to being supported as an entirely new filtering mode).
> Implementations are free to use the specified minification and
> magnification filter to select a particular anisotropic texture
> filtering scheme.  For example, a NEAREST filter with a maximum
> degree of anisotropy of two could be treated as a 2-tap filter that
> accounts for the direction of anisotropy.  Implementations are also
> permitted to ignore the minification or magnification filter and
> implement the highest quality of anisotropic filtering possible."

I think the key there is "free to use".  In other words, we are also free
to ignore the minification and magnification filters when anisotropic
filtering is enabled which is what this patch does.  With anisotropic
filtering enabled, they just become implementation-specific knobs to adjust
the algorithm.  With that understanding, this patch is 100% correct.

> So basically, some drivers/hardware may have nearest + anisotropy work
> slightly different than linear + anisotropy. Not sure when that is
> helpful, but it seems that it should be possible to do it.
>
> Iago
>
> > Reviewed-by: Jason Ekstrand 
> > > Sounds reasonable?
> > >
> > > > Iago
> > > >
> > > > >
> > > > > On 24/11/16 11:30, Iago Toral Quiroga wrote:
> > > > > >
> > > > > >
> > > > > > Fixes multiple Vulkan CTS tests that combine anisotropy and
> > > > > > VK_FILTER_NEAREST
> > > > > > in dEQP-VK.texture.filtering_anisotropy.*
> > > > > > ---
> > > > > >   src/intel/vulkan/genX_state.c | 2 +-
> > > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/src/intel/vulkan/genX_state.c
> >

Re: [Mesa-dev] [PATCH] anv/state: if enabled, use anisotropic filtering also with VK_FILTER_NEAREST

2016-11-24 Thread Iago Toral 
On Thu, 2016-11-24 at 16:28 -0800, Jason Ekstrand wrote:
> 
> On Nov 24, 2016 7:12 AM, "Iago Toral"  wrote:
> >
> > On Thu, 2016-11-24 at 14:33 +0100, Iago Toral wrote:
> > > Hi Lionel,
> > >
> > > On Thu, 2016-11-24 at 13:08 +, Lionel Landwerlin wrote:
> > > >
> > > > Hi Iago,
> > > >
> > > > Looking at the history, before
> > > > ed4fe3e9ba9018e68afe6fdd4f267218a537fdaa 
> > > > we seem to set min/mag filter to MAPFILTER_ANISOTROPIC if
> > > > maxAnisotropy 
> > > >  > 1. It seems your patch makes sense in using
> > > > MAPFILTER_ANISOTROPIC
> > > > in 
> > > > the NEAREST case, but I wonder whether we should also check
> for 
> > > > maxAnisotropy > 1.
> > > Right, good catch, although I think that if we do that it should
> be a
> > > separate change since we are not currently checking that for the
> > > linear
> > > filter either.
> In GL, there is no explicit enable.  It's just assumed that it's
> "always on" and the anisotropy value being 1.0 or > 1.0 enables and
> disables it.  Vulkan has an explicit bit so we should use that.
> > > It seems that we do check for this in OpenGL so I think we
> probably
> > > should do that here as well unless Jason dropped it for Vulkan on
> > > purpose for some reason in that commit.
> > >
> > > I'll send a separate patch for this after I confirm that it does
> not
> > > alter the results for the tests in CTS if we add that check.
> >
> > Actually, thinking about this a bit more, I don't think we need
>  That
> > commit is about honoring SamplerCreateInfo.anisotropyEnable to
> decide
> > whether to activate anisotropic filtering, so if that is true we
> want
> > to use that. Notice that since that commit we clamp the anisotropy
> > ratio to ensure it is in a valid range. If we pass a maxAnisotropy
> > value < 1, it will clamp it to the minimum value we can work with,
> so I
> > think Jason did that change on purpose.
> You overestimate the amount of thought I put into anisotropic
> filtering.  :-)
> I think this patch is probably correct.  At the very least, I don't
> think it's wrong.  I'm not sure that it makes sense to use
> anisotropic filtering with NEAREST.  I'm not even quite sure what
> that would mean.  The spec certainly doesn't say.  Anyway, I think
> I'm fine with this but a bit more digging to make sure we're actually
> doing it right might be in order.

If you're using anisotropic filtering to enhance texture quality it
would seem odd that someone would use it with a nearest filter, but I
have not seen any indication that this combination should not be
allowed. After all, anisotropic filtering is conceptually orthogonal to
nearest/linear filtering.

I did not find any references in the Vulkan docs, but at least in
OpenGL this is explained in EXT_texture_filter_anisotropic  and they
actually mention the combination of nearest+anisotropic filtering
explicitly: 

https://www.opengl.org/registry/specs/EXT/texture_filter_anisotropic.tx
t

   "A texture's maximum degree of anisotropy is specified independent
from the texture's minification and magnification filter (as
opposed to being supported as an entirely new filtering mode).
Implementations are free to use the specified minification and
magnification filter to select a particular anisotropic texture
filtering scheme.  For example, a NEAREST filter with a maximum
degree of anisotropy of two could be treated as a 2-tap filter that
accounts for the direction of anisotropy.  Implementations are also
permitted to ignore the minification or magnification filter and
    implement the highest quality of anisotropic filtering possible."

So basically, some drivers/hardware may have nearest + anisotropy work
slightly different than linear + anisotropy. Not sure when that is
helpful, but it seems that it should be possible to do it.

Iago

> Reviewed-by: Jason Ekstrand 
> > Sounds reasonable?
> >
> > > Iago
> > >
> > > >
> > > > On 24/11/16 11:30, Iago Toral Quiroga wrote:
> > > > >
> > > > >
> > > > > Fixes multiple Vulkan CTS tests that combine anisotropy and
> > > > > VK_FILTER_NEAREST
> > > > > in dEQP-VK.texture.filtering_anisotropy.*
> > > > > ---
> > > > >   src/intel/vulkan/genX_state.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/intel/vulkan/genX_state.c
> > > > > b/src/intel/vulkan/genX_state.c
> > > > > index 4122395..0f621f9 100644
> > > > > --- a/src/intel/vulkan/genX_state.c
> > > > > +++ b/src/intel/vulkan/genX_state.c
> > > > > @@ -101,7 +101,7 @@ vk_to_gen_tex_filter(VkFilter filter,
> bool
> > > > > anisotropyEnable)
> > > > >  default:
> > > > > assert(!"Invalid filter");
> > > > >  case VK_FILTER_NEAREST:
> > > > > -  return MAPFILTER_NEAREST;
> > > > > +  return anisotropyEnable ? MAPFILTER_ANISOTROPIC :
> > > > > MAPFILTER_NEAREST;
> > > > >  case VK_FILTER_LINEAR:
> > > > > return anisotropyEnable ? MAPFILTER_ANISOTROPIC :

Re: [Mesa-dev] [PATCH] anv/state: if enabled, use anisotropic filtering also with VK_FILTER_NEAREST

2016-11-24 Thread Jason Ekstrand 
On Nov 24, 2016 7:12 AM, "Iago Toral"  wrote:
>
> On Thu, 2016-11-24 at 14:33 +0100, Iago Toral wrote:
> > Hi Lionel,
> >
> > On Thu, 2016-11-24 at 13:08 +, Lionel Landwerlin wrote:
> > >
> > > Hi Iago,
> > >
> > > Looking at the history, before
> > > ed4fe3e9ba9018e68afe6fdd4f267218a537fdaa
> > > we seem to set min/mag filter to MAPFILTER_ANISOTROPIC if
> > > maxAnisotropy
> > >  > 1. It seems your patch makes sense in using
> > > MAPFILTER_ANISOTROPIC
> > > in
> > > the NEAREST case, but I wonder whether we should also check for
> > > maxAnisotropy > 1.
> > Right, good catch, although I think that if we do that it should be a
> > separate change since we are not currently checking that for the
> > linear
> > filter either.

In GL, there is no explicit enable.  It's just assumed that it's "always
on" and the anisotropy value being 1.0 or > 1.0 enables and disables it.
Vulkan has an explicit bit so we should use that.

> > It seems that we do check for this in OpenGL so I think we probably
> > should do that here as well unless Jason dropped it for Vulkan on
> > purpose for some reason in that commit.
> >
> > I'll send a separate patch for this after I confirm that it does not
> > alter the results for the tests in CTS if we add that check.
>
> Actually, thinking about this a bit more, I don't think we need  That
> commit is about honoring SamplerCreateInfo.anisotropyEnable to decide
> whether to activate anisotropic filtering, so if that is true we want
> to use that. Notice that since that commit we clamp the anisotropy
> ratio to ensure it is in a valid range. If we pass a maxAnisotropy
> value < 1, it will clamp it to the minimum value we can work with, so I
> think Jason did that change on purpose.

You overestimate the amount of thought I put into anisotropic filtering.
:-)

I think this patch is probably correct.  At the very least, I don't think
it's wrong.  I'm not sure that it makes sense to use anisotropic filtering
with NEAREST.  I'm not even quite sure what that would mean.  The spec
certainly doesn't say.  Anyway, I think I'm fine with this but a bit more
digging to make sure we're actually doing it right might be in order.

Reviewed-by: Jason Ekstrand 

> Sounds reasonable?
>
> > Iago
> >
> > >
> > > On 24/11/16 11:30, Iago Toral Quiroga wrote:
> > > >
> > > >
> > > > Fixes multiple Vulkan CTS tests that combine anisotropy and
> > > > VK_FILTER_NEAREST
> > > > in dEQP-VK.texture.filtering_anisotropy.*
> > > > ---
> > > >   src/intel/vulkan/genX_state.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/intel/vulkan/genX_state.c
> > > > b/src/intel/vulkan/genX_state.c
> > > > index 4122395..0f621f9 100644
> > > > --- a/src/intel/vulkan/genX_state.c
> > > > +++ b/src/intel/vulkan/genX_state.c
> > > > @@ -101,7 +101,7 @@ vk_to_gen_tex_filter(VkFilter filter, bool
> > > > anisotropyEnable)
> > > >  default:
> > > > assert(!"Invalid filter");
> > > >  case VK_FILTER_NEAREST:
> > > > -  return MAPFILTER_NEAREST;
> > > > +  return anisotropyEnable ? MAPFILTER_ANISOTROPIC :
> > > > MAPFILTER_NEAREST;
> > > >  case VK_FILTER_LINEAR:
> > > > return anisotropyEnable ? MAPFILTER_ANISOTROPIC :
> > > > MAPFILTER_LINEAR;
> > > >  }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv/state: if enabled, use anisotropic filtering also with VK_FILTER_NEAREST

2016-11-24 Thread Lionel Landwerlin 
Thanks for the explanation, feel free to wait for Jason's rb otherwise
:

Reviewed-by: Lionel Landwerlin 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv/state: if enabled, use anisotropic filtering also with VK_FILTER_NEAREST

2016-11-24 Thread Iago Toral 
On Thu, 2016-11-24 at 14:33 +0100, Iago Toral wrote:
> Hi Lionel,
> 
> On Thu, 2016-11-24 at 13:08 +, Lionel Landwerlin wrote:
> > 
> > Hi Iago,
> > 
> > Looking at the history, before
> > ed4fe3e9ba9018e68afe6fdd4f267218a537fdaa 
> > we seem to set min/mag filter to MAPFILTER_ANISOTROPIC if
> > maxAnisotropy 
> >  > 1. It seems your patch makes sense in using
> > MAPFILTER_ANISOTROPIC
> > in 
> > the NEAREST case, but I wonder whether we should also check for 
> > maxAnisotropy > 1.
> Right, good catch, although I think that if we do that it should be a
> separate change since we are not currently checking that for the
> linear
> filter either.
> 
> It seems that we do check for this in OpenGL so I think we probably
> should do that here as well unless Jason dropped it for Vulkan on
> purpose for some reason in that commit.
> 
> I'll send a separate patch for this after I confirm that it does not
> alter the results for the tests in CTS if we add that check.

Actually, thinking about this a bit more, I don't think we need  That
commit is about honoring SamplerCreateInfo.anisotropyEnable to decide
whether to activate anisotropic filtering, so if that is true we want
to use that. Notice that since that commit we clamp the anisotropy
ratio to ensure it is in a valid range. If we pass a maxAnisotropy
value < 1, it will clamp it to the minimum value we can work with, so I
think Jason did that change on purpose.

Sounds reasonable?

> Iago
> 
> > 
> > On 24/11/16 11:30, Iago Toral Quiroga wrote:
> > > 
> > > 
> > > Fixes multiple Vulkan CTS tests that combine anisotropy and
> > > VK_FILTER_NEAREST
> > > in dEQP-VK.texture.filtering_anisotropy.*
> > > ---
> > >   src/intel/vulkan/genX_state.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/intel/vulkan/genX_state.c
> > > b/src/intel/vulkan/genX_state.c
> > > index 4122395..0f621f9 100644
> > > --- a/src/intel/vulkan/genX_state.c
> > > +++ b/src/intel/vulkan/genX_state.c
> > > @@ -101,7 +101,7 @@ vk_to_gen_tex_filter(VkFilter filter, bool
> > > anisotropyEnable)
> > >  default:
> > > assert(!"Invalid filter");
> > >  case VK_FILTER_NEAREST:
> > > -  return MAPFILTER_NEAREST;
> > > +  return anisotropyEnable ? MAPFILTER_ANISOTROPIC :
> > > MAPFILTER_NEAREST;
> > >  case VK_FILTER_LINEAR:
> > > return anisotropyEnable ? MAPFILTER_ANISOTROPIC :
> > > MAPFILTER_LINEAR;
> > >  }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv/state: if enabled, use anisotropic filtering also with VK_FILTER_NEAREST

2016-11-24 Thread Iago Toral 
Hi Lionel,

On Thu, 2016-11-24 at 13:08 +, Lionel Landwerlin wrote:
> Hi Iago,
> 
> Looking at the history, before
> ed4fe3e9ba9018e68afe6fdd4f267218a537fdaa 
> we seem to set min/mag filter to MAPFILTER_ANISOTROPIC if
> maxAnisotropy 
>  > 1. It seems your patch makes sense in using MAPFILTER_ANISOTROPIC
> in 
> the NEAREST case, but I wonder whether we should also check for 
> maxAnisotropy > 1.

Right, good catch, although I think that if we do that it should be a
separate change since we are not currently checking that for the linear
filter either.

It seems that we do check for this in OpenGL so I think we probably
should do that here as well unless Jason dropped it for Vulkan on
purpose for some reason in that commit.

I'll send a separate patch for this after I confirm that it does not
alter the results for the tests in CTS if we add that check.

Iago

> On 24/11/16 11:30, Iago Toral Quiroga wrote:
> > 
> > Fixes multiple Vulkan CTS tests that combine anisotropy and
> > VK_FILTER_NEAREST
> > in dEQP-VK.texture.filtering_anisotropy.*
> > ---
> >   src/intel/vulkan/genX_state.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/intel/vulkan/genX_state.c
> > b/src/intel/vulkan/genX_state.c
> > index 4122395..0f621f9 100644
> > --- a/src/intel/vulkan/genX_state.c
> > +++ b/src/intel/vulkan/genX_state.c
> > @@ -101,7 +101,7 @@ vk_to_gen_tex_filter(VkFilter filter, bool
> > anisotropyEnable)
> >  default:
> > assert(!"Invalid filter");
> >  case VK_FILTER_NEAREST:
> > -  return MAPFILTER_NEAREST;
> > +  return anisotropyEnable ? MAPFILTER_ANISOTROPIC :
> > MAPFILTER_NEAREST;
> >  case VK_FILTER_LINEAR:
> > return anisotropyEnable ? MAPFILTER_ANISOTROPIC :
> > MAPFILTER_LINEAR;
> >  }
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] anv/state: if enabled, use anisotropic filtering also with VK_FILTER_NEAREST

2016-11-24 Thread Lionel Landwerlin 

Hi Iago,

Looking at the history, before ed4fe3e9ba9018e68afe6fdd4f267218a537fdaa 
we seem to set min/mag filter to MAPFILTER_ANISOTROPIC if maxAnisotropy 
> 1. It seems your patch makes sense in using MAPFILTER_ANISOTROPIC in 
the NEAREST case, but I wonder whether we should also check for 
maxAnisotropy > 1.


On 24/11/16 11:30, Iago Toral Quiroga wrote:

Fixes multiple Vulkan CTS tests that combine anisotropy and VK_FILTER_NEAREST
in dEQP-VK.texture.filtering_anisotropy.*
---
  src/intel/vulkan/genX_state.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/intel/vulkan/genX_state.c b/src/intel/vulkan/genX_state.c
index 4122395..0f621f9 100644
--- a/src/intel/vulkan/genX_state.c
+++ b/src/intel/vulkan/genX_state.c
@@ -101,7 +101,7 @@ vk_to_gen_tex_filter(VkFilter filter, bool anisotropyEnable)
 default:
assert(!"Invalid filter");
 case VK_FILTER_NEAREST:
-  return MAPFILTER_NEAREST;
+  return anisotropyEnable ? MAPFILTER_ANISOTROPIC : MAPFILTER_NEAREST;
 case VK_FILTER_LINEAR:
return anisotropyEnable ? MAPFILTER_ANISOTROPIC : MAPFILTER_LINEAR;
 }



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] anv/state: if enabled, use anisotropic filtering also with VK_FILTER_NEAREST

2016-11-24 Thread Iago Toral Quiroga 
Fixes multiple Vulkan CTS tests that combine anisotropy and VK_FILTER_NEAREST
in dEQP-VK.texture.filtering_anisotropy.*
---
 src/intel/vulkan/genX_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/intel/vulkan/genX_state.c b/src/intel/vulkan/genX_state.c
index 4122395..0f621f9 100644
--- a/src/intel/vulkan/genX_state.c
+++ b/src/intel/vulkan/genX_state.c
@@ -101,7 +101,7 @@ vk_to_gen_tex_filter(VkFilter filter, bool anisotropyEnable)
default:
   assert(!"Invalid filter");
case VK_FILTER_NEAREST:
-  return MAPFILTER_NEAREST;
+  return anisotropyEnable ? MAPFILTER_ANISOTROPIC : MAPFILTER_NEAREST;
case VK_FILTER_LINEAR:
   return anisotropyEnable ? MAPFILTER_ANISOTROPIC : MAPFILTER_LINEAR;
}
-- 
2.7.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev