Re: [iGVT-g] [ANNOUNCE][RFC] KVMGT - the implementation of Intel GVT-g(full GPU virtualization) for KVM
by Jike Song
Hi all,
We are pleased to announce another update of Intel GVT-g for KVM.
Intel GVT-g is a full GPU virtualization solution with mediated pass-through, starting from 4th generation Intel Core(TM) processors with Intel Graphics processors. A virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance among performance, feature, and sharing capability. KVM is supported by Intel GVT-g(a.k.a. KVMGT).
Repositories
Kernel: https://github.com/01org/igvtg-kernel (2015q3-3.18.0 branch)
Qemu: https://github.com/01org/igvtg-qemu (kvmgt_public2015q3 branch)
This update consists of:
- KVMGT is now merged with XenGT in unified repositories(kernel and qemu), but currently
different branches for qemu. KVMGT and XenGT share same iGVT-g core logic.
- PPGTT supported, hence the Windows guest support
- KVMGT now supports both 4th generation (Haswell) and 5th generation (Broadwell) Intel Core(TM) processors
- 2D/3D/Media decoding have been validated on Ubuntu 14.04 and Windows7/Windows 8.1
Next update will be around early Jan, 2016.
Known issues:
- At least 2GB memory is suggested for VM to run most 3D workloads.
- 3Dmark06 running in Windows VM may have some stability issue.
- Using VLC to play .ogg file may cause mosaic or slow response.
Please subscribe the mailing list to report BUGs, discuss, and/or contribute:
https://lists.01.org/mailman/listinfo/igvt-g
More information about Intel GVT-g background, architecture, etc can be found at(may not be up-to-date):
https://01.org/igvt-g
http://www.linux-kvm.org/images/f/f3/01x08b-KVMGT-a.pdf
https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian
Note:
The KVMGT project should be considered a work in progress. As such it is not a complete product nor should it be considered one. Extra care should be taken when testing and configuring a system to use the KVMGT project.
--
Thanks,
Jike
On 12/04/2014 10:24 AM, Jike Song wrote:
> Hi all,
>
> We are pleased to announce the first release of KVMGT project. KVMGT is the implementation of Intel GVT-g technology, a full GPU virtualization solution. Under Intel GVT-g, a virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance of performance, feature, and sharing capability.
>
>
> KVMGT is still in the early stage:
>
> - Basic functions of full GPU virtualization works, guest can see a full-featured vGPU.
> We ran several 3D workloads such as lightsmark, nexuiz, urbanterror and warsow.
>
> - Only Linux guest supported so far, and PPGTT must be disabled in guest through a
> kernel parameter(see README.kvmgt in QEMU).
>
> - This drop also includes some Xen specific changes, which will be cleaned up later.
>
> - Our end goal is to upstream both XenGT and KVMGT, which shares ~90% logic for vGPU
> device model (will be part of i915 driver), with only difference in hypervisor
> specific services
>
> - insufficient test coverage, so please bear with stability issues :)
>
>
>
> There are things need to be improved, esp. the KVM interfacing part:
>
> 1 a domid was added to each KVMGT guest
>
> An ID is needed for foreground OS switching, e.g.
>
> # echo <domid> > /sys/kernel/vgt/control/foreground_vm
>
> domid 0 is reserved for host OS.
>
>
> 2 SRCU workarounds.
>
> Some KVM functions, such as:
>
> kvm_io_bus_register_dev
> install_new_memslots
>
> must be called *without* &kvm->srcu read-locked. Otherwise it hangs.
>
> In KVMGT, we need to register an iodev only *after* BAR registers are
> written by guest. That means, we already have &kvm->srcu hold -
> trapping/emulating PIO(BAR registers) makes us in such a condition.
> That will make kvm_io_bus_register_dev hangs.
>
> Currently we have to disable rcu_assign_pointer() in such functions.
>
> These were dirty workarounds, your suggestions are high welcome!
>
>
> 3 syscalls were called to access "/dev/mem" from kernel
>
> An in-kernel memslot was added for aperture, but using syscalls like
> open and mmap to open and access the character device "/dev/mem",
> for pass-through.
>
>
>
>
> The source codes(kernel, qemu as well as seabios) are available at github:
>
> git://github.com/01org/KVMGT-kernel
> git://github.com/01org/KVMGT-qemu
> git://github.com/01org/KVMGT-seabios
>
> In the KVMGT-qemu repository, there is a "README.kvmgt" to be referred.
>
>
>
> More information about Intel GVT-g and KVMGT can be found at:
>
> https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian
> http://events.linuxfoundation.org/sites/events/files/slides/KVMGT-a%20Ful...
>
>
> Appreciate your comments, BUG reports, and contributions!
>
>
>
>
> --
> Thanks,
> Jike
>
4 years, 7 months
Re: [iGVT-g] [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
by Jike Song
Hi all,
We are pleased to announce another update of Intel GVT-g for Xen.
Intel GVT-g is a full GPU virtualization solution with mediated pass-through, starting from 4th generation Intel Core(TM) processors with Intel Graphics processors. A virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance among performance, feature, and sharing capability. Xen is currently supported on Intel Processor Graphics (a.k.a. XenGT); and the core logic can be easily ported to other hypervisors.
Repositories
Kernel: https://github.com/01org/igvtg-kernel (2015q3-3.18.0 branch)
Xen: https://github.com/01org/igvtg-xen (2015q3-4.5 branch)
Qemu: https://github.com/01org/igvtg-qemu (xengt_public2015q3 branch)
This update consists of:
- XenGT is now merged with KVMGT in unified repositories(kernel and qemu), but currently
different branches for qemu. XenGT and KVMGT share same iGVT-g core logic.
- fix sysfs/debugfs access seldom crash issue
- fix a BUG in XenGT I/O emulation logic
- improve 3d workload stability
Next update will be around early Jan, 2016.
Known issues:
- At least 2GB memory is suggested for VM to run most 3D workloads.
- Keymap might be incorrect in guest. Config file may need to explicitly specify "keymap='en-us'". Although it looks like the default value, earlier we saw the problem of wrong keymap code if it is not explicitly set.
- When using three monitors, doing hotplug between Guest pause/unpause may not be able to lightup all monitors automatically. Some specific monitor issues.
- Cannot move mouse pointer smoothly in guest by default launched by VNC mode. Configuration file need to explicitly specify "usb=1" to enable a USB bus, and "usbdevice='tablet'" to add pointer device using absolute coordinates.
- Resume dom0 from S3 may cause some error message.
- i915 unload/reload cannot works well with less than 3 vcpu when upowerd service was running
- Unigen Tropics running in multiple guests will cause dom0 and guests tdr.
Please subscribe the mailing list to report BUGs, discuss, and/or contribute:
https://lists.01.org/mailman/listinfo/igvt-g
More information about Intel GVT-g background, architecture, etc can be found at(may not be up-to-date):
https://01.org/igvt-g
https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian
http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20S...
http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20S...
https://01.org/xen/blogs/srclarkx/2013/graphics-virtualization-xengt
Note:
The XenGT project should be considered a work in progress. As such it is not a complete product nor should it be considered one. Extra care should be taken when testing and configuring a system to use the XenGT project.
--
Thanks,
Jike
On 07/07/2015 10:49 AM, Jike Song wrote:
> Hi all,
>
> We're pleased to announce a public update to Intel Graphics Virtualization Technology(Intel GVT-g, formerly known as XenGT).
>
> Intel GVT-g is a full GPU virtualization solution with mediated pass-through, starting from 4th generation Intel Core(TM) processors with Intel Graphics processors. A virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance among performance, feature, and sharing capability. Xen is currently supported on Intel Processor Graphics (a.k.a. XenGT); and the core logic can be easily ported to other hypervisors, for example, the experimental code has been released to support GVT-g running on a KVM hypervisor (a.k.a KVMGT).
>
> Tip of repositories
> -------------------------
>
> Kernel: 5b73653d5ca, Branch: master-2015Q2-3.18.0
> Qemu: 2a75bbff62c1, Branch: master
> Xen: 38c36f0f511b1, Branch: master-2015Q2-4.5
>
> This update consists of:
> - Change time based scheduler timer to be configurable to enhance stability
> - Fix stability issues that VM/Dom0 got tdr when hang up at some specific instruction on BDW
> - Optimize the emulation of el_status register to enhance stability
> - 2D/3D performance in linux VMs has been improved about 50% on BDW
> - Fix abnormal idle power consumption issue due to wrong forcewake policy
> - Fix tdr issue when running 2D/3D/Media workloads in Windows VMs simultaneously
> - KVM support is still in a separate branch as prototype work. We plan to integrate KVM/Xen support together in the future releases
> - Next update will be around early Oct, 2015
>
> Notice that this release can support both Intel 4th generation Core CPU(code name: Haswell) and Intel 5th generation Core CPU (code name: Broadwell), while the limitation of the latter include:
> * 3D conformance may have some failure
> * Under high demand 3D workloads, stability issues are detected
> * Multi-monitor scenario is not fully tested, while single monitor of VGA/HDMI/DP/eDP should just work
> * Hotplug DP may cause black screen even on native environment
>
> Where to get
>
> kernel: https://github.com/01org/XenGT-Preview-kernel.git
> xen: https://github.com/01org/XenGT-Preview-xen.git
> qemu: https://github.com/01org/XenGT-Preview-qemu.git
>
>
> We have a mailing list for GVT-g development, bug report and technical discussion:
>
> https://lists.01.org/mailman/listinfo/igvt-g
>
> More information about Intel GVT-g background, architecture, etc can be found at:
>
> https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian
> http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20S...
> https://01.org/xen/blogs/srclarkx/2013/graphics-virtualization-xengt
>
>
> Note: The XenGT project should be considered a work in progress. As such it is not a complete product nor should it be considered one. Extra care should be taken when testing and configuring a system to use the XenGT project.
>
>
> --
> Thanks,
> Jike
>
> On 04/10/2015 09:23 PM, Jike Song wrote:
>> Hi all,
>>
>> We're pleased to announce a public update to Intel Graphics Virtualization Technology (Intel GVT-g, formerly known as XenGT). Intel GVT-g is a complete vGPU solution with mediated pass-through, supported today on 4th generation Intel Core(TM) processors with Intel Graphics processors. A virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance among performance, feature, and sharing capability. Though we only support Xen on Intel Processor Graphics so far, the core logic can be easily ported to other hypervisors.
>>
>> Tip of repositories
>> -------------------------
>>
>> Kernel: a011c9f953e, Branch: master-2015Q1-3.18.0
>> Qemu: 2a75bbff62c1, Branch: master
>> Xen: 38c36f0f511b1, Branch: master-2015Q1-4.5
>>
>> Summary this update
>> -------------------------
>> - Preliminary Broadwell support.
>> - kernel update from drm-intel 3.17.0 to drm-intel 3.18.0(tag: drm-intel-next-fixes-2014-12-17, notice that i915 driver code is much newer than kernel stable version).
>> - Next update will be around early July, 2015.
>> - KVM support is still in a separate branch as prototype work. We plan to integrate KVM/Xen support together in future releases.
>>
>> This update consists of:
>> - gvt-g core logic code was moved into i915 driver directory.
>> - Host mediation is used for dom0 i915 driver access, instead of de-privileged dom0.
>> - The Xen-specific code was separated from vgt core logic into a new file "driver/xen/xengt.c".
>> - Broadwell is preliminarily supported in this release. Users could start multiple linux/windows 64-bit virtual machines simultaneously, and perform display switch among them.
>>
>> Notice that it is still preliminary release for BDW, which is not yet in the same level of HSW release. Differences include:
>> * Power/performance tuning on BDW is not yet done.
>> * Stability needs to be improved.
>> * No 32-bit guest OS support.
>> * Multi-monitor scenario is not fully tested, while single monitor of VGA/HDMI/DP/eDP should just work.
>>
>>
>> Where to get:
>> -----------------
>> kerenl: https://github.com/01org/XenGT-Preview-kernel.git
>> Xen: https://github.com/01org/XenGT-Preview-xen.git
>> Qemu: https://github.com/01org/XenGT-Preview-qemu.git
>>
>> Please refer to the new setup guide, which provides step-to-step details about building/configuring/running Intel GVT-g:
>> https://github.com/01org/XenGT-Preview-kernel/blob/master-2015Q1-3.18.0/X...
>>
>> More information about Intel GVT-g background, architecture, etc can be found at:
>> https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian
>> http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20S...
>> https://01.org/xen/blogs/srclarkx/2013/graphics-virtualization-xengt
>>
>> The previous update can be found here:
>> http://lists.xen.org/archives/html/xen-devel/2014-12/msg00474.html
>>
>>
>> Note
>> ---------------
>> The XenGT project should be considered a work in progress, As such it is not a complete product nor should it be considered one., Extra care should be taken when testing and configuring a system to use the XenGT project.
>>
>>
>> --
>> Thanks,
>> Jike
>>
>> On 01/09/2015 04:51 PM, Jike Song wrote:
>>> Hi all,
>>>
>>> We're pleased to announce a public update to Intel Graphics Virtualization Technology (Intel GVT-g, formerly known as XenGT). Intel GVT-g is a complete vGPU solution with mediated pass-through, supported today on 4th generation Intel Core(TM) processors with Intel Graphics processors. A virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance among performance, feature, and sharing capability. Though we only support Xen on Intel Processor Graphics so far, the core logic can be easily ported to other hypervisors. The XenGT project should be considered a work in progress, As such it is not a complete product nor should it be considered one., Extra care should be taken when testing and configuring a system to use the XenGT project.
>>>
>>> The news of this update:
>>>
>>> - kernel update from 3.14.1 to drm-intel 3.17.0.
>>> - We plan to integrate Intel GVT-g as a feature in i915 driver. That effort is still under review, not included in this update yet.
>>> - Next update will be around early Apr, 2015.
>>>
>>> This update consists of:
>>>
>>> - Including some bug fixes and stability enhancement.
>>> - Making XenGT device model to be aware of Broadwell. In this version BDW is not yet functioning.
>>> - Available Fence registers number is changed to 32 from 16 to align with HSW hardware.
>>> - New cascade interrupt framework for supporting interrupt virtualization on both Haswell and Broadwell.
>>> - Add back the gem_vgtbuffer. The previous release did not build that module for 3.14 kernel. In this release, the module is back and rebased to 3.17.
>>> - Enable the irq based context switch in vgt driver, which will help reduce the cpu utilization while doing context switch, it is enabled by default, and can be turn off by kernel flag irq_based_ctx_switch.
>>>
>>>
>>> Please refer to the new setup guide, which provides step-to-step details about building/configuring/running Intel GVT-g:
>>>
>>> https://github.com/01org/XenGT-Preview-kernel/blob/master/XenGT_Setup_Gui...
>>>
>>> The new source codes are available at the updated github repos:
>>>
>>> Linux: https://github.com/01org/XenGT-Preview-kernel.git
>>> Xen: https://github.com/01org/XenGT-Preview-xen.git
>>> Qemu: https://github.com/01org/XenGT-Preview-qemu.git
>>>
>>>
>>> More information about Intel GVT-g background, architecture, etc can be found at:
>>>
>>>
>>> https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian
>>> http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20S...
>>> https://01.org/xen/blogs/srclarkx/2013/graphics-virtualization-xengt
>>>
>>>
>>>
>>> The previous update can be found here:
>>>
>>>
>>> http://lists.xen.org/archives/html/xen-devel/2014-12/msg00474.html
>>>
>>>
>>>
>>> Appreciate your comments!
>>>
>>>
>>>
>>> --
>>> Thanks,
>>> Jike
>>>
>>>
>>> On 12/04/2014 10:45 AM, Jike Song wrote:
>>>> Hi all,
>>>>
>>>> We're pleased to announce a public release to Intel Graphics Virtualization Technology (Intel GVT-g, formerly known as XenGT). Intel GVT-g is a complete vGPU solution with mediated pass-through, supported today on 4th generation Intel Core(TM) processors with Intel Graphics processors. A virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance among performance, feature, and sharing capability. Though we only support Xen on Intel Processor Graphics so far, the core logic can be easily ported to other hypervisors.
>>>>
>>>>
>>>> The news of this update:
>>>>
>>>>
>>>> - kernel update from 3.11.6 to 3.14.1
>>>>
>>>> - We plan to integrate Intel GVT-g as a feature in i915 driver. That effort is still under review, not included in this update yet
>>>>
>>>> - Next update will be around early Jan, 2015
>>>>
>>>>
>>>> This update consists of:
>>>>
>>>> - Windows HVM support with driver version 15.33.3910
>>>>
>>>> - Stability fixes, e.g. stabilize GPU, the GPU hang occurrence rate becomes rare now
>>>>
>>>> - Hardware Media Acceleration for Decoding/Encoding/Transcoding, VC1, H264 etc. format supporting
>>>>
>>>> - Display enhancements, e.g. DP type is supported for virtual PORT
>>>>
>>>> - Display port capability virtualization: with this feature, dom0 manager could freely assign virtual DDI ports to VM, not necessary to check whether the corresponding physical DDI ports are available
>>>>
>>>>
>>>>
>>>> Please refer to the new setup guide, which provides step-to-step details about building/configuring/running Intel GVT-g:
>>>>
>>>>
>>>> https://github.com/01org/XenGT-Preview-kernel/blob/master/XenGT_Setup_Gui...
>>>>
>>>>
>>>>
>>>> The new source codes are available at the updated github repos:
>>>>
>>>>
>>>> Linux: https://github.com/01org/XenGT-Preview-kernel.git
>>>>
>>>> Xen: https://github.com/01org/XenGT-Preview-xen.git
>>>>
>>>> Qemu: https://github.com/01org/XenGT-Preview-qemu.git
>>>>
>>>>
>>>> More information about Intel GVT-g background, architecture, etc can be found at:
>>>>
>>>>
>>>> https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian
>>>>
>>>> http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20S...
>>>>
>>>> https://01.org/xen/blogs/srclarkx/2013/graphics-virtualization-xengt
>>>>
>>>>
>>>> The previous update can be found here:
>>>>
>>>>
>>>> http://lists.xen.org/archives/html/xen-devel/2014-07/msg03248.html
>>>>
>>>>
>>>> Appreciate your comments!
>>>>
>>>>
>>>> --
>>>> Thanks,
>>>> Jike
>>>>
>>>> On 07/25/2014 04:31 PM, Jike Song wrote:
>>>>> Hi all,
>>>>>
>>>>> We're pleased to announce an update to Intel Graphics Virtualization Technology (Intel GVT-g, formerly known as XenGT). Intel GVT-g is a complete vGPU solution with mediated pass-through, supported today on 4th generation Intel Core(TM) processors with Intel Graphics processors. A virtual GPU instance is maintained for each VM, with part of performance critical resources directly assigned. The capability of running native graphics driver inside a VM, without hypervisor intervention in performance critical paths, achieves a good balance among performance, feature, and sharing capability. Though we only support Xen on Intel Processor Graphics so far, the core logic can be easily ported to other hypervisors.
>>>>>
>>>>> The news of this update:
>>>>>
>>>>> - Project code name is "XenGT", now official name is Intel Graphics Virtualization Technology (Intel GVT-g)
>>>>> - Currently Intel GVT-g supports Intel Processor Graphics built into 4th generation Intel Core processors - Haswell platform
>>>>> - Moving forward, XenGT will change to quarterly release cadence. Next update will be around early October, 2014.
>>>>>
>>>>> This update consists of:
>>>>>
>>>>> - Stability fixes, e.g. stable DP support
>>>>> - Display enhancements, e.g. virtual monitor support. Users can define a virtual monitor type with customized EDID for virtual machines, not necessarily the same as physical monitors.
>>>>> - Improved support for GPU recovery
>>>>> - Experimental Windows HVM support. To download the experimental version, see setup guide for details
>>>>> - Experimental Hardware Media Acceleration for decoding.
>>>>>
>>>>>
>>>>> Please refer to the new setup guide, which provides step-to-step details about building/configuring/running Intel GVT-g:
>>>>>
>>>>> https://github.com/01org/XenGT-Preview-kernel/blob/master/XenGT_Setup_Gui...
>>>>>
>>>>>
>>>>> The new source codes are available at the updated github repos:
>>>>>
>>>>> Linux: https://github.com/01org/XenGT-Preview-kernel.git
>>>>> Xen: https://github.com/01org/XenGT-Preview-xen.git
>>>>> Qemu: https://github.com/01org/XenGT-Preview-qemu.git
>>>>>
>>>>>
>>>>> More information about Intel GVT-g background, architecture, etc can be found at:
>>>>>
>>>>> https://www.usenix.org/conference/atc14/technical-sessions/presentation/tian
>>>>> http://events.linuxfoundation.org/sites/events/files/slides/XenGT-Xen%20S...
>>>>> https://01.org/xen/blogs/srclarkx/2013/graphics-virtualization-xengt
>>>>>
>>>>> The previous update can be found here:
>>>>>
>>>>> http://lists.xen.org/archives/html/xen-devel/2014-02/msg01848.html
>>>>>
>>>>> Appreciate your comments!
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Thanks,
>>>>> Jike
>>>>>
4 years, 7 months
Re: [iGVT-g] [RFCv2 03/14] drm/i915: Introduce host graphics memory/fence partition for GVT-g
by Joonas Lahtinen
Hi,
On pe, 2016-02-26 at 13:21 +0800, Zhi Wang wrote:
>
> On 02/24/16 15:42, Tian, Kevin wrote:
> >
> > >
> > > From: Wang, Zhi A
> > > Sent: Tuesday, February 23, 2016 9:23 PM
> > > >
> > > > >
> > > > > --- a/drivers/gpu/drm/i915/gvt/gvt.c
> > > > > +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> > > > > @@ -348,6 +348,10 @@ void *gvt_create_pgt_device(struct drm_i915_private
> > > *dev_priv)
> > > >
> > > > >
> > > > > goto err;
> > > > > }
> > > > >
> > > > > + dev_priv->gvt.host_fence_sz = gvt.host_fence_sz;
> > > > > + dev_priv->gvt.host_low_gm_sz_in_mb = gvt.host_low_gm_sz;
> > > > > + dev_priv->gvt.host_high_gm_sz_in_mb = gvt.host_high_gm_sz;
> > > > I'm thinking, could we expose the pgt_device struct (at least
> > > > partially, and then have a PIMPL pattern), to avoid this kind of
> > > > duplication of declarations and unnecessary copies between i915 and
> > > > i915_gvt modules?
> > > >
> > > > It's little rough that the gvt driver writes to i915_private struct.
> > > > I'd rather see that gvt.host_fence_sz and other variables get sanitized
> > > > and then written to pgt_device (maybe the public part would be
> > > > i915_pgt_device) and used by gvt and i915 code.
> > > >
> > > > Was this ever considered?
> > > >
> > > The previous version do something similar like that, both i915 and gvt
> > > reads GVT module kernel parameter but considered that GVT modules could
> > > be configured as "n" in kernel configuration, probably we should let
> > > i915 to store this information and GVT to configure it if GVT is active?
> > Agree with Joonas. We don't need another gvt wrap. Let's just expose
> > pgt_device directly. I believe all other information can be encapsulated
> > under pgt_device.
> >
> How about this scheme:
>
> 1. Move GVT kernel parameter into intel_gvt.{h, c}
> 2. Sanitize the partition configuration for host in intel_gvt.c
> 3. If CONFIG_DRM_I915_GVT = y, write the configuration into pgt_device
> to inform GVT resource allocator ranges owned by host
>
This sounds fine, if i915 driver wants to know about the gvt driver, it
will read its structure (if gvt was enabled), instead of gvt driver
pushing information to i915.
> >
> > btw to match other description in the code, is it clear to rename pgt_device
> > to gvt_device?
> >
> For the name of GVT physical device, if we use "gvt_device", it looks a
> bit weird when both "gvt_device" and "vgt_device"(vGPU instance)
> appeared in our code? :( And "pgpu" and "vgpu" also looks weird...
>
The naming conventions are indeed very confusing, it would help if all
host related stuff was named gvt_* and client vgpu_*
Regards, Joonas
> >
> > Another minor thing needs Joonas' feedback. Is it usual to capture
> > all module parameters belonging to one feature structurized together
> > (like 'gvt' in this patch), or just to leave them directly exposed?
> >
>
> >
> > Thanks
> > Kevin
> >
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
5 years, 3 months
Re: [iGVT-g] [RFCv2 10/14] drm/i915: update PDPs by condition when submit the LRC context
by Joonas Lahtinen
Hi,
On to, 2016-02-25 at 15:02 +0000, Wang, Zhi A wrote:
>
> -----Original Message-----
> From: Tian, Kevin
> Sent: Wednesday, February 24, 2016 4:50 PM
> To: Wang, Zhi A; intel-gfx(a)lists.freedesktop.org; igvt-g(a)lists.01.org
> Cc: Lv, Zhiyuan; Niu, Bing; Song, Jike; daniel.vetter(a)ffwll.ch; Cowperthwaite, David J; chris(a)chris-wilson.co.uk; joonas.lahtinen(a)linux.intel.com
> Subject: RE: [RFCv2 10/14] drm/i915: update PDPs by condition when submit the LRC context
>
> >
> > From: Wang, Zhi A
> > Sent: Thursday, February 18, 2016 7:42 PM
> >
> > Previously the PDPs inside the ring context are updated at:
> >
> > - When populate a LRC context
> > - Before submitting a LRC context (only for 32 bit PPGTT, as the amount
> > of used PDPs may change)
> >
> > This patch postpones the PDPs upgrade to submission time, and will update
> > it by condition if the PPGTT is 48b. Under GVT-g, one GVT context will be
> > used by different guest, the PPGTT instance related to the context might
> > be changed before the submission time. And this patch gives GVT context
> > a chance to load the new PPGTT instance into an initialized context.
> Could you elaborate why we share one GVT context across different guest?
> A natural thought is that we'll create one GVT context per every guest
> context...
>
> [Zhi] We don't have context creation/destroy notification in guest i915 driver.
> Because in our implementation we need an unique context id to anchor the
> relationship between shadow context and guest context, while i915 uses GGTT
> address as context id. In each context pin/unpin, the context id may be changes.
Does not this lead to plenty of unnecessary storing and restoring of
the context parameters?
I would imagine this to destroy performance.
Regards, Joonas
>
> So it's not necessary to allocate multiple GVT context here.
>
> Thanks
> Kevin
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
5 years, 3 months
Re: [iGVT-g] [RFCv2 03/14] drm/i915: Introduce host graphics memory/fence partition for GVT-g
by Tian, Kevin
> From: Wang, Zhi A
> Sent: Tuesday, February 23, 2016 9:23 PM
> >> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> >> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> >> @@ -348,6 +348,10 @@ void *gvt_create_pgt_device(struct drm_i915_private
> *dev_priv)
> >> goto err;
> >> }
> >>
> >> + dev_priv->gvt.host_fence_sz = gvt.host_fence_sz;
> >> + dev_priv->gvt.host_low_gm_sz_in_mb = gvt.host_low_gm_sz;
> >> + dev_priv->gvt.host_high_gm_sz_in_mb = gvt.host_high_gm_sz;
> >
> > I'm thinking, could we expose the pgt_device struct (at least
> > partially, and then have a PIMPL pattern), to avoid this kind of
> > duplication of declarations and unnecessary copies between i915 and
> > i915_gvt modules?
> >
> > It's little rough that the gvt driver writes to i915_private struct.
> > I'd rather see that gvt.host_fence_sz and other variables get sanitized
> > and then written to pgt_device (maybe the public part would be
> > i915_pgt_device) and used by gvt and i915 code.
> >
> > Was this ever considered?
> >
> The previous version do something similar like that, both i915 and gvt
> reads GVT module kernel parameter but considered that GVT modules could
> be configured as "n" in kernel configuration, probably we should let
> i915 to store this information and GVT to configure it if GVT is active?
Agree with Joonas. We don't need another gvt wrap. Let's just expose
pgt_device directly. I believe all other information can be encapsulated
under pgt_device.
btw to match other description in the code, is it clear to rename pgt_device
to gvt_device?
Another minor thing needs Joonas' feedback. Is it usual to capture
all module parameters belonging to one feature structurized together
(like 'gvt' in this patch), or just to leave them directly exposed?
Thanks
Kevin
5 years, 3 months
Re: [iGVT-g] [RFCv2 02/14] drm/i915/gvt: Introduce the basic architecture of GVT-g
by Joonas Lahtinen
Hi,
Code is looking a lot better.
A general question still; why you seem to be preferring multi-stage
alloc and destroy?
Are there going to be scenarios when things will be allocated but not
initialized? I don't see a such use scenario.
I wouldn't split the init functions down as much as you currently do
because that'll require a lot of boilerplate code to propagate the
errors up, which is currently not done. The boilerplate for propagation
becomes necessary when the teardown function is complex, but currently
the teardown itself is less lines of code than the function
boilerplate.
So just squash those into gvt_device_create() and gvt_device_destroy()
where _create() will propagate any lower level errors up and tear down
a partially initialized struct. _destroy() can then expect to just tear
the whole struct down with no ifs.
Regards, Joonas
On to, 2016-02-18 at 19:42 +0800, Zhi Wang wrote:
> This patch introduces the very basic framework of GVT-g device model,
> includes basic prototypes, definitions, initialization.
>
> v2:
> - Introduce i915_gvt.c.
> It's necessary to introduce the stubs between i915 driver and GVT-g host,
> as GVT-g components is configurable in kernel config. When disabled, the
> stubs here do nothing.
>
> Take Joonas's comments:
> - Replace boolean return value with int.
> - Replace customized info/warn/debug macros with DRM macros.
> - Document all non-static functions like i915.
> - Remove empty and unused functions.
> - Replace magic number with marcos.
> - Set GVT-g in kernel config to "n" by default.
>
> Signed-off-by: Zhi Wang <zhi.a.wang(a)intel.com>
> ---
> drivers/gpu/drm/i915/Kconfig | 15 ++
> drivers/gpu/drm/i915/Makefile | 2 +
> drivers/gpu/drm/i915/gvt/Makefile | 5 +
> drivers/gpu/drm/i915/gvt/debug.h | 57 +++++
> drivers/gpu/drm/i915/gvt/gvt.c | 393 +++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/gvt/gvt.h | 100 +++++++++
> drivers/gpu/drm/i915/gvt/hypercall.h | 30 +++
> drivers/gpu/drm/i915/gvt/mpt.h | 34 +++
> drivers/gpu/drm/i915/gvt/params.c | 32 +++
> drivers/gpu/drm/i915/gvt/params.h | 34 +++
> drivers/gpu/drm/i915/gvt/reg.h | 34 +++
> drivers/gpu/drm/i915/i915_dma.c | 14 ++
> drivers/gpu/drm/i915/i915_drv.h | 12 ++
> drivers/gpu/drm/i915/i915_gvt.c | 93 +++++++++
> drivers/gpu/drm/i915/i915_gvt.h | 49 +++++
> 15 files changed, 904 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/gvt/Makefile
> create mode 100644 drivers/gpu/drm/i915/gvt/debug.h
> create mode 100644 drivers/gpu/drm/i915/gvt/gvt.c
> create mode 100644 drivers/gpu/drm/i915/gvt/gvt.h
> create mode 100644 drivers/gpu/drm/i915/gvt/hypercall.h
> create mode 100644 drivers/gpu/drm/i915/gvt/mpt.h
> create mode 100644 drivers/gpu/drm/i915/gvt/params.c
> create mode 100644 drivers/gpu/drm/i915/gvt/params.h
> create mode 100644 drivers/gpu/drm/i915/gvt/reg.h
> create mode 100644 drivers/gpu/drm/i915/i915_gvt.c
> create mode 100644 drivers/gpu/drm/i915/i915_gvt.h
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 4c59793..082e77d 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -45,3 +45,18 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT
> option changes the default for that module option.
>
> If in doubt, say "N".
> +
> +config DRM_I915_GVT
> + tristate "GVT-g host driver"
> + depends on DRM_I915
> + default n
> + help
> + Enabling GVT-g mediated graphics passthrough technique for Intel i915
> + based integrated graphics card. With GVT-g, it's possible to have one
> + integrated i915 device shared by multiple VMs. Performance critical
> + opterations such as apperture accesses and ring buffer operations
> + are pass-throughed to VM, with a minimal set of conflicting resources
> + (e.g. display settings) mediated by vGT driver. The benefit of vGT
> + is on both the performance, given that each VM could directly operate
> + its aperture space and submit commands like running on native, and
> + the feature completeness, given that a true GEN hardware is exposed.
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 0851de07..c65026c 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -91,6 +91,8 @@ i915-y += dvo_ch7017.o \
> intel_sdvo.o \
> intel_tv.o
>
> +obj-$(CONFIG_DRM_I915_GVT) += i915_gvt.o gvt/
> +
> # virtual gpu code
> i915-y += i915_vgpu.o
>
> diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
> new file mode 100644
> index 0000000..959305f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/Makefile
> @@ -0,0 +1,5 @@
> +GVT_SOURCE := gvt.o params.o
> +
> +ccflags-y += -I$(src) -I$(src)/.. -Wall -Werror -Wno-unused-function
> +i915_gvt-y := $(GVT_SOURCE)
> +obj-$(CONFIG_DRM_I915_GVT) += i915_gvt.o
> diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
> new file mode 100644
> index 0000000..0747f28
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/debug.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef __GVT_DEBUG_H__
> +#define __GVT_DEBUG_H__
> +
> +#define gvt_info(fmt, args...) \
> + DRM_INFO("gvt: "fmt"\n", ##args)
> +
Just;
DRM_INFO("gvt: " fmt, ##args)
Do not automatically add newlines, it will confuse developers. Applies
to all printing.
> +#define gvt_err(fmt, args...) \
> + DRM_ERROR("gvt: "fmt"\n", ##args)
> +
Same here.
> +#define gvt_warn(condition, fmt, args...) \
> + WARN((condition), "gvt: "fmt"\n", ##args)
> +
> +#define gvt_warn_once(condition, fmt, args...) \
> + WARN_ONCE((condition), "gvt: "fmt"\n", ##args)
WARN and WARN_ONCE will include backtrace so prefixing is unnecessary.
I would not prefix them at all. Just use what i915 kernel module
already uses. If needed, split them to their own file first,
i915_debug.h.
> +
> +#define gvt_dbg(level, fmt, args...) \
> + DRM_DEBUG_DRIVER("gvt: "fmt"\n", ##args)
> +
> +enum {
> + GVT_DBG_CORE = (1 << 0),
> + GVT_DBG_MM = (1 << 1),
> + GVT_DBG_IRQ = (1 << 2),
> +};
This enum is not of use.
> +
> +#define gvt_dbg_core(fmt, args...) \
> + gvt_dbg(GVT_DBG_CORE, fmt, ##args)
> +
Rahter like this (not to lose the topic)?
gvt_dbg("core: " fmt, ##args)
> +#define gvt_dbg_mm(fmt, args...) \
> + gvt_dbg(GVT_DBG_MM, fmt, ##args)
> +
> +#define gvt_dbg_irq(fmt, args...) \
> + gvt_dbg(GVT_DBG_IRQ, fmt, ##args)
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> new file mode 100644
> index 0000000..52cfa32
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -0,0 +1,393 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include "gvt.h"
> +
> +struct gvt_host gvt_host;
> +
> +static const char * const supported_hypervisors[] = {
> + [GVT_HYPERVISOR_TYPE_XEN] = "Xen Hypervisor",
> + [GVT_HYPERVISOR_TYPE_KVM] = "KVM",
> +};
> +
> +static int gvt_init_host(void)
> +{
> + struct gvt_host *host = &gvt_host;
> +
Is it really that much more to write gvt_host.initialized? Counting the
"->" vs "." it's three letters...
> + if (!gvt.enable) {
> + gvt_dbg_core("GVT-g has been disabled by kernel parameter");
> + return -EINVAL;
> + }
> +
> + if (host->initialized) {
> + gvt_err("GVT-g has already been initialized!");
> + return -EINVAL;
> + }
> +
> + /* Xen DOM U */
> + if (xen_domain() && !xen_initial_domain())
> + return -ENODEV;
> +
> + if (xen_initial_domain()) {
> + /* Xen Dom0 */
> + host->kdm = try_then_request_module(
> + symbol_get(xengt_kdm), "xengt");
> + host->hypervisor_type = GVT_HYPERVISOR_TYPE_XEN;
> + } else {
> + /* not in Xen. Try KVMGT */
> + host->kdm = try_then_request_module(
> + symbol_get(kvmgt_kdm), "kvm");
> + host->hypervisor_type = GVT_HYPERVISOR_TYPE_KVM;
> + }
> +
> + if (!host->kdm)
> + return -EINVAL;
I think this error should be reported, to aid detecting problems in
module loading.
> +
> + if (!hypervisor_detect_host())
> + return -ENODEV;
> +
This func should be prefixed gvt_, as it is not local to this file.
> + gvt_info("Running with hypervisor %s in host mode",
> + supported_hypervisors[host->hypervisor_type]);
> +
> + idr_init(&host->device_idr);
> + mutex_init(&host->device_idr_lock);
> +
> + host->initialized = true;
> + return 0;
> +}
> +
> +static int init_device_info(struct pgt_device *pdev)
> +{
> + struct gvt_device_info *info = &pdev->device_info;
> +
> + if (!IS_BROADWELL(pdev->dev_priv)) {
> + DRM_DEBUG_DRIVER("Unsupported GEN device");
> + return -ENODEV;
> + }
This could be "else" clause on the next if and will allow easier adding
of future platforms.
> +
> + if (IS_BROADWELL(pdev->dev_priv)) {
> + info->max_gtt_gm_sz = (1ULL << 32); /* 4GB */
> + /*
> + * The layout of BAR0 in BDW:
> + * |< - MMIO 2MB ->|<- Reserved 6MB ->|<- MAX GTT 8MB->|
> + *
> + * GTT offset in BAR0 starts from 8MB to 16MB, and
> + * Whatever GTT size is configured in BIOS,
> + * the size of BAR0 is always 16MB. The actual configured
> + * GTT size can be found in GMCH_CTRL.
> + */
> + info->gtt_start_offset = (1UL << 23); /* 8MB */
> + info->max_gtt_size = (1UL << 23); /* 8MB */
> + info->gtt_entry_size = 8;
> + info->gtt_entry_size_shift = 3;
> + info->gmadr_bytes_in_cmd = 8;
> + info->mmio_size = 2 * 1024 * 1024; /* 2MB */
> + }
> +
> + return 0;
> +}
> +
> +static void init_initial_cfg_space_state(struct pgt_device *pdev)
> +{
> + struct pci_dev *pci_dev = pdev->dev_priv->dev->pdev;
> + int i;
> +
> + gvt_dbg_core("init initial cfg space, id %d", pdev->id);
> +
> + for (i = 0; i < GVT_CFG_SPACE_SZ; i += 4)
> + pci_read_config_dword(pci_dev, i,
> + (u32 *)&pdev->initial_cfg_space[i]);
> +
> + for (i = 0; i < GVT_BAR_NUM; i++) {
> + pdev->bar_size[i] = pci_resource_len(pci_dev, i * 2);
> + gvt_dbg_core("bar %d size: %llx", i, pdev->bar_size[i]);
> + }
> +}
> +
> +static void clean_initial_mmio_state(struct pgt_device *pdev)
> +{
> + if (pdev->gttmmio_va) {
> + iounmap(pdev->gttmmio_va);
> + pdev->gttmmio_va = NULL;
> + }
> +
> + if (pdev->gmadr_va) {
> + iounmap(pdev->gmadr_va);
> + pdev->gmadr_va = NULL;
> + }
> +}
> +
> +static int init_initial_mmio_state(struct pgt_device *pdev)
> +{
> + struct gvt_device_info *info = &pdev->device_info;
> +
> + u64 bar0, bar1;
> + int rc;
> +
> + gvt_dbg_core("init initial mmio state, id %d", pdev->id);
> +
> + bar0 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR0];
> + bar1 = *(u64 *)&pdev->initial_cfg_space[GVT_REG_CFG_SPACE_BAR1];
> +
> + pdev->gttmmio_base = bar0 & ~0xf;
> + pdev->reg_num = info->mmio_size / 4;
> + pdev->gmadr_base = bar1 & ~0xf;
Magic numbers still.
> +
> + pdev->gttmmio_va = ioremap(pdev->gttmmio_base, pdev->bar_size[0]);
> + if (!pdev->gttmmio_va) {
> + gvt_err("fail to map GTTMMIO BAR.");
> + return -EFAULT;
> + }
> +
> + pdev->gmadr_va = ioremap(pdev->gmadr_base, pdev->bar_size[2]);
> + if (!pdev->gmadr_va) {
> + gvt_err("fail to map GMADR BAR.");
> + rc = -EFAULT;
> + goto err;
> + }
> +
> + gvt_dbg_core("bar0: 0x%llx, bar1: 0x%llx", bar0, bar1);
> + gvt_dbg_core("mmio size: %x", pdev->mmio_size);
> + gvt_dbg_core("gttmmio: 0x%llx, gmadr: 0x%llx", pdev->gttmmio_base,
> + pdev->gmadr_base);
> + gvt_dbg_core("gttmmio_va: %p", pdev->gttmmio_va);
> + gvt_dbg_core("gmadr_va: %p", pdev->gmadr_va);
> +
> + return 0;
> +err:
> + clean_initial_mmio_state(pdev);
> + return rc;
> +}
> +
> +static int gvt_service_thread(void *data)
> +{
> + struct pgt_device *pdev = (struct pgt_device *)data;
> + int r;
> +
> + gvt_dbg_core("service thread start, pgt %d", pdev->id);
> +
> + while (!kthread_should_stop()) {
> + r = wait_event_interruptible(pdev->service_thread_wq,
> + kthread_should_stop() || pdev->service_request);
> +
> + if (kthread_should_stop())
> + break;
> +
> + if (gvt_warn_once(r,
> + "service thread is waken up by unexpected signal."))
> + continue;
> + }
> +
> + return 0;
> +}
> +
> +static void clean_service_thread(struct pgt_device *pdev)
> +{
> + if (pdev->service_thread) {
> + kthread_stop(pdev->service_thread);
> + pdev->service_thread = NULL;
> + }
> +}
> +
> +static int init_service_thread(struct pgt_device *pdev)
> +{
> + init_waitqueue_head(&pdev->service_thread_wq);
> +
> + pdev->service_thread = kthread_run(gvt_service_thread,
> + pdev, "gvt_service_thread%d", pdev->id);
> +
> + if (!pdev->service_thread) {
> + gvt_err("fail to start service thread.");
> + return -ENOSPC;
> + }
> +
> + return 0;
> +}
> +
> +static void clean_pgt_device(struct pgt_device *pdev)
> +{
> + clean_service_thread(pdev);
> + clean_initial_mmio_state(pdev);
> +}
> +
> +static int init_pgt_device(struct pgt_device *pdev,
> + struct drm_i915_private *dev_priv)
> +{
> + int rc;
int ret;
> +
> + rc = init_device_info(pdev);
> + if (rc)
> + return rc;
> +
> + init_initial_cfg_space_state(pdev);
> +
> + rc = init_initial_mmio_state(pdev);
> + if (rc)
> + goto err;
> +
> + rc = init_service_thread(pdev);
> + if (rc)
> + goto err;
> +
> + return 0;
> +err:
> + clean_pgt_device(pdev);
Add teardown path, see below.
> + return rc;
> +}
> +
> +static int post_init_pgt_device(struct pgt_device *pdev)
> +{
> + return 0;
> +}
> +
> +static void free_pgt_device(struct pgt_device *pdev)
> +{
> + struct gvt_host *host = &gvt_host;
> +
> + mutex_lock(&host->device_idr_lock);
> + idr_remove(&host->device_idr, pdev->id);
> + mutex_unlock(&host->device_idr_lock);
> +
> + vfree(pdev);
> +}
> +
> +static struct pgt_device *alloc_pgt_device(struct drm_i915_private *dev_priv)
> +{
> + struct gvt_host *host = &gvt_host;
> + struct pgt_device *pdev = NULL;
> +
> + pdev = vzalloc(sizeof(*pdev));
> + if (!pdev)
> + return NULL;
This is a memory error, would like to see this propagated up as
return ERR_PTR(-ENOMEM);
> +
> + mutex_lock(&host->device_idr_lock);
> + pdev->id = idr_alloc(&host->device_idr, pdev, 0, 0, GFP_KERNEL);
> + mutex_unlock(&host->device_idr_lock);
> +
> + if (pdev->id < 0) {
> + gvt_err("fail to allocate pgt device id.");
> + goto err;
> + }
> +
Same here, propagate the error.
> + mutex_init(&pdev->lock);
> + pdev->dev_priv = dev_priv;
> + idr_init(&pdev->instance_idr);
> +
> + return pdev;
> +err:
> + free_pgt_device(pdev);
I'd like to see a teardown path here. Then free_pgt_device() (or rather
destroy_pgt_device() can expect everything to be initialized and when
it it is called, it doesn't need ifs. This makes the driver code more
robust.
Or are we expecting only partially initialized structs for some reason?
> + return NULL;
> +}
> +
> +void gvt_destroy_pgt_device(void *private_data)
> +{
> + struct pgt_device *pdev = (struct pgt_device *)private_data;
> +
> + clean_pgt_device(pdev);
> + free_pgt_device(pdev);
Why multiple calls to destroy a device, there's only one alloc still?
> +}
> +
> +/**
> + * gvt_create_pgt_device - create a GVT physical device
> + * @dev: drm device
> + *
> + * This function is called at the initialization stage, to create a physical
> + * GVT device and initialize necessary GVT components for it.
> + *
> + * Returns:
> + * pointer to the pgt_device structure, NULL if failed.
> + */
> +void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
> +{
> + struct pgt_device *pdev = NULL;
> + struct gvt_host *host = &gvt_host;
> + int rc;
> +
> + if (!host->initialized && !gvt_init_host()) {
> + gvt_err("gvt_init_host fail");
> + return NULL;
> + }
> +
> + gvt_dbg_core("create new pgt device, i915 dev_priv: %p", dev_priv);
> +
> + pdev = alloc_pgt_device(dev_priv);
> + if (!pdev) {
> + gvt_err("fail to allocate memory for pgt device.");
> + goto err;
> + }
> +
> + gvt_dbg_core("init pgt device, id %d", pdev->id);
> +
> + rc = init_pgt_device(pdev, dev_priv);
> + if (rc) {
Just call the return value variable ret like everywhere.
> + gvt_err("fail to init physical device state.");
> + goto err;
> + }
> +
> + gvt_dbg_core("pgt device creation done, id %d", pdev->id);
> +
> + return pdev;
> +err:
> + if (pdev) {
> + gvt_destroy_pgt_device(pdev);
> + pdev = NULL;
> + }
Proper goto label based teardown path should be used.
err_destroy_pgt:
gvt_destroy_pgt_device(pdev);
err:
> + return NULL;
> +}
> +
> +/**
> + * gvt_post_init_pgt_device - post init a GVT physical device
> + * @dev: drm device
Double check the kerneldocs to be correct per arguments of function.
> + *
> + * This function is called at the end of the initialization stage, to
> + * post-initialize a physical GVT device and initialize necessary
> + * GVT components rely on i915 components.
> + *
> + * Returns:
> + * zero on success, non-zero if failed.
> + */
> +int gvt_post_init_pgt_device(void *private_data)
> +{
> + struct pgt_device *pdev = (struct pgt_device *)private_data;
> + struct gvt_host *host = &gvt_host;
> + int rc;
> +
> + if (!host->initialized) {
> + gvt_err("gvt_host haven't been initialized.");
> + return -ENODEV;
> + }
> +
> + gvt_dbg_core("post init pgt device %d", pdev->id);
> +
> + rc = post_init_pgt_device(pdev);
> + if (rc) {
> + gvt_err("fail to post init physical device state.");
> + return rc;
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> new file mode 100644
> index 0000000..d450198
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _GVT_H_
> +#define _GVT_H_
> +
> +#include "i915_drv.h"
> +#include "i915_vgpu.h"
> +
> +#include "debug.h"
> +#include "params.h"
> +#include "reg.h"
> +#include "hypercall.h"
> +#include "mpt.h"
> +
> +#define GVT_MAX_VGPU 8
> +
> +enum {
> + GVT_HYPERVISOR_TYPE_XEN = 0,
> + GVT_HYPERVISOR_TYPE_KVM,
> +};
> +
> +struct gvt_host {
> + bool initialized;
> + int hypervisor_type;
> + struct mutex device_idr_lock;
> + struct idr device_idr;
> + struct gvt_kernel_dm *kdm;
> +};
> +
> +extern struct gvt_host gvt_host;
-->
> +extern struct gvt_kernel_dm xengt_kdm;
> +extern struct gvt_kernel_dm kvmgt_kdm;
<-- These should likely be declared somewhere in include/ rather than
here.
> +
> +/* Describe the limitation of HW.*/
> +struct gvt_device_info {
> + u64 max_gtt_gm_sz;
> + u32 gtt_start_offset;
> + u32 gtt_end_offset;
> + u32 max_gtt_size;
> + u32 gtt_entry_size;
> + u32 gtt_entry_size_shift;
> + u32 gmadr_bytes_in_cmd;
> + u32 mmio_size;
> +};
> +
> +struct vgt_device {
> + int id;
> + int vm_id;
> + struct pgt_device *pdev;
> + bool warn_untrack;
> +};
> +
> +struct pgt_device {
Comments to this and other structs about what the memebers are.
> + struct mutex lock;
> + int id;
> +
> + struct drm_i915_private *dev_priv;
> + struct idr instance_idr;
> +
> + struct gvt_device_info device_info;
> +
> + u8 initial_cfg_space[GVT_CFG_SPACE_SZ];
> + u64 bar_size[GVT_BAR_NUM];
> +
> + u64 gttmmio_base;
> + void *gttmmio_va;
> +
> + u64 gmadr_base;
> + void *gmadr_va;
> +
> + u32 mmio_size;
> + u32 reg_num;
> +
> + wait_queue_head_t service_thread_wq;
> + struct task_struct *service_thread;
> + unsigned long service_request;
> +};
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h
> new file mode 100644
> index 0000000..0a41874
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/hypercall.h
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _GVT_HYPERCALL_H_
> +#define _GVT_HYPERCALL_H_
> +
> +struct gvt_kernel_dm {
> +};
I would prefer more code introduced here in the initial patch, or this
can be introduced later in the series as whole.
It unnecessarily complicates the review if some files and calls are
introduced with no documentation and implementation and only later
their implementation is added.
I can't really review if using a structure is a good idea if I can't
see the context or implementation of their use.
> +
> +#endif /* _GVT_HYPERCALL_H_ */
> diff --git a/drivers/gpu/drm/i915/gvt/mpt.h b/drivers/gpu/drm/i915/gvt/mpt.h
> new file mode 100644
> index 0000000..e594bb8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _GVT_MPT_H_
> +#define _GVT_MPT_H_
> +
> +struct vgt_device;
> +
> +static inline bool hypervisor_detect_host(void)
> +{
> + return false;
> +}
This is also not very reviewable and there's an unnecessary forward
declaration.
> +
> +#endif /* _GVT_MPT_H_ */
> diff --git a/drivers/gpu/drm/i915/gvt/params.c b/drivers/gpu/drm/i915/gvt/params.c
> new file mode 100644
> index 0000000..d381d17
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/params.c
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include "gvt.h"
> +
> +struct gvt_kernel_params gvt = {
> + .enable = false,
> + .debug = 0,
> +};
> +
> +module_param_named(gvt_enable, gvt.enable, bool, 0600);
This should just be
module_param_named(enable, gvt.enable, bool, ...)
otherwise parameter has to be passed at boot time like this:
gvt.gvt_enable=1
Where we want
gvt.enable=1
Right?
> +MODULE_PARM_DESC(gvt_enable, "Enable Intel GVT-g host support");
> diff --git a/drivers/gpu/drm/i915/gvt/params.h b/drivers/gpu/drm/i915/gvt/params.h
> new file mode 100644
> index 0000000..d2955b9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/params.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _GVT_PARAMS_H_
> +#define _GVT_PARAMS_H_
> +
> +struct gvt_kernel_params {
> + bool enable;
> + int debug;
This member is unused currently, can be dropped.
> +};
> +
> +extern struct gvt_kernel_params gvt;
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/gvt/reg.h b/drivers/gpu/drm/i915/gvt/reg.h
> new file mode 100644
> index 0000000..d363b74
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gvt/reg.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _GVT_REG_H
> +#define _GVT_REG_H
> +
> +#define GVT_CFG_SPACE_SZ 256
> +#define GVT_BAR_NUM 4
> +
> +#define GVT_REG_CFG_SPACE_BAR0 0x10
> +#define GVT_REG_CFG_SPACE_BAR1 0x18
> +#define GVT_REG_CFG_SPACE_BAR2 0x20
Some documentation here would be great.
> +
> +#endif
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 1c6d227..f3bed37 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -35,6 +35,7 @@
> #include "intel_drv.h"
> #include
> #include "i915_drv.h"
> +#include "i915_gvt.h"
> #include "i915_vgpu.h"
> #include "i915_trace.h"
> #include
> @@ -1045,6 +1046,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>
> intel_uncore_init(dev);
>
> + ret = intel_gvt_init(dev);
> + if (ret)
> + goto out_uncore_fini;
> +
> ret = i915_gem_gtt_init(dev);
> if (ret)
> goto out_uncore_fini;
This needs to become "goto out_gvt_cleanup;"
> @@ -1135,6 +1140,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> goto out_power_well;
> }
>
> + ret = intel_gvt_post_init(dev);
> + if (ret) {
> + DRM_ERROR("failed to post init pgt device\n");
> + goto out_power_well;
> + }
> +
> /*
> * Notify a valid surface after modesetting,
> * when running inside a VM.
> @@ -1177,6 +1188,7 @@ out_gem_unload:
> out_gtt:
> i915_global_gtt_cleanup(dev);
out_gvt_cleanup:
> out_uncore_fini:
>
> + intel_gvt_cleanup(dev);
This needs to be lifted up to its own label ensure proper teardown if
i915_gem_gtt_init() fails.
> intel_uncore_fini(dev);
> i915_mmio_cleanup(dev);
> put_bridge:
> @@ -1223,6 +1235,8 @@ int i915_driver_unload(struct drm_device *dev)
>
> intel_modeset_cleanup(dev);
>
> + intel_gvt_cleanup(dev);
> +
> /*
> * free the memory space allocated for the child device
> * config parsed from VBT
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 20d9dbd..2f897c3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1705,6 +1705,10 @@ struct i915_workarounds {
> u32 hw_whitelist_count[I915_NUM_RINGS];
> };
>
> +struct i915_gvt {
> + void *pgt_device;
> +};
> +
> struct i915_virtual_gpu {
> bool active;
> };
> @@ -1744,6 +1748,8 @@ struct drm_i915_private {
>
> struct i915_virtual_gpu vgpu;
>
> + struct i915_gvt gvt;
> +
> struct intel_guc guc;
>
> struct intel_csr csr;
> @@ -2780,6 +2786,12 @@ void intel_uncore_forcewake_get__locked(struct drm_i915_private *dev_priv,
> void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
> enum forcewake_domains domains);
> void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
> +
> +static inline bool intel_gvt_active(struct drm_device *dev)
> +{
> + return to_i915(dev)->gvt.pgt_device ? true : false;
> +}
> +
> static inline bool intel_vgpu_active(struct drm_device *dev)
> {
> return to_i915(dev)->vgpu.active;
> diff --git a/drivers/gpu/drm/i915/i915_gvt.c b/drivers/gpu/drm/i915/i915_gvt.c
> new file mode 100644
> index 0000000..3ca7232
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gvt.c
> @@ -0,0 +1,93 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include "i915_drv.h"
> +#include "i915_gvt.h"
> +
> +/**
> + * DOC: Intel GVT-g host support
> + *
> + * Intel GVT-g is a graphics virtualization technology which shares the
> + * GPU among multiple virtual machines on a time-sharing basis. Each
> + * virtual machine is presented a virtual GPU (vGPU), which has equivalent
> + * features as the underlying physical GPU (pGPU), so i915 driver can run
> + * seamlessly in a virtual machine. This file provides the englightments
> + * of GVT and the necessary components used by GVT in i915 driver.
> + */
> +
> +/**
> + * intel_gvt_init - initialize GVT components at the beginning of i915
> + * driver loading.
> + * @dev: drm device *
> + *
> + * This function is called at the beginning of the initialization stage,
> + * to initialize the GVT components that have to be initialized
> + * before HW gets touched by other i915 components.
> + */
> +int intel_gvt_init(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + dev_priv->gvt.pgt_device = gvt_create_pgt_device(dev_priv);
> + if (intel_gvt_active(dev))
> + DRM_DEBUG_DRIVER("GVT-g is running in host mode\n");
> +
> + return 0;
> +}
> +
> +/**
> + * intel_gvt_post_init - initialize GVT components at the end of i915
> + * driver loading.
> + * @dev: drm device *
> + *
> + * This function is called at the end of the initialization stage,
> + * to initialize the GVT components that have to be initialized after
> + * other i915 components are ready.
> + */
> +int intel_gvt_post_init(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + if (!intel_gvt_active(dev))
> + return 0;
> +
> + return gvt_post_init_pgt_device(dev_priv->gvt.pgt_device);
> +}
> +
> +/**
> + * intel_gvt_cleanup - cleanup GVT components when i915 driver is unloading
> + * @dev: drm device *
> + *
> + * This function is called at the i915 driver unloading stage, to shutdown
> + * GVT components and release the related resources.
> + */
> +void intel_gvt_cleanup(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + if (!intel_gvt_active(dev))
> + return;
> +
> + gvt_destroy_pgt_device(dev_priv->gvt.pgt_device);
> + dev_priv->gvt.pgt_device = NULL;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_gvt.h b/drivers/gpu/drm/i915/i915_gvt.h
> new file mode 100644
> index 0000000..30cc207
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gvt.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _I915_GVT_H_
> +#define _I915_GVT_H_
I think this file could be intel_gvt.h (all remaining functions are
intel_gvt), and have respective intel_gvt.c file.
> +
> +#ifdef CONFIG_DRM_I915_GVT
Starting here --->
> +extern void *gvt_create_pgt_device(struct drm_i915_private *dev_priv);
> +extern int gvt_post_init_pgt_device(void *data);
> +extern void gvt_destroy_pgt_device(void *data);
> +
<-- to here, these should go to their own include file at
include/drm/i915_gvt.h For an example, see include/drm/i915_drm.h
The respective export symbols then need to be exported, For an example
see;
EXPORT_SYMBOL_GPL(i915_gpu_raise);
> +extern int intel_gvt_init(struct drm_device *dev);
> +extern int intel_gvt_post_init(struct drm_device *dev);
> +extern void intel_gvt_cleanup(struct drm_device *dev);
> +#else
> +extern int intel_gvt_init(struct drm_device *dev)
These should, by convention, rather be static inline;
static inline int intel_gvt_init(...)
> +{
> + return 0;
> +}
> +extern int intel_gvt_post_init(struct drm_device *dev)
> +{
> + return 0;
> +}
> +extern void intel_gvt_cleanup(struct drm_device *dev)
> +{
> +}
> +#endif
> +
> +#endif /* _I915_GVT_H_ */
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
5 years, 3 months
Re: [iGVT-g] [RFCv2 PATCH 00/14] gvt: Hacking i915 for GVT context requirement
by Tian, Kevin
> From: Wang, Zhi A
> Sent: Wednesday, February 24, 2016 5:19 PM
>
> Hi Kevin:
> Now our context switch is covered by context status change notification handler. In the
> status change handler we will save render registers. As GVT context will be a single
> submission context we will restore the render registers when the GVT context is
> scheduled-out by HW.
Thanks for explanation. It's fine then.
>
> > -----Original Message-----
> > From: Tian, Kevin
> > Sent: Wednesday, February 24, 2016 4:56 PM
> > To: Wang, Zhi A; intel-gfx(a)lists.freedesktop.org; igvt-g(a)lists.01.org
> > Cc: Lv, Zhiyuan; Niu, Bing; Song, Jike; daniel.vetter(a)ffwll.ch; Cowperthwaite,
> > David J; chris(a)chris-wilson.co.uk; joonas.lahtinen(a)linux.intel.com
> > Subject: RE: [RFCv2 PATCH 00/14] gvt: Hacking i915 for GVT context
> > requirement
> >
> > > From: Wang, Zhi A
> > > Sent: Thursday, February 18, 2016 7:42 PM
> > >
> > > This patchset is used to discuss and finalize the i915 changes
> > > required by GVT context. Previously we have discussed about how to
> > > hack i915 to meet GVT context requirement, and thanks for the idea and
> > comments.
> > >
> > > In this patchset, mostly it refactors the existing i915 APIs, spliting
> > > the hard-coded assumptions from its core logic, keep these assumptions
> > > in the high level wrapper and make the core logic much more flexible
> > > and config- urable, which is able to be used by GVT context creation and
> > submission.
> >
> > It would be good to note that this patch series is not the only change required
> > for GVT context management. It addresses creation/submission. We also need
> > some specific change in context switch time. Do you want to include them
> > together in next version to compose a full picture?
> >
> > Thanks
> > Kevin
5 years, 3 months
Re: [iGVT-g] [RFCv2 PATCH 00/14] gvt: Hacking i915 for GVT context requirement
by Tian, Kevin
> From: Wang, Zhi A
> Sent: Thursday, February 18, 2016 7:42 PM
>
> This patchset is used to discuss and finalize the i915 changes required by
> GVT context. Previously we have discussed about how to hack i915 to meet
> GVT context requirement, and thanks for the idea and comments.
>
> In this patchset, mostly it refactors the existing i915 APIs, spliting the
> hard-coded assumptions from its core logic, keep these assumptions in the
> high level wrapper and make the core logic much more flexible and config-
> urable, which is able to be used by GVT context creation and submission.
It would be good to note that this patch series is not the only change required
for GVT context management. It addresses creation/submission. We also
need some specific change in context switch time. Do you want to include them
together in next version to compose a full picture?
Thanks
Kevin
5 years, 3 months
Re: [iGVT-g] [RFCv2 13/14] drm/i915: Support context single submission when GVT is active
by Tian, Kevin
> From: Wang, Zhi A
> Sent: Thursday, February 18, 2016 7:42 PM
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 1c0366a..2a6d6fe 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -479,6 +479,40 @@ static void execlists_i915_pick_requests(struct intel_engine_cs
> *ring,
> }
> }
>
> +static void execlists_gvt_pick_requests(struct intel_engine_cs *ring,
> + struct drm_i915_gem_request **req0,
> + struct drm_i915_gem_request **req1)
not read carefully, but if single submission itself has nothing tied to GVT,
better to make it generic since there may be other users in the future...
Thanks
Kevin
5 years, 3 months
Re: [iGVT-g] [RFCv2 10/14] drm/i915: update PDPs by condition when submit the LRC context
by Tian, Kevin
> From: Wang, Zhi A
> Sent: Thursday, February 18, 2016 7:42 PM
>
> Previously the PDPs inside the ring context are updated at:
>
> - When populate a LRC context
> - Before submitting a LRC context (only for 32 bit PPGTT, as the amount
> of used PDPs may change)
>
> This patch postpones the PDPs upgrade to submission time, and will update
> it by condition if the PPGTT is 48b. Under GVT-g, one GVT context will be
> used by different guest, the PPGTT instance related to the context might
> be changed before the submission time. And this patch gives GVT context
> a chance to load the new PPGTT instance into an initialized context.
Could you elaborate why we share one GVT context across different guest?
A natural thought is that we'll create one GVT context per every guest
context...
Thanks
Kevin
5 years, 3 months