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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxx>
> >>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >>> ---
> >>>   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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux