[Mesa-dev] [PATCH v3] i965: Fix calculation of layers array length for isl_view

[Jason Ekstrand]

On Wed, Nov 14, 2018 at 6:02 AM Danylo Piliaiev <danylo.piliaiev at gmail.com>
wrote:

> Hi Ilia, Jason,
>
> All these cases are indeed could induce confusion,
> I'll try to explain all of them:
>
> Texture is not layered:
>      Layers: 1
>      Base Layer: %layer of image unit% + %layer of texture view%
>
> Layered 3D texture:
>      Layers: Entire level is bound, texture view does not affect layers
> of it
>      Base Layer: 0
>
> Layered non 3D texture (Array):
>      Immutable (it's a texture view, see _mesa_set_texture_view_state):
>          Layers: correct values of MinLayer and NumLayers is baked into
> texObject in texture_view and in _mesa_set_texture_view_state
>          Base Layer: MinLayer
>
>      Mutable (MinLayer is always 0 since it can be set only via texture
> view which makes texture immutable):
>          Layers: array_len
>          Base Layer: 0
>

Right.  Would you mind adding

assert(obj->Immutable || obj->MinLayer == 0);

With that,

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>


> So while the code from my pov should work it could be presented better.
> However I'm not sure what will be better:
>
>      } else if (obj->Immutable) {
>          base_layer = obj->MinLayer;
>          num_layers = obj->NumLayers;
>      } else {
>          base_layer = 0;
>          num_layers = mt->surf.logical_level0_px.array_len;
>      }
>
> Or always calculate base_layer as "obj->MinLayer + u->_Layer" before the
> conditions,
>      which will look slightly misleading and induce questions of why we
> don't subtract them from array_len.
>
> What do you think?
>
> Thanks!
>
> On 11/14/18 12:01 AM, Ilia Mirkin wrote:
> > On Tue, Nov 13, 2018 at 4:53 PM Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> >> On Mon, Sep 10, 2018 at 10:21 AM Danylo Piliaiev <
> danylo.piliaiev at gmail.com> wrote:
> >>> Handle all cases in calculation of layers count for isl_view
> >>> taking into account texture view and image unit.
> >>> st_convert_image was taken as a reference.
> >>>
> >>> When u->Layered is true the whole level is taken with respect to
> >>> image view. In other case only one layer is taken.
> >>>
> >>> v3: (Józef Kucia and Ilia Mirkin)
> >>>      - Rewrote patch by taking st_convert_image as a reference
> >>>      - Removed now unused get_image_num_layers function
> >>>      - Changed commit message
> >>>
> >>> Fixes: 5a8c8903
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107856
> >>>
> >>> Signed-off-by: Danylo Piliaiev <danylo.piliaiev at globallogic.com>
> >>> ---
> >>>   .../drivers/dri/i965/brw_wm_surface_state.c   | 32
> ++++++++++---------
> >>>   1 file changed, 17 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >>> index 944762ec46..9bfe6e2037 100644
> >>> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >>> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >>> @@ -1499,18 +1499,6 @@ update_buffer_image_param(struct brw_context
> *brw,
> >>>      param->stride[0] = _mesa_get_format_bytes(u->_ActualFormat);
> >>>   }
> >>>
> >>> -static unsigned
> >>> -get_image_num_layers(const struct intel_mipmap_tree *mt, GLenum
> target,
> >>> -                     unsigned level)
> >>> -{
> >>> -   if (target == GL_TEXTURE_CUBE_MAP)
> >>> -      return 6;
> >>> -
> >>> -   return target == GL_TEXTURE_3D ?
> >>> -      minify(mt->surf.logical_level0_px.depth, level) :
> >>> -      mt->surf.logical_level0_px.array_len;
> >>> -}
> >>> -
> >>>   static void
> >>>   update_image_surface(struct brw_context *brw,
> >>>                        struct gl_image_unit *u,
> >>> @@ -1541,14 +1529,28 @@ update_image_surface(struct brw_context *brw,
> >>>         } else {
> >>>            struct intel_texture_object *intel_obj =
> intel_texture_object(obj);
> >>>            struct intel_mipmap_tree *mt = intel_obj->mt;
> >>> -         const unsigned num_layers = u->Layered ?
> >>> -            get_image_num_layers(mt, obj->Target, u->Level) : 1;
> >>> +
> >>> +         unsigned base_layer, num_layers;
> >>> +         if (u->Layered) {
> >>> +            if (obj->Target == GL_TEXTURE_3D) {
> >>> +               base_layer = 0;
> >>> +               num_layers = minify(mt->surf.logical_level0_px.depth,
> u->Level);
> >>> +            } else {
> >>> +               base_layer = obj->MinLayer;
> >>> +               num_layers = obj->Immutable ?
> >>> +                                obj->NumLayers :
> >>> +                                mt->surf.logical_level0_px.array_len;
> >>
> >> Doesn't this need to be array_len - base_layer?  I'm not sure on the
> others without digging.
> > Probably not intuitively obvious, but MinLayer/NumLayers are only set
> > for Immutable textures.
> >
> >    -ilia
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181114/520e75dd/attachment-0001.html>