Message ID | 1452252592-24803-8-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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
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
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
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
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
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
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 --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++; }