[PATCH 0/9] drm/i915/gvt: Refine the gtt shadowing

[Yuan, Hang]

First I think both of your efforts are worth appreciation. It's a big effort to develop this large patch set. Also it takes quite a few time to review the patch set carefully. Both are helpful to GVT code quality. Second I think we all understand challenging comments don't mean any offence. Don’t worry:-)

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
> Behalf Of Zhi Wang
> Sent: Tuesday, December 26, 2017 1:15 PM
> To: Du, Changbin <changbin.du at intel.com>; Zhenyu Wang
> <zhenyuw at linux.intel.com>
> Cc: intel-gvt-dev at lists.freedesktop.org
> Subject: Re: [PATCH 0/9] drm/i915/gvt: Refine the gtt shadowing
> 
> It's a good idea. I think it's better to review the code in a meeting.
> 
> That's all my comments. Usually I don't like to challenge people, because we
> are such a small team, challenge brings a lot burden, and if people achieve
> something together, they will leave everything painful behind. If I am not
> assigned to review this series, everything is OK to me.
> 
> I'm not sure if you discussed with Zhenyu before you cook your patches.
> Usually, you need to discuss and communicate with reviewers during your
> development. If you just throw a bunch of patches, you are expecting
> comments. Just like what I said, people got convinced by data and at least
> investigations. You should find a way to let people know it.
> 
> You cannot just say it's has been used in Linux, then it's right for us.
> You tend to copy names like page_tree, and even behaviors in linux kernel.
> You even tend to add cond_resched like khugepaged. I mean you can
> convince people with benefit/advantage/disadvantage before you do that.
> It's also good for other people to know the history.
> 
> Thanks,
> Zhi.
> 
> On 12/26/17 11:23, Du, Changbin wrote:
> > On Tue, Dec 26, 2017 at 11:10:12AM +0800, Zhenyu Wang wrote:
> >> On 2017.12.25 21:17:26 +0800, Zhi Wang wrote:
> >>> Well, I think that's all my comments for this series. Since Zhenyu
> >>> invites me to review this patches, Zhenyu you can also give rb/ack
> >>> based on your opinion.
> >>>
> >>
> >> Several ones still seems not easy to review, e.g 1/9, even it's
> >> generally ok, but still can be refined to help reviewer, e.g change
> >> one part of struct at each time and clearly split those rename part.
> >> For those big change, we definitely like to see step by step change,
> >> although which means more effort on developer side.
> >>
> > Myabe an alternate solution is that we can review these two big one in a
> meeting.
> > Split it takes me several times efforts and it's really to rewrite
> > them again and again.
> >
> >> And for other internal data structure change, need at least some
> >> macro benchmark comparison or even micro bench to understand its
> impact.
> >>
> > yeah, the data will be in next revise.
> >
> >>>
> >>> On 12/25/17 17:11, changbin.du at intel.com wrote:
> >>>> From: Changbin Du <changbin.du at intel.com>
> >>>>
> >>>> This is the first part of patch set "drm/i915/gvt: Add support for huge
> gtt (2M/64K)".
> >>>> The GTT related code are refined. I just need a clean code base to
> >>>> add new feature.
> >>>>
> >>>> patch #1 and #7 are fat patch, please take care.
> >>>>
> >>>> Changbin Du (9):
> >>>>     drm/i915/gvt: Rework shadow graphic memory management code
> >>>>     drm/i915/gvt: Add verbose gtt shadow logs
> >>>>     drm/i915/gvt: Rename ggtt related functions to be more specific
> >>>>     drm/i915/gvt: Factor out
> >>>>       intel_vgpu_{get_or_create_ppgtt_mm,find_destroy_ppgtt_mm}
> interfaces
> >>>>     drm/i915/gvt: Use standard pte bit definition
> >>>>     drm/i915/gvt: Refine pte shadowing process
> >>>>     drm/i915/gvt: Rework shadow page management code
> >>>>     drm/i915/gvt: Manage shadow pages with radix tree
> >>>>     drm/i915/gvt: Define PTE addr mask with GENMASK_ULL
> >>>>
> >>>>    drivers/gpu/drm/i915/gvt/Makefile     |    2 +-
> >>>>    drivers/gpu/drm/i915/gvt/gtt.c        | 1427 +++++++++++++++-------------
> -----
> >>>>    drivers/gpu/drm/i915/gvt/gtt.h        |  182 ++---
> >>>>    drivers/gpu/drm/i915/gvt/gvt.c        |    2 +-
> >>>>    drivers/gpu/drm/i915/gvt/gvt.h        |    2 +
> >>>>    drivers/gpu/drm/i915/gvt/handlers.c   |   18 +-
> >>>>    drivers/gpu/drm/i915/gvt/mmio.c       |    9 +-
> >>>>    drivers/gpu/drm/i915/gvt/mpt.h        |   36 +-
> >>>>    drivers/gpu/drm/i915/gvt/page_track.c |  181 +++++
> >>>>    drivers/gpu/drm/i915/gvt/page_track.h |   54 ++
> >>>>    drivers/gpu/drm/i915/gvt/scheduler.c  |   48 +-
> >>>>    drivers/gpu/drm/i915/gvt/trace.h      |   38 +-
> >>>>    drivers/gpu/drm/i915/gvt/vgpu.c       |    1 +
> >>>>    13 files changed, 1002 insertions(+), 998 deletions(-)
> >>>>    create mode 100644 drivers/gpu/drm/i915/gvt/page_track.c
> >>>>    create mode 100644 drivers/gpu/drm/i915/gvt/page_track.h
> >>>>
> >>>
> >>> _______________________________________________
> >>> intel-gvt-dev mailing list
> >>> intel-gvt-dev at lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> >>
> >> --
> >> Open Source Technology Center, Intel ltd.
> >>
> >> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> >
> >
> >
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev