Re: [PATCH v2 2/6] drm/i915: Enable full ppgtt for vgpu on Broadwell

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Aug 31, 2015 at 03:55:58PM +0300, Joonas Lahtinen wrote:
> On pe, 2015-08-28 at 15:41 +0800, Zhiyuan Lv wrote:
> > The full ppgtt is supported now in Intel GVT-g device model.
> > Broadwell
> > is allowed to use it in virtual machines.
> > 
> > v2:
> > - Keep backward compatibility on HSW with old device model (daniel)
> > 
> > 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>
> 
> It's a good idea to add the version reviewed after Reviewed-by, when
> adding a new revision. This is not to make it look like the new
> revision had already been reviewed too.
> 
> I this case:
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> (v1)
> 
> Would have been appropriate.
> 
> But you can now leave it as it is, as this patch seems fine, too. Maybe
> could still add a comment in the code what makes Haswell special.
> 
> Regards, Joonas
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index ed10e77..56cc8e8 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -108,8 +108,8 @@ 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 */
> > +	if (intel_vgpu_active(dev) && (IS_HASWELL(dev)))
> > +		has_full_ppgtt = false;

I'd say the real check here should be for INTEL_INFO(dev)->gen < 8. Only
checking for hsw is a bit confusing since then people wonder why hsw is
special. But the only reason is that vgpu isn't supported on pre-hsw.

Using the gen check instead will make it clear that this is a generic
issue with pre-gen8 hw (no execlists) and imo be less confusing. Maybe
even add a comment like:

	/* virtualizing ppgtt with execlists is too hard */
> >  
> >  	/*
> >  	 * We don't allow disabling PPGTT for gen9+ as it's a
> > requirement for
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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