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

[Jason Ekstrand]

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:
   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
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160823/dca0b83a/attachment-0001.html>