On Fri, Jan 29, 2016 at 03:57:09PM +0200, Joonas Lahtinen wrote: > Hi, > > TL;DR Overall, we have same problem as with the scheduler series, there > is too much placeholder stuff for easy review. Just squash enough code > into one commit so it actually does something logical that can be > reviewed and then extend it later. Then it can be reviewed and pushed. > Just splitting the code down to achieve smaller patches is not the > right thing to do. > > Comments on the overall code: You need to document all header file > functions (in the source files), and it is good to document the static > functions within a file too, to make future maintenance easier. > > It is not about splitting the code down to small chunks, but splitting > it down to small *logical* chunks. It doesn't make sense to commit > dozens of empty bodied functions for review, and then later add their > code. > > If you add functions, only add them at a patch that takes them into use > too, unless we're talking about general purpose shared code. And also > remember to add the function body and documentation header. If you > simply add a "return 0;" or similar function body, do add a comment to > why the function does not exist and when it will. > > Then, there is a trend of having a boolean return values in the code. > When possible, it should rather be int and the cause for failure should > be propagated from the last level all the way to up (-ENOMEN etc.). > This way debugging becomes easier and if new error conditions appear, > there is less of a maintenance burden to add the propagation later. > > Finally, make sure to look at the existing driver parts and > https://www.kernel.org/doc/Documentation/CodingStyle > for proper coding style. There are lots of whitespace fixes needed in > this series, like array initializations. > > I hope to see this first patch rerolled so that you squash some of the > later commits into it so that all functions have a body and you add > documentation for the functions so I can both see what it should do and > what it actually does. Only reroll the first patch, to keep the > iterative step smaller. Lets only then continue with the rest of the > series once we've reached a consensus on the formatting and style > basics. > > See more comments below. I'm glad you did the nitpicking. As far as the integration goes, on the whole I'm happy with the way it is structured and the reuse of existing code. I tried to attack various aspects of the GVT contexts and came to the conclusion I couldn't suggest a better approach (though maybe tomorrow!). A few bits and pieces I got lost trying to pull together (in particular like how we do is read back through the GTT entries performed, the hypervisor_read_va abstraction iirc) and would appreciate having a branch available to get the complete picture. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
