[PATCH 1/2] drm/i915: Drop WARN_ON(flags) from ppgtt_bind_vma()

[PATCH 1/2] drm/i915: Drop WARN_ON(flags) from ppgtt_bind_vma()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We will call ppgtt_bind_vma() with flags != 0, so the WARN_ON(flags)
is bogus. Kill it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7727103..0dce6fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1243,8 +1243,6 @@ ppgtt_bind_vma(struct i915_vma *vma,
 	       enum i915_cache_level cache_level,
 	       u32 flags)
 {
-	WARN_ON(flags);
-
 	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
 				cache_level);
 }
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Unbind all vmas whose new cache_level doesn't agree with the neighbours

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When we change the cache_level for an object we need to make sure
we don't put differing types of snoopable memory too close to each
other on non-LLC machines.

Currently i915_gem_object_set_cache_level() will stop looking when
it finds just one vma that has such a conflict. Drop the bogus break
statement to make sure it will unbind all vmas which need to be moved
around to avoid the conflict.

I suppose this is a theoretical issue as currently we don't enable
ppgtt on non-LLC machines, so each object can only have one vma.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 92b0b41..70384c8d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3482,8 +3482,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 			ret = i915_vma_unbind(vma);
 			if (ret)
 				return ret;
-
-			break;
 		}
 	}
 
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Unbind all vmas whose new cache_level doesn't agree with the neighbours

[Daniel Vetter]

On Wed, Mar 12, 2014 at 07:32:27PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When we change the cache_level for an object we need to make sure
> we don't put differing types of snoopable memory too close to each
> other on non-LLC machines.
> 
> Currently i915_gem_object_set_cache_level() will stop looking when
> it finds just one vma that has such a conflict. Drop the bogus break
> statement to make sure it will unbind all vmas which need to be moved
> around to avoid the conflict.
> 
> I suppose this is a theoretical issue as currently we don't enable
> ppgtt on non-LLC machines, so each object can only have one vma.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Both patches merged, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Drop WARN_ON(flags) from ppgtt_bind_vma()

[Ben Widawsky]

On Wed, Mar 12, 2014 at 07:32:26PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We will call ppgtt_bind_vma() with flags != 0, so the WARN_ON(flags)
> is bogus. Kill it.
> 

This is not an appropriate commit message to change an invariant. The
case was true, and it apparently no longer holds. At the very least the
commit should have the SHA which changed the invariant, and preferably
an explanation as to why the invariant no longer holds ("is bogus"). I

The reason you have given to remove this WARN_ON can be used for any
assertion we ever hit and simply reiterates what the patch does.

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7727103..0dce6fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1243,8 +1243,6 @@ ppgtt_bind_vma(struct i915_vma *vma,
>  	       enum i915_cache_level cache_level,
>  	       u32 flags)
>  {
> -	WARN_ON(flags);
> -
>  	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
>  				cache_level);
>  }
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Drop WARN_ON(flags) from ppgtt_bind_vma()

[Daniel Vetter]

On Sat, Mar 22, 2014 at 09:59:50AM -0700, Ben Widawsky wrote:
> On Wed, Mar 12, 2014 at 07:32:26PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We will call ppgtt_bind_vma() with flags != 0, so the WARN_ON(flags)
> > is bogus. Kill it.
> > 
> 
> This is not an appropriate commit message to change an invariant. The
> case was true, and it apparently no longer holds. At the very least the
> commit should have the SHA which changed the invariant, and preferably
> an explanation as to why the invariant no longer holds ("is bogus"). I
> 
> The reason you have given to remove this WARN_ON can be used for any
> assertion we ever hit and simply reiterates what the patch does.

I agree that the commit message is a bit too thin. Unfortunately I've
already baked in dinq so I can't go back and rectify history :(

Since this patch removes something we also can't fix that by adding a
comment around the new code and supplying the missing commit messages bits
there.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch