Re: [RFC 01/29] drm/i915/gvt: Introduce the basic architecture of GVT-g

[Zhiyuan Lv <zhiyuan.lv@intel.com>]

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx