[Intel-gfx] [PULL] GVT-g device model core

[Daniel Vetter]

On Mon, Oct 17, 2016 at 03:45:47PM +0800, Zhenyu Wang wrote:
> On 2016.10.17 09:33:19 +0200, Daniel Vetter wrote:
> > On Mon, Oct 17, 2016 at 09:30:45AM +0200, Daniel Vetter wrote:
> > > 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.
> > >
> 
> Sorry for that although we liked to include only core GVT-g device
> model in first pull request, we do have continual testing internally
> to cover GVT-g features. Will do as you said.

Yeah might be best to have a new branch with upstreaming stuff (now you
need to at least split out bugfixes for the already merged stuff) and
treat that like a mostly stable branch. And the still unmerged features
(vfio and all that) would then get rebased on top of 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.
> > >
> 
> yeah, that's a little messy, we will try to clean it up.
> 
> > > - 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.
> 
> Will add that.
> 
> > > 
> > > - 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.
> > 
> > Quick addition: Since this will be a patch touching i915 core code pls
> > submit it to intel-gfx for review. You can then apply it to your tree once
> > it's reviewed (or Joonas or someone can commit directly to
> > drm-intel-next-queued).
> >
> 
> Will fix kernel-doc too.
> 
> > And another item:
> > - Please add me to the moderation whitelist of igt-g-dev, I don't want to
> >   be spammed by moderation mails every time I reply to your pull requests
> >   ;-)
> > 
> 
> Will ping 01.org admin for that.

Just noticed that my mail was outright rejected. If you want to keep the
list on 01.org as the official one, you need to switch to moderating for
non-subscribers. Otherwise it's way too hard for external people to submit
a quick bug report for your patches.

> > > 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 ;-)
> > > 
> 
> It's new thing for us to learn obviously. Thanks for the advice!

Also I already screwed up the merge, it doesn't even compile :( Sorry
about that. Can you pls create a quick fixup patch just to make it work
again and submit to intel-gfx? That way I can apply it directly.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch