On Thu, 2016-06-30 at 08:15 +0200, Iago Toral wrote: > On Wed, 2016-06-29 at 23:12 +1000, Timothy Arceri wrote: > > On Wed, 2016-06-29 at 14:42 +0200, Iago Toral wrote: > > > On Wed, 2016-06-29 at 14:40 +0200, Iago Toral wrote: > > > > On Tue, 2016-06-28 at 16:30 +0200, Iago Toral wrote: > > > > > On Tue, 2016-06-28 at 11:52 +1000, Timothy Arceri wrote: > > > > > > There are two distinctly different uses of this struct. The > > > > > > first > > > > > > is to store GL shader objects. The second is to store > > > > > > information > > > > > > about a shader stage thats been linked. > > > > > > > > > > > > The only place the new structs overlap is the shader layout > > > > > > fields and > > > > > > I intend to split that out into a third struct once this > > > > > > series > > > > > > lands. > > > > > > > > > > > > Having two well defined structs helps code readability and > > > > > > allows the removal > > > > > > of some unreachable code paths that were the result of > > > > > > confusion between > > > > > > the two uses. > > > > > > > > > > I think it is a good idea, thanks! > > > > > > > > > > I dropped a comment in patch 4, with that fixed patches 1-4 > > > > > are: > > > > > Reviewed-by: Iago Toral Quiroga <ito...@igalia.com> > > > > > > > > > > I'll try to review the last 3 patches tomorrow. > > > > > > > > Patches 5-6 are: > > > > > > I meant 6-7 here... > > > > > > > Reviewed-by: Iago Toral Quiroga <ito...@igalia.com> > > > > > > > > Patch 4 is huge so it is kind difficult to review it in detail, > > > > it > > > > looks > > > > > > ... and patch 5 here, sorry for the mess. > > > > > > > good to me in general but I dropped comments for few things > > > > that > > > > caught > > > > my eye. > > > > With those comments addressed do I have a r-b for 5 too? I'd settle > > for > > an acked-by :) > > Yeah, you can add my Ack with those fixed. > > > > > > > > > > > BTW, I think you should test the series against CTS too if you > > > > haven't > > > > already and maybe we should test this on something other than > > > > Intel > > > > too > > > > to be sure. > > > > No regressions using Intels Jenkins setup :) I guess I could try it > > on > > the r600 driver if I can manage to setup a dev environment on my > > old > > desktop. Unless there is someone out there kind enough to give the > > series a run for me? > > I was hoping that you could ping someone to give this a run on some > other hardware, maybe Marek? At the very least you should build as > many > drivers as you can, but I guess you already did that. > > If you can't test this on non-Intel hardware, knowing that Intel is > okay > I guess it would be fine to push and have people try this once it > lands > in master.
I asked on IRC and llvmpipe was suggested. That *should* be good enough as I don't think these structs are used past the glsl_to_tgsi conversion. Just finishing a run on master now to compare with but looks like everything is ok. Thanks again for the reviews. > > Iago > > > > > > > > > Iago > > > > > > > > > _______________________________________________ > > > mesa-dev mailing list > > > mesa-dev@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev