[Mesa-dev] [PATCH] i965: consider a 'base level' when calculating width0, height0, depth0

[andrey simiklit]

ping. the piglit test was pushed)

Thanks,
Andrii.

On Tue, Jan 22, 2019 at 1:31 PM andrey simiklit <asimiklit.work at gmail.com>
wrote:

> Hello,
>
> Could somebody help me with a push of the following patch?
> https://patchwork.freedesktop.org/patch/254397
>
> This fix is needed to fix these fails:
>
> https://mesa-ci.01.org/global_logic/builds/56/group/ac3c5a0dc1f15492570367c6c8ec835c
>
> When this fix is pushed we will be able to remove the following test:
> tex-upside-down-miptree
> from Intel CI "[expected-crashes]" sections.
>
> Thanks,
> Andrii.
>
> On Mon, Jan 14, 2019 at 3:05 PM andrey simiklit <asimiklit.work at gmail.com>
> wrote:
>
>> Hello,
>>
>> The test for this issue is pushed to the piglit.
>> It would be great to push the mesa fix too if it is still an acceptable
>> for all :)
>>
>> Thanks,
>> Andrii.
>> On Sat, Oct 20, 2018 at 12:29 PM andrey simiklit <
>> asimiklit.work at gmail.com> wrote:
>>
>>> Hello,
>>>
>>> On Fri, Oct 19, 2018 at 15:14 Kenneth Graunke <kenneth at whitecape.org>
>>> wrote:
>>>
>>>> On Thursday, October 11, 2018 12:12:38 PM PDT Kenneth Graunke wrote:
>>>> > On Thursday, October 11, 2018 11:58:40 AM PDT Kenneth Graunke wrote:
>>>> > > On Tuesday, October 2, 2018 9:16:01 AM PDT asimiklit.work at gmail.com
>>>> wrote:
>>>> > > > From: Andrii Simiklit <andrii.simiklit at globallogic.com>
>>>> > > >
>>>> > > > I guess that when we calculating the width0, height0, depth0
>>>> > > > to use for function 'intel_miptree_create' we need to consider
>>>> > > > the 'base level' like it is done in the
>>>> 'intel_miptree_create_for_teximage'
>>>> > > > function.
>>>> > > >
>>>> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107987
>>>> > > > Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com>
>>>> > > > ---
>>>> > > >  .../drivers/dri/i965/intel_tex_validate.c     | 26
>>>> ++++++++++++++++++-
>>>> > > >  1 file changed, 25 insertions(+), 1 deletion(-)
>>>> > >
>>>> > > I believe this patch is correct - we're assembling things into a new
>>>> > > miptree, which we start at level 0 - so we need the sizes for level
>>>> 0.
>>>> > >
>>>> > > Alternatively, we might be able to pass validate_first_level instead
>>>> > > of 0 when calling intel_miptree_create, to make one that's only good
>>>> > > up until the new base...and have to re-assemble it the next time
>>>> they
>>>> > > change the base.  It would save memory potentially.  But more
>>>> copies.
>>>> > > I don't have a strong preference which is better.
>>>> > >
>>>> > > Please do make a Piglit or dEQP test for this.
>>>> > >
>>>> > > Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>>>> >
>>>> > Sorry, withdrawing my review. :(  Chris Forbes pointed out on IRC that
>>>> > your reproducer case is backwards:
>>>> >
>>>> > miplevel 0 - 1x1
>>>> > miplevel 1 - 2x2
>>>> > miplevel 2 - 4x4
>>>> >
>>>> > That's upside down.  A proper miptree would have the base be largest:
>>>> >
>>>> > miplevel 0 - 4x4
>>>> > miplevel 1 - 2x2
>>>> > miplevel 2 - 1x1
>>>> >
>>>> > So, yes, I could see this tripping an assert...but such a crazy
>>>> texture
>>>> > will never be mipmap complete.  If they're expecting mipmapping, then
>>>> > it seems like they should get a fallback black texture (which normally
>>>> > happens for incomplete textures).  If not, maybe they should get a
>>>> > single miplevel?  Either way, seems like we should detect insanity and
>>>> > bail, rather than change size calculations for the normal sane case.
>>>> >
>>>>
>>>> So...looked at this again.  I'm not sure why upside-down matters.
>>>>
>>>> At DrawArrays time, we have a single miplevel (base = 2), and are trying
>>>> to put that single miplevel's image into a miptree.  We do properly
>>>> ignore levels 0..1 as they're beyond the base.
>>>>
>>>> We appear to use level 0 as the actual base, and want to store our
>>>> single level at level 2.  Other places (TexImage) seem to work that way
>>>> too.
>>>>
>>>> But, we're creating the miplevel with level 0 as the base, but where
>>>> level 0 has the dimensions of level 2.  This doesn't work.  And your
>>>> patch fixes that.
>>>>
>>>> I tried making the actual base of the unified tree be level 2, rather
>>>> than level 0...so that the BaseLevel is the actual base...but tons of
>>>> things broke.
>>>>
>>>> So, back to Reviewed-by.  I think once we get a Piglit test, I'm happy
>>>> to land this patch.
>>>
>>>
>>> Thanks for reviewing :-) I will start to work on it as soon as come back
>>> from vacation (on Monday)
>>>
>>>
>>>>
>>>> --Ken
>>>
>>>
>>> Thanks,
>>> Andrii.
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190207/2bd39b05/attachment-0001.html>