On Fri, May 20, 2016 at 12:18:10PM +0000, Wang, Zhi A wrote: > 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? :) I'm never going to argue against giving good advice to the reader. Even if it is "for further details please see 01.org/blah" > > > +#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]? :) Just a comment to say to hesitate before using anything above DRM_DEBUG and ask yourself "is this the right way to provide this information to the user, is it as clear/useful as possible?" > > > +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. It's going to be on a case by case basis, but I would rather have hw descriptions in core. > > > +/** > > > + * 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? :) That's one approach. For starters you can just struct intel_gvt; struct i915_gvt { struct intel_gvt *gvt; }; But I wouldn't worry about encapsulation too much. You are only ever an include away from us getting the innermost details of your structs. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
