Re: [Mesa-dev] [PATCH] i965: Fix calculation of layers array length for isl_view
Thank you Józef and Ilia for pointing this out! I believe I have fully understood what's happening here and I'll send a final version of the fix soon. On 9/7/18 7:47 PM, Józef Kucia wrote: On Fri, Sep 7, 2018 at 6:44 PM Ilia Mirkin wrote: On Fri, Sep 7, 2018 at 12:35 PM, Józef Kucia wrote: On Fri, Sep 7, 2018 at 4:42 PM Danylo Piliaiev wrote: @@ -1546,8 +1548,8 @@ update_image_surface(struct brw_context *brw, .format = format, .base_level = obj->MinLevel + u->Level, .levels = 1, -.base_array_layer = obj->MinLayer + u->_Layer, -.array_len = num_layers, +.base_array_layer = base_layer, +.array_len = num_layers - base_layer, .swizzle = ISL_SWIZZLE_IDENTITY, .usage = ISL_SURF_USAGE_STORAGE_BIT, }; This sets the "array_len" to the number of layers remaining in the original texture. Shouldn't it take into account the number of layers in the GL texture view? Errr, right. Here is the logic in st/mesa, which I believe is correct. (But convoluted. Because there are so many bits to it.) https://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/state_tracker/st_atom_image.c#n101 There is also view_num_layers in brw_update_texture_surface(). It could be adapted. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix calculation of layers array length for isl_view
On Fri, Sep 7, 2018 at 6:44 PM Ilia Mirkin wrote: > > On Fri, Sep 7, 2018 at 12:35 PM, Józef Kucia wrote: > > On Fri, Sep 7, 2018 at 4:42 PM Danylo Piliaiev > > wrote: > > > >> @@ -1546,8 +1548,8 @@ update_image_surface(struct brw_context *brw, > >> .format = format, > >> .base_level = obj->MinLevel + u->Level, > >> .levels = 1, > >> -.base_array_layer = obj->MinLayer + u->_Layer, > >> -.array_len = num_layers, > >> +.base_array_layer = base_layer, > >> +.array_len = num_layers - base_layer, > >> .swizzle = ISL_SWIZZLE_IDENTITY, > >> .usage = ISL_SURF_USAGE_STORAGE_BIT, > >> }; > > > > This sets the "array_len" to the number of layers remaining in the > > original texture. Shouldn't it take into account the number of layers > > in the GL texture view? > > Errr, right. Here is the logic in st/mesa, which I believe is correct. > (But convoluted. Because there are so many bits to it.) > > https://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/state_tracker/st_atom_image.c#n101 There is also view_num_layers in brw_update_texture_surface(). It could be adapted. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix calculation of layers array length for isl_view
On Fri, Sep 7, 2018 at 12:35 PM, Józef Kucia wrote: > On Fri, Sep 7, 2018 at 4:42 PM Danylo Piliaiev > wrote: > >> @@ -1546,8 +1548,8 @@ update_image_surface(struct brw_context *brw, >> .format = format, >> .base_level = obj->MinLevel + u->Level, >> .levels = 1, >> -.base_array_layer = obj->MinLayer + u->_Layer, >> -.array_len = num_layers, >> +.base_array_layer = base_layer, >> +.array_len = num_layers - base_layer, >> .swizzle = ISL_SWIZZLE_IDENTITY, >> .usage = ISL_SURF_USAGE_STORAGE_BIT, >> }; > > This sets the "array_len" to the number of layers remaining in the > original texture. Shouldn't it take into account the number of layers > in the GL texture view? Errr, right. Here is the logic in st/mesa, which I believe is correct. (But convoluted. Because there are so many bits to it.) https://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/state_tracker/st_atom_image.c#n101 -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix calculation of layers array length for isl_view
On Fri, Sep 7, 2018 at 4:42 PM Danylo Piliaiev wrote: > @@ -1546,8 +1548,8 @@ update_image_surface(struct brw_context *brw, > .format = format, > .base_level = obj->MinLevel + u->Level, > .levels = 1, > -.base_array_layer = obj->MinLayer + u->_Layer, > -.array_len = num_layers, > +.base_array_layer = base_layer, > +.array_len = num_layers - base_layer, > .swizzle = ISL_SWIZZLE_IDENTITY, > .usage = ISL_SURF_USAGE_STORAGE_BIT, > }; This sets the "array_len" to the number of layers remaining in the original texture. Shouldn't it take into account the number of layers in the GL texture view? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix calculation of layers array length for isl_view
On 07/09/2018 16:32, Ilia Mirkin wrote:
On Fri, Sep 7, 2018 at 11:09 AM, Danylo Piliaiev
wrote:
On 9/7/18 5:48 PM, Ilia Mirkin wrote:
On Fri, Sep 7, 2018 at 10:41 AM, Danylo Piliaiev
wrote:
Comment for array_len field states:
"Indicates the number of array elements starting at
Base Array Layer."
And most usages of array_len expect it to be equal or less than
total layers - base layer
Fixes: 5a8c8903
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107856
Signed-off-by: Danylo Piliaiev
---
src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 6 --
1 file changed, 4 insertions(+), 2 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 42af41aca3..6adf4a5836 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -1539,6 +1539,8 @@ 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 base_layer = obj->MinLayer + u->_Layer;
const unsigned num_layers = u->Layered ?
get_image_num_layers(mt, obj->Target, u->Level) : 1;
@@ -1546,8 +1548,8 @@ update_image_surface(struct brw_context *brw,
.format = format,
.base_level = obj->MinLevel + u->Level,
.levels = 1,
-.base_array_layer = obj->MinLayer + u->_Layer,
-.array_len = num_layers,
+.base_array_layer = base_layer,
+.array_len = num_layers - base_layer,
See above - num_layers can be 1 if the image isn't bound as a layered
image. But base layer can be whatever -- so this will end up as
negative. I think the adjustment needs to be done only for the
u->Layered case.
Oh, I see it now, thanks! Unless Lionel's patch for this issue is better
I'll send v2 of my patch.
I believe yours is much closer to right. Lionel's was conceptually
wrong. (Or I'm the one who's confused - reminder - I'm not an Intel
driver developer, and ultimately won't approve or reject your patches.
But I will point out things that I think are off.)
-ilia
_
Yep, looks like I don't really understand the relationship between LOD,
depth and layers...
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix calculation of layers array length for isl_view
On Fri, Sep 7, 2018 at 11:09 AM, Danylo Piliaiev
wrote:
>
> On 9/7/18 5:48 PM, Ilia Mirkin wrote:
>>
>> On Fri, Sep 7, 2018 at 10:41 AM, Danylo Piliaiev
>> wrote:
>>>
>>> Comment for array_len field states:
>>> "Indicates the number of array elements starting at
>>> Base Array Layer."
>>>
>>> And most usages of array_len expect it to be equal or less than
>>> total layers - base layer
>>>
>>> Fixes: 5a8c8903
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107856
>>>
>>> Signed-off-by: Danylo Piliaiev
>>> ---
>>> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 6 --
>>> 1 file changed, 4 insertions(+), 2 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 42af41aca3..6adf4a5836 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>>> @@ -1539,6 +1539,8 @@ 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 base_layer = obj->MinLayer + u->_Layer;
>>>const unsigned num_layers = u->Layered ?
>>> get_image_num_layers(mt, obj->Target, u->Level) : 1;
>>>
>>> @@ -1546,8 +1548,8 @@ update_image_surface(struct brw_context *brw,
>>> .format = format,
>>> .base_level = obj->MinLevel + u->Level,
>>> .levels = 1,
>>> -.base_array_layer = obj->MinLayer + u->_Layer,
>>> -.array_len = num_layers,
>>> +.base_array_layer = base_layer,
>>> +.array_len = num_layers - base_layer,
>>
>> See above - num_layers can be 1 if the image isn't bound as a layered
>> image. But base layer can be whatever -- so this will end up as
>> negative. I think the adjustment needs to be done only for the
>> u->Layered case.
>
> Oh, I see it now, thanks! Unless Lionel's patch for this issue is better
> I'll send v2 of my patch.
I believe yours is much closer to right. Lionel's was conceptually
wrong. (Or I'm the one who's confused - reminder - I'm not an Intel
driver developer, and ultimately won't approve or reject your patches.
But I will point out things that I think are off.)
-ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix calculation of layers array length for isl_view
On 9/7/18 5:48 PM, Ilia Mirkin wrote:
On Fri, Sep 7, 2018 at 10:41 AM, Danylo Piliaiev
wrote:
Comment for array_len field states:
"Indicates the number of array elements starting at
Base Array Layer."
And most usages of array_len expect it to be equal or less than
total layers - base layer
Fixes: 5a8c8903
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107856
Signed-off-by: Danylo Piliaiev
---
src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 6 --
1 file changed, 4 insertions(+), 2 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 42af41aca3..6adf4a5836 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -1539,6 +1539,8 @@ 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 base_layer = obj->MinLayer + u->_Layer;
const unsigned num_layers = u->Layered ?
get_image_num_layers(mt, obj->Target, u->Level) : 1;
@@ -1546,8 +1548,8 @@ update_image_surface(struct brw_context *brw,
.format = format,
.base_level = obj->MinLevel + u->Level,
.levels = 1,
-.base_array_layer = obj->MinLayer + u->_Layer,
-.array_len = num_layers,
+.base_array_layer = base_layer,
+.array_len = num_layers - base_layer,
See above - num_layers can be 1 if the image isn't bound as a layered
image. But base layer can be whatever -- so this will end up as
negative. I think the adjustment needs to be done only for the
u->Layered case.
Oh, I see it now, thanks! Unless Lionel's patch for this issue is better
I'll send v2 of my patch.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix calculation of layers array length for isl_view
On Fri, Sep 7, 2018 at 10:41 AM, Danylo Piliaiev
wrote:
> Comment for array_len field states:
> "Indicates the number of array elements starting at
>Base Array Layer."
>
> And most usages of array_len expect it to be equal or less than
> total layers - base layer
>
> Fixes: 5a8c8903
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107856
>
> Signed-off-by: Danylo Piliaiev
> ---
> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 6 --
> 1 file changed, 4 insertions(+), 2 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 42af41aca3..6adf4a5836 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -1539,6 +1539,8 @@ 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 base_layer = obj->MinLayer + u->_Layer;
> const unsigned num_layers = u->Layered ?
> get_image_num_layers(mt, obj->Target, u->Level) : 1;
>
> @@ -1546,8 +1548,8 @@ update_image_surface(struct brw_context *brw,
> .format = format,
> .base_level = obj->MinLevel + u->Level,
> .levels = 1,
> -.base_array_layer = obj->MinLayer + u->_Layer,
> -.array_len = num_layers,
> +.base_array_layer = base_layer,
> +.array_len = num_layers - base_layer,
See above - num_layers can be 1 if the image isn't bound as a layered
image. But base layer can be whatever -- so this will end up as
negative. I think the adjustment needs to be done only for the
u->Layered case.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
