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

[Zhi Wang]

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
> 
> 
>