<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Nov 14, 2018 at 6:02 AM Danylo Piliaiev <<a href="mailto:danylo.piliaiev@gmail.com">danylo.piliaiev@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 </blockquote><div> </div><div>Right. Would you mind adding</div><div> </div><div>assert(obj->Immutable || obj->MinLayer == 0);</div><div> </div><div>With that,</div><div> </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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 <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: >> On Mon, Sep 10, 2018 at 10:21 AM Danylo Piliaiev <<a href="mailto:danylo.piliaiev@gmail.com" target="_blank">danylo.piliaiev@gmail.com</a>> 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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=107856" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=107856</a> >>> >>> Signed-off-by: Danylo Piliaiev <<a href="mailto:danylo.piliaiev@globallogic.com" target="_blank">danylo.piliaiev@globallogic.com</a>> >>> --- >>> .../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 </blockquote></div></div>