i965: Fix calculation of layers array length for isl_view

Submitted by Danylo Piliaiev on Sept. 7, 2018, 2:41 p.m.

Details

Message ID 20180907144159.28440-1-danylo.piliaiev@globallogic.com
State New
Headers show
Series "i965: Fix calculation of layers array length for isl_view" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Danylo Piliaiev Sept. 7, 2018, 2:41 p.m.
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 <danylo.piliaiev@globallogic.com>
---
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

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,
             .swizzle = ISL_SWIZZLE_IDENTITY,
             .usage = ISL_SURF_USAGE_STORAGE_BIT,
          };

Comments

On Fri, Sep 7, 2018 at 10:41 AM, Danylo Piliaiev
<danylo.piliaiev@gmail.com> 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 <danylo.piliaiev@globallogic.com>
> ---
>  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.
On 9/7/18 5:48 PM, Ilia Mirkin wrote:
> On Fri, Sep 7, 2018 at 10:41 AM, Danylo Piliaiev
> <danylo.piliaiev@gmail.com> 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 <danylo.piliaiev@globallogic.com>
>> ---
>>   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.
On Fri, Sep 7, 2018 at 11:09 AM, Danylo Piliaiev
<danylo.piliaiev@gmail.com> wrote:
>
> On 9/7/18 5:48 PM, Ilia Mirkin wrote:
>>
>> On Fri, Sep 7, 2018 at 10:41 AM, Danylo Piliaiev
>> <danylo.piliaiev@gmail.com> 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 <danylo.piliaiev@globallogic.com>
>>> ---
>>>   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
On 07/09/2018 16:32, Ilia Mirkin wrote:
> On Fri, Sep 7, 2018 at 11:09 AM, Danylo Piliaiev
> <danylo.piliaiev@gmail.com> wrote:
>> On 9/7/18 5:48 PM, Ilia Mirkin wrote:
>>> On Fri, Sep 7, 2018 at 10:41 AM, Danylo Piliaiev
>>> <danylo.piliaiev@gmail.com> 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 <danylo.piliaiev@globallogic.com>
>>>> ---
>>>>    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...
On Fri, Sep 7, 2018 at 4:42 PM Danylo Piliaiev
<danylo.piliaiev@gmail.com> 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?
On Fri, Sep 7, 2018 at 12:35 PM, Józef Kucia <joseph.kucia@gmail.com> wrote:
> On Fri, Sep 7, 2018 at 4:42 PM Danylo Piliaiev
> <danylo.piliaiev@gmail.com> 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
On Fri, Sep 7, 2018 at 6:44 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
> On Fri, Sep 7, 2018 at 12:35 PM, Józef Kucia <joseph.kucia@gmail.com> wrote:
> > On Fri, Sep 7, 2018 at 4:42 PM Danylo Piliaiev
> > <danylo.piliaiev@gmail.com> 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.
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 <imirkin@alum.mit.edu> wrote:
>> On Fri, Sep 7, 2018 at 12:35 PM, Józef Kucia <joseph.kucia@gmail.com> wrote:
>>> On Fri, Sep 7, 2018 at 4:42 PM Danylo Piliaiev
>>> <danylo.piliaiev@gmail.com> 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.