Re: [Mesa-dev] [PATCH] st/mesa: ensure that images don't try to reference non-existent levels

[Ilia Mirkin]

The problem is that st_finalize_texture isn't doing its job in some
circumstances. Specifically, if I have a 2-level 2d array with level0
= (2,2,7) and level1 = (1,1,7) with a non-mipmap filter (e.g.
GL_NEAREST), then at no point are two resources combined into one.

Clamping the level will avoid the assert, but it's not the right thing
either since it'll still be passing in the wrong resource.

On Mon, Mar 5, 2018 at 2:05 PM, Marek Olšák  wrote:
> Maybe st/mesa should clamp the level instead of assert.
>
> Marek
>
> On Thu, Mar 1, 2018 at 7:23 AM, Ilia Mirkin  wrote:
>> Yes, as I mentioned this makes some tests assert.
>>
>> They were passing before, but it was through luck since the actual images
>> were never accessed.
>>
>> On Mar 1, 2018 04:04, "Timothy Arceri"  wrote:
>>>
>>> This causes the CTS tests to assert on radeonsi where they previously
>>> passed. If that expected?
>>>
>>> On 27/02/18 16:19, Ilia Mirkin wrote:

 Ideally the st_finalize_texture call would take care of that, but it
 doesn't seem to with KHR-GL45.shader_image_size.advanced-nonMS-*. This
 assertions makes sure that no such values are passed to the driver.

 Signed-off-by: Ilia Mirkin 
 ---

 This will trigger asserts in CTS, but I think that's better than feeding
 broken values to driver backends.

   src/mesa/state_tracker/st_atom_image.c | 1 +
   1 file changed, 1 insertion(+)

 diff --git a/src/mesa/state_tracker/st_atom_image.c
 b/src/mesa/state_tracker/st_atom_image.c
 index 1c4980173f4..421c926cf04 100644
 --- a/src/mesa/state_tracker/st_atom_image.c
 +++ b/src/mesa/state_tracker/st_atom_image.c
 @@ -97,6 +97,7 @@ st_convert_image(const struct st_context *st, const
 struct gl_image_unit *u,
   img->resource = stObj->pt;
 img->u.tex.level = u->Level + stObj->base.MinLevel;
 +  assert(img->u.tex.level <= img->resource->last_level);
 if (stObj->pt->target == PIPE_TEXTURE_3D) {
if (u->Layered) {
   img->u.tex.first_layer = 0;

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

Re: [Mesa-dev] [PATCH] st/mesa: ensure that images don't try to reference non-existent levels

[Marek Olšák]

Maybe st/mesa should clamp the level instead of assert.

Marek

On Thu, Mar 1, 2018 at 7:23 AM, Ilia Mirkin  wrote:
> Yes, as I mentioned this makes some tests assert.
>
> They were passing before, but it was through luck since the actual images
> were never accessed.
>
> On Mar 1, 2018 04:04, "Timothy Arceri"  wrote:
>>
>> This causes the CTS tests to assert on radeonsi where they previously
>> passed. If that expected?
>>
>> On 27/02/18 16:19, Ilia Mirkin wrote:
>>>
>>> Ideally the st_finalize_texture call would take care of that, but it
>>> doesn't seem to with KHR-GL45.shader_image_size.advanced-nonMS-*. This
>>> assertions makes sure that no such values are passed to the driver.
>>>
>>> Signed-off-by: Ilia Mirkin 
>>> ---
>>>
>>> This will trigger asserts in CTS, but I think that's better than feeding
>>> broken values to driver backends.
>>>
>>>   src/mesa/state_tracker/st_atom_image.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/mesa/state_tracker/st_atom_image.c
>>> b/src/mesa/state_tracker/st_atom_image.c
>>> index 1c4980173f4..421c926cf04 100644
>>> --- a/src/mesa/state_tracker/st_atom_image.c
>>> +++ b/src/mesa/state_tracker/st_atom_image.c
>>> @@ -97,6 +97,7 @@ st_convert_image(const struct st_context *st, const
>>> struct gl_image_unit *u,
>>>   img->resource = stObj->pt;
>>> img->u.tex.level = u->Level + stObj->base.MinLevel;
>>> +  assert(img->u.tex.level <= img->resource->last_level);
>>> if (stObj->pt->target == PIPE_TEXTURE_3D) {
>>>if (u->Layered) {
>>>   img->u.tex.first_layer = 0;
>>>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/mesa: ensure that images don't try to reference non-existent levels

[Ilia Mirkin]

Yes, as I mentioned this makes some tests assert.

They were passing before, but it was through luck since the actual images
were never accessed.

On Mar 1, 2018 04:04, "Timothy Arceri"  wrote:

> This causes the CTS tests to assert on radeonsi where they previously
> passed. If that expected?
>
> On 27/02/18 16:19, Ilia Mirkin wrote:
>
>> Ideally the st_finalize_texture call would take care of that, but it
>> doesn't seem to with KHR-GL45.shader_image_size.advanced-nonMS-*. This
>> assertions makes sure that no such values are passed to the driver.
>>
>> Signed-off-by: Ilia Mirkin 
>> ---
>>
>> This will trigger asserts in CTS, but I think that's better than feeding
>> broken values to driver backends.
>>
>>   src/mesa/state_tracker/st_atom_image.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/mesa/state_tracker/st_atom_image.c
>> b/src/mesa/state_tracker/st_atom_image.c
>> index 1c4980173f4..421c926cf04 100644
>> --- a/src/mesa/state_tracker/st_atom_image.c
>> +++ b/src/mesa/state_tracker/st_atom_image.c
>> @@ -97,6 +97,7 @@ st_convert_image(const struct st_context *st, const
>> struct gl_image_unit *u,
>>   img->resource = stObj->pt;
>> img->u.tex.level = u->Level + stObj->base.MinLevel;
>> +  assert(img->u.tex.level <= img->resource->last_level);
>> if (stObj->pt->target == PIPE_TEXTURE_3D) {
>>if (u->Layered) {
>>   img->u.tex.first_layer = 0;
>>
>>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] st/mesa: ensure that images don't try to reference non-existent levels

[Timothy Arceri]

This causes the CTS tests to assert on radeonsi where they previously 
passed. If that expected?


On 27/02/18 16:19, Ilia Mirkin wrote:

Ideally the st_finalize_texture call would take care of that, but it
doesn't seem to with KHR-GL45.shader_image_size.advanced-nonMS-*. This
assertions makes sure that no such values are passed to the driver.

Signed-off-by: Ilia Mirkin 
---

This will trigger asserts in CTS, but I think that's better than feeding
broken values to driver backends.

  src/mesa/state_tracker/st_atom_image.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/mesa/state_tracker/st_atom_image.c 
b/src/mesa/state_tracker/st_atom_image.c
index 1c4980173f4..421c926cf04 100644
--- a/src/mesa/state_tracker/st_atom_image.c
+++ b/src/mesa/state_tracker/st_atom_image.c
@@ -97,6 +97,7 @@ st_convert_image(const struct st_context *st, const struct 
gl_image_unit *u,
  
img->resource = stObj->pt;

img->u.tex.level = u->Level + stObj->base.MinLevel;
+  assert(img->u.tex.level <= img->resource->last_level);
if (stObj->pt->target == PIPE_TEXTURE_3D) {
   if (u->Layered) {
  img->u.tex.first_layer = 0;


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

Re: [Mesa-dev] [PATCH] st/mesa: ensure that images don't try to reference non-existent levels

[Marek Olšák]

Reviewed-by: Marek Olšák 

Marek

On Tue, Feb 27, 2018 at 6:19 AM, Ilia Mirkin  wrote:
> Ideally the st_finalize_texture call would take care of that, but it
> doesn't seem to with KHR-GL45.shader_image_size.advanced-nonMS-*. This
> assertions makes sure that no such values are passed to the driver.
>
> Signed-off-by: Ilia Mirkin 
> ---
>
> This will trigger asserts in CTS, but I think that's better than feeding
> broken values to driver backends.
>
>  src/mesa/state_tracker/st_atom_image.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/mesa/state_tracker/st_atom_image.c 
> b/src/mesa/state_tracker/st_atom_image.c
> index 1c4980173f4..421c926cf04 100644
> --- a/src/mesa/state_tracker/st_atom_image.c
> +++ b/src/mesa/state_tracker/st_atom_image.c
> @@ -97,6 +97,7 @@ st_convert_image(const struct st_context *st, const struct 
> gl_image_unit *u,
>
>img->resource = stObj->pt;
>img->u.tex.level = u->Level + stObj->base.MinLevel;
> +  assert(img->u.tex.level <= img->resource->last_level);
>if (stObj->pt->target == PIPE_TEXTURE_3D) {
>   if (u->Layered) {
>  img->u.tex.first_layer = 0;
> --
> 2.16.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] st/mesa: ensure that images don't try to reference non-existent levels

[Ilia Mirkin]

Ideally the st_finalize_texture call would take care of that, but it
doesn't seem to with KHR-GL45.shader_image_size.advanced-nonMS-*. This
assertions makes sure that no such values are passed to the driver.

Signed-off-by: Ilia Mirkin 
---

This will trigger asserts in CTS, but I think that's better than feeding
broken values to driver backends.

 src/mesa/state_tracker/st_atom_image.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/mesa/state_tracker/st_atom_image.c 
b/src/mesa/state_tracker/st_atom_image.c
index 1c4980173f4..421c926cf04 100644
--- a/src/mesa/state_tracker/st_atom_image.c
+++ b/src/mesa/state_tracker/st_atom_image.c
@@ -97,6 +97,7 @@ st_convert_image(const struct st_context *st, const struct 
gl_image_unit *u,
 
   img->resource = stObj->pt;
   img->u.tex.level = u->Level + stObj->base.MinLevel;
+  assert(img->u.tex.level <= img->resource->last_level);
   if (stObj->pt->target == PIPE_TEXTURE_3D) {
  if (u->Layered) {
 img->u.tex.first_layer = 0;
-- 
2.16.1

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