Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
On 12/01/16 16:19, Daniel Vetter wrote: On Mon, Jan 11, 2016 at 09:51:38AM +, Tvrtko Ursulin wrote: On 11/01/16 08:43, Daniel Vetter wrote: On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote: On 08/01/16 11:29, Tvrtko Ursulin wrote: From: Tvrtko UrsulinPurpose 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 Cc: Daniel Vetter --- 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, >vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (i915_is_ggtt(vma->vm) && drm_mm_node_allocated(>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, >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, >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, >vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { struct i915_hw_ppgtt *ppgtt; if (!drm_mm_node_allocated(>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_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] a0227790 8800a98439b8 81280d82 8800a9843a00 [ 17.451677]
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
On Mon, Jan 11, 2016 at 09:51:38AM +, Tvrtko Ursulin wrote: > > On 11/01/16 08:43, Daniel Vetter wrote: > > On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote: > >> > >> On 08/01/16 11:29, Tvrtko Ursulin wrote: > >>> From: Tvrtko Ursulin> >>> > >>> 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 > >>> Cc: Daniel Vetter > >>> --- > >>> 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, >vma_list, vma_link) { > >>> + i915_gem_obj_for_each_vma(vma, obj) { > >>> if (i915_is_ggtt(vma->vm) && > >>> drm_mm_node_allocated(>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, >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, >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, >vma_list, vma_link) { > >>> + i915_gem_obj_for_each_vma(vma, obj) { > >>> struct i915_hw_ppgtt *ppgtt; > >>> > >>> if (!drm_mm_node_allocated(>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_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
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
Op 12-01-16 om 17:19 schreef Daniel Vetter: > On Mon, Jan 11, 2016 at 09:51:38AM +, Tvrtko Ursulin wrote: >> On 11/01/16 08:43, Daniel Vetter wrote: >>> On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote: On 08/01/16 11:29, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin> > 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 > Cc: Daniel Vetter > --- > 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, >vma_list, vma_link) { > + i915_gem_obj_for_each_vma(vma, obj) { > if (i915_is_ggtt(vma->vm) && > drm_mm_node_allocated(>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, >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, >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, >vma_list, vma_link) { > + i915_gem_obj_for_each_vma(vma, obj) { > struct i915_hw_ppgtt *ppgtt; > > if (!drm_mm_node_allocated(>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_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 >>
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote: > > On 08/01/16 11:29, Tvrtko Ursulin wrote: > >From: Tvrtko Ursulin> > > >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 > >Cc: Daniel Vetter > >--- > > 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, >vma_list, vma_link) { > >+i915_gem_obj_for_each_vma(vma, obj) { > > if (i915_is_ggtt(vma->vm) && > > drm_mm_node_allocated(>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, >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, >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, >vma_list, vma_link) { > >+i915_gem_obj_for_each_vma(vma, obj) { > > struct i915_hw_ppgtt *ppgtt; > > > > if (!drm_mm_node_allocated(>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_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 Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
On 11/01/16 08:43, Daniel Vetter wrote: > On Fri, Jan 08, 2016 at 01:29:14PM +, Tvrtko Ursulin wrote: >> >> On 08/01/16 11:29, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin>>> >>> 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 >>> Cc: Daniel Vetter >>> --- >>> 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, >vma_list, vma_link) { >>> + i915_gem_obj_for_each_vma(vma, obj) { >>> if (i915_is_ggtt(vma->vm) && >>> drm_mm_node_allocated(>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, >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, >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, >vma_list, vma_link) { >>> + i915_gem_obj_for_each_vma(vma, obj) { >>> struct i915_hw_ppgtt *ppgtt; >>> >>> if (!drm_mm_node_allocated(>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_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]
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
On Fri, Jan 08, 2016 at 11:44:04AM +, Chris Wilson wrote: > On Fri, Jan 08, 2016 at 11:29:46AM +, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin> > > > 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 > > Cc: Daniel Vetter > > +#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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
From: Tvrtko UrsulinPurpose 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 Cc: Daniel Vetter --- 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, >vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (i915_is_ggtt(vma->vm) && drm_mm_node_allocated(>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, >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, >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, >vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { struct i915_hw_ppgtt *ppgtt; if (!drm_mm_node_allocated(>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_link != (&(obj)->vma_list); \ +vma = list_next_entry(vma, vma_link)) + /* Flags used by pin/bind */ #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(>global_list, _i915(obj->base.dev)->mm.bound_list); - list_for_each_entry(vma, >vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (!list_empty(>mm_list)) list_move_tail(>mm_list, >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, >vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (!drm_mm_node_allocated(>node)) continue; @@ -3844,7 +3844,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, } } - list_for_each_entry(vma, >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; -
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
On Fri, Jan 08, 2016 at 11:29:46AM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin> > 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 > Cc: Daniel Vetter > +#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 -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Introduce dedicated object VMA iterator
On 08/01/16 11:29, Tvrtko Ursulin wrote: From: Tvrtko UrsulinPurpose 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 Cc: Daniel Vetter --- 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, >vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { if (i915_is_ggtt(vma->vm) && drm_mm_node_allocated(>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, >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, >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, >vma_list, vma_link) { + i915_gem_obj_for_each_vma(vma, obj) { struct i915_hw_ppgtt *ppgtt; if (!drm_mm_node_allocated(>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_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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx