Thanks! See my replies below. > -----Original Message----- > From: Chris Wilson [mailto:chris@xxxxxxxxxxxxxxxxxx] > Sent: Friday, May 20, 2016 2:49 PM > To: Wang, Zhi A <zhi.a.wang@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; tvrtko.ursulin@xxxxxxxxxxxxxxx; > joonas.lahtinen@xxxxxxxxxxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; Lv, > Zhiyuan <zhiyuan.lv@xxxxxxxxx> > Subject: Re: [PATCH 3/9] drm/i915: gvt: Introduce the basic architecture of > GVT-g > > On Tue, May 17, 2016 at 04:19:03AM -0400, Zhi Wang wrote: > > +config DRM_I915_GVT > > + bool "Intel GVT-g host driver" > > + depends on DRM_I915 > > + default n > > + help > > + Enabling GVT-g mediated graphics pass-through 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 > > + operations such as aperture accesses and ring buffer > operations > > + are passed-through to VM, with a minimal set of conflicting > resources > > + (e.g. display settings) mediated by GVT host driver. The benefit > of GVT > > + 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. > > Just like that? No userspace support required? i.e. can it work with kvm or xen? > Wants a comment on any possible danger (or why is it 'n' by default)? > Basically that text is only technical introduction. Or maybe we could discuss what we want here: a. Basic technical introduction. b. Need Xen and KVM c. Possible danger as it's only preliminary for now. i915 Kconfig introduction consists 3 parts: a. Introduction b. Userspace requirement c. Limitiation Can I follow that? :) > > +#ifndef __GVT_DEBUG_H__ > > +#define __GVT_DEBUG_H__ > > + > > +#define gvt_info(fmt, args...) \ > > + DRM_INFO("gvt: "fmt, ##args) > > + > > +#define gvt_err(fmt, args...) \ > > + DRM_ERROR("gvt: "fmt, ##args) > > I think it is fair to say that neither of these will look nice in user-facing > messages. > Then how about [drm][gvt]? :) > > + > > +#define gvt_dbg_core(fmt, args...) \ > > + DRM_DEBUG_DRIVER("gvt: core: "fmt, ##args) > > + > > +#endif > > diff --git a/drivers/gpu/drm/i915/gvt/gvt.c > > b/drivers/gpu/drm/i915/gvt/gvt.c new file mode 100644 index > > 0000000..aa40357 > > --- /dev/null > > +++ b/drivers/gpu/drm/i915/gvt/gvt.c > > @@ -0,0 +1,205 @@ > > +/* > > + * 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 <linux/types.h> > > +#include <xen/xen.h> > > +#include <linux/kthread.h> > > + > > +#include "gvt.h" > > + > > +struct intel_gvt_host intel_gvt_host; > > + > > +static const char * const supported_hypervisors[] = { > > + [INTEL_GVT_HYPERVISOR_XEN] = "XEN", > > + [INTEL_GVT_HYPERVISOR_KVM] = "KVM", > > +}; > > + > > +#define MB(x) (x * 1024ULL * 1024ULL) #define GB(x) (x * MB(1024)) > > + > > +/* > > + * 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. > > + */ > > +static struct intel_gvt_device_info broadwell_device_info = { > > + .max_gtt_gm_sz = GB(4), /* 4GB */ > > + .gtt_start_offset = MB(8), /* 8MB */ > > + .max_gtt_size = MB(8), /* 8MB */ > > + .gtt_entry_size = 8, > > + .gtt_entry_size_shift = 3, > > + .gmadr_bytes_in_cmd = 8, > > + .mmio_size = MB(2), /* 2MB */ > > + .mmio_bar = 0, /* BAR 0 */ > > + .max_support_vgpu = 8, > > Too much superfluous detail upfront. It is hard to critique when you have no > idea what it is for. At the moment, it looks like you are duplicating information > we have elsewhere, and I'd rather not. (For starters, it makes GVT a bigger > maintainance burden.) > OK. Then I think I should remove most them in the first path and add them in the later patch. And do you want me to merge these HW descriptions into i915_device_info? Some of them might not be used by i915 driver itself. That's why I keep a dedicated data structure here. > > +/** > > + * intel_gvt_destroy_device - destroy a GVT device > > + * @gvt_device: gvt device > > + * > > + * This function is called at the driver unloading stage, to destroy > > +a > > + * GVT device and free the related resources. > > + * > > + */ > > +void intel_gvt_destroy_device(void *device) > > This should not be void *. You can forward declare struct intel_gvt such that the > core has an opaque pointer. > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h index 72f0b02..7d0b8d3 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1703,6 +1703,10 @@ struct i915_workarounds { > > u32 hw_whitelist_count[I915_NUM_ENGINES]; > > }; > > > > +struct i915_gvt { > > + void *gvt; > > As before. No void * here. > Let me think an approach to remove the "void *" here. How about we do like this: struct a{ Struct b; }; Let i915 take only struct b, then in gvt we do container_of() so that we don't need to expose the declaration of struct a to i915? :) > > +}; > > > +struct gvt_kernel_params { > > + bool enable; > > +}; > > + > > +extern struct gvt_kernel_params gvt_kparams; > > No. Just make room for them inside i915_params. Module parameters deserve > extra scrutiny as they are (soft) ABI (even if we think users shouldn't be using > them). OK. Will do. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
