[Intel-gfx] [RFC 01/29] drm/i915/gvt: Introduce the basic architecture of GVT-g
Joonas Lahtinen
joonas.lahtinen at linux.intel.com
Tue Feb 16 14:08:42 UTC 2016
Hi,
On ti, 2016-02-16 at 17:54 +0800, Zhi Wang 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 :(.
>
I think from upstream kernel perspective the right answer is that code
like above really needs to use dynamic debugging instead of self-baked
system. When using dynamic debugging framework, debugging could be
enabled in different granularity levels (module, file, function, line,
or format). More at: https://lwn.net/Articles/434833/
Passing dynamic debug filter 'format "i915: gvt: core: " +p' would then
enable GVT core debugging.
On the other hand, Daniel does not want us to break drm.debug or add
i915.debug (so I guess i915.gvt_debug is not fine either). Discussion
occurred in this patch:
https://patchwork.freedesktop.org/patch/71412/
So I think best you can currently do is:
#define gvt_dbg_xxx(fmt, xxx) \
DRM_DEBUG_DRIVER("gvt: xxx: " fmt, ...)>
And then we wait for DRM to get converted to dynamic debug.
I discussed this with Daniel, and it should be doable. Only down-fall
is that in order to make drm.debug=0x0e syntax still work, we need to
make make ddebug_exec_queries symbol exported and add code like this
for backwards compatability;
if (drm.debug & DRM_UT_CORE)
ddebug_exec_queries("format \"drm: core: \" +p", ...);
if (drm.debug & DRM_UT_DRIVER)
ddebug_exec_queries("format \"drm: drv: \" +p", ...);
What do you guys think? One could export more symbols and add the
filters in a more direct manner, but I'm not sure if it's worth the
effort.
Regards, Joonas
> 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).
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
More information about the Intel-gfx
mailing list