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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > [Imre: updated to v2]
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > 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);
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
