Re: [PULL] GVT-g device model core

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 14, 2016 at 06:30:30PM +0800, Zhenyu Wang wrote:
> 
> Hi,
> 
> This is first pull request to merge GVT-g device model in i915
> which contains core GVT-g device model work to virtualize GPU
> resources. This tries to add feature of Intel GVT-g technology
> for full GPU virtualization. This version will support KVM based
> virtualization solution named as KVMGT.
> 
> More background is on official project home: https://01.org/igvt-g
> 
> To manage mediated device between virtual GPU and physical device it
> will rely on VFIO/mdev framework, this version has not included GVT-g
> device model integration work for VFIO/mdev yet as VFIO community is
> still under work to refine code base. Currently we're basing on
> VFIO/mdev v8 patch series (http://www.spinics.net/lists/kvm/msg138515.html)
> and doing more testings on that.
> 
> There're also several KVM change dependences. KVM maintainer has
> merged two and we will ensure left hits KVM tree before sending new
> pull request to enable that.
> 
> p.s, There would require some coordinate work for VFIO/mdev. We will
> send device model work for that after Alex merged mdev framework in
> VFIO tree. Alex has promised to merge that in early of Nov.
> 
> Let me know if there's any issue with this our first pull request.

Ok applied, but a few things to keep in mind before your next pull
request:

- Dont rebase everything 5 seconds before sending out the pull request.
  That just invalidates all the testing you've done, so not a good idea.
  In general try to avoid rebases as much as possible, and only rebase to
  take out a truly embarassing mistake. And then only rebase up to the
  patch that needs a hotfix, not your entire tree.

- Similar, don't base your pull requests upon a random commit of the day
  (that's why I noticed you rebased). Instead pick something meaningful,
  like a tag I (or Dave Airlie or Linus Torvalds) push out. Or another
  good option is to base it right on top of the last merge commit from
  gvt. Once you've picked a baseline, don't change it (except when you
  have a good reason). And if you need a patch from upstream also don't
  rebase, just send out a pull request with your current patch pile, and
  then continue applying more stuff on top once I merged that.

- One technical nit on the integration: My idea was that i915 core code
  only calls a few specific functions and structures exposed through
  intel_gvt.h. But that file now seems to include gvt-internal headers,
  which is a bit a mess. Please clean that up in the next pull request:

  * Anything that core i915 code or headers needs must be moved into
    intel_gvt.h.
  * Everything else, including the 2 gvt includes we now have (gvt/gvt.h
    and i915_pvinfo.h) should only be included from code in
    drm/i915/gvt.h. So either sprinkle include directives over your source
    files for everything, or make gvt/gvt.h the main gvt header that pulls
    in everything.

  The idea here is similar to drm core vs. i915: drm core headers never
  pull in i915 headers, and all communication happens through the
  well-defined interfaces in drm core header files. I think our goal with
  gvt should be similar, with all the interfaces being in intel_gvt.h.
  Otherwise I fear the submaintainer model will be a bit painful, if we
  don't aim for strict separation here.

- There's not yet a MAINTAINERS entry for i915/gvt with gvt mailing lists,
  git repos and your name on it. Please fix that in the next pull request,
  too.

- gvt seems to lack kernel-doc entirely. I think we need at least an
  overview file and interface documentation for the stuff in
  intel_gvt.[hc]. Please run

	$ make hmtldocs

  to make sure it all looks pretty (you need to add stanzas in
  Documenation/gpu/i915.rst to include things). Another item for the next
  pull request please.

Also, this is the first time ever I've taken a pull request, so some
learning involved on my side too. Please bear with me ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx