[Intel-gfx] [RFC 01/29] drm/i915/gvt: Introduce the basic architecture of GVT-g
Jani Nikula
jani.nikula at linux.intel.com
Tue Feb 16 12:44:32 UTC 2016
On Tue, 16 Feb 2016, Zhi Wang <zhi.a.wang at intel.com> wrote:
> Hi Joonas:
> For the debug function/macros, could we use DRM_DEBUG_DRIVER as
> output, but keep our internal debug switch & marcos, like:
>
> #define gvt_dbg_xxx(xxx, xxx) \
> if (gvt_debug & xxx) \
> DRM_DEBUG_DRIVER(xxxxxx)
>
> Is it OK for the maintainers? Or we have to use DRM_DEBUG_DRIVER
> directly :(.
Rather than introducing new debug macros of your own, I think I'd rather
see any combination of the following:
* DRM_DEBUG_*, DRM_ERROR, and friends.
* dev_dbg, dev_info, dev_err, and friends. See the dynamic debug
facilities for fine grained control of what is printed.
* pr_debug, pr_info, pr_err, and friends. See pr_fmt for defining custom
prefixes for what is printed.
BR,
Jani.
>
> Thanks,
> Zhi.
>
> 于 02/03/16 14:01, Zhi Wang wrote:
>> Hi Joonas:
>> Thanks you very much! We're very excited for receiving your advice
>> and continue to be open to any comments. :)
>>
>> I'm supposed that we should make the agreements on i915 host change at
>> the very beginning, as I'm concerned during the discussion of i915 host
>> change, you know, maybe some code of GVT-g needs to be refined or
>> re-designed. So we put the i915 host changes to the beginning of the
>> patch set for the convenience of discussion.
>>
>> I summarize your comments as below, very applicate. :)
>>
>> - Replace boolean return value with int as much as possible.
>> - Replace customized debug ASSERT() function & macros with DRM_DEBUG_*
>> - Document all non-static functions like i915
>> - Fix all whitespace via scripts/cleanpatch.pl
>> - Commit function structure refinement.
>> - Change the register access behavior just like what i915 does.
>>
>> For other comments, see my comments below. :)
>>
>> On 01/29/16 21:57, Joonas Lahtinen wrote:
>>> Hi,
>>>
>>> TL;DR Overall, we have same problem as with the scheduler series, there
>>> is too much placeholder stuff for easy review. Just squash enough code
>>> into one commit so it actually does something logical that can be
>>> reviewed and then extend it later. Then it can be reviewed and pushed.
>>> Just splitting the code down to achieve smaller patches is not the
>>> right thing to do.
>>>
>>> Comments on the overall code: You need to document all header file
>>> functions (in the source files), and it is good to document the static
>>> functions within a file too, to make future maintenance easier.
>>>
>>> It is not about splitting the code down to small chunks, but splitting
>>> it down to small *logical* chunks. It doesn't make sense to commit
>>> dozens of empty bodied functions for review, and then later add their
>>> code.
>>>
>>> If you add functions, only add them at a patch that takes them into use
>>> too, unless we're talking about general purpose shared code. And also
>>> remember to add the function body and documentation header. If you
>>> simply add a "return 0;" or similar function body, do add a comment to
>>> why the function does not exist and when it will.
>>>
>>> Then, there is a trend of having a boolean return values in the code.
>>> When possible, it should rather be int and the cause for failure should
>>> be propagated from the last level all the way to up (-ENOMEN etc.).
>>> This way debugging becomes easier and if new error conditions appear,
>>> there is less of a maintenance burden to add the propagation later.
>>>
>>> Finally, make sure to look at the existing driver parts and
>>> https://www.kernel.org/doc/Documentation/CodingStyle
>>> for proper coding style. There are lots of whitespace fixes needed in
>>> this series, like array initializations.
>>>
>>> I hope to see this first patch rerolled so that you squash some of the
>>> later commits into it so that all functions have a body and you add
>>> documentation for the functions so I can both see what it should do and
>>> what it actually does. Only reroll the first patch, to keep the
>>> iterative step smaller. Lets only then continue with the rest of the
>>> series once we've reached a consensus on the formatting and style
>>> basics.
>>>
>>> See more comments below.
>>>
>>> On to, 2016-01-28 at 18:21 +0800, Zhi Wang wrote:
>>>> This patch introduces the very basic framework of GVT-g device model,
>>>> includes basic prototypes, definitions, initialization.
>>>> ---
>>>> arch/x86/include/asm/xen/interface.h | 3 +
>>>> drivers/gpu/drm/i915/Kconfig | 16 ++
>>>> drivers/gpu/drm/i915/Makefile | 2 +
>>>> drivers/gpu/drm/i915/gvt/Makefile | 5 +
>>>> drivers/gpu/drm/i915/gvt/debug.h | 72 +++++++
>>>> drivers/gpu/drm/i915/gvt/fb_decoder.c | 34 ++++
>>>> drivers/gpu/drm/i915/gvt/fb_decoder.h | 110 ++++++++++
>>>> drivers/gpu/drm/i915/gvt/gvt.c | 366
>>>> ++++++++++++++++++++++++++++++++++
>>>> drivers/gpu/drm/i915/gvt/gvt.h | 130 ++++++++++++
>>>> drivers/gpu/drm/i915/gvt/hypercall.h | 30 +++
>>>> drivers/gpu/drm/i915/gvt/mpt.h | 97 +++++++++
>>>> drivers/gpu/drm/i915/gvt/params.c | 29 +++
>>>> drivers/gpu/drm/i915/gvt/params.h | 34 ++++
>>>> drivers/gpu/drm/i915/gvt/reg.h | 34 ++++
>>>> drivers/gpu/drm/i915/i915_dma.c | 19 ++
>>>> drivers/gpu/drm/i915/i915_drv.h | 6 +
>>>> drivers/gpu/drm/i915/i915_vgpu.h | 3 +
>>>> include/xen/interface/xen.h | 1 +
>>>> 18 files changed, 991 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/fb_decoder.c
>>>> create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.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
>>>>
>>>> diff --git a/arch/x86/include/asm/xen/interface.h
>>>> b/arch/x86/include/asm/xen/interface.h
>>>> index 62ca03e..6ff4986 100644
>>>> --- a/arch/x86/include/asm/xen/interface.h
>>>> +++ b/arch/x86/include/asm/xen/interface.h
>>>> @@ -73,6 +73,9 @@
>>>> #endif
>>>>
>>>> #ifndef __ASSEMBLY__
>>>> +
>>>> +#include
>>>> +
>>>
>>> I don't follow why this would need to be added if the file is not
>>> modified otherwise. Each header should only include what they use.
>>>
>>> If this is an existing bug (that xen/interface.h can not be included
>>> without including linux/types.h), it should be split to a separate
>>> patch and sent to Xen team. Same for include/xen/interface/xen.h.
>>>
>>>> /* Explicitly size integers that represent pfns in the public
>>>> interface
>>>> * with Xen so that on ARM we can have one ABI that works for 32
>>>> and 64
>>>> * bit guests. */
>>>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>>>> index 051eab3..89ff723 100644
>>>> --- a/drivers/gpu/drm/i915/Kconfig
>>>> +++ b/drivers/gpu/drm/i915/Kconfig
>>>> @@ -47,3 +47,19 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT
>>>> option changes the default for that module option.
>>>>
>>>> If in doubt, say "N".
>>>> +
>>>> +config I915_GVT
>>>> + tristate "GVT-g host driver"
>>>> + depends on DRM_I915
>>>> + select IRQ_WORK
>>>> + default y
>>>
>>> Defaulting to "n" would make sense initially.
>>>
>> [Zhi] Sure, will do.
>>>> + 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..d4df410 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_I915_GVT) += 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..6935b78
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/gvt/Makefile
>>>> @@ -0,0 +1,5 @@
>>>> +GVT_SOURCE := gvt.o params.o fb_decoder.o
>>>> +
>>>> +ccflags-y += -I$(src) -I$(src)/.. -Wall -Werror
>>>> -Wno-unused-function
>>>> +i915_gvt-y := $(GVT_SOURCE)
>>>> +obj-$(CONFIG_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..18e1467
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/gvt/debug.h
>>>> @@ -0,0 +1,72 @@
>>>> +/*
>>>> + * 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
>>>> ASSERT(x) \
>>>> + do
>>>> { \
>>>> + if (!(x))
>>>> { \
>>>> + printk("Assert at %s line
>>>> %d\n", \
>>>> + __FILE__,
>>>> __LINE__); \
>>>> +
>>>> } \
>>>> + } while (0);
>>>> +
>>>> +#define ASSERT_NUM(x,
>>>> y) \
>>>> + do
>>>> { \
>>>> + if (!(x))
>>>> { \
>>>> + printk("Assert at %s line %d para
>>>> 0x%llx\n", \
>>>> + __FILE__, __LINE__,
>>>> (u64)y); \
>>>> +
>>>> } \
>>>> + } while (0);
>>>> +
>>>
>>> There already is WARN_ON (and i915_drv.h modifies it a little). Do not
>>> introduce custom functions like this, if the existing ones need
>>> improvement, improve them.
>>>
>> [Zhi] OK. Will do as what i915 does
>>>> +#define gvt_info(fmt, args...) \
>>>> + printk(KERN_INFO"[GVT-g] "fmt"\n", ##args)
>>>> +
>>>> +#define gvt_err(fmt, args...) \
>>>> + printk(KERN_ERR"%s() - %d: "fmt"\n", __func__, __LINE__, ##args)
>>>> +
>>>> +#define gvt_warn(fmt, args...) \
>>>> + printk(KERN_WARNING"%s() - %d: "fmt"\n", __func__, __LINE__,
>>>> ##args)
>>>> +
>>>> +#define gvt_dbg(level, fmt, args...) do { \
>>>> + if (gvt.debug & level) \
>>>> + printk(KERN_DEBUG"%s() - %d: "fmt"\n", __func__,
>>>> __LINE__, ##args); \
>>>> + }while(0)
>>>> +
>>>> +enum {
>>>> + GVT_DBG_CORE = (1 << 0),
>>>> + GVT_DBG_MM = (1 << 1),
>>>> + GVT_DBG_IRQ = (1 << 2),
>>>> +};
>>>> +
>>>> +#define gvt_dbg_core(fmt, args...) \
>>>> + gvt_dbg(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
>>>
>>> This should be integrated better to DRM debugging options, custom
>>> debugging code only for i915 was rejected a while ago.
>>>
>> [ZHI] Thanks for sharing the history. :) Will change that.
>>>> diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c
>>>> b/drivers/gpu/drm/i915/gvt/fb_decoder.c
>>>> new file mode 100644
>>>> index 0000000..a219819
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c
>>>> @@ -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.
>>>> + */
>>>> +
>>>> +#include "gvt.h"
>>>> +
>>>> +int gvt_decode_fb_format(struct pgt_device *pdev, int vmid, struct
>>>> gvt_fb_format *fb)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int gvt_fb_notifier_call_chain(unsigned long val, void *data)
>>>> +{
>>>> + return 0;
>>>> +}
>>>
>>> Kerneldoc missing for these functions. It is all the same to squash
>>> later patches to introduce the code to these functions already,
>>> reviewing utility functions with no kerneldoc and no body makes it
>>> somewhat difficult to see the big picture.
>>>
>>
>> [Zhi] One question. I saw i915 put some function instruction in *.c. Is
>> it also OK for kernel doc generating the proper doc files?
>>
>>>> diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.h
>>>> b/drivers/gpu/drm/i915/gvt/fb_decoder.h
>>>> new file mode 100644
>>>> index 0000000..2c29ed4
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.h
>>>> @@ -0,0 +1,110 @@
>>>> +/*
>>>> + * 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_FB_DECODER_H_
>>>> +#define _GVT_FB_DECODER_H_
>>>> +
>>>> +typedef enum {
>>>> + FB_MODE_SET_START = 1,
>>>> + FB_MODE_SET_END,
>>>> + FB_DISPLAY_FLIP,
>>>> +}gvt_fb_event_t;
>>>> +
>>>> +typedef enum {
>>>> + DDI_PORT_NONE = 0,
>>>> + DDI_PORT_B = 1,
>>>> + DDI_PORT_C = 2,
>>>> + DDI_PORT_D = 3,
>>>> + DDI_PORT_E = 4
>>>> +} ddi_port_t;
>>>> +
>>>> +struct pgt_device;
>>>> +
>>>> +struct gvt_fb_notify_msg {
>>>> + unsigned vm_id;
>>>> + unsigned pipe_id; /* id starting from 0 */
>>>> + unsigned plane_id; /* primary, cursor, or sprite */
>>>> +};
>>>> +
>>>> +/* color space conversion and gamma correction are not included */
>>>> +struct gvt_primary_plane_format {
>>>> + u8 enabled; /* plane is enabled */
>>>> + u8 tiled; /* X-tiled */
>>>> + u8 bpp; /* bits per pixel */
>>>> + u32 hw_format; /* format field in the PRI_CTL register */
>>>> + u32 drm_format; /* format in DRM definition */
>>>> + u32 base; /* framebuffer base in graphics memory */
>>>> + u32 x_offset; /* in pixels */
>>>> + u32 y_offset; /* in lines */
>>>> + u32 width; /* in pixels */
>>>> + u32 height; /* in lines */
>>>> + u32 stride; /* in bytes */
>>>> +};
>>>> +
>>>> +struct gvt_sprite_plane_format {
>>>> + u8 enabled; /* plane is enabled */
>>>> + u8 tiled; /* X-tiled */
>>>> + u8 bpp; /* bits per pixel */
>>>> + u32 hw_format; /* format field in the SPR_CTL register */
>>>> + u32 drm_format; /* format in DRM definition */
>>>> + u32 base; /* sprite base in graphics memory */
>>>> + u32 x_pos; /* in pixels */
>>>> + u32 y_pos; /* in lines */
>>>> + u32 x_offset; /* in pixels */
>>>> + u32 y_offset; /* in lines */
>>>> + u32 width; /* in pixels */
>>>> + u32 height; /* in lines */
>>>> +};
>>>> +
>>>> +struct gvt_cursor_plane_format {
>>>> + u8 enabled;
>>>> + u8 mode; /* cursor mode select */
>>>> + u8 bpp; /* bits per pixel */
>>>> + u32 drm_format; /* format in DRM definition */
>>>> + u32 base; /* cursor base in graphics memory */
>>>> + u32 x_pos; /* in pixels */
>>>> + u32 y_pos; /* in lines */
>>>> + u8 x_sign; /* X Position Sign */
>>>> + u8 y_sign; /* Y Position Sign */
>>>> + u32 width; /* in pixels */
>>>> + u32 height; /* in lines */
>>>> + u32 x_hot; /* in pixels */
>>>> + u32 y_hot; /* in pixels */
>>>> +};
>>>> +
>>>
>>> The above structs have a lot in common, would it make sense to have the
>>> common members + plane type and then union for the plane type specific
>>> data. I suspect having it all split this way will lead to more utility
>>> functions somewhere.
>>>
>> [Zhi] Thanks. Will do that.
>>
>>>> +struct gvt_pipe_format {
>>>> + struct gvt_primary_plane_format primary;
>>>> + struct gvt_sprite_plane_format sprite;
>>>> + struct gvt_cursor_plane_format cursor;
>>>> + ddi_port_t ddi_port; /* the DDI port that the pipe is connected
>>>> to */
>>>> +};
>>>> +
>>>> +struct gvt_fb_format{
>>>> + struct gvt_pipe_format pipes[I915_MAX_PIPES];
>>>> +};
>>>> +
>>>> +extern int gvt_fb_notifier_call_chain(unsigned long val, void *data);
>>>> +extern int gvt_decode_fb_format(struct pgt_device *pdev, int vmid,
>>>> + struct gvt_fb_format *fb);
>>>> +
>>>> +#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..041d10f
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
>>>> @@ -0,0 +1,366 @@
>>>> +/*
>>>> + * 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 "gvt.h"
>>>> +
>>>> +struct gvt_host gvt_host;
>>>> +
>>>> +extern struct gvt_kernel_dm xengt_kdm;
>>>> +extern struct gvt_kernel_dm kvmgt_kdm;
>>>> +
>>>> +static const char *supported_hypervisors[] = {
>>>
>>> should be "static const char * const".
>>>
>>>> + [GVT_HYPERVISOR_TYPE_XEN] = "Xen Hypervisor",
>>>> + [GVT_HYPERVISOR_TYPE_KVM] = "KVM",
>>>> +};
>>>> +
>>>> +static bool gvt_init_host(void)
>>>> +{
>>>> + struct gvt_host *host = &gvt_host;
>>>> +
>>>> + if (!gvt.enable) {
>>>> + gvt_dbg_core("GVT-g has been disabled by kernel parameter");
>>>> + return false;
>>>> + }
>>>> +
>>>> + if (host->initialized) {
>>>> + gvt_err("GVT-g has already been initialized!");
>>>> + return false;
>>>> + }
>>>> +
>>>> + if (xen_initial_domain()) {
>>>
>>> Shouldn't Xen code be CONFIG_XEN_DOM0 and CONFIG_XEN #ifdef protected?
>>>
>> [Zhi] The following code piece shows xen_initial_domain()/xen_domain()
>> could also be used when CONFIG_XEN_DOM0 is not set.
>>
>> #ifdef CONFIG_XEN
>> extern enum xen_domain_type xen_domain_type;
>> #else
>> #define xen_domain_type XEN_NATIVE
>> #endif
>>
>> #define xen_domain() (xen_domain_type != XEN_NATIVE)
>> #define xen_pv_domain() (xen_domain() && \
>> xen_domain_type == XEN_PV_DOMAIN)
>> #define xen_hvm_domain() (xen_domain() && \
>> xen_domain_type == XEN_HVM_DOMAIN)
>>
>> #ifdef CONFIG_XEN_DOM0
>> #include <xen/interface/xen.h>
>> #include <asm/xen/hypervisor.h>
>>
>> #define xen_initial_domain() (xen_domain() && \
>> xen_start_info &&
>> xen_start_info->flags & SIF_INITDOMAIN)
>> #else /* !CONFIG_XEN_DOM0 */
>> #define xen_initial_domain() (0)
>> #endif /* CONFIG_XEN_DOM0 */
>>
>>>> + /* Xen Dom0 */
>>>> + host->kdm = try_then_request_module(symbol_get(xengt_kdm),
>>>> "xengt");
>>>> + host->hypervisor_type = GVT_HYPERVISOR_TYPE_XEN;
>>>> + } else if(xen_domain()) {
>>>> + /* Xen DomU */
>>>> + return false;
>>>> + } else {
>>>> + /* not in Xen. Try KVMGT */
>>>> + host->kdm = try_then_request_module(symbol_get(kvmgt_kdm),
>>>> "kvm");
>>>> + host->hypervisor_type = GVT_HYPERVISOR_TYPE_KVM;
>>>> + }
>>>> +
>>>
>>> Why do we need to keep track of hypervisor type, are there plenty of
>>> hypervisor specific behaviour differences?
>>
>> [Zhi] As GVT-g needs some hypervisor services to work, we write a
>> abstraction layer to connect GVT-g to different hypervisor. But still
>> there are some emulation logic couldn't be fitted into that abstraction
>> layer, like OpRegion emulation. In Xen, we emulates it in kernel space,
>> while we emulates it in Qemu under KVM. I also agreed that we should
>> find some approach to improve it just like what you said.
>>
>> If it is just for printing
>>> the below debug, I think it's better to move the debug into above code
>>> that detects the hypervisor, wrap them with appropriate CONFIG #ifdefs.
>>>
>>>> + if (!host->kdm)
>>>> + return false;
>>>> +
>>>> + if (!hypervisor_detect_host())
>>>> + return false;
>>>> +
>>>> + 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 true;
>>>> +}
>>>> +
>>>> +static bool init_device_info(struct pgt_device *pdev)
>>>> +{
>>>> + struct gvt_device_info *info = &pdev->device_info;
>>>> +
>>>> + if (!IS_BROADWELL(pdev->dev_priv)) {
>>>> + gvt_err("Unsupported GEN device");
>>>> + return false;
>>>> + }
>>>> +
>>>> + if (IS_BROADWELL(pdev->dev_priv)) {
>>>> + info->max_gtt_gm_sz = (1UL << 32);
>>>> + /*
>>>> + * 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);
>>>> + info->max_gtt_size = (1UL << 23);
>>>> + info->gtt_entry_size = 8;
>>>> + info->gtt_entry_size_shift = 3;
>>>> + info->gmadr_bytes_in_cmd = 8;
>>>
>>> Would this information be useful in a header as #defines too?
>>>
>> [Zhi] Probably. Consider that one GVT-g featured kernel could be able to
>> run on different platforms, the informaion data structure could be
>> different. But I agree to define the magic numbers in a header files. :)
>>
>>>> + }
>>>> +
>>>> + gvt_info("Device info:");
>>>> + printk(" max_gtt_gm_sz: %llx\n", info->max_gtt_gm_sz);
>>>> + printk(" max_gtt_size: %x\n", info->max_gtt_size);
>>>> + printk(" gtt_size_entry: %x\n", info->gtt_entry_size);
>>>> + printk(" gtt_entry_size_shift: %x\n",
>>>> info->gtt_entry_size_shift);
>>>> + printk(" gtt_start_offset: %x\n", info->gtt_start_offset);
>>>> + printk(" gtt_end_offset: %x\n", info->gtt_end_offset);
>>>> +
>>>
>>> Just put this to kind of stuff to i915_debugfs.c.
>>>
>> [Zhi] Thanks.
>>
>>>> + return true;
>>>> +}
>>>> +
>>>> +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)
>>>
>>> += sizeof(...)
>>>
>>>> + pci_read_config_dword(pci_dev, i,
>>>> + (u32 *)&pdev->initial_cfg_space[i]);
>>>> +
>>>> + for (i = 0; i < 3; i++) {
>>>
>>> No magic numbers make a #define for 3 and give it a descriptive name.
>>>
>>>> + pdev->bar_size[i] = pci_resource_len(pci_dev, i * 2);
>>>> + gvt_info("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 bool init_initial_mmio_state(struct pgt_device *pdev)
>>>> +{
>>>> + u64 bar0, bar1;
>>>> +
>>>> + 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->mmio_size = 2 * 1024 * 1024;
>>>> + pdev->reg_num = pdev->mmio_size / 4;
>>>> + pdev->gmadr_base = bar1 & ~0xf;
>>>> +
>>>
>>> Many magic numbers.
>>>
>>>> + pdev->gttmmio_va = ioremap(pdev->gttmmio_base, pdev->bar_size[0]);
>>>> + if (!pdev->gttmmio_va) {
>>>> + gvt_err("fail to map GTTMMIO BAR.");
>>>
>>> These should be if(WARN_ON(...))
>>>
>>>> + return false;
>>>> + }
>>>> +
>>>> + pdev->gmadr_va = ioremap(pdev->gmadr_base, pdev->bar_size[2]);
>>>> + if (!pdev->gmadr_va) {
>>>> + gvt_err("fail to map GMADR BAR.");
>>>> + goto err;
>>>> + }
>>>> +
>>>> + gvt_info("bar0: 0x%llx, bar1: 0x%llx", bar0, bar1);
>>>> + gvt_info("mmio size: %x", pdev->mmio_size);
>>>> + gvt_info("gttmmio: 0x%llx, gmadr: 0x%llx", pdev->gttmmio_base,
>>>> pdev->gmadr_base);
>>>> + gvt_info("gttmmio_va: %p", pdev->gttmmio_va);
>>>> + gvt_info("gmadr_va: %p", pdev->gmadr_va);
>>>> +
>>>> + return true;
>>>> +err:
>>>> + clean_initial_mmio_state(pdev);
>>>> + return false;
>>>> +}
>>>> +
>>>> +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 (r) {
>>>> + gvt_warn("service thread is waken up by unexpected
>>>> signal.");
>>>
>>> Should be WARN_ONCE, to avoid future disasters with CI.
>>>
>> [Zhi] Thanks.
>>>> + 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 bool 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 false;
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> +static void clean_pgt_device(struct pgt_device *pdev)
>>>> +{
>>>> + clean_service_thread(pdev);
>>>> + clean_initial_mmio_state(pdev);
>>>> +}
>>>> +
>>>> +static bool init_pgt_device(struct pgt_device *pdev, struct
>>>> drm_i915_private *dev_priv)
>>>> +{
>>>> + if (!init_device_info(pdev))
>>>> + return false;
>>>> +
>>>> + init_initial_cfg_space_state(pdev);
>>>> +
>>>> + if (!init_initial_mmio_state(pdev))
>>>> + goto err;
>>>> +
>>>> + if (!init_service_thread(pdev))
>>>> + goto err;
>>>> +
>>>> + return true;
>>>> +err:
>>>> + clean_pgt_device(pdev);
>>>> + return false;
>>>> +}
>>>> +
>>>> +static bool post_init_pgt_device(struct pgt_device *pdev)
>>>> +{
>>>> + return true;
>>>> +}
>>>> +
>>>> +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) {
>>>> + gvt_err("fail to allocate memory for pgt device.");
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + 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;
>>>> + }
>>>> +
>>>> + mutex_init(&pdev->lock);
>>>> + pdev->dev_priv = dev_priv;
>>>> + idr_init(&pdev->instance_idr);
>>>> +
>>>> + return pdev;
>>>> +err:
>>>> + free_pgt_device(pdev);
>>>> + 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);
>>>> +}
>>>> +
>>>> +void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
>>>> +{
>>>> + struct pgt_device *pdev = NULL;
>>>> + struct gvt_host *host = &gvt_host;
>>>> +
>>>> + 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);
>>>> +
>>>> + if (!init_pgt_device(pdev, dev_priv)) {
>>>> + 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;
>>>> + }
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +bool gvt_post_init_pgt_device(void *private_data)
>>>> +{
>>>> + struct pgt_device *pdev = (struct pgt_device *)private_data;
>>>> + struct gvt_host *host = &gvt_host;
>>>> +
>>>> + if (!host->initialized) {
>>>> + gvt_err("gvt_host haven't been initialized.");
>>>> + return false;
>>>> + }
>>>> +
>>>> + gvt_dbg_core("post init pgt device %d", pdev->id);
>>>> +
>>>> + if (!post_init_pgt_device(pdev)) {
>>>> + gvt_err("fail to post init physical device state.");
>>>> + return false;
>>>> + }
>>>> +
>>>> + return true;
>>>> +}
>>>> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h
>>>> b/drivers/gpu/drm/i915/gvt/gvt.h
>>>> new file mode 100644
>>>> index 0000000..6c85bba
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
>>>> @@ -0,0 +1,130 @@
>>>> +/*
>>>> + * 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"
>>>> +#include "fb_decoder.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;
>>>> +
>>>> +/* 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;
>>>> +};
>>>> +
>>>> +struct vgt_device {
>>>> + int id;
>>>> + int vm_id;
>>>> + struct pgt_device *pdev;
>>>> + bool warn_untrack;
>>>> +};
>>>> +
>>>> +struct pgt_device {
>>>> + 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;
>>>> +};
>>>> +
>>>
>>> Here-->
>>>
>>>> +static inline u32 gvt_mmio_read(struct pgt_device *pdev,
>>>> + u32 reg)
>>>> +{
>>>> + struct drm_i915_private *dev_priv = pdev->dev_priv;
>>>> + i915_reg_t tmp = {.reg = reg};
>>>> + return I915_READ(tmp);
>>>> +}
>>>> +
>>>> +static inline void gvt_mmio_write(struct pgt_device *pdev,
>>>> + u32 reg, u32 val)
>>>> +{
>>>> + struct drm_i915_private *dev_priv = pdev->dev_priv;
>>>> + i915_reg_t tmp = {.reg = reg};
>>>> + I915_WRITE(tmp, val);
>>>> +}
>>>> +
>>>> +static inline u64 gvt_mmio_read64(struct pgt_device *pdev,
>>>> + u32 reg)
>>>> +{
>>>> + struct drm_i915_private *dev_priv = pdev->dev_priv;
>>>> + i915_reg_t tmp = {.reg = reg};
>>>> + return I915_READ64(tmp);
>>>> +}
>>>> +
>>>> +static inline void gvt_mmio_write64(struct pgt_device *pdev,
>>>> + u32 reg, u64 val)
>>>> +{
>>>> + struct drm_i915_private *dev_priv = pdev->dev_priv;
>>>> + i915_reg_t tmp = {.reg = reg};
>>>> + I915_WRITE64(tmp, val);
>>>> +}
>>>> +
>>>
>>> <-- Why? The i915_reg_t type was added to avoid problems, this code is
>>> not used anywhere, and it has no documentation, so I can not review it.
>>>
>>> I wrote comments at the top of the post.
>>>
>> [Zhi] Thanks, I will check that.
>>
>>> Regards, Joonas
>>>
>>>> +#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 {
>>>> +};
>>>> +
>>>> +#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..bbe4465
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/gvt/mpt.h
>>>> @@ -0,0 +1,97 @@
>>>> +/*
>>>> + * 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 unsigned long hypervisor_g2m_pfn(struct vgt_device *vgt,
>>>> + unsigned long g_pfn)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static inline int hypervisor_pause_domain(struct vgt_device *vgt)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static inline int hypervisor_shutdown_domain(struct vgt_device *vgt)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static inline int hypervisor_set_trap_area(struct vgt_device *vgt,
>>>> + uint64_t start, uint64_t end, bool map)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static inline bool hypervisor_detect_host(void)
>>>> +{
>>>> + return false;
>>>> +}
>>>> +
>>>> +static inline int hypervisor_virt_to_mfn(void *addr)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static inline void *hypervisor_mfn_to_virt(int mfn)
>>>> +{
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +static inline void hypervisor_inject_msi(struct vgt_device *vgt)
>>>> +{
>>>> + return;
>>>> +}
>>>> +
>>>> +static inline int hypervisor_hvm_init(struct vgt_device *vgt)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static inline void hypervisor_hvm_exit(struct vgt_device *vgt)
>>>> +{
>>>> +}
>>>> +
>>>> +static inline void *hypervisor_gpa_to_va(struct vgt_device *vgt,
>>>> unsigned long gpa)
>>>> +{
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +static inline bool hypervisor_read_va(struct vgt_device *vgt, void *va,
>>>> + void *val, int len, int atomic)
>>>> +{
>>>> + return false;
>>>> +}
>>>> +
>>>> +static inline bool hypervisor_write_va(struct vgt_device *vgt, void
>>>> *va,
>>>> + void *val, int len, int atomic)
>>>> +{
>>>> + return false;
>>>> +}
>>>> +
>>>> +#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..dfc33c3
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/i915/gvt/params.c
>>>> @@ -0,0 +1,29 @@
>>>> +/*
>>>> + * 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 = true,
>>>> + .debug = 0,
>>>> +};
>>>> 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;
>>>> +};
>>>> +
>>>> +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
>>>> +
>>>> +#endif
>>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c
>>>> b/drivers/gpu/drm/i915/i915_dma.c
>>>> index 4725e8d..eca8e50 100644
>>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>>> @@ -943,6 +943,10 @@ int i915_driver_load(struct drm_device *dev,
>>>> unsigned long flags)
>>>>
>>>> intel_uncore_init(dev);
>>>>
>>>> + dev_priv->vgpu.host_private_data = gvt_create_pgt_device(dev_priv);
>>>> + if(intel_gvt_host_active(dev))
>>>> + DRM_INFO("GVT-g is running in host mode\n");
>>>> +
>>>> ret = i915_gem_gtt_init(dev);
>>>> if (ret)
>>>> goto out_freecsr;
>>>> @@ -1067,6 +1071,13 @@ int i915_driver_load(struct drm_device *dev,
>>>> unsigned long flags)
>>>> goto out_power_well;
>>>> }
>>>>
>>>> + if (intel_gvt_host_active(dev)) {
>>>> + if
>>>> (!gvt_post_init_pgt_device(dev_priv->vgpu.host_private_data)) {
>>>> + DRM_ERROR("failed to post init pgt device\n");
>>>> + goto out_power_well;
>>>> + }
>>>> + }
>>>> +
>>>> /*
>>>> * Notify a valid surface after modesetting,
>>>> * when running inside a VM.
>>>> @@ -1117,6 +1128,10 @@ out_gtt:
>>>> i915_global_gtt_cleanup(dev);
>>>> out_freecsr:
>>>> intel_csr_ucode_fini(dev_priv);
>>>> + if (intel_gvt_host_active(dev)) {
>>>> + gvt_destroy_pgt_device(dev_priv->vgpu.host_private_data);
>>>> + dev_priv->vgpu.host_private_data = NULL;
>>>> + }
>>>> intel_uncore_fini(dev);
>>>> pci_iounmap(dev->pdev, dev_priv->regs);
>>>> put_bridge:
>>>> @@ -1165,6 +1180,10 @@ int i915_driver_unload(struct drm_device *dev)
>>>>
>>>> intel_modeset_cleanup(dev);
>>>>
>>>> + if (intel_gvt_host_active(dev)) {
>>>> + gvt_destroy_pgt_device(dev_priv->vgpu.host_private_data);
>>>> + dev_priv->vgpu.host_private_data = NULL;
>>>> + }
>>>> /*
>>>> * 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 01cc982..db3c79b 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1673,6 +1673,7 @@ struct i915_workarounds {
>>>>
>>>> struct i915_virtual_gpu {
>>>> bool active;
>>>> + void *host_private_data;
>>>> };
>>>>
>>>> struct i915_execbuffer_params {
>>>> @@ -2747,6 +2748,11 @@ static inline bool intel_vgpu_active(struct
>>>> drm_device *dev)
>>>> return to_i915(dev)->vgpu.active;
>>>> }
>>>>
>>>> +static inline bool intel_gvt_host_active(struct drm_device *dev)
>>>> +{
>>>> + return to_i915(dev)->vgpu.host_private_data ? true : false;
>>>> +}
>>>> +
>>>> void
>>>> i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe
>>>> pipe,
>>>> u32 status_mask);
>>>> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h
>>>> b/drivers/gpu/drm/i915/i915_vgpu.h
>>>> index 3c83b47..942490a 100644
>>>> --- a/drivers/gpu/drm/i915/i915_vgpu.h
>>>> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
>>>> @@ -113,5 +113,8 @@ struct vgt_if {
>>>> extern void i915_check_vgpu(struct drm_device *dev);
>>>> extern int intel_vgt_balloon(struct drm_device *dev);
>>>> extern void intel_vgt_deballoon(void);
>>>> +extern void *gvt_create_pgt_device(struct drm_i915_private *dev_priv);
>>>> +extern bool gvt_post_init_pgt_device(void *private_data);
>>>> +extern void gvt_destroy_pgt_device(void *private_data);
>>>>
>>>> #endif /* _I915_VGPU_H_ */
>>>> diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
>>>> index d133112..78a38f1 100644
>>>> --- a/include/xen/interface/xen.h
>>>> +++ b/include/xen/interface/xen.h
>>>> @@ -28,6 +28,7 @@
>>>> #define __XEN_PUBLIC_XEN_H__
>>>>
>>>> #include
>>>> +#include
>>>>
>>>> /*
>>>> * XEN "SYSTEM CALLS" (a.k.a. HYPERCALLS).
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list