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

[Roland Scheidegger]

Am 24.09.2014 23:23, schrieb Ilia Mirkin:
> On Wed, Sep 24, 2014 at 5:17 PM, Roland Scheidegger <sroland at 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 at 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 at alum.mit.edu> wrote:
>>>>>> On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <maraeo at 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 at 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 at 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 at 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 at 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