Am 24.09.2014 23:23, schrieb Ilia Mirkin: > On Wed, Sep 24, 2014 at 5:17 PM, Roland Scheidegger <srol...@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 <srol...@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 <imir...@alum.mit.edu> wrote: >>>>>> On Wed, Sep 24, 2014 at 12:20 PM, Marek Olšák <mar...@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 <imir...@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 <mar...@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 <imir...@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 <imir...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
