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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
