<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 26, 2017 at 5:26 PM, Timothy Arceri <span dir="ltr"><<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 27/07/17 04:53, Jason Ekstrand wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On July 26, 2017 8:48:18 AM Samuel Pitoiset <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi guys,<br>
<br>
I didn't review the radeonsi and radv patches, but I have sent comments<br>
on other parts.<br>
<br>
More generally:<br>
<br>
- I wonder if some intermediate patches can break Mesa, would be nice to<br>
avoid that (especially for bisecting) Can you double check?<br>
<br>
- I don't see any piglit tests... or did I miss them? :)<br>
</blockquote>
<br>
+1 on the need for tests.  I'll go even further and say that the obvious minimal testing to exercise the extension is far from sufficient.  There are a lot of complex ways in which two APIs can interact and we need to ensure we get it right.  In particular, if we're going to claim that the extension actually works properly we need:<br>
<br>
1. Tests which use more than just GL.  In particular, we need to test sharing between GL and Vulcan.<br>
<br>
2. Test which exercise "complex" textures, i.e. textures with multiple mip-levels, multiple array slices, 1D, 2D, 3D, etc.<br>
<br>
3. Tests which test different texture formats.<br>
<br>
4. Tests which test several different combinations of rendering, clearing, glTexSubImage, etc. in one API and combinations of texturing, glGetTexSubImage, etc. in the other API.<br>
</blockquote></span></blockquote><div><br></div><div>5. Multisampling<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Preferably, a good set of combinations of the above.<br>
<br>
Also, the Vulcan bits need to pass validation.  I shouldn't have to say this but it's important and an easy thing to forget so I thought it worth mentioning.<br>
</blockquote>
<br></span>
Sure I agree on the need for tests but this isn't a tiny amount of work.</blockquote><div><br></div><div>Yes, it is.  However, there are a lot of potentially very complex interactions in this extension and it's impossible to get them right without tests.  Claiming that it must work fine because SteamVR works is about like claiming a C compiler works because it compiles hello world.  Yes, SteamVR is a complex app, but it's use of the extension is likely very simple: only 2D non-array single-LOD images where rendering always hapens in GL and texturing always happens in Vulkan.  If we want people to be able to run arbitrary apps that use this extension, we need way more testing than that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I don't necessarily think should block an initial implementation from landing.<br></blockquote><div><br></div><div>That's a good question.  There's a part of me that's inclined to say we should only block it on basic testing if any.  However...  I'm sure that as the tests get written, we'll find bugs in the implementation and we may want to sort those out before enabling it; certainly before 17.3.  Also, no one likes writing piglit tests and I'm a bit concerned that if we land it based on the promise of tests to be written in the future, they'll never happen.  I don't want to be too onerous but I've seen this before (and been the lazy guy who doesn't want to write tests) and I don't want to end up in a place where we have an important and un-tested piece of functionality.<br><br></div><div>What I'm inclined to say is that I won't NAK it so long as we have at least some Vulkan <-> GL sharing tests.  After all, radeon isn't my driver and you can land untested (read broken) stuff if you want.  However, it won't be turned on in i965 until that list is covered.  And, no, that doesn't mean that I'm signing our team up to write the tests. :-)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Do you have a suggestion for how we should implement testing between Vulkan and OpenGL? Can we leverage the crucible framework at all? Or should we just create some basic Vulkan framework in piglit from scratch? I only picked up this work yesterday so I haven't put too much thought into it.<br>
</blockquote><div><br></div><div>i don't think we should try to "leverage" crucible directly.  However, looking at crucible and liberally copying+pasting is probably a good idea.  Crucible does have some very nifty helpers in qonos.c which wrap various Vulkan calls and make them a bit saner.  Also, it wouldn't hurt to grab the image download code and the device enumeration code.  However, most of the crucible framework is more for the multi-threaded test runner than for actually making Vulkan easier.<br><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
--Jason<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks,<br>
Samuel.<br>
<br>
On 07/26/2017 01:46 PM, Timothy Arceri wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi all,<br>
<br>
Andres is not around at the moment so as well are reviewing the<br>
remaining patches I've rebased and added all Marek's suggestions.<br>
<br>
I've also made a few minor changes (see commit messages) and<br>
reworked some of the patches to reduce code churn.<br>
<br>
I thought I'd send it out one last time to see if there was any<br>
more feedback otherwise I'll probably push later in the week.<br>
<br>
Thanks,<br>
Tim<br>
<br>
Series available at:<br>
   <a href="https://github.com/tarceri/Mesa.git" rel="noreferrer" target="_blank">https://github.com/tarceri/Me<wbr>sa.git</a> (memobj2)<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
<br>
</blockquote>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</blockquote>
<br>
<br>
</blockquote>
</div></div></blockquote></div><br></div></div>