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