[PATCH] drm/i915: Remove wrong warning from i915_gem_context_clean

[PATCH] drm/i915: Remove wrong warning from i915_gem_context_clean

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Date:   Mon Oct 5 13:26:36 2015 +0100

    drm/i915: Clean up associated VMAs on context destruction

Introduced a wrong assumption that all contexts have a ppgtt
instance. This is not true when full PPGTT is not active so
remove the WARN_ON_ONCE from the context cleanup code.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 680b4c9f6b73..8c688a5f1589 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -138,7 +138,7 @@ static void i915_gem_context_clean(struct intel_context *ctx)
 	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
 	struct i915_vma *vma, *next;
 
-	if (WARN_ON_ONCE(!ppgtt))
+	if (!ppgtt)
 		return;
 
 	WARN_ON(!list_empty(&ppgtt->base.active_list));
-- 
1.9.1

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

Re: [PATCH] drm/i915: Remove wrong warning from i915_gem_context_clean

[Michel Thierry]

On 10/8/2015 3:37 PM, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
> Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Date:   Mon Oct 5 13:26:36 2015 +0100
>
>      drm/i915: Clean up associated VMAs on context destruction
>
> Introduced a wrong assumption that all contexts have a ppgtt
> instance. This is not true when full PPGTT is not active so
> remove the WARN_ON_ONCE from the context cleanup code.

Aliasing ppgtt? (for the record, I tried with enable_ppgtt=1 while 
reviewing the patch and didn't see the warning... maybe I didn't try 
hard enough).

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 680b4c9f6b73..8c688a5f1589 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -138,7 +138,7 @@ static void i915_gem_context_clean(struct intel_context *ctx)
>   	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
>   	struct i915_vma *vma, *next;
>
> -	if (WARN_ON_ONCE(!ppgtt))
> +	if (!ppgtt)
>   		return;
>
>   	WARN_ON(!list_empty(&ppgtt->base.active_list));
>

Reviewed-by: Michel Thierry <michel.thierry@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Remove wrong warning from i915_gem_context_clean

[Chris Wilson]

On Thu, Oct 08, 2015 at 04:12:31PM +0100, Michel Thierry wrote:
> On 10/8/2015 3:37 PM, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
> >Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Date:   Mon Oct 5 13:26:36 2015 +0100
> >
> >     drm/i915: Clean up associated VMAs on context destruction
> >
> >Introduced a wrong assumption that all contexts have a ppgtt
> >instance. This is not true when full PPGTT is not active so
> >remove the WARN_ON_ONCE from the context cleanup code.
> 
> Aliasing ppgtt? (for the record, I tried with enable_ppgtt=1 while
> reviewing the patch and didn't see the warning... maybe I didn't try
> hard enough).

We create a context per-fd on all platforms, as in it we store user
specific information (such as hangstats, process identifiers etc). Think
of a context as a view of driver state not just GPU state.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Remove wrong warning from i915_gem_context_clean

[Daniel Vetter]

On Thu, Oct 08, 2015 at 04:12:31PM +0100, Michel Thierry wrote:
> On 10/8/2015 3:37 PM, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
> >Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Date:   Mon Oct 5 13:26:36 2015 +0100
> >
> >     drm/i915: Clean up associated VMAs on context destruction
> >
> >Introduced a wrong assumption that all contexts have a ppgtt
> >instance. This is not true when full PPGTT is not active so
> >remove the WARN_ON_ONCE from the context cleanup code.
> 
> Aliasing ppgtt? (for the record, I tried with enable_ppgtt=1 while reviewing
> the patch and didn't see the warning... maybe I didn't try hard enough).

Aliasing ppgtt isn't a real ppgtt attached to a context, but just a pile
of pagetables attached to the global gtt in dev_priv->mm.aliasing_ppgtt.

Originally I had the idea that there would be another structure for the
real per-process stuff like

struct i915_ppgtt {
	i915_address_space base;
	i915_hw_ppgtt ppgtt;
};

but that kinda never got anywhere with the original per-process
infrastructure merge. So now I get to have a broken record about "aliasing
ppgtt isnt' a real ppgtt and will never show up in ctx->ppgtt".

> 
> >
> >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Cc: Michel Thierry <michel.thierry@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> >index 680b4c9f6b73..8c688a5f1589 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_context.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_context.c
> >@@ -138,7 +138,7 @@ static void i915_gem_context_clean(struct intel_context *ctx)
> >  	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
> >  	struct i915_vma *vma, *next;
> >
> >-	if (WARN_ON_ONCE(!ppgtt))
> >+	if (!ppgtt)
> >  		return;
> >
> >  	WARN_ON(!list_empty(&ppgtt->base.active_list));
> >
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
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