diff mbox

[07/13] drm/i915: Introduce dedicated object VMA iterator

Message ID 1452252592-24803-8-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Jan. 8, 2016, 11:29 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Purpose is to catch places which iterate the object VMA list
without holding the big lock.

Implemented by open coding list_for_each_entry to make the
macro compatible with existing call sites.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
 drivers/gpu/drm/i915/i915_drv.h          |  6 ++++++
 drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
 drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c    |  4 ++--
 6 files changed, 26 insertions(+), 20 deletions(-)

Comments

Chris Wilson Jan. 8, 2016, 11:44 a.m. UTC | #1 
On Fri, Jan 08, 2016 at 11:29:46AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Purpose is to catch places which iterate the object VMA list
> without holding the big lock.
> 
> Implemented by open coding list_for_each_entry to make the
> macro compatible with existing call sites.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> +#define i915_gem_obj_for_each_vma(vma, obj) \
> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \

Let's not go around adding WARN(!mutex_locked) to GEM code when
lockdep_assert_held doesn't add overhead outside of testing.
-Chris
Tvrtko Ursulin Jan. 8, 2016, 1:29 p.m. UTC | #2 
On 08/01/16 11:29, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Purpose is to catch places which iterate the object VMA list
> without holding the big lock.
>
> Implemented by open coding list_for_each_entry to make the
> macro compatible with existing call sites.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
>   drivers/gpu/drm/i915/i915_drv.h          |  6 ++++++
>   drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
>   drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>   drivers/gpu/drm/i915/i915_gpu_error.c    |  4 ++--
>   6 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 714a45cf8a51..d7c2a3201161 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
>   	u64 size = 0;
>   	struct i915_vma *vma;
>
> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +	i915_gem_obj_for_each_vma(vma, obj) {
>   		if (i915_is_ggtt(vma->vm) &&
>   		    drm_mm_node_allocated(&vma->node))
>   			size += vma->node.size;
> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>   		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>   	if (obj->base.name)
>   		seq_printf(m, " (name: %d)", obj->base.name);
> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +	i915_gem_obj_for_each_vma(vma, obj) {
>   		if (vma->pin_count > 0)
>   			pin_count++;
>   	}
> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>   		seq_printf(m, " (display)");
>   	if (obj->fence_reg != I915_FENCE_REG_NONE)
>   		seq_printf(m, " (fence: %d)", obj->fence_reg);
> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +	i915_gem_obj_for_each_vma(vma, obj) {
>   		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>   			   i915_is_ggtt(vma->vm) ? "g" : "pp",
>   			   vma->node.start, vma->node.size);
> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>   		stats->shared += obj->base.size;
>
>   	if (USES_FULL_PPGTT(obj->base.dev)) {
> -		list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		i915_gem_obj_for_each_vma(vma, obj) {
>   			struct i915_hw_ppgtt *ppgtt;
>
>   			if (!drm_mm_node_allocated(&vma->node))
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b77a5d84eac2..0406a020dfcc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
>   void i915_gem_free_object(struct drm_gem_object *obj);
>   void i915_gem_vma_destroy(struct i915_vma *vma);
>
> +#define i915_gem_obj_for_each_vma(vma, obj) \
> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
> +	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> +	     &vma->vma_link != (&(obj)->vma_list); \
> +	     vma = list_next_entry(vma, vma_link))
> +


Unfortunately error capture is not happy with this approach. Can't even 
see that error capture attempts to grab the mutex anywhere.

So what? Drop the idea or add a "doing error capture" flag somewhere?

Regards,

Tvrtko
Daniel Vetter Jan. 11, 2016, 8:43 a.m. UTC | #3 
On Fri, Jan 08, 2016 at 01:29:14PM +0000, Tvrtko Ursulin wrote:
> 
> On 08/01/16 11:29, Tvrtko Ursulin wrote:
> >From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> >Purpose is to catch places which iterate the object VMA list
> >without holding the big lock.
> >
> >Implemented by open coding list_for_each_entry to make the
> >macro compatible with existing call sites.
> >
> >Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >---
> >  drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
> >  drivers/gpu/drm/i915/i915_drv.h          |  6 ++++++
> >  drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
> >  drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
> >  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
> >  drivers/gpu/drm/i915/i915_gpu_error.c    |  4 ++--
> >  6 files changed, 26 insertions(+), 20 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >index 714a45cf8a51..d7c2a3201161 100644
> >--- a/drivers/gpu/drm/i915/i915_debugfs.c
> >+++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >@@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
> >  	u64 size = 0;
> >  	struct i915_vma *vma;
> >
> >-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >+	i915_gem_obj_for_each_vma(vma, obj) {
> >  		if (i915_is_ggtt(vma->vm) &&
> >  		    drm_mm_node_allocated(&vma->node))
> >  			size += vma->node.size;
> >@@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> >  		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
> >  	if (obj->base.name)
> >  		seq_printf(m, " (name: %d)", obj->base.name);
> >-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >+	i915_gem_obj_for_each_vma(vma, obj) {
> >  		if (vma->pin_count > 0)
> >  			pin_count++;
> >  	}
> >@@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> >  		seq_printf(m, " (display)");
> >  	if (obj->fence_reg != I915_FENCE_REG_NONE)
> >  		seq_printf(m, " (fence: %d)", obj->fence_reg);
> >-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >+	i915_gem_obj_for_each_vma(vma, obj) {
> >  		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
> >  			   i915_is_ggtt(vma->vm) ? "g" : "pp",
> >  			   vma->node.start, vma->node.size);
> >@@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> >  		stats->shared += obj->base.size;
> >
> >  	if (USES_FULL_PPGTT(obj->base.dev)) {
> >-		list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >+		i915_gem_obj_for_each_vma(vma, obj) {
> >  			struct i915_hw_ppgtt *ppgtt;
> >
> >  			if (!drm_mm_node_allocated(&vma->node))
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index b77a5d84eac2..0406a020dfcc 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2852,6 +2852,12 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
> >  void i915_gem_free_object(struct drm_gem_object *obj);
> >  void i915_gem_vma_destroy(struct i915_vma *vma);
> >
> >+#define i915_gem_obj_for_each_vma(vma, obj) \
> >+	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
> >+	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> >+	     &vma->vma_link != (&(obj)->vma_list); \
> >+	     vma = list_next_entry(vma, vma_link))
> >+
> 
> 
> Unfortunately error capture is not happy with this approach. Can't even see
> that error capture attempts to grab the mutex anywhere.
> 
> So what? Drop the idea or add a "doing error capture" flag somewhere?

Fix the bugs. Not surprise at all that we've screwed this up all over the
place ;-) Afaics modeset code isn't much better either ...
-Daniel
Daniel Vetter Jan. 11, 2016, 8:48 a.m. UTC | #4 
On Fri, Jan 08, 2016 at 11:44:04AM +0000, Chris Wilson wrote:
> On Fri, Jan 08, 2016 at 11:29:46AM +0000, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Purpose is to catch places which iterate the object VMA list
> > without holding the big lock.
> > 
> > Implemented by open coding list_for_each_entry to make the
> > macro compatible with existing call sites.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > +#define i915_gem_obj_for_each_vma(vma, obj) \
> > +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
> 
> Let's not go around adding WARN(!mutex_locked) to GEM code when
> lockdep_assert_held doesn't add overhead outside of testing.

Hm yeah I still prefere WARN_ON for modeset code (where it doesn't matter)
because of increased test coverage. But for gem it indeed makes more sense
to only do this for lockdep-enabled builds. CI runs with lockdep, so we're
good.
-Daniel
Tvrtko Ursulin Jan. 11, 2016, 9:51 a.m. UTC | #5 
On 11/01/16 08:43, Daniel Vetter wrote:
> On Fri, Jan 08, 2016 at 01:29:14PM +0000, Tvrtko Ursulin wrote:
>>
>> On 08/01/16 11:29, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Purpose is to catch places which iterate the object VMA list
>>> without holding the big lock.
>>>
>>> Implemented by open coding list_for_each_entry to make the
>>> macro compatible with existing call sites.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
>>>   drivers/gpu/drm/i915/i915_drv.h          |  6 ++++++
>>>   drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
>>>   drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
>>>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>>>   drivers/gpu/drm/i915/i915_gpu_error.c    |  4 ++--
>>>   6 files changed, 26 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 714a45cf8a51..d7c2a3201161 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
>>>   	u64 size = 0;
>>>   	struct i915_vma *vma;
>>>
>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>   		if (i915_is_ggtt(vma->vm) &&
>>>   		    drm_mm_node_allocated(&vma->node))
>>>   			size += vma->node.size;
>>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>>   		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>>>   	if (obj->base.name)
>>>   		seq_printf(m, " (name: %d)", obj->base.name);
>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>   		if (vma->pin_count > 0)
>>>   			pin_count++;
>>>   	}
>>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>>   		seq_printf(m, " (display)");
>>>   	if (obj->fence_reg != I915_FENCE_REG_NONE)
>>>   		seq_printf(m, " (fence: %d)", obj->fence_reg);
>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>   		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>>>   			   i915_is_ggtt(vma->vm) ? "g" : "pp",
>>>   			   vma->node.start, vma->node.size);
>>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>>   		stats->shared += obj->base.size;
>>>
>>>   	if (USES_FULL_PPGTT(obj->base.dev)) {
>>> -		list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>> +		i915_gem_obj_for_each_vma(vma, obj) {
>>>   			struct i915_hw_ppgtt *ppgtt;
>>>
>>>   			if (!drm_mm_node_allocated(&vma->node))
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index b77a5d84eac2..0406a020dfcc 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
>>>   void i915_gem_free_object(struct drm_gem_object *obj);
>>>   void i915_gem_vma_destroy(struct i915_vma *vma);
>>>
>>> +#define i915_gem_obj_for_each_vma(vma, obj) \
>>> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
>>> +	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
>>> +	     &vma->vma_link != (&(obj)->vma_list); \
>>> +	     vma = list_next_entry(vma, vma_link))
>>> +
>>
>>
>> Unfortunately error capture is not happy with this approach. Can't even see
>> that error capture attempts to grab the mutex anywhere.
>>
>> So what? Drop the idea or add a "doing error capture" flag somewhere?
> 
> Fix the bugs. Not surprise at all that we've screwed this up all over the
> place ;-) Afaics modeset code isn't much better either ...

Ok I'll drop this patch then since the series contains fixes to all but one
related issues. The remaining one is then:

[   17.370366] ------------[ cut here ]------------
[   17.375633] WARNING: CPU: 0 PID: 1128 at drivers/gpu/drm/i915/i915_gem.c:5166 i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]()
[   17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
[   17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet libphy mii i915 gpio_lynxpoint i2c_hid hid video i2c_algo_bit drm_kms_helper acpi_pad drm lpc_ich mfd_core nls_iso8859_1 e1000e ptp ahci libahci pps_core
[   17.419484] CPU: 0 PID: 1128 Comm: Xorg Tainted: G     U          4.4.0-rc8-160107+ #105
[   17.428771] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014
[   17.443161]  ffffffffa0227790 ffff8800a98439b8 ffffffff81280d82 ffff8800a9843a00
[   17.451677]  ffff8800a98439f0 ffffffff81049c8c ffff8801495d0000 ffff8800aa934900
[   17.460166]  ffff8801495d8668 ffffffffa0242520 ffff8800aacea000 ffff8800a9843a50
[   17.468674] Call Trace:
[   17.471470]  [<ffffffff81280d82>] dump_stack+0x4b/0x79
[   17.477355]  [<ffffffff81049c8c>] warn_slowpath_common+0x7c/0xc0
[   17.484255]  [<ffffffff81049d17>] warn_slowpath_fmt+0x47/0x50
[   17.490869]  [<ffffffffa018f7ef>] i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]
[   17.499572]  [<ffffffffa01a7290>] ? gen9_write8+0x2d0/0x2d0 [i915]
[   17.506663]  [<ffffffffa01bdbfe>] ironlake_update_primary_plane+0x1ee/0x3a0 [i915]
[   17.515358]  [<ffffffffa01ace1f>] intel_plane_atomic_update+0x5f/0x70 [i915]
[   17.523391]  [<ffffffffa00ef122>] drm_atomic_helper_commit_planes_on_crtc+0x142/0x230 [drm_kms_helper]
[   17.534063]  [<ffffffffa01c1744>] intel_atomic_commit+0x424/0x2270 [i915]
[   17.541763]  [<ffffffff814ee04a>] ? __ww_mutex_lock+0x4a/0x82
[   17.548355]  [<ffffffffa0097edb>] ? drm_atomic_check_only+0x12b/0x5e0 [drm]
[   17.556352]  [<ffffffffa0097a07>] ? drm_atomic_set_crtc_for_connector+0x77/0xf0 [drm]
[   17.565346]  [<ffffffffa00983c2>] drm_atomic_commit+0x32/0x50 [drm]
[   17.572553]  [<ffffffffa00ef782>] drm_atomic_helper_set_config+0x72/0xb0 [drm_kms_helper]
[   17.581954]  [<ffffffffa0088e8f>] drm_mode_set_config_internal+0x5f/0x100 [drm]
[   17.590313]  [<ffffffffa008cdc8>] drm_mode_setcrtc+0xd8/0x500 [drm]
[   17.597491]  [<ffffffffa007f5d8>] drm_ioctl+0x258/0x4f0 [drm]
[   17.604065]  [<ffffffff810c34ac>] ? unlock_page+0x4c/0x50
[   17.610245]  [<ffffffffa008ccf0>] ? drm_mode_setplane+0x1c0/0x1c0 [drm]
[   17.617844]  [<ffffffff8111597d>] do_vfs_ioctl+0x2cd/0x4a0
[   17.624137]  [<ffffffff81052572>] ? recalc_sigpending+0x12/0x40
[   17.630943]  [<ffffffff8111eb32>] ? __fget+0x72/0xb0
[   17.636645]  [<ffffffff81115b8c>] SyS_ioctl+0x3c/0x70
[   17.642448]  [<ffffffff81055301>] ? SyS_rt_sigprocmask+0x81/0xa0
[   17.649334]  [<ffffffff814f0017>] entry_SYSCALL_64_fastpath+0x12/0x6a
[   17.656740] ---[ end trace 3f29190006c97826 ]---

Would you like a BZ for this or maybe it is known already?

Regards,

Tvrtko
Daniel Vetter Jan. 12, 2016, 4:19 p.m. UTC | #6 
On Mon, Jan 11, 2016 at 09:51:38AM +0000, Tvrtko Ursulin wrote:
> 
> On 11/01/16 08:43, Daniel Vetter wrote:
> > On Fri, Jan 08, 2016 at 01:29:14PM +0000, Tvrtko Ursulin wrote:
> >>
> >> On 08/01/16 11:29, Tvrtko Ursulin wrote:
> >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>> Purpose is to catch places which iterate the object VMA list
> >>> without holding the big lock.
> >>>
> >>> Implemented by open coding list_for_each_entry to make the
> >>> macro compatible with existing call sites.
> >>>
> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
> >>>   drivers/gpu/drm/i915/i915_drv.h          |  6 ++++++
> >>>   drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
> >>>   drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
> >>>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
> >>>   drivers/gpu/drm/i915/i915_gpu_error.c    |  4 ++--
> >>>   6 files changed, 26 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >>> index 714a45cf8a51..d7c2a3201161 100644
> >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
> >>>   	u64 size = 0;
> >>>   	struct i915_vma *vma;
> >>>
> >>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >>> +	i915_gem_obj_for_each_vma(vma, obj) {
> >>>   		if (i915_is_ggtt(vma->vm) &&
> >>>   		    drm_mm_node_allocated(&vma->node))
> >>>   			size += vma->node.size;
> >>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> >>>   		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
> >>>   	if (obj->base.name)
> >>>   		seq_printf(m, " (name: %d)", obj->base.name);
> >>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >>> +	i915_gem_obj_for_each_vma(vma, obj) {
> >>>   		if (vma->pin_count > 0)
> >>>   			pin_count++;
> >>>   	}
> >>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> >>>   		seq_printf(m, " (display)");
> >>>   	if (obj->fence_reg != I915_FENCE_REG_NONE)
> >>>   		seq_printf(m, " (fence: %d)", obj->fence_reg);
> >>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >>> +	i915_gem_obj_for_each_vma(vma, obj) {
> >>>   		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
> >>>   			   i915_is_ggtt(vma->vm) ? "g" : "pp",
> >>>   			   vma->node.start, vma->node.size);
> >>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> >>>   		stats->shared += obj->base.size;
> >>>
> >>>   	if (USES_FULL_PPGTT(obj->base.dev)) {
> >>> -		list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >>> +		i915_gem_obj_for_each_vma(vma, obj) {
> >>>   			struct i915_hw_ppgtt *ppgtt;
> >>>
> >>>   			if (!drm_mm_node_allocated(&vma->node))
> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>> index b77a5d84eac2..0406a020dfcc 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
> >>>   void i915_gem_free_object(struct drm_gem_object *obj);
> >>>   void i915_gem_vma_destroy(struct i915_vma *vma);
> >>>
> >>> +#define i915_gem_obj_for_each_vma(vma, obj) \
> >>> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
> >>> +	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> >>> +	     &vma->vma_link != (&(obj)->vma_list); \
> >>> +	     vma = list_next_entry(vma, vma_link))
> >>> +
> >>
> >>
> >> Unfortunately error capture is not happy with this approach. Can't even see
> >> that error capture attempts to grab the mutex anywhere.
> >>
> >> So what? Drop the idea or add a "doing error capture" flag somewhere?
> > 
> > Fix the bugs. Not surprise at all that we've screwed this up all over the
> > place ;-) Afaics modeset code isn't much better either ...
> 
> Ok I'll drop this patch then since the series contains fixes to all but one
> related issues. The remaining one is then:
> 
> [   17.370366] ------------[ cut here ]------------
> [   17.375633] WARNING: CPU: 0 PID: 1128 at drivers/gpu/drm/i915/i915_gem.c:5166 i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]()
> [   17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
> [   17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet libphy mii i915 gpio_lynxpoint i2c_hid hid video i2c_algo_bit drm_kms_helper acpi_pad drm lpc_ich mfd_core nls_iso8859_1 e1000e ptp ahci libahci pps_core
> [   17.419484] CPU: 0 PID: 1128 Comm: Xorg Tainted: G     U          4.4.0-rc8-160107+ #105
> [   17.428771] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014
> [   17.443161]  ffffffffa0227790 ffff8800a98439b8 ffffffff81280d82 ffff8800a9843a00
> [   17.451677]  ffff8800a98439f0 ffffffff81049c8c ffff8801495d0000 ffff8800aa934900
> [   17.460166]  ffff8801495d8668 ffffffffa0242520 ffff8800aacea000 ffff8800a9843a50
> [   17.468674] Call Trace:
> [   17.471470]  [<ffffffff81280d82>] dump_stack+0x4b/0x79
> [   17.477355]  [<ffffffff81049c8c>] warn_slowpath_common+0x7c/0xc0
> [   17.484255]  [<ffffffff81049d17>] warn_slowpath_fmt+0x47/0x50
> [   17.490869]  [<ffffffffa018f7ef>] i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]
> [   17.499572]  [<ffffffffa01a7290>] ? gen9_write8+0x2d0/0x2d0 [i915]
> [   17.506663]  [<ffffffffa01bdbfe>] ironlake_update_primary_plane+0x1ee/0x3a0 [i915]
> [   17.515358]  [<ffffffffa01ace1f>] intel_plane_atomic_update+0x5f/0x70 [i915]
> [   17.523391]  [<ffffffffa00ef122>] drm_atomic_helper_commit_planes_on_crtc+0x142/0x230 [drm_kms_helper]
> [   17.534063]  [<ffffffffa01c1744>] intel_atomic_commit+0x424/0x2270 [i915]
> [   17.541763]  [<ffffffff814ee04a>] ? __ww_mutex_lock+0x4a/0x82
> [   17.548355]  [<ffffffffa0097edb>] ? drm_atomic_check_only+0x12b/0x5e0 [drm]
> [   17.556352]  [<ffffffffa0097a07>] ? drm_atomic_set_crtc_for_connector+0x77/0xf0 [drm]
> [   17.565346]  [<ffffffffa00983c2>] drm_atomic_commit+0x32/0x50 [drm]
> [   17.572553]  [<ffffffffa00ef782>] drm_atomic_helper_set_config+0x72/0xb0 [drm_kms_helper]
> [   17.581954]  [<ffffffffa0088e8f>] drm_mode_set_config_internal+0x5f/0x100 [drm]
> [   17.590313]  [<ffffffffa008cdc8>] drm_mode_setcrtc+0xd8/0x500 [drm]
> [   17.597491]  [<ffffffffa007f5d8>] drm_ioctl+0x258/0x4f0 [drm]
> [   17.604065]  [<ffffffff810c34ac>] ? unlock_page+0x4c/0x50
> [   17.610245]  [<ffffffffa008ccf0>] ? drm_mode_setplane+0x1c0/0x1c0 [drm]
> [   17.617844]  [<ffffffff8111597d>] do_vfs_ioctl+0x2cd/0x4a0
> [   17.624137]  [<ffffffff81052572>] ? recalc_sigpending+0x12/0x40
> [   17.630943]  [<ffffffff8111eb32>] ? __fget+0x72/0xb0
> [   17.636645]  [<ffffffff81115b8c>] SyS_ioctl+0x3c/0x70
> [   17.642448]  [<ffffffff81055301>] ? SyS_rt_sigprocmask+0x81/0xa0
> [   17.649334]  [<ffffffff814f0017>] entry_SYSCALL_64_fastpath+0x12/0x6a
> [   17.656740] ---[ end trace 3f29190006c97826 ]---
> 
> Would you like a BZ for this or maybe it is known already?

Maarten is working on this, or at least we're tracking it already in
VIZ-7000.

Can you pls coordinate with Maarten to make sure the final "enable the
warning" patch lands only once all the bits are in?

Thanks, Daniel
Maarten Lankhorst Jan. 12, 2016, 4:43 p.m. UTC | #7 
Op 12-01-16 om 17:19 schreef Daniel Vetter:
> On Mon, Jan 11, 2016 at 09:51:38AM +0000, Tvrtko Ursulin wrote:
>> On 11/01/16 08:43, Daniel Vetter wrote:
>>> On Fri, Jan 08, 2016 at 01:29:14PM +0000, Tvrtko Ursulin wrote:
>>>> On 08/01/16 11:29, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> Purpose is to catch places which iterate the object VMA list
>>>>> without holding the big lock.
>>>>>
>>>>> Implemented by open coding list_for_each_entry to make the
>>>>> macro compatible with existing call sites.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
>>>>>   drivers/gpu/drm/i915/i915_drv.h          |  6 ++++++
>>>>>   drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
>>>>>   drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
>>>>>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>>>>>   drivers/gpu/drm/i915/i915_gpu_error.c    |  4 ++--
>>>>>   6 files changed, 26 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> index 714a45cf8a51..d7c2a3201161 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
>>>>>   	u64 size = 0;
>>>>>   	struct i915_vma *vma;
>>>>>
>>>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>>>   		if (i915_is_ggtt(vma->vm) &&
>>>>>   		    drm_mm_node_allocated(&vma->node))
>>>>>   			size += vma->node.size;
>>>>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>>>>   		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>>>>>   	if (obj->base.name)
>>>>>   		seq_printf(m, " (name: %d)", obj->base.name);
>>>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>>>   		if (vma->pin_count > 0)
>>>>>   			pin_count++;
>>>>>   	}
>>>>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>>>>   		seq_printf(m, " (display)");
>>>>>   	if (obj->fence_reg != I915_FENCE_REG_NONE)
>>>>>   		seq_printf(m, " (fence: %d)", obj->fence_reg);
>>>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>>>   		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>>>>>   			   i915_is_ggtt(vma->vm) ? "g" : "pp",
>>>>>   			   vma->node.start, vma->node.size);
>>>>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>>>>   		stats->shared += obj->base.size;
>>>>>
>>>>>   	if (USES_FULL_PPGTT(obj->base.dev)) {
>>>>> -		list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> +		i915_gem_obj_for_each_vma(vma, obj) {
>>>>>   			struct i915_hw_ppgtt *ppgtt;
>>>>>
>>>>>   			if (!drm_mm_node_allocated(&vma->node))
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index b77a5d84eac2..0406a020dfcc 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
>>>>>   void i915_gem_free_object(struct drm_gem_object *obj);
>>>>>   void i915_gem_vma_destroy(struct i915_vma *vma);
>>>>>
>>>>> +#define i915_gem_obj_for_each_vma(vma, obj) \
>>>>> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
>>>>> +	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
>>>>> +	     &vma->vma_link != (&(obj)->vma_list); \
>>>>> +	     vma = list_next_entry(vma, vma_link))
>>>>> +
>>>>
>>>> Unfortunately error capture is not happy with this approach. Can't even see
>>>> that error capture attempts to grab the mutex anywhere.
>>>>
>>>> So what? Drop the idea or add a "doing error capture" flag somewhere?
>>> Fix the bugs. Not surprise at all that we've screwed this up all over the
>>> place ;-) Afaics modeset code isn't much better either ...
>> Ok I'll drop this patch then since the series contains fixes to all but one
>> related issues. The remaining one is then:
>>
>> [   17.370366] ------------[ cut here ]------------
>> [   17.375633] WARNING: CPU: 0 PID: 1128 at drivers/gpu/drm/i915/i915_gem.c:5166 i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]()
>> [   17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
>> [   17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet libphy mii i915 gpio_lynxpoint i2c_hid hid video i2c_algo_bit drm_kms_helper acpi_pad drm lpc_ich mfd_core nls_iso8859_1 e1000e ptp ahci libahci pps_core
>> [   17.419484] CPU: 0 PID: 1128 Comm: Xorg Tainted: G     U          4.4.0-rc8-160107+ #105
>> [   17.428771] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014
>> [   17.443161]  ffffffffa0227790 ffff8800a98439b8 ffffffff81280d82 ffff8800a9843a00
>> [   17.451677]  ffff8800a98439f0 ffffffff81049c8c ffff8801495d0000 ffff8800aa934900
>> [   17.460166]  ffff8801495d8668 ffffffffa0242520 ffff8800aacea000 ffff8800a9843a50
>> [   17.468674] Call Trace:
>> [   17.471470]  [<ffffffff81280d82>] dump_stack+0x4b/0x79
>> [   17.477355]  [<ffffffff81049c8c>] warn_slowpath_common+0x7c/0xc0
>> [   17.484255]  [<ffffffff81049d17>] warn_slowpath_fmt+0x47/0x50
>> [   17.490869]  [<ffffffffa018f7ef>] i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]
>> [   17.499572]  [<ffffffffa01a7290>] ? gen9_write8+0x2d0/0x2d0 [i915]
>> [   17.506663]  [<ffffffffa01bdbfe>] ironlake_update_primary_plane+0x1ee/0x3a0 [i915]
>> [   17.515358]  [<ffffffffa01ace1f>] intel_plane_atomic_update+0x5f/0x70 [i915]
>> [   17.523391]  [<ffffffffa00ef122>] drm_atomic_helper_commit_planes_on_crtc+0x142/0x230 [drm_kms_helper]
>> [   17.534063]  [<ffffffffa01c1744>] intel_atomic_commit+0x424/0x2270 [i915]
>> [   17.541763]  [<ffffffff814ee04a>] ? __ww_mutex_lock+0x4a/0x82
>> [   17.548355]  [<ffffffffa0097edb>] ? drm_atomic_check_only+0x12b/0x5e0 [drm]
>> [   17.556352]  [<ffffffffa0097a07>] ? drm_atomic_set_crtc_for_connector+0x77/0xf0 [drm]
>> [   17.565346]  [<ffffffffa00983c2>] drm_atomic_commit+0x32/0x50 [drm]
>> [   17.572553]  [<ffffffffa00ef782>] drm_atomic_helper_set_config+0x72/0xb0 [drm_kms_helper]
>> [   17.581954]  [<ffffffffa0088e8f>] drm_mode_set_config_internal+0x5f/0x100 [drm]
>> [   17.590313]  [<ffffffffa008cdc8>] drm_mode_setcrtc+0xd8/0x500 [drm]
>> [   17.597491]  [<ffffffffa007f5d8>] drm_ioctl+0x258/0x4f0 [drm]
>> [   17.604065]  [<ffffffff810c34ac>] ? unlock_page+0x4c/0x50
>> [   17.610245]  [<ffffffffa008ccf0>] ? drm_mode_setplane+0x1c0/0x1c0 [drm]
>> [   17.617844]  [<ffffffff8111597d>] do_vfs_ioctl+0x2cd/0x4a0
>> [   17.624137]  [<ffffffff81052572>] ? recalc_sigpending+0x12/0x40
>> [   17.630943]  [<ffffffff8111eb32>] ? __fget+0x72/0xb0
>> [   17.636645]  [<ffffffff81115b8c>] SyS_ioctl+0x3c/0x70
>> [   17.642448]  [<ffffffff81055301>] ? SyS_rt_sigprocmask+0x81/0xa0
>> [   17.649334]  [<ffffffff814f0017>] entry_SYSCALL_64_fastpath+0x12/0x6a
>> [   17.656740] ---[ end trace 3f29190006c97826 ]---
>>
>> Would you like a BZ for this or maybe it is known already?
> Maarten is working on this, or at least we're tracking it already in
> VIZ-7000.
>
> Can you pls coordinate with Maarten to make sure the final "enable the
> warning" patch lands only once all the bits are in?
>
Chris Wilson's patch bomb makes it possible to use the vma instead of grabbing the view over and over, and that should fix this specific error.

~Maarten
Tvrtko Ursulin Jan. 13, 2016, 2:35 p.m. UTC | #8 
On 12/01/16 16:19, Daniel Vetter wrote:
> On Mon, Jan 11, 2016 at 09:51:38AM +0000, Tvrtko Ursulin wrote:
>>
>> On 11/01/16 08:43, Daniel Vetter wrote:
>>> On Fri, Jan 08, 2016 at 01:29:14PM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 08/01/16 11:29, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> Purpose is to catch places which iterate the object VMA list
>>>>> without holding the big lock.
>>>>>
>>>>> Implemented by open coding list_for_each_entry to make the
>>>>> macro compatible with existing call sites.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
>>>>>    drivers/gpu/drm/i915/i915_drv.h          |  6 ++++++
>>>>>    drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
>>>>>    drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
>>>>>    drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>>>>>    drivers/gpu/drm/i915/i915_gpu_error.c    |  4 ++--
>>>>>    6 files changed, 26 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> index 714a45cf8a51..d7c2a3201161 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
>>>>>    	u64 size = 0;
>>>>>    	struct i915_vma *vma;
>>>>>
>>>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>>>    		if (i915_is_ggtt(vma->vm) &&
>>>>>    		    drm_mm_node_allocated(&vma->node))
>>>>>    			size += vma->node.size;
>>>>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>>>>    		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>>>>>    	if (obj->base.name)
>>>>>    		seq_printf(m, " (name: %d)", obj->base.name);
>>>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>>>    		if (vma->pin_count > 0)
>>>>>    			pin_count++;
>>>>>    	}
>>>>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>>>>    		seq_printf(m, " (display)");
>>>>>    	if (obj->fence_reg != I915_FENCE_REG_NONE)
>>>>>    		seq_printf(m, " (fence: %d)", obj->fence_reg);
>>>>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>>>>    		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>>>>>    			   i915_is_ggtt(vma->vm) ? "g" : "pp",
>>>>>    			   vma->node.start, vma->node.size);
>>>>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>>>>    		stats->shared += obj->base.size;
>>>>>
>>>>>    	if (USES_FULL_PPGTT(obj->base.dev)) {
>>>>> -		list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>>> +		i915_gem_obj_for_each_vma(vma, obj) {
>>>>>    			struct i915_hw_ppgtt *ppgtt;
>>>>>
>>>>>    			if (!drm_mm_node_allocated(&vma->node))
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index b77a5d84eac2..0406a020dfcc 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -2852,6 +2852,12 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
>>>>>    void i915_gem_free_object(struct drm_gem_object *obj);
>>>>>    void i915_gem_vma_destroy(struct i915_vma *vma);
>>>>>
>>>>> +#define i915_gem_obj_for_each_vma(vma, obj) \
>>>>> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
>>>>> +	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
>>>>> +	     &vma->vma_link != (&(obj)->vma_list); \
>>>>> +	     vma = list_next_entry(vma, vma_link))
>>>>> +
>>>>
>>>>
>>>> Unfortunately error capture is not happy with this approach. Can't even see
>>>> that error capture attempts to grab the mutex anywhere.
>>>>
>>>> So what? Drop the idea or add a "doing error capture" flag somewhere?
>>>
>>> Fix the bugs. Not surprise at all that we've screwed this up all over the
>>> place ;-) Afaics modeset code isn't much better either ...
>>
>> Ok I'll drop this patch then since the series contains fixes to all but one
>> related issues. The remaining one is then:
>>
>> [   17.370366] ------------[ cut here ]------------
>> [   17.375633] WARNING: CPU: 0 PID: 1128 at drivers/gpu/drm/i915/i915_gem.c:5166 i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]()
>> [   17.388879] WARN_ON_ONCE(!mutex_is_locked(&(o)->base.dev->struct_mutex))
>> [   17.396364] Modules linked in: hid_generic usbhid coretemp asix usbnet libphy mii i915 gpio_lynxpoint i2c_hid hid video i2c_algo_bit drm_kms_helper acpi_pad drm lpc_ich mfd_core nls_iso8859_1 e1000e ptp ahci libahci pps_core
>> [   17.419484] CPU: 0 PID: 1128 Comm: Xorg Tainted: G     U          4.4.0-rc8-160107+ #105
>> [   17.428771] Hardware name: Intel Corporation Broadwell Client platform/WhiteTip Mountain 1, BIOS BDW-E1R1.86C.0080.R01.1406120446 06/12/2014
>> [   17.443161]  ffffffffa0227790 ffff8800a98439b8 ffffffff81280d82 ffff8800a9843a00
>> [   17.451677]  ffff8800a98439f0 ffffffff81049c8c ffff8801495d0000 ffff8800aa934900
>> [   17.460166]  ffff8801495d8668 ffffffffa0242520 ffff8800aacea000 ffff8800a9843a50
>> [   17.468674] Call Trace:
>> [   17.471470]  [<ffffffff81280d82>] dump_stack+0x4b/0x79
>> [   17.477355]  [<ffffffff81049c8c>] warn_slowpath_common+0x7c/0xc0
>> [   17.484255]  [<ffffffff81049d17>] warn_slowpath_fmt+0x47/0x50
>> [   17.490869]  [<ffffffffa018f7ef>] i915_gem_obj_ggtt_offset_view+0x10f/0x120 [i915]
>> [   17.499572]  [<ffffffffa01a7290>] ? gen9_write8+0x2d0/0x2d0 [i915]
>> [   17.506663]  [<ffffffffa01bdbfe>] ironlake_update_primary_plane+0x1ee/0x3a0 [i915]
>> [   17.515358]  [<ffffffffa01ace1f>] intel_plane_atomic_update+0x5f/0x70 [i915]
>> [   17.523391]  [<ffffffffa00ef122>] drm_atomic_helper_commit_planes_on_crtc+0x142/0x230 [drm_kms_helper]
>> [   17.534063]  [<ffffffffa01c1744>] intel_atomic_commit+0x424/0x2270 [i915]
>> [   17.541763]  [<ffffffff814ee04a>] ? __ww_mutex_lock+0x4a/0x82
>> [   17.548355]  [<ffffffffa0097edb>] ? drm_atomic_check_only+0x12b/0x5e0 [drm]
>> [   17.556352]  [<ffffffffa0097a07>] ? drm_atomic_set_crtc_for_connector+0x77/0xf0 [drm]
>> [   17.565346]  [<ffffffffa00983c2>] drm_atomic_commit+0x32/0x50 [drm]
>> [   17.572553]  [<ffffffffa00ef782>] drm_atomic_helper_set_config+0x72/0xb0 [drm_kms_helper]
>> [   17.581954]  [<ffffffffa0088e8f>] drm_mode_set_config_internal+0x5f/0x100 [drm]
>> [   17.590313]  [<ffffffffa008cdc8>] drm_mode_setcrtc+0xd8/0x500 [drm]
>> [   17.597491]  [<ffffffffa007f5d8>] drm_ioctl+0x258/0x4f0 [drm]
>> [   17.604065]  [<ffffffff810c34ac>] ? unlock_page+0x4c/0x50
>> [   17.610245]  [<ffffffffa008ccf0>] ? drm_mode_setplane+0x1c0/0x1c0 [drm]
>> [   17.617844]  [<ffffffff8111597d>] do_vfs_ioctl+0x2cd/0x4a0
>> [   17.624137]  [<ffffffff81052572>] ? recalc_sigpending+0x12/0x40
>> [   17.630943]  [<ffffffff8111eb32>] ? __fget+0x72/0xb0
>> [   17.636645]  [<ffffffff81115b8c>] SyS_ioctl+0x3c/0x70
>> [   17.642448]  [<ffffffff81055301>] ? SyS_rt_sigprocmask+0x81/0xa0
>> [   17.649334]  [<ffffffff814f0017>] entry_SYSCALL_64_fastpath+0x12/0x6a
>> [   17.656740] ---[ end trace 3f29190006c97826 ]---
>>
>> Would you like a BZ for this or maybe it is known already?
>
> Maarten is working on this, or at least we're tracking it already in
> VIZ-7000.
>
> Can you pls coordinate with Maarten to make sure the final "enable the
> warning" patch lands only once all the bits are in?

Oh I dropped that patch due the error capture problem - do you have a 
suggestion how to handle that?

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 714a45cf8a51..d7c2a3201161 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -117,7 +117,7 @@  static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
 	u64 size = 0;
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (i915_is_ggtt(vma->vm) &&
 		    drm_mm_node_allocated(&vma->node))
 			size += vma->node.size;
@@ -155,7 +155,7 @@  describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
 	if (obj->base.name)
 		seq_printf(m, " (name: %d)", obj->base.name);
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (vma->pin_count > 0)
 			pin_count++;
 	}
@@ -164,7 +164,7 @@  describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		seq_printf(m, " (display)");
 	if (obj->fence_reg != I915_FENCE_REG_NONE)
 		seq_printf(m, " (fence: %d)", obj->fence_reg);
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
 			   i915_is_ggtt(vma->vm) ? "g" : "pp",
 			   vma->node.start, vma->node.size);
@@ -342,7 +342,7 @@  static int per_file_stats(int id, void *ptr, void *data)
 		stats->shared += obj->base.size;
 
 	if (USES_FULL_PPGTT(obj->base.dev)) {
-		list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		i915_gem_obj_for_each_vma(vma, obj) {
 			struct i915_hw_ppgtt *ppgtt;
 
 			if (!drm_mm_node_allocated(&vma->node))
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b77a5d84eac2..0406a020dfcc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2852,6 +2852,12 @@  struct drm_i915_gem_object *i915_gem_object_create_from_data(
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
+#define i915_gem_obj_for_each_vma(vma, obj) \
+	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
+	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
+	     &vma->vma_link != (&(obj)->vma_list); \
+	     vma = list_next_entry(vma, vma_link))
+
 /* Flags used by pin/bind&friends. */
 #define PIN_MAPPABLE	(1<<0)
 #define PIN_NONBLOCK	(1<<1)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c4f69579eb7a..415bb5ef8b3a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2442,7 +2442,7 @@  i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
 	list_move_tail(&obj->global_list,
 		       &to_i915(obj->base.dev)->mm.bound_list);
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (!list_empty(&vma->mm_list))
 			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
 	}
@@ -3834,7 +3834,7 @@  int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 			 */
 		}
 
-		list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		i915_gem_obj_for_each_vma(vma, obj) {
 			if (!drm_mm_node_allocated(&vma->node))
 				continue;
 
@@ -3844,7 +3844,7 @@  int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		}
 	}
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, obj)
 		vma->node.color = cache_level;
 	obj->cache_level = cache_level;
 
@@ -4564,7 +4564,7 @@  struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
 				     struct i915_address_space *vm)
 {
 	struct i915_vma *vma;
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL &&
 		    vma->vm == vm)
 			return vma;
@@ -4581,7 +4581,7 @@  struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
 	if (WARN_ONCE(!view, "no view specified"))
 		return ERR_PTR(-EINVAL);
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, obj)
 		if (vma->vm == ggtt &&
 		    i915_ggtt_view_equal(&vma->ggtt_view, view))
 			return vma;
@@ -5144,7 +5144,7 @@  u64 i915_gem_obj_offset(struct drm_i915_gem_object *o,
 
 	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
 
-	list_for_each_entry(vma, &o->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, o) {
 		if (i915_is_ggtt(vma->vm) &&
 		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
 			continue;
@@ -5163,7 +5163,7 @@  u64 i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
 	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &o->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, o)
 		if (vma->vm == ggtt &&
 		    i915_ggtt_view_equal(&vma->ggtt_view, view))
 			return vma->node.start;
@@ -5177,7 +5177,7 @@  bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
 {
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &o->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, o) {
 		if (i915_is_ggtt(vma->vm) &&
 		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
 			continue;
@@ -5194,7 +5194,7 @@  bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
 	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &o->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, o)
 		if (vma->vm == ggtt &&
 		    i915_ggtt_view_equal(&vma->ggtt_view, view) &&
 		    drm_mm_node_allocated(&vma->node))
@@ -5207,7 +5207,7 @@  bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o)
 {
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &o->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, o)
 		if (drm_mm_node_allocated(&vma->node))
 			return true;
 
@@ -5224,7 +5224,7 @@  unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 
 	BUG_ON(list_empty(&o->vma_list));
 
-	list_for_each_entry(vma, &o->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, o) {
 		if (i915_is_ggtt(vma->vm) &&
 		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
 			continue;
@@ -5237,7 +5237,7 @@  unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
 {
 	struct i915_vma *vma;
-	list_for_each_entry(vma, &obj->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, obj)
 		if (vma->pin_count > 0)
 			return true;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 56f4f2e58d53..40fe2bf6bd91 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3220,7 +3220,7 @@  void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 	vm = &dev_priv->gtt.base;
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		flush = false;
-		list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		i915_gem_obj_for_each_vma(vma, obj) {
 			if (vma->vm != vm)
 				continue;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 16da9c1422cc..4106666a4303 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -52,7 +52,7 @@  static int num_vma_bound(struct drm_i915_gem_object *obj)
 	struct i915_vma *vma;
 	int count = 0;
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (drm_mm_node_allocated(&vma->node))
 			count++;
 		if (vma->pin_count)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 06ca4082735b..8b7a8c4b9b37 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -755,7 +755,7 @@  static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
 		if (err == last)
 			break;
 
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
+		i915_gem_obj_for_each_vma(vma, obj)
 			if (vma->vm == vm && vma->pin_count > 0)
 				capture_bo(err++, vma);
 	}
@@ -1128,7 +1128,7 @@  static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
 	error->active_bo_count[ndx] = i;
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		list_for_each_entry(vma, &obj->vma_list, vma_link)
+		i915_gem_obj_for_each_vma(vma, obj)
 			if (vma->vm == vm && vma->pin_count > 0)
 				i++;
 	}