Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx