Hi Chris, On Fri, Jan 29, 2016 at 04:48:07PM +0000, Chris Wilson wrote: > 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. I pushed the RFC code into below repo: https://github.com/01org/Igvtg-kernel.git Branch: gvt-upstream-rfc Thanks for review and look forward to more comments! Regards, -Zhiyuan > -Chris > -- > Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
