[Mesa-dev] [PATCH v2] i965: don't fail to shift height images for levels.

[Ilia Mirkin]

On Tue, Aug 23, 2016 at 12:34 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Tue, Aug 23, 2016 at 9:28 AM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
>>
>> This smells like fish... I'm going to have a look.
>
>
> Ok, I looked...
>
>>
>> On Tue, Aug 23, 2016 at 8:29 AM, Antia Puentes <apuentes at igalia.com>
>> wrote:
>>>
>>> From: Dave Airlie <airlied at redhat.com>
>>>
>>> This fixes one subtest of:
>>> GL44-CTS.shader_image_size.advanced-nonMS-fs-int
>>>
>>> I've no idea why this wouldn't be scaled up here,
>>> and I've no idea what else will break, but I might
>>> as well open for discussion.
>>>
>>> v2: Only shift height if the texture is not an 1D_ARRAY,
>>> it fixes assertion in GL44-CTS.texture_view.gettexparameter
>>> due to the original patch (Antia).
>>>
>>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>>> Signed-off-by: Antia Puentes <apuentes at igalia.com>
>>> ---
>>>
>>> I have not taken a deep look to the test so take this with a grain of
>>> salt.
>>> As I said in a previous email, this patch raises an assertion in
>>> GL44-CTS.texture_view.gettexparameter:
>>>
>>> "glcts: intel_mipmap_tree.c:368: intel_miptree_create_layout: Assertion
>>> `height0 = 1' failed."
>>>
>>> Looking at the code surrounding the assertion, we have:
>>>
>>>    if (target == GL_TEXTURE_1D_ARRAY)
>>>          assert(height0 == 1);
>>>
>>> which suggests that we should avoid shifting the height at least for
>>> TEXTURE_1D_ARRAYs. Sending a second version of the patch.
>>>
>>>  src/mesa/drivers/dri/i965/intel_tex_image.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c
>>> b/src/mesa/drivers/dri/i965/intel_tex_image.c
>>> index 958f8bd..120e7e0 100644
>>> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
>>> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
>>> @@ -49,7 +49,7 @@ intel_miptree_create_for_teximage(struct brw_context
>>> *brw,
>>>     /* Figure out image dimensions at start level. */
>>>     for (i = intelImage->base.Base.Level; i > 0; i--) {
>>>        width <<= 1;
>>> -      if (height != 1)
>>> +      if (intelObj->base.Target != GL_TEXTURE_1D_ARRAY)
>>>           height <<= 1;
>>>        if (intelObj->base.Target == GL_TEXTURE_3D)
>>>           depth <<= 1;
>
>
> I think the whole pile of code is bogus and needs to be rewritten.  First
> off, why are we using a for loop?  Seriously?  Second, I think what we want
> is more like
>
> switch (intelObj->base.Target) {
> case GL_TEXTURE_3D:
>    depth <<= intelImage->base.Base.Level;
>    /* Fall through */
>
> case GL_TEXTURE_2D:
> case GL_TEXTURE_2D_ARRAY:
> case GL_TEXTURE_RECTANGLE:

and cube/cube_array
and ms/ms_array

[this is why != 1d_array tends to be attractive]

>    height <<= intelImage->base.Base.Level
>    /* Fall through */
>
> default:
>    width <<= intelImage->base.Base.Level;
> }
>
> I think that would be far more clear and correct.  I don't have a build of
> the CTS handy so I didn't send it as a 3rd counter-patch.
>
> --Jason
>
>>>
>>> --
>>> 2.7.4
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>