Re: [PATCH 2/7] drm/i915: Enable full ppgtt for vgpu

Daniel Vetter
2015-09-02T08:06:30Z

On Thu, Aug 27, 2015 at 10:28:49AM +0800, Zhiyuan Lv wrote:
> Hi Danie,
>
> On Wed, Aug 26, 2015 at 10:47:37AM +0200, Daniel Vetter wrote:
> > On Thu, Aug 20, 2015 at 01:57:13PM +0300, Joonas Lahtinen wrote:
> > > On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> > > > The full ppgtt is supported in Intel GVT-g device model. So the
> > > > restriction can be removed.
> > > >
> > > > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > > > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> > >
> > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > >
> > > > ---
> > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ---
> > > > 1 file changed, 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > index ed10e77..823005c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -108,9 +108,6 @@ static int sanitize_enable_ppgtt(struct
> > > > drm_device *dev, int enable_ppgtt)
> > > > has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
> > > > has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
> > > >
> > > > - if (intel_vgpu_active(dev))
> > > > - has_full_ppgtt = false; /* emulation is too hard */
> >
> > Don't we need a feature check for the virtual gpu here? Or at least a
> > platform check? Seems like the backwards/forwards compat story isn't too
> > thought out yet here. Note that the kernel of the host and the guest might
> > not be the same at all, much less the kvm part.
>
> Yeah, backwards/forwards compatibility is not considered, since we are
> just to start the upstream of iGVT-g host changes. Right now if people
> uses new enough iGVT-g code (off-tree now), both HSW and BDW should
> work with PPGTT.
>
> So new host with both old guest or new guest are working. The only
> thing impacted is old host with new guest kernel. In order to keep it
> work, I can change the code like:
>
> + if (intel_vgpu_active(dev))
> + has_full_ppgtt = !IS_HASWELL(dev);
>
> Any comments? Thanks for the review!

In the end that's your call (well until we have users screaming that a
kernel upgrade breaks their xengt setup, then you have to keep backwards
compat). But since it's a requirement from Linus Torvalds for upstream I
highly suggest you start thinking/testing for backwards compat (both on
host and guest side when the other part is older) right away. Otherwise
trying to shoehorn a good backwards compat scheme later on could be really
painful.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation