[Mesa-dev] mesa/st: NumLayers is only valid for array textures

Submitted by Ilia Mirkin on Sept. 24, 2014, 5:01 a.m.

Details

Message ID 1411534906-923-1-git-send-email-imirkin@alum.mit.edu
State Accepted
Commit 9d2e298dd4159651323cac54dbc43527e7fd6d16
Headers show

Not browsing as part of any series.

Commit Message

Ilia Mirkin Sept. 24, 2014, 5:01 a.m.
For 3d textures, NumLayers is set to 1, which is not what we want. This
fixes the newly added gl-layer-render-storage test (which constructs
immutable 3d textures). Fixes regression introduced in d82bd7eb060.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84145
Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
---
 src/mesa/state_tracker/st_atom_texture.c | 2 +-
 src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
 src/mesa/state_tracker/st_texture.c      | 3 ++-
 3 files changed, 5 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
index ed9a444..19072ae 100644
--- a/src/mesa/state_tracker/st_atom_texture.c
+++ b/src/mesa/state_tracker/st_atom_texture.c
@@ -223,7 +223,7 @@  static unsigned last_level(struct st_texture_object *stObj)
 
 static unsigned last_layer(struct st_texture_object *stObj)
 {
-   if (stObj->base.Immutable)
+   if (stObj->base.Immutable && stObj->pt->array_size > 1)
       return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
                   stObj->pt->array_size - 1);
    return stObj->pt->array_size - 1;
diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
index 470ab27..7b6a444 100644
--- a/src/mesa/state_tracker/st_cb_fbo.c
+++ b/src/mesa/state_tracker/st_cb_fbo.c
@@ -451,7 +451,8 @@  st_update_renderbuffer_surface(struct st_context *st,
    }
 
    /* Adjust for texture views */
-   if (strb->is_rtt) {
+   if (strb->is_rtt && resource->array_size > 1 &&
+       strb->Base.TexImage->TexObject->Immutable) {
       struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
       first_layer += tex->MinLayer;
       if (!strb->rtt_layered)
diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
index c84aa45..2cd95ec 100644
--- a/src/mesa/state_tracker/st_texture.c
+++ b/src/mesa/state_tracker/st_texture.c
@@ -263,7 +263,8 @@  st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
    if (stObj->base.Immutable) {
       level += stObj->base.MinLevel;
       z += stObj->base.MinLayer;
-      d = MIN2(d, stObj->base.NumLayers);
+      if (stObj->pt->array_size > 1)
+         d = MIN2(d, stObj->base.NumLayers);
    }
 
    z += stImage->base.Face;

Comments

On 24.09.2014 14:01, Ilia Mirkin wrote:
> For 3d textures, NumLayers is set to 1, which is not what we want. This
> fixes the newly added gl-layer-render-storage test (which constructs
> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84145
> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>

Tested-by: Michel Dänzer <michel.daenzer@amd.com>
Maybe something similar also needs to be done for cubemaps, because
they are just layered textures in disguise?

Marek

On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> For 3d textures, NumLayers is set to 1, which is not what we want. This
> fixes the newly added gl-layer-render-storage test (which constructs
> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84145
> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
> ---
>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>  3 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
> index ed9a444..19072ae 100644
> --- a/src/mesa/state_tracker/st_atom_texture.c
> +++ b/src/mesa/state_tracker/st_atom_texture.c
> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>
>  static unsigned last_layer(struct st_texture_object *stObj)
>  {
> -   if (stObj->base.Immutable)
> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>                    stObj->pt->array_size - 1);
>     return stObj->pt->array_size - 1;
> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
> index 470ab27..7b6a444 100644
> --- a/src/mesa/state_tracker/st_cb_fbo.c
> +++ b/src/mesa/state_tracker/st_cb_fbo.c
> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>     }
>
>     /* Adjust for texture views */
> -   if (strb->is_rtt) {
> +   if (strb->is_rtt && resource->array_size > 1 &&
> +       strb->Base.TexImage->TexObject->Immutable) {
>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>        first_layer += tex->MinLayer;
>        if (!strb->rtt_layered)
> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
> index c84aa45..2cd95ec 100644
> --- a/src/mesa/state_tracker/st_texture.c
> +++ b/src/mesa/state_tracker/st_texture.c
> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>     if (stObj->base.Immutable) {
>        level += stObj->base.MinLevel;
>        z += stObj->base.MinLayer;
> -      d = MIN2(d, stObj->base.NumLayers);
> +      if (stObj->pt->array_size > 1)
> +         d = MIN2(d, stObj->base.NumLayers);
>     }
>
>     z += stImage->base.Face;
> --
> 1.8.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
The disguise of cubemap's layeredness is insufficient to trip up this
code :) They still get their NumLayers set, and the resources still
have an array size. Perhaps there's a scenario I'm not considering?

On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
> Maybe something similar also needs to be done for cubemaps, because
> they are just layered textures in disguise?
>
> Marek
>
> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>> fixes the newly added gl-layer-render-storage test (which constructs
>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84145
>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>> ---
>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>> index ed9a444..19072ae 100644
>> --- a/src/mesa/state_tracker/st_atom_texture.c
>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>
>>  static unsigned last_layer(struct st_texture_object *stObj)
>>  {
>> -   if (stObj->base.Immutable)
>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>                    stObj->pt->array_size - 1);
>>     return stObj->pt->array_size - 1;
>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>> index 470ab27..7b6a444 100644
>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>     }
>>
>>     /* Adjust for texture views */
>> -   if (strb->is_rtt) {
>> +   if (strb->is_rtt && resource->array_size > 1 &&
>> +       strb->Base.TexImage->TexObject->Immutable) {
>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>        first_layer += tex->MinLayer;
>>        if (!strb->rtt_layered)
>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>> index c84aa45..2cd95ec 100644
>> --- a/src/mesa/state_tracker/st_texture.c
>> +++ b/src/mesa/state_tracker/st_texture.c
>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>     if (stObj->base.Immutable) {
>>        level += stObj->base.MinLevel;
>>        z += stObj->base.MinLayer;
>> -      d = MIN2(d, stObj->base.NumLayers);
>> +      if (stObj->pt->array_size > 1)
>> +         d = MIN2(d, stObj->base.NumLayers);
>>     }
>>
>>     z += stImage->base.Face;
>> --
>> 1.8.5.5
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Cubemaps have array_size == 1, but you can still set the target to 2D
and set first_layer <= last_layer <= 6 in the sample view. Instead of
checking array_size, maybe NumLayers should be used instead. Just
guessing.

Marek

On Wed, Sep 24, 2014 at 5:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> The disguise of cubemap's layeredness is insufficient to trip up this
> code :) They still get their NumLayers set, and the resources still
> have an array size. Perhaps there's a scenario I'm not considering?
>
> On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
>> Maybe something similar also needs to be done for cubemaps, because
>> they are just layered textures in disguise?
>>
>> Marek
>>
>> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>>> fixes the newly added gl-layer-render-storage test (which constructs
>>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84145
>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>> ---
>>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>>> index ed9a444..19072ae 100644
>>> --- a/src/mesa/state_tracker/st_atom_texture.c
>>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>>
>>>  static unsigned last_layer(struct st_texture_object *stObj)
>>>  {
>>> -   if (stObj->base.Immutable)
>>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>>                    stObj->pt->array_size - 1);
>>>     return stObj->pt->array_size - 1;
>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>>> index 470ab27..7b6a444 100644
>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>>     }
>>>
>>>     /* Adjust for texture views */
>>> -   if (strb->is_rtt) {
>>> +   if (strb->is_rtt && resource->array_size > 1 &&
>>> +       strb->Base.TexImage->TexObject->Immutable) {
>>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>>        first_layer += tex->MinLayer;
>>>        if (!strb->rtt_layered)
>>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>>> index c84aa45..2cd95ec 100644
>>> --- a/src/mesa/state_tracker/st_texture.c
>>> +++ b/src/mesa/state_tracker/st_texture.c
>>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>>     if (stObj->base.Immutable) {
>>>        level += stObj->base.MinLevel;
>>>        z += stObj->base.MinLayer;
>>> -      d = MIN2(d, stObj->base.NumLayers);
>>> +      if (stObj->pt->array_size > 1)
>>> +         d = MIN2(d, stObj->base.NumLayers);
>>>     }
>>>
>>>     z += stImage->base.Face;
>>> --
>>> 1.8.5.5
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <maraeo@gmail.com> wrote:
> Cubemaps have array_size == 1, but you can still set the target to 2D

Are you *sure* about that? Everything I'm seeing indicates that
cubemaps have array_size == 6. For example this code in nv50_tex.c:

   depth = MAX2(mt->base.base.array_size, mt->base.base.depth0);
...
   case PIPE_TEXTURE_CUBE:
      depth /= 6;
...

> and set first_layer <= last_layer <= 6 in the sample view. Instead of
> checking array_size, maybe NumLayers should be used instead. Just
> guessing.
>
> Marek
>
> On Wed, Sep 24, 2014 at 5:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> The disguise of cubemap's layeredness is insufficient to trip up this
>> code :) They still get their NumLayers set, and the resources still
>> have an array size. Perhaps there's a scenario I'm not considering?
>>
>> On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>> Maybe something similar also needs to be done for cubemaps, because
>>> they are just layered textures in disguise?
>>>
>>> Marek
>>>
>>> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>>>> fixes the newly added gl-layer-render-storage test (which constructs
>>>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84145
>>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>>> ---
>>>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>>>> index ed9a444..19072ae 100644
>>>> --- a/src/mesa/state_tracker/st_atom_texture.c
>>>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>>>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>>>
>>>>  static unsigned last_layer(struct st_texture_object *stObj)
>>>>  {
>>>> -   if (stObj->base.Immutable)
>>>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>>>                    stObj->pt->array_size - 1);
>>>>     return stObj->pt->array_size - 1;
>>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>>>> index 470ab27..7b6a444 100644
>>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>>>     }
>>>>
>>>>     /* Adjust for texture views */
>>>> -   if (strb->is_rtt) {
>>>> +   if (strb->is_rtt && resource->array_size > 1 &&
>>>> +       strb->Base.TexImage->TexObject->Immutable) {
>>>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>>>        first_layer += tex->MinLayer;
>>>>        if (!strb->rtt_layered)
>>>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>>>> index c84aa45..2cd95ec 100644
>>>> --- a/src/mesa/state_tracker/st_texture.c
>>>> +++ b/src/mesa/state_tracker/st_texture.c
>>>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>>>     if (stObj->base.Immutable) {
>>>>        level += stObj->base.MinLevel;
>>>>        z += stObj->base.MinLayer;
>>>> -      d = MIN2(d, stObj->base.NumLayers);
>>>> +      if (stObj->pt->array_size > 1)
>>>> +         d = MIN2(d, stObj->base.NumLayers);
>>>>     }
>>>>
>>>>     z += stImage->base.Face;
>>>> --
>>>> 1.8.5.5
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Interesting, I didn't know about that. Nevermind. st/mesa indeed sets it to 6.

Marek

On Wed, Sep 24, 2014 at 6:26 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> Cubemaps have array_size == 1, but you can still set the target to 2D
>
> Are you *sure* about that? Everything I'm seeing indicates that
> cubemaps have array_size == 6. For example this code in nv50_tex.c:
>
>    depth = MAX2(mt->base.base.array_size, mt->base.base.depth0);
> ...
>    case PIPE_TEXTURE_CUBE:
>       depth /= 6;
> ...
>
>> and set first_layer <= last_layer <= 6 in the sample view. Instead of
>> checking array_size, maybe NumLayers should be used instead. Just
>> guessing.
>>
>> Marek
>>
>> On Wed, Sep 24, 2014 at 5:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>> The disguise of cubemap's layeredness is insufficient to trip up this
>>> code :) They still get their NumLayers set, and the resources still
>>> have an array size. Perhaps there's a scenario I'm not considering?
>>>
>>> On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>>> Maybe something similar also needs to be done for cubemaps, because
>>>> they are just layered textures in disguise?
>>>>
>>>> Marek
>>>>
>>>> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>>>>> fixes the newly added gl-layer-render-storage test (which constructs
>>>>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>>>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84145
>>>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>>>> ---
>>>>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>>>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>>>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>>>>> index ed9a444..19072ae 100644
>>>>> --- a/src/mesa/state_tracker/st_atom_texture.c
>>>>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>>>>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>>>>
>>>>>  static unsigned last_layer(struct st_texture_object *stObj)
>>>>>  {
>>>>> -   if (stObj->base.Immutable)
>>>>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>>>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>>>>                    stObj->pt->array_size - 1);
>>>>>     return stObj->pt->array_size - 1;
>>>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>>>>> index 470ab27..7b6a444 100644
>>>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>>>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>>>>     }
>>>>>
>>>>>     /* Adjust for texture views */
>>>>> -   if (strb->is_rtt) {
>>>>> +   if (strb->is_rtt && resource->array_size > 1 &&
>>>>> +       strb->Base.TexImage->TexObject->Immutable) {
>>>>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>>>>        first_layer += tex->MinLayer;
>>>>>        if (!strb->rtt_layered)
>>>>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>>>>> index c84aa45..2cd95ec 100644
>>>>> --- a/src/mesa/state_tracker/st_texture.c
>>>>> +++ b/src/mesa/state_tracker/st_texture.c
>>>>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>>>>     if (stObj->base.Immutable) {
>>>>>        level += stObj->base.MinLevel;
>>>>>        z += stObj->base.MinLayer;
>>>>> -      d = MIN2(d, stObj->base.NumLayers);
>>>>> +      if (stObj->pt->array_size > 1)
>>>>> +         d = MIN2(d, stObj->base.NumLayers);
>>>>>     }
>>>>>
>>>>>     z += stImage->base.Face;
>>>>> --
>>>>> 1.8.5.5
>>>>>
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Yes cubemaps should have array_size == 6 always in gallium. You just
have to be careful whenever translating things from mesa to gallium as
things like that won't be true in core mesa of course (similar to 1d
array textures having height and so on) due to OpenGL weirdness for
historical reasons.

Roland


Am 24.09.2014 19:19, schrieb Marek Olšák:
> Interesting, I didn't know about that. Nevermind. st/mesa indeed sets it to 6.
> 
> Marek
> 
> On Wed, Sep 24, 2014 at 6:26 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>> Cubemaps have array_size == 1, but you can still set the target to 2D
>>
>> Are you *sure* about that? Everything I'm seeing indicates that
>> cubemaps have array_size == 6. For example this code in nv50_tex.c:
>>
>>    depth = MAX2(mt->base.base.array_size, mt->base.base.depth0);
>> ...
>>    case PIPE_TEXTURE_CUBE:
>>       depth /= 6;
>> ...
>>
>>> and set first_layer <= last_layer <= 6 in the sample view. Instead of
>>> checking array_size, maybe NumLayers should be used instead. Just
>>> guessing.
>>>
>>> Marek
>>>
>>> On Wed, Sep 24, 2014 at 5:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>> The disguise of cubemap's layeredness is insufficient to trip up this
>>>> code :) They still get their NumLayers set, and the resources still
>>>> have an array size. Perhaps there's a scenario I'm not considering?
>>>>
>>>> On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> Maybe something similar also needs to be done for cubemaps, because
>>>>> they are just layered textures in disguise?
>>>>>
>>>>> Marek
>>>>>
>>>>> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>>>>>> fixes the newly added gl-layer-render-storage test (which constructs
>>>>>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>>>>>
>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugs.freedesktop.org/show_bug.cgi?id%3D84145&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=5599aec76e1694c8ec71dcfe6209603bf8b152884c942363fd84e9d56edaaa72
>>>>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>>>>> ---
>>>>>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>>>>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>>>>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>>>>>> index ed9a444..19072ae 100644
>>>>>> --- a/src/mesa/state_tracker/st_atom_texture.c
>>>>>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>>>>>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>>>>>
>>>>>>  static unsigned last_layer(struct st_texture_object *stObj)
>>>>>>  {
>>>>>> -   if (stObj->base.Immutable)
>>>>>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>>>>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>>>>>                    stObj->pt->array_size - 1);
>>>>>>     return stObj->pt->array_size - 1;
>>>>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>> index 470ab27..7b6a444 100644
>>>>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>>>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>>>>>     }
>>>>>>
>>>>>>     /* Adjust for texture views */
>>>>>> -   if (strb->is_rtt) {
>>>>>> +   if (strb->is_rtt && resource->array_size > 1 &&
>>>>>> +       strb->Base.TexImage->TexObject->Immutable) {
>>>>>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>>>>>        first_layer += tex->MinLayer;
>>>>>>        if (!strb->rtt_layered)
>>>>>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>>>>>> index c84aa45..2cd95ec 100644
>>>>>> --- a/src/mesa/state_tracker/st_texture.c
>>>>>> +++ b/src/mesa/state_tracker/st_texture.c
>>>>>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>>>>>     if (stObj->base.Immutable) {
>>>>>>        level += stObj->base.MinLevel;
>>>>>>        z += stObj->base.MinLayer;
>>>>>> -      d = MIN2(d, stObj->base.NumLayers);
>>>>>> +      if (stObj->pt->array_size > 1)
>>>>>> +         d = MIN2(d, stObj->base.NumLayers);
>>>>>>     }
>>>>>>
>>>>>>     z += stImage->base.Face;
>>>>>> --
>>>>>> 1.8.5.5
>>>>>>
>>>>>> _______________________________________________
>>>>>> mesa-dev mailing list
>>>>>> mesa-dev@lists.freedesktop.org
>>>>>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/mesa-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=da68def8d99a0b8cd601334ed2b794d24bcb8e278d84e5a8aa06cc21afc5967d
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/mesa-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=da68def8d99a0b8cd601334ed2b794d24bcb8e278d84e5a8aa06cc21afc5967d
>
Marek/Roland -- do either of those comments count as a R-b? I'd like
to push this out tonight, pending a full piglit run.

On Wed, Sep 24, 2014 at 1:35 PM, Roland Scheidegger <sroland@vmware.com> wrote:
> Yes cubemaps should have array_size == 6 always in gallium. You just
> have to be careful whenever translating things from mesa to gallium as
> things like that won't be true in core mesa of course (similar to 1d
> array textures having height and so on) due to OpenGL weirdness for
> historical reasons.
>
> Roland
>
>
> Am 24.09.2014 19:19, schrieb Marek Olšák:
>> Interesting, I didn't know about that. Nevermind. st/mesa indeed sets it to 6.
>>
>> Marek
>>
>> On Wed, Sep 24, 2014 at 6:26 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>> On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>> Cubemaps have array_size == 1, but you can still set the target to 2D
>>>
>>> Are you *sure* about that? Everything I'm seeing indicates that
>>> cubemaps have array_size == 6. For example this code in nv50_tex.c:
>>>
>>>    depth = MAX2(mt->base.base.array_size, mt->base.base.depth0);
>>> ...
>>>    case PIPE_TEXTURE_CUBE:
>>>       depth /= 6;
>>> ...
>>>
>>>> and set first_layer <= last_layer <= 6 in the sample view. Instead of
>>>> checking array_size, maybe NumLayers should be used instead. Just
>>>> guessing.
>>>>
>>>> Marek
>>>>
>>>> On Wed, Sep 24, 2014 at 5:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>> The disguise of cubemap's layeredness is insufficient to trip up this
>>>>> code :) They still get their NumLayers set, and the resources still
>>>>> have an array size. Perhaps there's a scenario I'm not considering?
>>>>>
>>>>> On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>> Maybe something similar also needs to be done for cubemaps, because
>>>>>> they are just layered textures in disguise?
>>>>>>
>>>>>> Marek
>>>>>>
>>>>>> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>>>>>>> fixes the newly added gl-layer-render-storage test (which constructs
>>>>>>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>>>>>>
>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugs.freedesktop.org/show_bug.cgi?id%3D84145&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=5599aec76e1694c8ec71dcfe6209603bf8b152884c942363fd84e9d56edaaa72
>>>>>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>>>>>> ---
>>>>>>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>>>>>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>>>>>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>>>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>>>>>>> index ed9a444..19072ae 100644
>>>>>>> --- a/src/mesa/state_tracker/st_atom_texture.c
>>>>>>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>>>>>>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>>>>>>
>>>>>>>  static unsigned last_layer(struct st_texture_object *stObj)
>>>>>>>  {
>>>>>>> -   if (stObj->base.Immutable)
>>>>>>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>>>>>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>>>>>>                    stObj->pt->array_size - 1);
>>>>>>>     return stObj->pt->array_size - 1;
>>>>>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>> index 470ab27..7b6a444 100644
>>>>>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>>>>>>     }
>>>>>>>
>>>>>>>     /* Adjust for texture views */
>>>>>>> -   if (strb->is_rtt) {
>>>>>>> +   if (strb->is_rtt && resource->array_size > 1 &&
>>>>>>> +       strb->Base.TexImage->TexObject->Immutable) {
>>>>>>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>>>>>>        first_layer += tex->MinLayer;
>>>>>>>        if (!strb->rtt_layered)
>>>>>>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>>>>>>> index c84aa45..2cd95ec 100644
>>>>>>> --- a/src/mesa/state_tracker/st_texture.c
>>>>>>> +++ b/src/mesa/state_tracker/st_texture.c
>>>>>>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>>>>>>     if (stObj->base.Immutable) {
>>>>>>>        level += stObj->base.MinLevel;
>>>>>>>        z += stObj->base.MinLayer;
>>>>>>> -      d = MIN2(d, stObj->base.NumLayers);
>>>>>>> +      if (stObj->pt->array_size > 1)
>>>>>>> +         d = MIN2(d, stObj->base.NumLayers);
>>>>>>>     }
>>>>>>>
>>>>>>>     z += stImage->base.Face;
>>>>>>> --
>>>>>>> 1.8.5.5
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> mesa-dev mailing list
>>>>>>> mesa-dev@lists.freedesktop.org
>>>>>>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/mesa-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=da68def8d99a0b8cd601334ed2b794d24bcb8e278d84e5a8aa06cc21afc5967d
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/mesa-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=da68def8d99a0b8cd601334ed2b794d24bcb8e278d84e5a8aa06cc21afc5967d
>>
>
I don't really qualified to review, IIRC I mentioned it was tricky to
see if it's right when you pushed it first, and this has not changed.
Some comment inline though...


Am 24.09.2014 20:30, schrieb Ilia Mirkin:
> Marek/Roland -- do either of those comments count as a R-b? I'd like
> to push this out tonight, pending a full piglit run.
> 
> On Wed, Sep 24, 2014 at 1:35 PM, Roland Scheidegger <sroland@vmware.com> wrote:
>> Yes cubemaps should have array_size == 6 always in gallium. You just
>> have to be careful whenever translating things from mesa to gallium as
>> things like that won't be true in core mesa of course (similar to 1d
>> array textures having height and so on) due to OpenGL weirdness for
>> historical reasons.
>>
>> Roland
>>
>>
>> Am 24.09.2014 19:19, schrieb Marek Olšák:
>>> Interesting, I didn't know about that. Nevermind. st/mesa indeed sets it to 6.
>>>
>>> Marek
>>>
>>> On Wed, Sep 24, 2014 at 6:26 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>> On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> Cubemaps have array_size == 1, but you can still set the target to 2D
>>>>
>>>> Are you *sure* about that? Everything I'm seeing indicates that
>>>> cubemaps have array_size == 6. For example this code in nv50_tex.c:
>>>>
>>>>    depth = MAX2(mt->base.base.array_size, mt->base.base.depth0);
>>>> ...
>>>>    case PIPE_TEXTURE_CUBE:
>>>>       depth /= 6;
>>>> ...
>>>>
>>>>> and set first_layer <= last_layer <= 6 in the sample view. Instead of
>>>>> checking array_size, maybe NumLayers should be used instead. Just
>>>>> guessing.
>>>>>
>>>>> Marek
>>>>>
>>>>> On Wed, Sep 24, 2014 at 5:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>> The disguise of cubemap's layeredness is insufficient to trip up this
>>>>>> code :) They still get their NumLayers set, and the resources still
>>>>>> have an array size. Perhaps there's a scenario I'm not considering?
>>>>>>
>>>>>> On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>> Maybe something similar also needs to be done for cubemaps, because
>>>>>>> they are just layered textures in disguise?
>>>>>>>
>>>>>>> Marek
>>>>>>>
>>>>>>> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>>>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>>>>>>>> fixes the newly added gl-layer-render-storage test (which constructs
>>>>>>>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>>>>>>>
>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugs.freedesktop.org/show_bug.cgi?id%3D84145&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=5599aec76e1694c8ec71dcfe6209603bf8b152884c942363fd84e9d56edaaa72
>>>>>>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>>>>>>> ---
>>>>>>>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>>>>>>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>>>>>>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>>>>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>> index ed9a444..19072ae 100644
>>>>>>>> --- a/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>>>>>>>
>>>>>>>>  static unsigned last_layer(struct st_texture_object *stObj)
>>>>>>>>  {
>>>>>>>> -   if (stObj->base.Immutable)
>>>>>>>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>>>>>>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>>>>>>>                    stObj->pt->array_size - 1);
>>>>>>>>     return stObj->pt->array_size - 1;
>>>>>>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>> index 470ab27..7b6a444 100644
>>>>>>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     /* Adjust for texture views */
>>>>>>>> -   if (strb->is_rtt) {
>>>>>>>> +   if (strb->is_rtt && resource->array_size > 1 &&
>>>>>>>> +       strb->Base.TexImage->TexObject->Immutable) {
>>>>>>>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>>>>>>>        first_layer += tex->MinLayer;
>>>>>>>>        if (!strb->rtt_layered)
>>>>>>>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>>>>>>>> index c84aa45..2cd95ec 100644
>>>>>>>> --- a/src/mesa/state_tracker/st_texture.c
>>>>>>>> +++ b/src/mesa/state_tracker/st_texture.c
>>>>>>>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>>>>>>>     if (stObj->base.Immutable) {
>>>>>>>>        level += stObj->base.MinLevel;
>>>>>>>>        z += stObj->base.MinLayer;
>>>>>>>> -      d = MIN2(d, stObj->base.NumLayers);
>>>>>>>> +      if (stObj->pt->array_size > 1)
>>>>>>>> +         d = MIN2(d, stObj->base.NumLayers);
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     z += stImage->base.Face;
This is pretty confusing, so for a cubemap you get the initial z
(presumably zero), add the MinLayer for the view, then also the Face
from the initial image? This isn't new but all the translation from mesa
seems really tricky here and elsewhere.
The fix though is probably correct but really I can't tell...
But as long as it fixes things go for it...

Roland


>>>>>>>> --
>>>>>>>> 1.8.5.5
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> mesa-dev mailing list
>>>>>>>> mesa-dev@lists.freedesktop.org
>>>>>>>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/mesa-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=da68def8d99a0b8cd601334ed2b794d24bcb8e278d84e5a8aa06cc21afc5967d
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/mesa-dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=da68def8d99a0b8cd601334ed2b794d24bcb8e278d84e5a8aa06cc21afc5967d
>>>
>>
On Wed, Sep 24, 2014 at 5:17 PM, Roland Scheidegger <sroland@vmware.com> wrote:
> I don't really qualified to review, IIRC I mentioned it was tricky to
> see if it's right when you pushed it first, and this has not changed.
> Some comment inline though...
>
>
> Am 24.09.2014 20:30, schrieb Ilia Mirkin:
>> Marek/Roland -- do either of those comments count as a R-b? I'd like
>> to push this out tonight, pending a full piglit run.
>>
>> On Wed, Sep 24, 2014 at 1:35 PM, Roland Scheidegger <sroland@vmware.com> wrote:
>>> Yes cubemaps should have array_size == 6 always in gallium. You just
>>> have to be careful whenever translating things from mesa to gallium as
>>> things like that won't be true in core mesa of course (similar to 1d
>>> array textures having height and so on) due to OpenGL weirdness for
>>> historical reasons.
>>>
>>> Roland
>>>
>>>
>>> Am 24.09.2014 19:19, schrieb Marek Olšák:
>>>> Interesting, I didn't know about that. Nevermind. st/mesa indeed sets it to 6.
>>>>
>>>> Marek
>>>>
>>>> On Wed, Sep 24, 2014 at 6:26 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>> On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>> Cubemaps have array_size == 1, but you can still set the target to 2D
>>>>>
>>>>> Are you *sure* about that? Everything I'm seeing indicates that
>>>>> cubemaps have array_size == 6. For example this code in nv50_tex.c:
>>>>>
>>>>>    depth = MAX2(mt->base.base.array_size, mt->base.base.depth0);
>>>>> ...
>>>>>    case PIPE_TEXTURE_CUBE:
>>>>>       depth /= 6;
>>>>> ...
>>>>>
>>>>>> and set first_layer <= last_layer <= 6 in the sample view. Instead of
>>>>>> checking array_size, maybe NumLayers should be used instead. Just
>>>>>> guessing.
>>>>>>
>>>>>> Marek
>>>>>>
>>>>>> On Wed, Sep 24, 2014 at 5:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>>> The disguise of cubemap's layeredness is insufficient to trip up this
>>>>>>> code :) They still get their NumLayers set, and the resources still
>>>>>>> have an array size. Perhaps there's a scenario I'm not considering?
>>>>>>>
>>>>>>> On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>>> Maybe something similar also needs to be done for cubemaps, because
>>>>>>>> they are just layered textures in disguise?
>>>>>>>>
>>>>>>>> Marek
>>>>>>>>
>>>>>>>> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>>>>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>>>>>>>>> fixes the newly added gl-layer-render-storage test (which constructs
>>>>>>>>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>>>>>>>>
>>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugs.freedesktop.org/show_bug.cgi?id%3D84145&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=5599aec76e1694c8ec71dcfe6209603bf8b152884c942363fd84e9d56edaaa72
>>>>>>>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>>>>>>>> ---
>>>>>>>>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>>>>>>>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>>>>>>>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>>>>>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>>> index ed9a444..19072ae 100644
>>>>>>>>> --- a/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>>>>>>>>
>>>>>>>>>  static unsigned last_layer(struct st_texture_object *stObj)
>>>>>>>>>  {
>>>>>>>>> -   if (stObj->base.Immutable)
>>>>>>>>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>>>>>>>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>>>>>>>>                    stObj->pt->array_size - 1);
>>>>>>>>>     return stObj->pt->array_size - 1;
>>>>>>>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>>> index 470ab27..7b6a444 100644
>>>>>>>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>>>>>>>>     }
>>>>>>>>>
>>>>>>>>>     /* Adjust for texture views */
>>>>>>>>> -   if (strb->is_rtt) {
>>>>>>>>> +   if (strb->is_rtt && resource->array_size > 1 &&
>>>>>>>>> +       strb->Base.TexImage->TexObject->Immutable) {
>>>>>>>>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>>>>>>>>        first_layer += tex->MinLayer;
>>>>>>>>>        if (!strb->rtt_layered)
>>>>>>>>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>>>>>>>>> index c84aa45..2cd95ec 100644
>>>>>>>>> --- a/src/mesa/state_tracker/st_texture.c
>>>>>>>>> +++ b/src/mesa/state_tracker/st_texture.c
>>>>>>>>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>>>>>>>>     if (stObj->base.Immutable) {
>>>>>>>>>        level += stObj->base.MinLevel;
>>>>>>>>>        z += stObj->base.MinLayer;
>>>>>>>>> -      d = MIN2(d, stObj->base.NumLayers);
>>>>>>>>> +      if (stObj->pt->array_size > 1)
>>>>>>>>> +         d = MIN2(d, stObj->base.NumLayers);
>>>>>>>>>     }
>>>>>>>>>
>>>>>>>>>     z += stImage->base.Face;
> This is pretty confusing, so for a cubemap you get the initial z
> (presumably zero), add the MinLayer for the view, then also the Face
> from the initial image? This isn't new but all the translation from mesa
> seems really tricky here and elsewhere.
> The fix though is probably correct but really I can't tell...
> But as long as it fixes things go for it...

I think the key is that mesa (supposedly) prevents illegal situations
from happening. The above didn't actually fix anything, but I audited
all NumLayers usages to ensure that they were done only for array
textures, and this was one of the usages [that my previous patch
introduced].

I don't think your comment is directly related to my patch, but
imagine the following scenario:

2d array with 10 layers, cast as a cube map view for layers 2..7. Then
you want to get the 1 (-x? I forget) face from it. That'll end up
being layer 3 in the original texture. Which is what the above code
will end up doing, I believe.

  -ilia
Am 24.09.2014 23:23, schrieb Ilia Mirkin:
> On Wed, Sep 24, 2014 at 5:17 PM, Roland Scheidegger <sroland@vmware.com> wrote:
>> I don't really qualified to review, IIRC I mentioned it was tricky to
>> see if it's right when you pushed it first, and this has not changed.
>> Some comment inline though...
>>
>>
>> Am 24.09.2014 20:30, schrieb Ilia Mirkin:
>>> Marek/Roland -- do either of those comments count as a R-b? I'd like
>>> to push this out tonight, pending a full piglit run.
>>>
>>> On Wed, Sep 24, 2014 at 1:35 PM, Roland Scheidegger <sroland@vmware.com> wrote:
>>>> Yes cubemaps should have array_size == 6 always in gallium. You just
>>>> have to be careful whenever translating things from mesa to gallium as
>>>> things like that won't be true in core mesa of course (similar to 1d
>>>> array textures having height and so on) due to OpenGL weirdness for
>>>> historical reasons.
>>>>
>>>> Roland
>>>>
>>>>
>>>> Am 24.09.2014 19:19, schrieb Marek Olšák:
>>>>> Interesting, I didn't know about that. Nevermind. st/mesa indeed sets it to 6.
>>>>>
>>>>> Marek
>>>>>
>>>>> On Wed, Sep 24, 2014 at 6:26 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>> On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>> Cubemaps have array_size == 1, but you can still set the target to 2D
>>>>>>
>>>>>> Are you *sure* about that? Everything I'm seeing indicates that
>>>>>> cubemaps have array_size == 6. For example this code in nv50_tex.c:
>>>>>>
>>>>>>    depth = MAX2(mt->base.base.array_size, mt->base.base.depth0);
>>>>>> ...
>>>>>>    case PIPE_TEXTURE_CUBE:
>>>>>>       depth /= 6;
>>>>>> ...
>>>>>>
>>>>>>> and set first_layer <= last_layer <= 6 in the sample view. Instead of
>>>>>>> checking array_size, maybe NumLayers should be used instead. Just
>>>>>>> guessing.
>>>>>>>
>>>>>>> Marek
>>>>>>>
>>>>>>> On Wed, Sep 24, 2014 at 5:05 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>>>> The disguise of cubemap's layeredness is insufficient to trip up this
>>>>>>>> code :) They still get their NumLayers set, and the resources still
>>>>>>>> have an array size. Perhaps there's a scenario I'm not considering?
>>>>>>>>
>>>>>>>> On Wed, Sep 24, 2014 at 5:23 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>>>> Maybe something similar also needs to be done for cubemaps, because
>>>>>>>>> they are just layered textures in disguise?
>>>>>>>>>
>>>>>>>>> Marek
>>>>>>>>>
>>>>>>>>> On Wed, Sep 24, 2014 at 7:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>>>>>>>> For 3d textures, NumLayers is set to 1, which is not what we want. This
>>>>>>>>>> fixes the newly added gl-layer-render-storage test (which constructs
>>>>>>>>>> immutable 3d textures). Fixes regression introduced in d82bd7eb060.
>>>>>>>>>>
>>>>>>>>>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugs.freedesktop.org/show_bug.cgi?id%3D84145&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=p4L3w5mFWeprslDAPtBcySi4H8PhKEdqz0pOU77lcjg%3D%0A&s=5599aec76e1694c8ec71dcfe6209603bf8b152884c942363fd84e9d56edaaa72
>>>>>>>>>> Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
>>>>>>>>>> ---
>>>>>>>>>>  src/mesa/state_tracker/st_atom_texture.c | 2 +-
>>>>>>>>>>  src/mesa/state_tracker/st_cb_fbo.c       | 3 ++-
>>>>>>>>>>  src/mesa/state_tracker/st_texture.c      | 3 ++-
>>>>>>>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/src/mesa/state_tracker/st_atom_texture.c b/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>>>> index ed9a444..19072ae 100644
>>>>>>>>>> --- a/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>>>> +++ b/src/mesa/state_tracker/st_atom_texture.c
>>>>>>>>>> @@ -223,7 +223,7 @@ static unsigned last_level(struct st_texture_object *stObj)
>>>>>>>>>>
>>>>>>>>>>  static unsigned last_layer(struct st_texture_object *stObj)
>>>>>>>>>>  {
>>>>>>>>>> -   if (stObj->base.Immutable)
>>>>>>>>>> +   if (stObj->base.Immutable && stObj->pt->array_size > 1)
>>>>>>>>>>        return MIN2(stObj->base.MinLayer + stObj->base.NumLayers - 1,
>>>>>>>>>>                    stObj->pt->array_size - 1);
>>>>>>>>>>     return stObj->pt->array_size - 1;
>>>>>>>>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>>>> index 470ab27..7b6a444 100644
>>>>>>>>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>>>>>>>>> @@ -451,7 +451,8 @@ st_update_renderbuffer_surface(struct st_context *st,
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     /* Adjust for texture views */
>>>>>>>>>> -   if (strb->is_rtt) {
>>>>>>>>>> +   if (strb->is_rtt && resource->array_size > 1 &&
>>>>>>>>>> +       strb->Base.TexImage->TexObject->Immutable) {
>>>>>>>>>>        struct gl_texture_object *tex = strb->Base.TexImage->TexObject;
>>>>>>>>>>        first_layer += tex->MinLayer;
>>>>>>>>>>        if (!strb->rtt_layered)
>>>>>>>>>> diff --git a/src/mesa/state_tracker/st_texture.c b/src/mesa/state_tracker/st_texture.c
>>>>>>>>>> index c84aa45..2cd95ec 100644
>>>>>>>>>> --- a/src/mesa/state_tracker/st_texture.c
>>>>>>>>>> +++ b/src/mesa/state_tracker/st_texture.c
>>>>>>>>>> @@ -263,7 +263,8 @@ st_texture_image_map(struct st_context *st, struct st_texture_image *stImage,
>>>>>>>>>>     if (stObj->base.Immutable) {
>>>>>>>>>>        level += stObj->base.MinLevel;
>>>>>>>>>>        z += stObj->base.MinLayer;
>>>>>>>>>> -      d = MIN2(d, stObj->base.NumLayers);
>>>>>>>>>> +      if (stObj->pt->array_size > 1)
>>>>>>>>>> +         d = MIN2(d, stObj->base.NumLayers);
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>>     z += stImage->base.Face;
>> This is pretty confusing, so for a cubemap you get the initial z
>> (presumably zero), add the MinLayer for the view, then also the Face
>> from the initial image? This isn't new but all the translation from mesa
>> seems really tricky here and elsewhere.
>> The fix though is probably correct but really I can't tell...
>> But as long as it fixes things go for it...
> 
> I think the key is that mesa (supposedly) prevents illegal situations
> from happening. The above didn't actually fix anything, but I audited
> all NumLayers usages to ensure that they were done only for array
> textures, and this was one of the usages [that my previous patch
> introduced].
> 
> I don't think your comment is directly related to my patch, but
> imagine the following scenario:
> 
> 2d array with 10 layers, cast as a cube map view for layers 2..7. Then
> you want to get the 1 (-x? I forget) face from it. That'll end up
> being layer 3 in the original texture. Which is what the above code
> will end up doing, I believe.
> 

That makes sense indeed. I am just always confused because array
textures and cube maps are treated so differently, even though the
storage representation really ought to be the same. All due to
historical reasons...

Roland