[v2,2/6] drm/i915: Enable full ppgtt for vgpu on Broadwell
diff mbox

Message ID 1440747679-20716-3-git-send-email-zhiyuan.lv@intel.com
State New
Headers show

Commit Message

Zhiyuan Lv Aug. 28, 2015, 7:41 a.m. UTC
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>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Joonas Lahtinen Aug. 31, 2015, 12:55 p.m. UTC | #1
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;
>  
>  	/*
>  	 * We don't allow disabling PPGTT for gen9+ as it's a
> requirement for
Daniel Vetter Sept. 2, 2015, 9:45 a.m. UTC | #2
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

Patch
diff mbox

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;
 
 	/*
 	 * We don't allow disabling PPGTT for gen9+ as it's a requirement for