| Message ID | 1440056724-26976-4-git-send-email-zhiyuan.lv@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > Broadwell hardware supports both ring buffer mode and execlist mode. > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode > only. The reason is that GVT-g does not support the dynamic mode > switch between ring buffer mode and execlist mode when running > multiple virtual machines. Consider that ring buffer mode is legacy > mode, it makes sense to drop it inside virtual machines. If that is the case, you should query the host as to what mode it is running. -Chris
Hi Chris, On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote: > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > > Broadwell hardware supports both ring buffer mode and execlist mode. > > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode > > only. The reason is that GVT-g does not support the dynamic mode > > switch between ring buffer mode and execlist mode when running > > multiple virtual machines. Consider that ring buffer mode is legacy > > mode, it makes sense to drop it inside virtual machines. > > If that is the case, you should query the host as to what mode it is > running. Thanks for the reply! You mean we query the host mode, then tell the guest driver inside VM, so that it could use the same mode as host right? That might be a little complicated, and the only benefit is to support legacy ring buffer mode ... And GVT-g host implementation was not well tested with ring buffer mode since it cannot start windows VM. Probably I should change the commit message to say explicitly that ring buffer mode is not supported with GVT-g? Thanks! Regards, -Zhiyuan > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote: > Hi Chris, > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote: > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > > > Broadwell hardware supports both ring buffer mode and execlist mode. > > > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode > > > only. The reason is that GVT-g does not support the dynamic mode > > > switch between ring buffer mode and execlist mode when running > > > multiple virtual machines. Consider that ring buffer mode is legacy > > > mode, it makes sense to drop it inside virtual machines. > > > > If that is the case, you should query the host as to what mode it is > > running. > > Thanks for the reply! You mean we query the host mode, then tell the > guest driver inside VM, so that it could use the same mode as host > right? That might be a little complicated, and the only benefit is to > support legacy ring buffer mode ... The only benefit being that the guest works no matter what the host does? -Chris
On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote: > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote: > > Hi Chris, > > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote: > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > > > > Broadwell hardware supports both ring buffer mode and execlist mode. > > > > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode > > > > only. The reason is that GVT-g does not support the dynamic mode > > > > switch between ring buffer mode and execlist mode when running > > > > multiple virtual machines. Consider that ring buffer mode is legacy > > > > mode, it makes sense to drop it inside virtual machines. > > > > > > If that is the case, you should query the host as to what mode it is > > > running. > > > > Thanks for the reply! You mean we query the host mode, then tell the > > guest driver inside VM, so that it could use the same mode as host > > right? That might be a little complicated, and the only benefit is to > > support legacy ring buffer mode ... > > The only benefit being that the guest works no matter what the host > does? Supporting ring buffer mode may need more work in GVT-g. When we started to enable BDW support, ring buffer mode used to work but was not well tested. And the inter-VM switch (all in ringbuffer mode) may be tricker comparing with driver context switch with "MI_SET_CONTEXT", because we need to switch ring buffer whereas driver does not. Regarding this, the EXECLIST mode looks cleaner. In order to support that, we may have to: 1, change more LRI commands to MMIO in current driver; 2, more testing/debugging of inter-VM context switch flow. Based on that, I think we should really make statement that "ring buffer mode" is not supported by GVT-g on BDW :-) > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, On to, 2015-08-20 at 17:40 +0800, Zhiyuan Lv wrote: > On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote: > > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote: > > > Hi Chris, > > > > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote: > > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > > > > > Broadwell hardware supports both ring buffer mode and > > > > > execlist mode. > > > > > When i915 runs inside a VM with Intel GVT-g, we allow > > > > > execlist mode > > > > > only. The reason is that GVT-g does not support the dynamic > > > > > mode > > > > > switch between ring buffer mode and execlist mode when > > > > > running > > > > > multiple virtual machines. Consider that ring buffer mode is > > > > > legacy > > > > > mode, it makes sense to drop it inside virtual machines. > > > > > > > > If that is the case, you should query the host as to what mode > > > > it is > > > > running. > > > > > > Thanks for the reply! You mean we query the host mode, then tell > > > the > > > guest driver inside VM, so that it could use the same mode as > > > host > > > right? That might be a little complicated, and the only benefit > > > is to > > > support legacy ring buffer mode ... > > > > The only benefit being that the guest works no matter what the host > > does? > > Supporting ring buffer mode may need more work in GVT-g. When we > started to > enable BDW support, ring buffer mode used to work but was not well > tested. And > the inter-VM switch (all in ringbuffer mode) may be tricker comparing > with > driver context switch with "MI_SET_CONTEXT", because we need to > switch ring > buffer whereas driver does not. Regarding this, the EXECLIST mode > looks > cleaner. In order to support that, we may have to: 1, change more LRI > commands > to MMIO in current driver; 2, more testing/debugging of inter-VM > context > switch flow. > > Based on that, I think we should really make statement that "ring > buffer mode" > is not supported by GVT-g on BDW :-) > I think just move the vpgu test even before test for GEN9 just making it return true on intel_vgpu_active(dev) and add a comment that currently vGPU only supports execlist command submission, and then add an error early in the init in some appropriate spot if intel_vgpu_active is true but logical ring context are not available (which would practically mean GEN < 8). Does that sound OK? Regards, Joonas > > -Chris > >
Hi Joonas, Thanks for the review! And my reply inline. Regards, -Zhiyuan On Thu, Aug 20, 2015 at 02:23:11PM +0300, Joonas Lahtinen wrote: > Hi, > > On to, 2015-08-20 at 17:40 +0800, Zhiyuan Lv wrote: > > On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote: > > > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote: > > > > Hi Chris, > > > > > > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote: > > > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > > > > > > Broadwell hardware supports both ring buffer mode and > > > > > > execlist mode. > > > > > > When i915 runs inside a VM with Intel GVT-g, we allow > > > > > > execlist mode > > > > > > only. The reason is that GVT-g does not support the dynamic > > > > > > mode > > > > > > switch between ring buffer mode and execlist mode when > > > > > > running > > > > > > multiple virtual machines. Consider that ring buffer mode is > > > > > > legacy > > > > > > mode, it makes sense to drop it inside virtual machines. > > > > > > > > > > If that is the case, you should query the host as to what mode > > > > > it is > > > > > running. > > > > > > > > Thanks for the reply! You mean we query the host mode, then tell > > > > the > > > > guest driver inside VM, so that it could use the same mode as > > > > host > > > > right? That might be a little complicated, and the only benefit > > > > is to > > > > support legacy ring buffer mode ... > > > > > > The only benefit being that the guest works no matter what the host > > > does? > > > > Supporting ring buffer mode may need more work in GVT-g. When we > > started to > > enable BDW support, ring buffer mode used to work but was not well > > tested. And > > the inter-VM switch (all in ringbuffer mode) may be tricker comparing > > with > > driver context switch with "MI_SET_CONTEXT", because we need to > > switch ring > > buffer whereas driver does not. Regarding this, the EXECLIST mode > > looks > > cleaner. In order to support that, we may have to: 1, change more LRI > > commands > > to MMIO in current driver; 2, more testing/debugging of inter-VM > > context > > switch flow. > > > > Based on that, I think we should really make statement that "ring > > buffer mode" > > is not supported by GVT-g on BDW :-) > > > > I think just move the vpgu test even before test for GEN9 just making > it return true on intel_vgpu_active(dev) and add a comment that > currently vGPU only supports execlist command submission, and then add Will do. Thanks! > an error early in the init in some appropriate spot if > intel_vgpu_active is true but logical ring context are not available > (which would practically mean GEN < 8). > > Does that sound OK? Ring buffer mode for Haswell with vgpu is still supported. So probably I have the check like below? Thanks! @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device *dev) if (WARN_ON(dev_priv->ring[RCS].default_context)) return 0; + if (intel_vgpu_active(dev)) { + if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) && + !i915.enable_execlist)) + return 0; + } + if (i915.enable_execlists) { /* NB: intentionally left blank. We will allocate our own * backing objects as we need them, thank you very much */ > > Regards, Joonas > > > > -Chris > > >
> From: iGVT-g [mailto:igvt-g-bounces@lists.01.org] On Behalf Of Zhiyuan Lv > Sent: Thursday, August 20, 2015 5:40 PM > > On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote: > > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote: > > > Hi Chris, > > > > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote: > > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > > > > > Broadwell hardware supports both ring buffer mode and execlist mode. > > > > > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode > > > > > only. The reason is that GVT-g does not support the dynamic mode > > > > > switch between ring buffer mode and execlist mode when running > > > > > multiple virtual machines. Consider that ring buffer mode is legacy > > > > > mode, it makes sense to drop it inside virtual machines. > > > > > > > > If that is the case, you should query the host as to what mode it is > > > > running. > > > > > > Thanks for the reply! You mean we query the host mode, then tell the > > > guest driver inside VM, so that it could use the same mode as host > > > right? That might be a little complicated, and the only benefit is to > > > support legacy ring buffer mode ... > > > > The only benefit being that the guest works no matter what the host > > does? > > Supporting ring buffer mode may need more work in GVT-g. When we started to > enable BDW support, ring buffer mode used to work but was not well tested. And > the inter-VM switch (all in ringbuffer mode) may be tricker comparing with > driver context switch with "MI_SET_CONTEXT", because we need to switch ring > buffer whereas driver does not. Regarding this, the EXECLIST mode looks > cleaner. In order to support that, we may have to: 1, change more LRI commands > to MMIO in current driver; 2, more testing/debugging of inter-VM context > switch flow. > > Based on that, I think we should really make statement that "ring buffer mode" > is not supported by GVT-g on BDW :-) > I think the primary reason is that Windows gfx driver only uses execution list mode. They don't have plan to add back ring buffer mode. So we have to assume only execution list for all guests powered by GVT-g (being that dynamic mode switch is not feasible). Thanks Kevin
On pe, 2015-08-21 at 10:24 +0800, Zhiyuan Lv wrote: > Hi Joonas, > > Thanks for the review! And my reply inline. > > Regards, > -Zhiyuan > > On Thu, Aug 20, 2015 at 02:23:11PM +0300, Joonas Lahtinen wrote: > > Hi, > > > > On to, 2015-08-20 at 17:40 +0800, Zhiyuan Lv wrote: > > > On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote: > > > > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote: > > > > > Hi Chris, > > > > > > > > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote: > > > > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > > > > > > > Broadwell hardware supports both ring buffer mode and > > > > > > > execlist mode. > > > > > > > When i915 runs inside a VM with Intel GVT-g, we allow > > > > > > > execlist mode > > > > > > > only. The reason is that GVT-g does not support the > > > > > > > dynamic > > > > > > > mode > > > > > > > switch between ring buffer mode and execlist mode when > > > > > > > running > > > > > > > multiple virtual machines. Consider that ring buffer mode > > > > > > > is > > > > > > > legacy > > > > > > > mode, it makes sense to drop it inside virtual machines. > > > > > > > > > > > > If that is the case, you should query the host as to what > > > > > > mode > > > > > > it is > > > > > > running. > > > > > > > > > > Thanks for the reply! You mean we query the host mode, then > > > > > tell > > > > > the > > > > > guest driver inside VM, so that it could use the same mode as > > > > > host > > > > > right? That might be a little complicated, and the only > > > > > benefit > > > > > is to > > > > > support legacy ring buffer mode ... > > > > > > > > The only benefit being that the guest works no matter what the > > > > host > > > > does? > > > > > > Supporting ring buffer mode may need more work in GVT-g. When we > > > started to > > > enable BDW support, ring buffer mode used to work but was not > > > well > > > tested. And > > > the inter-VM switch (all in ringbuffer mode) may be tricker > > > comparing > > > with > > > driver context switch with "MI_SET_CONTEXT", because we need to > > > switch ring > > > buffer whereas driver does not. Regarding this, the EXECLIST mode > > > looks > > > cleaner. In order to support that, we may have to: 1, change more > > > LRI > > > commands > > > to MMIO in current driver; 2, more testing/debugging of inter-VM > > > context > > > switch flow. > > > > > > Based on that, I think we should really make statement that "ring > > > buffer mode" > > > is not supported by GVT-g on BDW :-) > > > > > > > I think just move the vpgu test even before test for GEN9 just > > making > > it return true on intel_vgpu_active(dev) and add a comment that > > currently vGPU only supports execlist command submission, and then > > add > > Will do. Thanks! > > > an error early in the init in some appropriate spot if > > intel_vgpu_active is true but logical ring context are not > > available > > (which would practically mean GEN < 8). > > > > Does that sound OK? > > Ring buffer mode for Haswell with vgpu is still supported. So > probably I have > the check like below? Thanks! > Ah sorry, I missed that. > @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device > *dev) > if (WARN_ON(dev_priv->ring[RCS].default_context)) > return 0; > > + if (intel_vgpu_active(dev)) { > + if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) && > + !i915.enable_execlist)) > + return 0; > + } > + This looks fine to me. Maybe comment might be in place stating that support is not yet implemented, but could be. Regards, Joonas > if (i915.enable_execlists) { > /* NB: intentionally left blank. We will allocate our > own > * backing objects as we need them, thank you very > much */ > > > > > Regards, Joonas > > > > > > -Chris > > > >
On Mon, Aug 24, 2015 at 03:36:46PM +0300, Joonas Lahtinen wrote: > On pe, 2015-08-21 at 10:24 +0800, Zhiyuan Lv wrote: > > Hi Joonas, > > > > Thanks for the review! And my reply inline. > > > > Regards, > > -Zhiyuan > > > > On Thu, Aug 20, 2015 at 02:23:11PM +0300, Joonas Lahtinen wrote: > > > Hi, > > > > > > On to, 2015-08-20 at 17:40 +0800, Zhiyuan Lv wrote: > > > > On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote: > > > > > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote: > > > > > > Hi Chris, > > > > > > > > > > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote: > > > > > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote: > > > > > > > > Broadwell hardware supports both ring buffer mode and > > > > > > > > execlist mode. > > > > > > > > When i915 runs inside a VM with Intel GVT-g, we allow > > > > > > > > execlist mode > > > > > > > > only. The reason is that GVT-g does not support the > > > > > > > > dynamic > > > > > > > > mode > > > > > > > > switch between ring buffer mode and execlist mode when > > > > > > > > running > > > > > > > > multiple virtual machines. Consider that ring buffer mode > > > > > > > > is > > > > > > > > legacy > > > > > > > > mode, it makes sense to drop it inside virtual machines. > > > > > > > > > > > > > > If that is the case, you should query the host as to what > > > > > > > mode > > > > > > > it is > > > > > > > running. > > > > > > > > > > > > Thanks for the reply! You mean we query the host mode, then > > > > > > tell > > > > > > the > > > > > > guest driver inside VM, so that it could use the same mode as > > > > > > host > > > > > > right? That might be a little complicated, and the only > > > > > > benefit > > > > > > is to > > > > > > support legacy ring buffer mode ... > > > > > > > > > > The only benefit being that the guest works no matter what the > > > > > host > > > > > does? > > > > > > > > Supporting ring buffer mode may need more work in GVT-g. When we > > > > started to > > > > enable BDW support, ring buffer mode used to work but was not > > > > well > > > > tested. And > > > > the inter-VM switch (all in ringbuffer mode) may be tricker > > > > comparing > > > > with > > > > driver context switch with "MI_SET_CONTEXT", because we need to > > > > switch ring > > > > buffer whereas driver does not. Regarding this, the EXECLIST mode > > > > looks > > > > cleaner. In order to support that, we may have to: 1, change more > > > > LRI > > > > commands > > > > to MMIO in current driver; 2, more testing/debugging of inter-VM > > > > context > > > > switch flow. > > > > > > > > Based on that, I think we should really make statement that "ring > > > > buffer mode" > > > > is not supported by GVT-g on BDW :-) > > > > > > > > > > I think just move the vpgu test even before test for GEN9 just > > > making > > > it return true on intel_vgpu_active(dev) and add a comment that > > > currently vGPU only supports execlist command submission, and then > > > add > > > > Will do. Thanks! > > > > > an error early in the init in some appropriate spot if > > > intel_vgpu_active is true but logical ring context are not > > > available > > > (which would practically mean GEN < 8). > > > > > > Does that sound OK? > > > > Ring buffer mode for Haswell with vgpu is still supported. So > > probably I have > > the check like below? Thanks! > > > > Ah sorry, I missed that. > > > @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device > > *dev) > > if (WARN_ON(dev_priv->ring[RCS].default_context)) > > return 0; > > > > + if (intel_vgpu_active(dev)) { > > + if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) && > > + !i915.enable_execlist)) > > + return 0; > > + } > > + > > This looks fine to me. Maybe comment might be in place stating that > support is not yet implemented, but could be. You should fail this instead so that i915.ko knows that the render side of the gpu doesn't work. And maybe just DRM_INFO with a useful informational notice? Also same comment here: Don't we need to coordinate this a bit better with the host? -Daniel
Hi Daniel, On Wed, Aug 26, 2015 at 10:50:23AM +0200, Daniel Vetter wrote: > > > @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device > > > *dev) > > > if (WARN_ON(dev_priv->ring[RCS].default_context)) > > > return 0; > > > > > > + if (intel_vgpu_active(dev)) { > > > + if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) && > > > + !i915.enable_execlist)) > > > + return 0; > > > + } > > > + > > > > This looks fine to me. Maybe comment might be in place stating that > > support is not yet implemented, but could be. > > You should fail this instead so that i915.ko knows that the render side of > the gpu doesn't work. And maybe just DRM_INFO with a useful informational > notice? Thanks for the comments! Will change. > > Also same comment here: Don't we need to coordinate this a bit better with > the host? In host side, if driver is running in ring buffer mode, we will let GVT-g initialization fail, so that guest cannot set vgpu active. Would that be good enough? Thanks! > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Aug 27, 2015 at 10:49:28AM +0800, Zhiyuan Lv wrote: > Hi Daniel, > > On Wed, Aug 26, 2015 at 10:50:23AM +0200, Daniel Vetter wrote: > > > > @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device > > > > *dev) > > > > if (WARN_ON(dev_priv->ring[RCS].default_context)) > > > > return 0; > > > > > > > > + if (intel_vgpu_active(dev)) { > > > > + if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) && > > > > + !i915.enable_execlist)) > > > > + return 0; > > > > + } > > > > + > > > > > > This looks fine to me. Maybe comment might be in place stating that > > > support is not yet implemented, but could be. > > > > You should fail this instead so that i915.ko knows that the render side of > > the gpu doesn't work. And maybe just DRM_INFO with a useful informational > > notice? > > Thanks for the comments! Will change. > > > > > Also same comment here: Don't we need to coordinate this a bit better with > > the host? > > In host side, if driver is running in ring buffer mode, we will let GVT-g > initialization fail, so that guest cannot set vgpu active. Would that be good > enough? Thanks! Yeah that seems sensible. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 2dc8709..39df304 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -239,6 +239,9 @@ int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists if (INTEL_INFO(dev)->gen >= 9) return 1; + if (HAS_LOGICAL_RING_CONTEXTS(dev) && intel_vgpu_active(dev)) + return 1; + if (enable_execlists == 0) return 0;