Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
On 27/07/16 13:47, Alejandro Piñeiro wrote: > On 26/07/16 22:16, Francisco Jerez wrote: -- However that spec change would have the side effect of making the CTS test you're trying to fix non-compliant... >>> This would not be the first case of a CTS test that is not correct due >>> the complexities/ambiguities of the OpenGL spec. Or worse, CTS tests >>> that are just >>> >> Heh, seems like some words were censored from your last paragraph. :P > Totally unintentional and innocent, I promise ;) When writing the email > I didn't find a suitable word when writing that paragraph, and then I > forgot to got back. Let's go simple and just say "wrong". > >>> So what about this: I open a Khronos bug (public unless someone thinks >>> that it would be better private), using your explanation as a base to >>> the description (although tempted to just point this email), and we put >>> the patch/bugfix on hold until we get an answer? >>> >> Sure, feel free to use my last post as starting point, and thanks for >> filing the bug at Khronos. :) > Thanks to you for the patience and the long and detailed explanation. > > You can find the bug here: > https://www.khronos.org/bugzilla/show_bug.cgi?id=1654 > > Is basically a edited form of your explanation (some sections are your > explanation word by word). FWIW, the list of CTS tests that would need to be tweaked is bigger. 8 CTS tests would need to be updated if the conclusion is that as soon as the texture is incomplete the image unit is invalid. Full list on the ping I just sent to the khronos bug: https://www.khronos.org/bugzilla/show_bug.cgi?id=1654#c1 BR signature.asc Description: OpenPGP digital signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
On 26/07/16 22:16, Francisco Jerez wrote: > >>> -- However that spec change would have the side effect of making >>> the CTS test you're trying to fix non-compliant... >> This would not be the first case of a CTS test that is not correct due >> the complexities/ambiguities of the OpenGL spec. Or worse, CTS tests >> that are just >> > Heh, seems like some words were censored from your last paragraph. :P Totally unintentional and innocent, I promise ;) When writing the email I didn't find a suitable word when writing that paragraph, and then I forgot to got back. Let's go simple and just say "wrong". >> So what about this: I open a Khronos bug (public unless someone thinks >> that it would be better private), using your explanation as a base to >> the description (although tempted to just point this email), and we put >> the patch/bugfix on hold until we get an answer? >> > Sure, feel free to use my last post as starting point, and thanks for > filing the bug at Khronos. :) Thanks to you for the patience and the long and detailed explanation. You can find the bug here: https://www.khronos.org/bugzilla/show_bug.cgi?id=1654 Is basically a edited form of your explanation (some sections are your explanation word by word). BR signature.asc Description: OpenPGP digital signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
Alejandro Piñeiro writes:
> On 23/07/16 00:31, Francisco Jerez wrote:
>> Alejandro Piñeiro writes:
>>
>>> Hi,
>>>
>>> On 15/07/16 22:46, Francisco Jerez wrote:
Alejandro Piñeiro writes:
> On 14/07/16 21:24, Francisco Jerez wrote:
>> Alejandro Piñeiro writes:
>>
>>> Without this commit, a image is considered valid if the level of the
>>> texture bound to the image is complete, something we can check as mesa
>>> save independently if it is "base incomplete" of "mipmap incomplete".
>>>
>>> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
>>> Image Loads and Stores"):
>>>
>>> "An access is considered invalid if:
>>> the texture bound to the selected image unit is incomplete;"
>>>
>>> This implies that the access to the image unit is invalid if the
>>> texture is incomplete, no mattering details about the specific texture
>>> level bound to the image.
>>>
>>> This fixes:
>>> GL44-CTS.shader_image_load_store.incomplete_textures
>>> ---
>>>
>>> Current piglit test is not testing what this commit tries to fix. I
>>> will send a patch to piglit in short.
>>>
>>> src/mesa/main/shaderimage.c | 14 +++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
>>> index 90643c4..d20cd90 100644
>>> --- a/src/mesa/main/shaderimage.c
>>> +++ b/src/mesa/main/shaderimage.c
>>> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
>>> struct gl_image_unit *u)
>>> if (!t->_BaseComplete && !t->_MipmapComplete)
>>> _mesa_test_texobj_completeness(ctx, t);
>>>
>>> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture
>>> Image
>>> +* Loads and Stores:
>>> +*
>>> +* "An access is considered invalid if:
>>> +*the texture bound to the selected image unit is incomplete;"
>>> +*/
>>> + if (!t->_BaseComplete ||
>>> + !t->_MipmapComplete)
>>> + return GL_FALSE;
>> I don't think this is correct, AFAIUI a texture having _MipmapComplete
>> equal to false doesn't imply that the texture as a whole would be
>> considered incomplete according to the GL's definition of completeness.
>> Whether or not it's considered complete usually depends on the sampler
>> state while you're doing regular texture sampling: If the sampler a
>> texture object is used with has any of the mipmap filtering modes
>> enabled you need to check _MipmapComplete, otherwise you need to check
>> _BaseComplete. The problem when you attempt to carry over this
>> definition to shader images (as the spec implies) is that image units
>> have no sampler state as such, and that they can only ever access one
>> specified level of the texture at a time (potentially a texture level
>> other than the base). This patch makes image units behave like a
>> sampler unit with mipmap filtering enabled for the purpose of texture
>> completeness validation, which is almost definitely too strong.
> Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
> checking the state at all. Thanks for pointing it.
>
>> An alternative would be to do something along the lines of:
>>
>> | if (!_mesa_is_texture_complete(t, &t->Sampler))
>> |return GL_FALSE;
> Yes, that is what I wanted, to return false if the texture is incomplete.
>
>> The problem is that you would then run into problems when some of the
>> non-base mipmap levels are missing but the sampler state baked into the
>> gl_texture_object says that you aren't mipmapping, so the GL spec would
>> normally consider the texture to be complete and
>> _mesa_is_texture_complete would return true accordingly, but still you
>> wouldn't be able to use any of the missing texture levels as shader
>> image if the application tried to bind them to an image unit (that's the
>> reason for the u->Level vs t->BaseLevel checks below you're removing).
> Ok, then if I understand correctly, the solution is not about replacing
> the level checks for _mesa_is_texture_complete, but keeping current
> checks, and add a _mesa_is_texture_complete check. Just checked and
> everything seems to work fine (except that now the behaviour is more
> strict, see below). I will send a patch in short.
>
Yeah, that would likely work and get the CTS test to pass, but it would
still be more strict than the spec says and consider cases that are OK
according to the spec to be incomplete, so I was reluctant to call it a
solution.
I think the ideal solution would be for the state of an image unit to be
independent from the filtering and sampling state, and depend on the
completeness of the bound l
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
On 23/07/16 00:31, Francisco Jerez wrote:
> Alejandro Piñeiro writes:
>
>> Hi,
>>
>> On 15/07/16 22:46, Francisco Jerez wrote:
>>> Alejandro Piñeiro writes:
>>>
On 14/07/16 21:24, Francisco Jerez wrote:
> Alejandro Piñeiro writes:
>
>> Without this commit, a image is considered valid if the level of the
>> texture bound to the image is complete, something we can check as mesa
>> save independently if it is "base incomplete" of "mipmap incomplete".
>>
>> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
>> Image Loads and Stores"):
>>
>> "An access is considered invalid if:
>> the texture bound to the selected image unit is incomplete;"
>>
>> This implies that the access to the image unit is invalid if the
>> texture is incomplete, no mattering details about the specific texture
>> level bound to the image.
>>
>> This fixes:
>> GL44-CTS.shader_image_load_store.incomplete_textures
>> ---
>>
>> Current piglit test is not testing what this commit tries to fix. I
>> will send a patch to piglit in short.
>>
>> src/mesa/main/shaderimage.c | 14 +++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
>> index 90643c4..d20cd90 100644
>> --- a/src/mesa/main/shaderimage.c
>> +++ b/src/mesa/main/shaderimage.c
>> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
>> struct gl_image_unit *u)
>> if (!t->_BaseComplete && !t->_MipmapComplete)
>> _mesa_test_texobj_completeness(ctx, t);
>>
>> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture
>> Image
>> +* Loads and Stores:
>> +*
>> +* "An access is considered invalid if:
>> +*the texture bound to the selected image unit is incomplete;"
>> +*/
>> + if (!t->_BaseComplete ||
>> + !t->_MipmapComplete)
>> + return GL_FALSE;
> I don't think this is correct, AFAIUI a texture having _MipmapComplete
> equal to false doesn't imply that the texture as a whole would be
> considered incomplete according to the GL's definition of completeness.
> Whether or not it's considered complete usually depends on the sampler
> state while you're doing regular texture sampling: If the sampler a
> texture object is used with has any of the mipmap filtering modes
> enabled you need to check _MipmapComplete, otherwise you need to check
> _BaseComplete. The problem when you attempt to carry over this
> definition to shader images (as the spec implies) is that image units
> have no sampler state as such, and that they can only ever access one
> specified level of the texture at a time (potentially a texture level
> other than the base). This patch makes image units behave like a
> sampler unit with mipmap filtering enabled for the purpose of texture
> completeness validation, which is almost definitely too strong.
Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
checking the state at all. Thanks for pointing it.
> An alternative would be to do something along the lines of:
>
> | if (!_mesa_is_texture_complete(t, &t->Sampler))
> |return GL_FALSE;
Yes, that is what I wanted, to return false if the texture is incomplete.
> The problem is that you would then run into problems when some of the
> non-base mipmap levels are missing but the sampler state baked into the
> gl_texture_object says that you aren't mipmapping, so the GL spec would
> normally consider the texture to be complete and
> _mesa_is_texture_complete would return true accordingly, but still you
> wouldn't be able to use any of the missing texture levels as shader
> image if the application tried to bind them to an image unit (that's the
> reason for the u->Level vs t->BaseLevel checks below you're removing).
Ok, then if I understand correctly, the solution is not about replacing
the level checks for _mesa_is_texture_complete, but keeping current
checks, and add a _mesa_is_texture_complete check. Just checked and
everything seems to work fine (except that now the behaviour is more
strict, see below). I will send a patch in short.
>>> Yeah, that would likely work and get the CTS test to pass, but it would
>>> still be more strict than the spec says and consider cases that are OK
>>> according to the spec to be incomplete, so I was reluctant to call it a
>>> solution.
>>>
>>> I think the ideal solution would be for the state of an image unit to be
>>> independent from the filtering and sampling state, and depend on the
>>> completeness of the bound level *only*. Any idea if this CTS (or your
>>> equivalent piglit test) passes on other GL implementations that support
>>> imag
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
On 23/07/16 00:31, Francisco Jerez wrote:
> Alejandro Piñeiro writes:
>
>> Hi,
>>
>> On 15/07/16 22:46, Francisco Jerez wrote:
>>> Alejandro Piñeiro writes:
>>>
On 14/07/16 21:24, Francisco Jerez wrote:
> Alejandro Piñeiro writes:
>
>> Without this commit, a image is considered valid if the level of the
>> texture bound to the image is complete, something we can check as mesa
>> save independently if it is "base incomplete" of "mipmap incomplete".
>>
>> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
>> Image Loads and Stores"):
>>
>> "An access is considered invalid if:
>> the texture bound to the selected image unit is incomplete;"
>>
>> This implies that the access to the image unit is invalid if the
>> texture is incomplete, no mattering details about the specific texture
>> level bound to the image.
>>
>> This fixes:
>> GL44-CTS.shader_image_load_store.incomplete_textures
>> ---
>>
>> Current piglit test is not testing what this commit tries to fix. I
>> will send a patch to piglit in short.
>>
>> src/mesa/main/shaderimage.c | 14 +++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
>> index 90643c4..d20cd90 100644
>> --- a/src/mesa/main/shaderimage.c
>> +++ b/src/mesa/main/shaderimage.c
>> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
>> struct gl_image_unit *u)
>> if (!t->_BaseComplete && !t->_MipmapComplete)
>> _mesa_test_texobj_completeness(ctx, t);
>>
>> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture
>> Image
>> +* Loads and Stores:
>> +*
>> +* "An access is considered invalid if:
>> +*the texture bound to the selected image unit is incomplete;"
>> +*/
>> + if (!t->_BaseComplete ||
>> + !t->_MipmapComplete)
>> + return GL_FALSE;
> I don't think this is correct, AFAIUI a texture having _MipmapComplete
> equal to false doesn't imply that the texture as a whole would be
> considered incomplete according to the GL's definition of completeness.
> Whether or not it's considered complete usually depends on the sampler
> state while you're doing regular texture sampling: If the sampler a
> texture object is used with has any of the mipmap filtering modes
> enabled you need to check _MipmapComplete, otherwise you need to check
> _BaseComplete. The problem when you attempt to carry over this
> definition to shader images (as the spec implies) is that image units
> have no sampler state as such, and that they can only ever access one
> specified level of the texture at a time (potentially a texture level
> other than the base). This patch makes image units behave like a
> sampler unit with mipmap filtering enabled for the purpose of texture
> completeness validation, which is almost definitely too strong.
Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
checking the state at all. Thanks for pointing it.
> An alternative would be to do something along the lines of:
>
> | if (!_mesa_is_texture_complete(t, &t->Sampler))
> |return GL_FALSE;
Yes, that is what I wanted, to return false if the texture is incomplete.
> The problem is that you would then run into problems when some of the
> non-base mipmap levels are missing but the sampler state baked into the
> gl_texture_object says that you aren't mipmapping, so the GL spec would
> normally consider the texture to be complete and
> _mesa_is_texture_complete would return true accordingly, but still you
> wouldn't be able to use any of the missing texture levels as shader
> image if the application tried to bind them to an image unit (that's the
> reason for the u->Level vs t->BaseLevel checks below you're removing).
Ok, then if I understand correctly, the solution is not about replacing
the level checks for _mesa_is_texture_complete, but keeping current
checks, and add a _mesa_is_texture_complete check. Just checked and
everything seems to work fine (except that now the behaviour is more
strict, see below). I will send a patch in short.
>>> Yeah, that would likely work and get the CTS test to pass, but it would
>>> still be more strict than the spec says and consider cases that are OK
>>> according to the spec to be incomplete, so I was reluctant to call it a
>>> solution.
>>>
>>> I think the ideal solution would be for the state of an image unit to be
>>> independent from the filtering and sampling state, and depend on the
>>> completeness of the bound level *only*. Any idea if this CTS (or your
>>> equivalent piglit test) passes on other GL implementations that support
>>> imag
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
Alejandro Piñeiro writes:
> Hi,
>
> On 15/07/16 22:46, Francisco Jerez wrote:
>> Alejandro Piñeiro writes:
>>
>>> On 14/07/16 21:24, Francisco Jerez wrote:
Alejandro Piñeiro writes:
> Without this commit, a image is considered valid if the level of the
> texture bound to the image is complete, something we can check as mesa
> save independently if it is "base incomplete" of "mipmap incomplete".
>
> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
> Image Loads and Stores"):
>
> "An access is considered invalid if:
> the texture bound to the selected image unit is incomplete;"
>
> This implies that the access to the image unit is invalid if the
> texture is incomplete, no mattering details about the specific texture
> level bound to the image.
>
> This fixes:
> GL44-CTS.shader_image_load_store.incomplete_textures
> ---
>
> Current piglit test is not testing what this commit tries to fix. I
> will send a patch to piglit in short.
>
> src/mesa/main/shaderimage.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
> index 90643c4..d20cd90 100644
> --- a/src/mesa/main/shaderimage.c
> +++ b/src/mesa/main/shaderimage.c
> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
> struct gl_image_unit *u)
> if (!t->_BaseComplete && !t->_MipmapComplete)
> _mesa_test_texobj_completeness(ctx, t);
>
> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture Image
> +* Loads and Stores:
> +*
> +* "An access is considered invalid if:
> +*the texture bound to the selected image unit is incomplete;"
> +*/
> + if (!t->_BaseComplete ||
> + !t->_MipmapComplete)
> + return GL_FALSE;
I don't think this is correct, AFAIUI a texture having _MipmapComplete
equal to false doesn't imply that the texture as a whole would be
considered incomplete according to the GL's definition of completeness.
Whether or not it's considered complete usually depends on the sampler
state while you're doing regular texture sampling: If the sampler a
texture object is used with has any of the mipmap filtering modes
enabled you need to check _MipmapComplete, otherwise you need to check
_BaseComplete. The problem when you attempt to carry over this
definition to shader images (as the spec implies) is that image units
have no sampler state as such, and that they can only ever access one
specified level of the texture at a time (potentially a texture level
other than the base). This patch makes image units behave like a
sampler unit with mipmap filtering enabled for the purpose of texture
completeness validation, which is almost definitely too strong.
>>> Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
>>> checking the state at all. Thanks for pointing it.
>>>
An alternative would be to do something along the lines of:
| if (!_mesa_is_texture_complete(t, &t->Sampler))
|return GL_FALSE;
>>> Yes, that is what I wanted, to return false if the texture is incomplete.
>>>
The problem is that you would then run into problems when some of the
non-base mipmap levels are missing but the sampler state baked into the
gl_texture_object says that you aren't mipmapping, so the GL spec would
normally consider the texture to be complete and
_mesa_is_texture_complete would return true accordingly, but still you
wouldn't be able to use any of the missing texture levels as shader
image if the application tried to bind them to an image unit (that's the
reason for the u->Level vs t->BaseLevel checks below you're removing).
>>> Ok, then if I understand correctly, the solution is not about replacing
>>> the level checks for _mesa_is_texture_complete, but keeping current
>>> checks, and add a _mesa_is_texture_complete check. Just checked and
>>> everything seems to work fine (except that now the behaviour is more
>>> strict, see below). I will send a patch in short.
>>>
>> Yeah, that would likely work and get the CTS test to pass, but it would
>> still be more strict than the spec says and consider cases that are OK
>> according to the spec to be incomplete, so I was reluctant to call it a
>> solution.
>>
>> I think the ideal solution would be for the state of an image unit to be
>> independent from the filtering and sampling state, and depend on the
>> completeness of the bound level *only*. Any idea if this CTS (or your
>> equivalent piglit test) passes on other GL implementations that support
>> image load/store (e.g. nVidia's -- I would be surprised if it does).
>
> Just checked today with NVIDIA 352.30 and 352.30. I was not able to
> directly te
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
Ping. Also including Ian Romanick on the conversation, as right now the
main question is about the spec.
The thread is somewhat messy, so in order to be clear, this is the last
patch I sent to the list (and the one Im proposing):
https://patchwork.freedesktop.org/patch/98877/
On 18/07/16 14:44, Alejandro Piñeiro wrote:
> Hi,
>
> On 15/07/16 22:46, Francisco Jerez wrote:
>> Alejandro Piñeiro writes:
>>
>>> On 14/07/16 21:24, Francisco Jerez wrote:
Alejandro Piñeiro writes:
> Without this commit, a image is considered valid if the level of the
> texture bound to the image is complete, something we can check as mesa
> save independently if it is "base incomplete" of "mipmap incomplete".
>
> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
> Image Loads and Stores"):
>
> "An access is considered invalid if:
> the texture bound to the selected image unit is incomplete;"
>
> This implies that the access to the image unit is invalid if the
> texture is incomplete, no mattering details about the specific texture
> level bound to the image.
>
> This fixes:
> GL44-CTS.shader_image_load_store.incomplete_textures
> ---
>
> Current piglit test is not testing what this commit tries to fix. I
> will send a patch to piglit in short.
>
> src/mesa/main/shaderimage.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
> index 90643c4..d20cd90 100644
> --- a/src/mesa/main/shaderimage.c
> +++ b/src/mesa/main/shaderimage.c
> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
> struct gl_image_unit *u)
> if (!t->_BaseComplete && !t->_MipmapComplete)
> _mesa_test_texobj_completeness(ctx, t);
>
> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture Image
> +* Loads and Stores:
> +*
> +* "An access is considered invalid if:
> +*the texture bound to the selected image unit is incomplete;"
> +*/
> + if (!t->_BaseComplete ||
> + !t->_MipmapComplete)
> + return GL_FALSE;
I don't think this is correct, AFAIUI a texture having _MipmapComplete
equal to false doesn't imply that the texture as a whole would be
considered incomplete according to the GL's definition of completeness.
Whether or not it's considered complete usually depends on the sampler
state while you're doing regular texture sampling: If the sampler a
texture object is used with has any of the mipmap filtering modes
enabled you need to check _MipmapComplete, otherwise you need to check
_BaseComplete. The problem when you attempt to carry over this
definition to shader images (as the spec implies) is that image units
have no sampler state as such, and that they can only ever access one
specified level of the texture at a time (potentially a texture level
other than the base). This patch makes image units behave like a
sampler unit with mipmap filtering enabled for the purpose of texture
completeness validation, which is almost definitely too strong.
>>> Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
>>> checking the state at all. Thanks for pointing it.
>>>
An alternative would be to do something along the lines of:
| if (!_mesa_is_texture_complete(t, &t->Sampler))
|return GL_FALSE;
>>> Yes, that is what I wanted, to return false if the texture is incomplete.
>>>
The problem is that you would then run into problems when some of the
non-base mipmap levels are missing but the sampler state baked into the
gl_texture_object says that you aren't mipmapping, so the GL spec would
normally consider the texture to be complete and
_mesa_is_texture_complete would return true accordingly, but still you
wouldn't be able to use any of the missing texture levels as shader
image if the application tried to bind them to an image unit (that's the
reason for the u->Level vs t->BaseLevel checks below you're removing).
>>> Ok, then if I understand correctly, the solution is not about replacing
>>> the level checks for _mesa_is_texture_complete, but keeping current
>>> checks, and add a _mesa_is_texture_complete check. Just checked and
>>> everything seems to work fine (except that now the behaviour is more
>>> strict, see below). I will send a patch in short.
>>>
>> Yeah, that would likely work and get the CTS test to pass, but it would
>> still be more strict than the spec says and consider cases that are OK
>> according to the spec to be incomplete, so I was reluctant to call it a
>> solution.
>>
>> I think the ideal solution would be for the state of an image unit to be
>> independent from the filtering and sampling state, and depend on the
>> complet
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
Hi,
On 15/07/16 22:46, Francisco Jerez wrote:
> Alejandro Piñeiro writes:
>
>> On 14/07/16 21:24, Francisco Jerez wrote:
>>> Alejandro Piñeiro writes:
>>>
Without this commit, a image is considered valid if the level of the
texture bound to the image is complete, something we can check as mesa
save independently if it is "base incomplete" of "mipmap incomplete".
But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
Image Loads and Stores"):
"An access is considered invalid if:
the texture bound to the selected image unit is incomplete;"
This implies that the access to the image unit is invalid if the
texture is incomplete, no mattering details about the specific texture
level bound to the image.
This fixes:
GL44-CTS.shader_image_load_store.incomplete_textures
---
Current piglit test is not testing what this commit tries to fix. I
will send a patch to piglit in short.
src/mesa/main/shaderimage.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
index 90643c4..d20cd90 100644
--- a/src/mesa/main/shaderimage.c
+++ b/src/mesa/main/shaderimage.c
@@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
struct gl_image_unit *u)
if (!t->_BaseComplete && !t->_MipmapComplete)
_mesa_test_texobj_completeness(ctx, t);
+ /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture Image
+* Loads and Stores:
+*
+* "An access is considered invalid if:
+*the texture bound to the selected image unit is incomplete;"
+*/
+ if (!t->_BaseComplete ||
+ !t->_MipmapComplete)
+ return GL_FALSE;
>>> I don't think this is correct, AFAIUI a texture having _MipmapComplete
>>> equal to false doesn't imply that the texture as a whole would be
>>> considered incomplete according to the GL's definition of completeness.
>>> Whether or not it's considered complete usually depends on the sampler
>>> state while you're doing regular texture sampling: If the sampler a
>>> texture object is used with has any of the mipmap filtering modes
>>> enabled you need to check _MipmapComplete, otherwise you need to check
>>> _BaseComplete. The problem when you attempt to carry over this
>>> definition to shader images (as the spec implies) is that image units
>>> have no sampler state as such, and that they can only ever access one
>>> specified level of the texture at a time (potentially a texture level
>>> other than the base). This patch makes image units behave like a
>>> sampler unit with mipmap filtering enabled for the purpose of texture
>>> completeness validation, which is almost definitely too strong.
>> Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
>> checking the state at all. Thanks for pointing it.
>>
>>> An alternative would be to do something along the lines of:
>>>
>>> | if (!_mesa_is_texture_complete(t, &t->Sampler))
>>> |return GL_FALSE;
>> Yes, that is what I wanted, to return false if the texture is incomplete.
>>
>>> The problem is that you would then run into problems when some of the
>>> non-base mipmap levels are missing but the sampler state baked into the
>>> gl_texture_object says that you aren't mipmapping, so the GL spec would
>>> normally consider the texture to be complete and
>>> _mesa_is_texture_complete would return true accordingly, but still you
>>> wouldn't be able to use any of the missing texture levels as shader
>>> image if the application tried to bind them to an image unit (that's the
>>> reason for the u->Level vs t->BaseLevel checks below you're removing).
>> Ok, then if I understand correctly, the solution is not about replacing
>> the level checks for _mesa_is_texture_complete, but keeping current
>> checks, and add a _mesa_is_texture_complete check. Just checked and
>> everything seems to work fine (except that now the behaviour is more
>> strict, see below). I will send a patch in short.
>>
> Yeah, that would likely work and get the CTS test to pass, but it would
> still be more strict than the spec says and consider cases that are OK
> according to the spec to be incomplete, so I was reluctant to call it a
> solution.
>
> I think the ideal solution would be for the state of an image unit to be
> independent from the filtering and sampling state, and depend on the
> completeness of the bound level *only*. Any idea if this CTS (or your
> equivalent piglit test) passes on other GL implementations that support
> image load/store (e.g. nVidia's -- I would be surprised if it does).
Just checked today with NVIDIA 352.30 and 352.30. I was not able to
directly test this issue with the CTS test, as it fails before with a
different error (the shader doesn't compile, and the shader info log is
g
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
Alejandro Piñeiro writes:
> On 14/07/16 21:24, Francisco Jerez wrote:
>> Alejandro Piñeiro writes:
>>
>>> Without this commit, a image is considered valid if the level of the
>>> texture bound to the image is complete, something we can check as mesa
>>> save independently if it is "base incomplete" of "mipmap incomplete".
>>>
>>> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
>>> Image Loads and Stores"):
>>>
>>> "An access is considered invalid if:
>>> the texture bound to the selected image unit is incomplete;"
>>>
>>> This implies that the access to the image unit is invalid if the
>>> texture is incomplete, no mattering details about the specific texture
>>> level bound to the image.
>>>
>>> This fixes:
>>> GL44-CTS.shader_image_load_store.incomplete_textures
>>> ---
>>>
>>> Current piglit test is not testing what this commit tries to fix. I
>>> will send a patch to piglit in short.
>>>
>>> src/mesa/main/shaderimage.c | 14 +++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
>>> index 90643c4..d20cd90 100644
>>> --- a/src/mesa/main/shaderimage.c
>>> +++ b/src/mesa/main/shaderimage.c
>>> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
>>> struct gl_image_unit *u)
>>> if (!t->_BaseComplete && !t->_MipmapComplete)
>>> _mesa_test_texobj_completeness(ctx, t);
>>>
>>> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture Image
>>> +* Loads and Stores:
>>> +*
>>> +* "An access is considered invalid if:
>>> +*the texture bound to the selected image unit is incomplete;"
>>> +*/
>>> + if (!t->_BaseComplete ||
>>> + !t->_MipmapComplete)
>>> + return GL_FALSE;
>> I don't think this is correct, AFAIUI a texture having _MipmapComplete
>> equal to false doesn't imply that the texture as a whole would be
>> considered incomplete according to the GL's definition of completeness.
>> Whether or not it's considered complete usually depends on the sampler
>> state while you're doing regular texture sampling: If the sampler a
>> texture object is used with has any of the mipmap filtering modes
>> enabled you need to check _MipmapComplete, otherwise you need to check
>> _BaseComplete. The problem when you attempt to carry over this
>> definition to shader images (as the spec implies) is that image units
>> have no sampler state as such, and that they can only ever access one
>> specified level of the texture at a time (potentially a texture level
>> other than the base). This patch makes image units behave like a
>> sampler unit with mipmap filtering enabled for the purpose of texture
>> completeness validation, which is almost definitely too strong.
>
> Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
> checking the state at all. Thanks for pointing it.
>
>> An alternative would be to do something along the lines of:
>>
>> | if (!_mesa_is_texture_complete(t, &t->Sampler))
>> |return GL_FALSE;
>
> Yes, that is what I wanted, to return false if the texture is incomplete.
>
>>
>> The problem is that you would then run into problems when some of the
>> non-base mipmap levels are missing but the sampler state baked into the
>> gl_texture_object says that you aren't mipmapping, so the GL spec would
>> normally consider the texture to be complete and
>> _mesa_is_texture_complete would return true accordingly, but still you
>> wouldn't be able to use any of the missing texture levels as shader
>> image if the application tried to bind them to an image unit (that's the
>> reason for the u->Level vs t->BaseLevel checks below you're removing).
>
> Ok, then if I understand correctly, the solution is not about replacing
> the level checks for _mesa_is_texture_complete, but keeping current
> checks, and add a _mesa_is_texture_complete check. Just checked and
> everything seems to work fine (except that now the behaviour is more
> strict, see below). I will send a patch in short.
>
Yeah, that would likely work and get the CTS test to pass, but it would
still be more strict than the spec says and consider cases that are OK
according to the spec to be incomplete, so I was reluctant to call it a
solution.
I think the ideal solution would be for the state of an image unit to be
independent from the filtering and sampling state, and depend on the
completeness of the bound level *only*. Any idea if this CTS (or your
equivalent piglit test) passes on other GL implementations that support
image load/store (e.g. nVidia's -- I would be surprised if it does).
>> Honestly I think the current definition of image unit completeness is
>> broken, because it considers the image unit state to be complete in
>> cases where the image unit state is unusable (because the target image
>> level may be missing even though the texture object itself is considered
>> complete), and because it considers it to be incomplete
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
On 14/07/16 21:24, Francisco Jerez wrote:
> Alejandro Piñeiro writes:
>
>> Without this commit, a image is considered valid if the level of the
>> texture bound to the image is complete, something we can check as mesa
>> save independently if it is "base incomplete" of "mipmap incomplete".
>>
>> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
>> Image Loads and Stores"):
>>
>> "An access is considered invalid if:
>> the texture bound to the selected image unit is incomplete;"
>>
>> This implies that the access to the image unit is invalid if the
>> texture is incomplete, no mattering details about the specific texture
>> level bound to the image.
>>
>> This fixes:
>> GL44-CTS.shader_image_load_store.incomplete_textures
>> ---
>>
>> Current piglit test is not testing what this commit tries to fix. I
>> will send a patch to piglit in short.
>>
>> src/mesa/main/shaderimage.c | 14 +++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
>> index 90643c4..d20cd90 100644
>> --- a/src/mesa/main/shaderimage.c
>> +++ b/src/mesa/main/shaderimage.c
>> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
>> struct gl_image_unit *u)
>> if (!t->_BaseComplete && !t->_MipmapComplete)
>> _mesa_test_texobj_completeness(ctx, t);
>>
>> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture Image
>> +* Loads and Stores:
>> +*
>> +* "An access is considered invalid if:
>> +*the texture bound to the selected image unit is incomplete;"
>> +*/
>> + if (!t->_BaseComplete ||
>> + !t->_MipmapComplete)
>> + return GL_FALSE;
> I don't think this is correct, AFAIUI a texture having _MipmapComplete
> equal to false doesn't imply that the texture as a whole would be
> considered incomplete according to the GL's definition of completeness.
> Whether or not it's considered complete usually depends on the sampler
> state while you're doing regular texture sampling: If the sampler a
> texture object is used with has any of the mipmap filtering modes
> enabled you need to check _MipmapComplete, otherwise you need to check
> _BaseComplete. The problem when you attempt to carry over this
> definition to shader images (as the spec implies) is that image units
> have no sampler state as such, and that they can only ever access one
> specified level of the texture at a time (potentially a texture level
> other than the base). This patch makes image units behave like a
> sampler unit with mipmap filtering enabled for the purpose of texture
> completeness validation, which is almost definitely too strong.
Yes, I didn't realize that _BaseComplete and _MipmapComplete were not
checking the state at all. Thanks for pointing it.
> An alternative would be to do something along the lines of:
>
> | if (!_mesa_is_texture_complete(t, &t->Sampler))
> |return GL_FALSE;
Yes, that is what I wanted, to return false if the texture is incomplete.
>
> The problem is that you would then run into problems when some of the
> non-base mipmap levels are missing but the sampler state baked into the
> gl_texture_object says that you aren't mipmapping, so the GL spec would
> normally consider the texture to be complete and
> _mesa_is_texture_complete would return true accordingly, but still you
> wouldn't be able to use any of the missing texture levels as shader
> image if the application tried to bind them to an image unit (that's the
> reason for the u->Level vs t->BaseLevel checks below you're removing).
Ok, then if I understand correctly, the solution is not about replacing
the level checks for _mesa_is_texture_complete, but keeping current
checks, and add a _mesa_is_texture_complete check. Just checked and
everything seems to work fine (except that now the behaviour is more
strict, see below). I will send a patch in short.
> Honestly I think the current definition of image unit completeness is
> broken, because it considers the image unit state to be complete in
> cases where the image unit state is unusable (because the target image
> level may be missing even though the texture object itself is considered
> complete), and because it considers it to be incomplete in cases where
> you really have no reason to care (e.g. why should you care what the
> filtering mode from the sampler state says if you aren't doing any
> filtering at all, as long as the one level you've bound to the image
> unit is present and complete).
Well, yes in general the spec is being perhaps too strict. But I assume
that if we want to follow the spec, we don't have any other option.
FWIW, when testing the patch I mentioned before, some of the piglit
tests start to fail. The reason is that with that patch everything is
more strict, so the piglit tests need to set the filtering to NEAREST in
order to not fail. I will send a piglit patch soon too.
>
>> +
>> if (u->Level < t->BaseLevel
Re: [Mesa-dev] [PATCH] main/shaderimage: image unit invalid if texture is incomplete, independently of the level
Alejandro Piñeiro writes:
> Without this commit, a image is considered valid if the level of the
> texture bound to the image is complete, something we can check as mesa
> save independently if it is "base incomplete" of "mipmap incomplete".
>
> But, from the OpenGL 4.3 Core Specification, section 8.25 ("Texture
> Image Loads and Stores"):
>
> "An access is considered invalid if:
> the texture bound to the selected image unit is incomplete;"
>
> This implies that the access to the image unit is invalid if the
> texture is incomplete, no mattering details about the specific texture
> level bound to the image.
>
> This fixes:
> GL44-CTS.shader_image_load_store.incomplete_textures
> ---
>
> Current piglit test is not testing what this commit tries to fix. I
> will send a patch to piglit in short.
>
> src/mesa/main/shaderimage.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/main/shaderimage.c b/src/mesa/main/shaderimage.c
> index 90643c4..d20cd90 100644
> --- a/src/mesa/main/shaderimage.c
> +++ b/src/mesa/main/shaderimage.c
> @@ -469,10 +469,18 @@ _mesa_is_image_unit_valid(struct gl_context *ctx,
> struct gl_image_unit *u)
> if (!t->_BaseComplete && !t->_MipmapComplete)
> _mesa_test_texobj_completeness(ctx, t);
>
> + /* From the OpenGL 4.3 Core Specification, Chapter 8.25, Texture Image
> +* Loads and Stores:
> +*
> +* "An access is considered invalid if:
> +*the texture bound to the selected image unit is incomplete;"
> +*/
> + if (!t->_BaseComplete ||
> + !t->_MipmapComplete)
> + return GL_FALSE;
I don't think this is correct, AFAIUI a texture having _MipmapComplete
equal to false doesn't imply that the texture as a whole would be
considered incomplete according to the GL's definition of completeness.
Whether or not it's considered complete usually depends on the sampler
state while you're doing regular texture sampling: If the sampler a
texture object is used with has any of the mipmap filtering modes
enabled you need to check _MipmapComplete, otherwise you need to check
_BaseComplete. The problem when you attempt to carry over this
definition to shader images (as the spec implies) is that image units
have no sampler state as such, and that they can only ever access one
specified level of the texture at a time (potentially a texture level
other than the base). This patch makes image units behave like a
sampler unit with mipmap filtering enabled for the purpose of texture
completeness validation, which is almost definitely too strong.
An alternative would be to do something along the lines of:
| if (!_mesa_is_texture_complete(t, &t->Sampler))
|return GL_FALSE;
The problem is that you would then run into problems when some of the
non-base mipmap levels are missing but the sampler state baked into the
gl_texture_object says that you aren't mipmapping, so the GL spec would
normally consider the texture to be complete and
_mesa_is_texture_complete would return true accordingly, but still you
wouldn't be able to use any of the missing texture levels as shader
image if the application tried to bind them to an image unit (that's the
reason for the u->Level vs t->BaseLevel checks below you're removing).
Honestly I think the current definition of image unit completeness is
broken, because it considers the image unit state to be complete in
cases where the image unit state is unusable (because the target image
level may be missing even though the texture object itself is considered
complete), and because it considers it to be incomplete in cases where
you really have no reason to care (e.g. why should you care what the
filtering mode from the sampler state says if you aren't doing any
filtering at all, as long as the one level you've bound to the image
unit is present and complete).
> +
> if (u->Level < t->BaseLevel ||
> - u->Level > t->_MaxLevel ||
> - (u->Level == t->BaseLevel && !t->_BaseComplete) ||
> - (u->Level != t->BaseLevel && !t->_MipmapComplete))
> + u->Level > t->_MaxLevel)
>return GL_FALSE;
>
> if (_mesa_tex_target_is_layered(t->Target) &&
> --
> 2.7.4
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
