[Intel-gfx] [PATCH] drm/i915: Eliminate dead code in intel_sanitize_enable_ppgtt()

Imre Deak imre.deak at intel.com
Wed Jul 26 14:51:33 UTC 2017


Yep, looks better in the end. If you could send a proper patch I'd
review it.

On Wed, Jul 26, 2017 at 03:44:00PM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2017-07-25 22:12:36)
> > From: Damien Lespiau <damien.lespiau at intel.com>
> > 
> > We exit early if has_aliasing_ppgtt is 0, so towards the end of the
> > function has_aliasing_ppgtt can only be 1.
> > 
> > Also:
> > 
> >         if (foo)
> >                 return 1;
> >         else
> >                 return 0;
> > 
> > when foo is already a bool is really just:
> > 
> >         return foo;
> > 
> > v2:
> > - Simplify more using the fact that i915.enable_execlists requires that
> >   GEN >= 8. (Chris)
> > 
> > Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > [Imre: updated to v2]
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 10aa7762d9a6..f401318430ed 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -133,7 +133,7 @@ static inline void i915_ggtt_invalidate(struct drm_i915_private *i915)
> >  }
> >  
> >  int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
> > -                               int enable_ppgtt)
> > +                               int enable_ppgtt)
> >  {
> >         bool has_aliasing_ppgtt;
> >         bool has_full_ppgtt;
> > @@ -180,10 +180,14 @@ int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
> >                 return 0;
> >         }
> >  
> > -       if (INTEL_GEN(dev_priv) >= 8 && i915.enable_execlists && has_full_ppgtt)
> > -               return has_full_48bit_ppgtt ? 3 : 2;
> > -       else
> > -               return has_aliasing_ppgtt ? 1 : 0;
> > +       if (!has_full_ppgtt)
> > +               return 1;
> > +
> > +       /* full-ppgtt doesn't yet work reliably in legacy ringbuffer mode */
> > +       if (!i915.enable_execlists)
> > +               return 1;
> > +
> > +       return has_full_48bit_ppgtt ? 3 : 2;
> >  }
> 
> I was looking at a more complete overhaul, say
> 
> nt intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
> -                               int enable_ppgtt)
> +                               int enable_ppgtt)
>  {
> -       bool has_aliasing_ppgtt;
> -       bool has_full_ppgtt;
> -       bool has_full_48bit_ppgtt;
> +       unsigned int min, max;
>  
> -       has_aliasing_ppgtt = dev_priv->info.has_aliasing_ppgtt;
> -       has_full_ppgtt = dev_priv->info.has_full_ppgtt;
> -       has_full_48bit_ppgtt = dev_priv->info.has_full_48bit_ppgtt;
> -
> -       if (intel_vgpu_active(dev_priv)) {
> -               /* emulation is too hard */
> -               has_full_ppgtt = false;
> -               has_full_48bit_ppgtt = false;
> -       }
> -
> -       if (!has_aliasing_ppgtt)
> -               return 0;
> -
> -       /*
> -        * We don't allow disabling PPGTT for gen9+ as it's a requirement for
> -        * execlists, the sole mechanism available to submit work.
> -        */
> -       if (enable_ppgtt == 0 && INTEL_GEN(dev_priv) < 9)
> -               return 0;
> -
> -       if (enable_ppgtt == 1)
> -               return 1;
> -
> -       if (enable_ppgtt == 2 && has_full_ppgtt)
> -               return 2;
> -
> -       if (enable_ppgtt == 3 && has_full_48bit_ppgtt)
> -               return 3;
> +       if (dev_priv->info.has_full_48bit_ppgtt)
> +               max = 3;
> +       else if (dev_priv->info.has_full_ppgtt)
> +               max = 2;
> +       else if (dev_priv->info.has_aliasing_ppgtt)
> +               max = 1;
> +       else
> +               max = 0;
>  
>         /* Disable ppgtt on SNB if VT-d is on. */
>         if (IS_GEN6(dev_priv) && intel_vtd_active()) {
>                 DRM_INFO("Disabling PPGTT because VT-d is on\n");
> -               return 0;
> +               max = 0;
>         }
>  
>         /* Early VLV doesn't have this */
>         if (IS_VALLEYVIEW(dev_priv) && dev_priv->drm.pdev->revision < 0xb) {
>                 DRM_DEBUG_DRIVER("disabling PPGTT on pre-B3 step VLV\n");
> -               return 0;
> +               max = 0;
>         }
>  
> -       if (INTEL_GEN(dev_priv) >= 8 && i915.enable_execlists && has_full_ppgtt)
> -               return has_full_48bit_ppgtt ? 3 : 2;
> -       else
> -               return has_aliasing_ppgtt ? 1 : 0;
> +       /* full-ppgtt doesn't yet work reliably in legacy ringbuffer mode */
> +       if (!i915.enable_execlists)
> +               max = min(max, 1u);
> +
> +       /* emulation of full-ppgtt is too hard */
> +       if (intel_vgpu_active(dev_priv))
> +               max = min(max, 1u);
> +
> +       /*
> +        * We don't allow disabling PPGTT for gen9+ as it's a requirement for
> +        * execlists, the sole mechanism available to submit work.
> +        */
> +       min = 0;
> +       if (INTEL_GEN(dev_priv) >= 9)
> +               min = 1;
> +
> +       return clamp_t(unsigned int, enable_ppgtt, min, max);
>  }
>  
>  static int ppgtt_bind_vma(struct i915_vma *vma,
> 
> 
> i.e.
> 
> int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
>                                 int enable_ppgtt)
> {
>         unsigned int min, max;
> 
>         if (dev_priv->info.has_full_48bit_ppgtt)
>                 max = 3;
>         else if (dev_priv->info.has_full_ppgtt)
>                 max = 2;
>         else if (dev_priv->info.has_aliasing_ppgtt)
>                 max = 1;
>         else
>                 max = 0;
> 
>         /* Disable ppgtt on SNB if VT-d is on. */
>         if (IS_GEN6(dev_priv) && intel_vtd_active()) {
>                 DRM_INFO("Disabling PPGTT because VT-d is on\n");
>                 max = 0;
>         }
> 
>         /* Early VLV doesn't have this */
>         if (IS_VALLEYVIEW(dev_priv) && dev_priv->drm.pdev->revision < 0xb) {
>                 DRM_DEBUG_DRIVER("disabling PPGTT on pre-B3 step VLV\n");
>                 max = 0;
>         }
> 
>         /* full-ppgtt doesn't yet work reliably in legacy ringbuffer mode */
>         if (!i915.enable_execlists)
>                 max = min(max, 1u);
> 
>         /* emulation of full-ppgtt is too hard */
>         if (intel_vgpu_active(dev_priv))
>                 max = min(max, 1u);
> 
>         /*
>          * We don't allow disabling PPGTT for gen9+ as it's a requirement for
>          * execlists, the sole mechanism available to submit work.
>          */
>         min = 0;
>         if (INTEL_GEN(dev_priv) >= 9)
>                 min = 1;
> 
>         return clamp_t(unsigned int, enable_ppgtt, min, max);
> }


More information about the Intel-gfx mailing list