* [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list
@ 2015-09-24 9:02 Chris Wilson
2015-09-24 9:09 ` Chris Wilson
2015-09-24 10:35 ` Jani Nikula
0 siblings, 2 replies; 7+ messages in thread
From: Chris Wilson @ 2015-09-24 9:02 UTC (permalink / raw)
To: intel-gfx; +Cc: Jesse Barnes, Daniel Vetter, Mika Kuoppala
When preallocating a stolen object during early initialisation, we may
be running before we have setup the the global GTT VM state, in
particular before we have initialised the range manager and associated
lists. As this is the case, we defer binding the stolen object until we
call i915_gem_setup_global_gtt(). Not only should we defer the binding,
but we should also defer the VM list manipulation.
Fixes regression from commit a2cad9dff4dd44d0244b966d980de9d602d87593
Author: Michał Winiarski <michal.winiarski@intel.com>
Date: Wed Sep 16 11:49:00 2015 +0200
drm/i915/gtt: Do not initialize drm_mm twice.
Whilst I am here remove the duplicate work leaving dangling pointers
from the error path...
Reported-by: Jesse Barnes <jesse@virtuousgeek.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jesse@virtuousgeek.org>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++----------
2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 01f3521e77d3..9cc68624410e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
* the bound flag ourselves.
*/
vma->bound |= GLOBAL_BIND;
-
}
if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
@@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
return ret;
}
vma->bound |= GLOBAL_BIND;
+ list_add_tail(&vma->mm_list, &ggtt->inactive_list);
}
/* Clear any non-preallocated blocks */
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 55df6ce34751..15207796e1b3 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -584,7 +584,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
if (IS_ERR(vma)) {
ret = PTR_ERR(vma);
- goto err_out;
+ goto err;
}
/* To simplify the initialisation sequence between KMS and GTT,
@@ -598,23 +598,19 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
ret = drm_mm_reserve_node(&ggtt->mm, &vma->node);
if (ret) {
DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
- goto err_vma;
+ goto err;
}
- }
- vma->bound |= GLOBAL_BIND;
+ vma->bound |= GLOBAL_BIND;
+ list_add_tail(&vma->mm_list, &ggtt->inactive_list);
+ }
list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
- list_add_tail(&vma->mm_list, &ggtt->inactive_list);
i915_gem_object_pin_pages(obj);
return obj;
-err_vma:
- i915_gem_vma_destroy(vma);
-err_out:
- i915_gem_stolen_remove_node(dev_priv, stolen);
- kfree(stolen);
+err:
drm_gem_object_unreference(&obj->base);
return NULL;
}
--
2.5.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list
2015-09-24 9:02 [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list Chris Wilson
@ 2015-09-24 9:09 ` Chris Wilson
2015-09-24 10:35 ` Jani Nikula
1 sibling, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2015-09-24 9:09 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Mika Kuoppala
On Thu, Sep 24, 2015 at 10:02:00AM +0100, Chris Wilson wrote:
> When preallocating a stolen object during early initialisation, we may
> be running before we have setup the the global GTT VM state, in
> particular before we have initialised the range manager and associated
> lists. As this is the case, we defer binding the stolen object until we
> call i915_gem_setup_global_gtt(). Not only should we defer the binding,
> but we should also defer the VM list manipulation.
>
> Fixes regression from commit a2cad9dff4dd44d0244b966d980de9d602d87593
> Author: Michał Winiarski <michal.winiarski@intel.com>
> Date: Wed Sep 16 11:49:00 2015 +0200
>
> drm/i915/gtt: Do not initialize drm_mm twice.
For the record, this is not the root cause of the bug just the trigger
fr the current breakage. s/from/uncovered by/ ?
> Whilst I am here remove the duplicate work leaving dangling pointers
> from the error path...
>
> Reported-by: Jesse Barnes <jesse@virtuousgeek.org>
Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jesse@virtuousgeek.org>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
-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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list
2015-09-24 9:02 [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list Chris Wilson
2015-09-24 9:09 ` Chris Wilson
@ 2015-09-24 10:35 ` Jani Nikula
2015-09-24 10:57 ` Chris Wilson
1 sibling, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2015-09-24 10:35 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Jesse Barnes, Mika Kuoppala, Daniel Vetter
On Thu, 24 Sep 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> When preallocating a stolen object during early initialisation, we may
> be running before we have setup the the global GTT VM state, in
> particular before we have initialised the range manager and associated
> lists. As this is the case, we defer binding the stolen object until we
> call i915_gem_setup_global_gtt(). Not only should we defer the binding,
> but we should also defer the VM list manipulation.
>
> Fixes regression from commit a2cad9dff4dd44d0244b966d980de9d602d87593
> Author: Michał Winiarski <michal.winiarski@intel.com>
> Date: Wed Sep 16 11:49:00 2015 +0200
>
> drm/i915/gtt: Do not initialize drm_mm twice.
>
> Whilst I am here remove the duplicate work leaving dangling pointers
> from the error path...
>
> Reported-by: Jesse Barnes <jesse@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jesse@virtuousgeek.org>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
CC [M] drivers/gpu/drm/i915/i915_gem_stolen.o
drivers/gpu/drm/i915/i915_gem_gtt.c: In function ‘i915_gem_setup_global_gtt’:
drivers/gpu/drm/i915/i915_gem_gtt.c:2659:33: error: ‘ggtt’ undeclared (first use in this function)
list_add_tail(&vma->mm_list, &ggtt->inactive_list);
^
Jani.
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++----------
> 2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 01f3521e77d3..9cc68624410e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
> * the bound flag ourselves.
> */
> vma->bound |= GLOBAL_BIND;
> -
> }
>
> if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
> @@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
> return ret;
> }
> vma->bound |= GLOBAL_BIND;
> + list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> }
>
> /* Clear any non-preallocated blocks */
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 55df6ce34751..15207796e1b3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -584,7 +584,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
> if (IS_ERR(vma)) {
> ret = PTR_ERR(vma);
> - goto err_out;
> + goto err;
> }
>
> /* To simplify the initialisation sequence between KMS and GTT,
> @@ -598,23 +598,19 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> ret = drm_mm_reserve_node(&ggtt->mm, &vma->node);
> if (ret) {
> DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
> - goto err_vma;
> + goto err;
> }
> - }
>
> - vma->bound |= GLOBAL_BIND;
> + vma->bound |= GLOBAL_BIND;
> + list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> + }
>
> list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
> - list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> i915_gem_object_pin_pages(obj);
>
> return obj;
>
> -err_vma:
> - i915_gem_vma_destroy(vma);
> -err_out:
> - i915_gem_stolen_remove_node(dev_priv, stolen);
> - kfree(stolen);
> +err:
> drm_gem_object_unreference(&obj->base);
> return NULL;
> }
> --
> 2.5.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list
2015-09-24 10:35 ` Jani Nikula
@ 2015-09-24 10:57 ` Chris Wilson
2015-09-24 11:16 ` Winiarski, Michal
0 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2015-09-24 10:57 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter, Mika Kuoppala
When preallocating a stolen object during early initialisation, we may
be running before we have setup the the global GTT VM state, in
particular before we have initialised the range manager and associated
lists. As this is the case, we defer binding the stolen object until we
call i915_gem_setup_global_gtt(). Not only should we defer the binding,
but we should also defer the VM list manipulation.
Fixes regression uncovered by commit a2cad9dff4dd44d0244b966d980de9d602d87593
Author: Michał Winiarski <michal.winiarski@intel.com>
Date: Wed Sep 16 11:49:00 2015 +0200
drm/i915/gtt: Do not initialize drm_mm twice.
Whilst I am here remove the duplicate work leaving dangling pointers
from the error path...
v2: Typos galore before coffee.
Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++----------
2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 01f3521e77d3..291305fb2f3c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
* the bound flag ourselves.
*/
vma->bound |= GLOBAL_BIND;
-
}
if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
@@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
return ret;
}
vma->bound |= GLOBAL_BIND;
+ list_add_tail(&vma->mm_list, &ggtt_vm->inactive_list);
}
/* Clear any non-preallocated blocks */
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 55df6ce34751..15207796e1b3 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -584,7 +584,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
if (IS_ERR(vma)) {
ret = PTR_ERR(vma);
- goto err_out;
+ goto err;
}
/* To simplify the initialisation sequence between KMS and GTT,
@@ -598,23 +598,19 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
ret = drm_mm_reserve_node(&ggtt->mm, &vma->node);
if (ret) {
DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
- goto err_vma;
+ goto err;
}
- }
- vma->bound |= GLOBAL_BIND;
+ vma->bound |= GLOBAL_BIND;
+ list_add_tail(&vma->mm_list, &ggtt->inactive_list);
+ }
list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
- list_add_tail(&vma->mm_list, &ggtt->inactive_list);
i915_gem_object_pin_pages(obj);
return obj;
-err_vma:
- i915_gem_vma_destroy(vma);
-err_out:
- i915_gem_stolen_remove_node(dev_priv, stolen);
- kfree(stolen);
+err:
drm_gem_object_unreference(&obj->base);
return NULL;
}
--
2.5.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list
2015-09-24 10:57 ` Chris Wilson
@ 2015-09-24 11:16 ` Winiarski, Michal
2015-09-24 11:24 ` Jani Nikula
0 siblings, 1 reply; 7+ messages in thread
From: Winiarski, Michal @ 2015-09-24 11:16 UTC (permalink / raw)
To: intel-gfx@lists.freedesktop.org, chris@chris-wilson.co.uk
Cc: daniel.vetter@ffwll.ch, Kuoppala, Mika
On Thu, 2015-09-24 at 11:57 +0100, Chris Wilson wrote:
> When preallocating a stolen object during early initialisation, we
> may
> be running before we have setup the the global GTT VM state, in
> particular before we have initialised the range manager and
> associated
> lists. As this is the case, we defer binding the stolen object until
> we
> call i915_gem_setup_global_gtt(). Not only should we defer the
> binding,
> but we should also defer the VM list manipulation.
>
> Fixes regression uncovered by commit
> a2cad9dff4dd44d0244b966d980de9d602d87593
> Author: Michał Winiarski <michal.winiarski@intel.com>
> Date: Wed Sep 16 11:49:00 2015 +0200
>
> drm/i915/gtt: Do not initialize drm_mm twice.
>
> Whilst I am here remove the duplicate work leaving dangling pointers
> from the error path...
>
> v2: Typos galore before coffee.
>
> Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
-Michał
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++----------
> 2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 01f3521e77d3..291305fb2f3c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
> * the bound flag ourselves.
> */
> vma->bound |= GLOBAL_BIND;
> -
> }
>
> if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
> @@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct
> drm_device *dev,
> return ret;
> }
> vma->bound |= GLOBAL_BIND;
> + list_add_tail(&vma->mm_list, &ggtt_vm
> ->inactive_list);
> }
>
> /* Clear any non-preallocated blocks */
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 55df6ce34751..15207796e1b3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -584,7 +584,7 @@
> i915_gem_object_create_stolen_for_preallocated(struct drm_device
> *dev,
> vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
> if (IS_ERR(vma)) {
> ret = PTR_ERR(vma);
> - goto err_out;
> + goto err;
> }
>
> /* To simplify the initialisation sequence between KMS and
> GTT,
> @@ -598,23 +598,19 @@
> i915_gem_object_create_stolen_for_preallocated(struct drm_device
> *dev,
> ret = drm_mm_reserve_node(&ggtt->mm, &vma->node);
> if (ret) {
> DRM_DEBUG_KMS("failed to allocate stolen GTT
> space\n");
> - goto err_vma;
> + goto err;
> }
> - }
>
> - vma->bound |= GLOBAL_BIND;
> + vma->bound |= GLOBAL_BIND;
> + list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> + }
>
> list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
> - list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> i915_gem_object_pin_pages(obj);
>
> return obj;
>
> -err_vma:
> - i915_gem_vma_destroy(vma);
> -err_out:
> - i915_gem_stolen_remove_node(dev_priv, stolen);
> - kfree(stolen);
> +err:
> drm_gem_object_unreference(&obj->base);
> return NULL;
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list
2015-09-24 11:16 ` Winiarski, Michal
@ 2015-09-24 11:24 ` Jani Nikula
2015-09-24 13:58 ` Daniel Vetter
0 siblings, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2015-09-24 11:24 UTC (permalink / raw)
To: Winiarski, Michal, intel-gfx@lists.freedesktop.org,
chris@chris-wilson.co.uk
Cc: daniel.vetter@ffwll.ch, Kuoppala, Mika
On Thu, 24 Sep 2015, "Winiarski, Michal" <michal.winiarski@intel.com> wrote:
> On Thu, 2015-09-24 at 11:57 +0100, Chris Wilson wrote:
>> When preallocating a stolen object during early initialisation, we
>> may
>> be running before we have setup the the global GTT VM state, in
>> particular before we have initialised the range manager and
>> associated
>> lists. As this is the case, we defer binding the stolen object until
>> we
>> call i915_gem_setup_global_gtt(). Not only should we defer the
>> binding,
>> but we should also defer the VM list manipulation.
>>
>> Fixes regression uncovered by commit
>> a2cad9dff4dd44d0244b966d980de9d602d87593
>> Author: Michał Winiarski <michal.winiarski@intel.com>
>> Date: Wed Sep 16 11:49:00 2015 +0200
>>
>> drm/i915/gtt: Do not initialize drm_mm twice.
I confirm reverting the above works.
This patch is
Tested-by: Jani Nikula <jani.nikula@intel.com>
on a BWD GT2 ULT.
>>
>> Whilst I am here remove the duplicate work leaving dangling pointers
>> from the error path...
>>
>> v2: Typos galore before coffee.
>>
>> Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>
> Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
>
> -Michał
>
>> ---
>> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
>> drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++----------
>> 2 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 01f3521e77d3..291305fb2f3c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
>> * the bound flag ourselves.
>> */
>> vma->bound |= GLOBAL_BIND;
>> -
>> }
>>
>> if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
>> @@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct
>> drm_device *dev,
>> return ret;
>> }
>> vma->bound |= GLOBAL_BIND;
>> + list_add_tail(&vma->mm_list, &ggtt_vm
>> ->inactive_list);
>> }
>>
>> /* Clear any non-preallocated blocks */
>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> index 55df6ce34751..15207796e1b3 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -584,7 +584,7 @@
>> i915_gem_object_create_stolen_for_preallocated(struct drm_device
>> *dev,
>> vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
>> if (IS_ERR(vma)) {
>> ret = PTR_ERR(vma);
>> - goto err_out;
>> + goto err;
>> }
>>
>> /* To simplify the initialisation sequence between KMS and
>> GTT,
>> @@ -598,23 +598,19 @@
>> i915_gem_object_create_stolen_for_preallocated(struct drm_device
>> *dev,
>> ret = drm_mm_reserve_node(&ggtt->mm, &vma->node);
>> if (ret) {
>> DRM_DEBUG_KMS("failed to allocate stolen GTT
>> space\n");
>> - goto err_vma;
>> + goto err;
>> }
>> - }
>>
>> - vma->bound |= GLOBAL_BIND;
>> + vma->bound |= GLOBAL_BIND;
>> + list_add_tail(&vma->mm_list, &ggtt->inactive_list);
>> + }
>>
>> list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
>> - list_add_tail(&vma->mm_list, &ggtt->inactive_list);
>> i915_gem_object_pin_pages(obj);
>>
>> return obj;
>>
>> -err_vma:
>> - i915_gem_vma_destroy(vma);
>> -err_out:
>> - i915_gem_stolen_remove_node(dev_priv, stolen);
>> - kfree(stolen);
>> +err:
>> drm_gem_object_unreference(&obj->base);
>> return NULL;
>> }
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list
2015-09-24 11:24 ` Jani Nikula
@ 2015-09-24 13:58 ` Daniel Vetter
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2015-09-24 13:58 UTC (permalink / raw)
To: Jani Nikula
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
Kuoppala, Mika
On Thu, Sep 24, 2015 at 02:24:36PM +0300, Jani Nikula wrote:
> On Thu, 24 Sep 2015, "Winiarski, Michal" <michal.winiarski@intel.com> wrote:
> > On Thu, 2015-09-24 at 11:57 +0100, Chris Wilson wrote:
> >> When preallocating a stolen object during early initialisation, we
> >> may
> >> be running before we have setup the the global GTT VM state, in
> >> particular before we have initialised the range manager and
> >> associated
> >> lists. As this is the case, we defer binding the stolen object until
> >> we
> >> call i915_gem_setup_global_gtt(). Not only should we defer the
> >> binding,
> >> but we should also defer the VM list manipulation.
> >>
> >> Fixes regression uncovered by commit
> >> a2cad9dff4dd44d0244b966d980de9d602d87593
> >> Author: Michał Winiarski <michal.winiarski@intel.com>
> >> Date: Wed Sep 16 11:49:00 2015 +0200
> >>
> >> drm/i915/gtt: Do not initialize drm_mm twice.
>
> I confirm reverting the above works.
>
> This patch is
>
> Tested-by: Jani Nikula <jani.nikula@intel.com>
>
> on a BWD GT2 ULT.
Queued for -next, thanks for the patch.
-Daniel
>
> >>
> >> Whilst I am here remove the duplicate work leaving dangling pointers
> >> from the error path...
> >>
> >> v2: Typos galore before coffee.
> >>
> >> Reported-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> Cc: Michel Thierry <michel.thierry@intel.com>
> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >> Cc: Michał Winiarski <michal.winiarski@intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >
> > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
> >
> > -Michał
> >
> >> ---
> >> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
> >> drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++----------
> >> 2 files changed, 7 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> index 01f3521e77d3..291305fb2f3c 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> @@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
> >> * the bound flag ourselves.
> >> */
> >> vma->bound |= GLOBAL_BIND;
> >> -
> >> }
> >>
> >> if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
> >> @@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct
> >> drm_device *dev,
> >> return ret;
> >> }
> >> vma->bound |= GLOBAL_BIND;
> >> + list_add_tail(&vma->mm_list, &ggtt_vm
> >> ->inactive_list);
> >> }
> >>
> >> /* Clear any non-preallocated blocks */
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> index 55df6ce34751..15207796e1b3 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >> @@ -584,7 +584,7 @@
> >> i915_gem_object_create_stolen_for_preallocated(struct drm_device
> >> *dev,
> >> vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
> >> if (IS_ERR(vma)) {
> >> ret = PTR_ERR(vma);
> >> - goto err_out;
> >> + goto err;
> >> }
> >>
> >> /* To simplify the initialisation sequence between KMS and
> >> GTT,
> >> @@ -598,23 +598,19 @@
> >> i915_gem_object_create_stolen_for_preallocated(struct drm_device
> >> *dev,
> >> ret = drm_mm_reserve_node(&ggtt->mm, &vma->node);
> >> if (ret) {
> >> DRM_DEBUG_KMS("failed to allocate stolen GTT
> >> space\n");
> >> - goto err_vma;
> >> + goto err;
> >> }
> >> - }
> >>
> >> - vma->bound |= GLOBAL_BIND;
> >> + vma->bound |= GLOBAL_BIND;
> >> + list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> >> + }
> >>
> >> list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
> >> - list_add_tail(&vma->mm_list, &ggtt->inactive_list);
> >> i915_gem_object_pin_pages(obj);
> >>
> >> return obj;
> >>
> >> -err_vma:
> >> - i915_gem_vma_destroy(vma);
> >> -err_out:
> >> - i915_gem_stolen_remove_node(dev_priv, stolen);
> >> - kfree(stolen);
> >> +err:
> >> drm_gem_object_unreference(&obj->base);
> >> return NULL;
> >> }
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-09-24 13:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-24 9:02 [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list Chris Wilson
2015-09-24 9:09 ` Chris Wilson
2015-09-24 10:35 ` Jani Nikula
2015-09-24 10:57 ` Chris Wilson
2015-09-24 11:16 ` Winiarski, Michal
2015-09-24 11:24 ` Jani Nikula
2015-09-24 13:58 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.