Re: [RFC 01/29] drm/i915/gvt: Introduce the basic architecture of GVT-g

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 16 Feb 2016, Zhi Wang <zhi.a.wang@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux